Re: [Devel] [PATCH VZ10 v3 5/5] drivers/vhost/blk: rework queue/backend setup

[Konstantin Khorenko <khorenko@virtuozzo.com>]

A side finding, unrelated to this patch, but still, please fix it in a separate patch:

  In drivers/vhost/vhost.h the backend access rules are documented explicitly (drivers/vhost/vhost.h:288-310):

  /**
   * vhost_vq_set_backend - Set backend.
   * ...
   * Context: Need to call with vq->mutex acquired.
   */

  /**
   * vhost_vq_get_backend - Get backend.
   * ...
   * Context: Need to call with vq->mutex acquired.
   */

  So vq->private_data is a plain field with no barriers whatsoever - its consistency is guaranteed solely by both sides
  holding vq->mutex. Every other vhost driver honors that: handle_tx/handle_rx in vhost-net take
  mutex_lock_nested(&vq->mutex, ...) (drivers/vhost/net.c:948, 1117), vsock and scsi do the same in their kick paths. Only
  vhost_blk_handle_guest_kick() calls vhost_vq_get_backend() and then works with the ring without taking the mutex at all.

  Where exactly the race is

  The writer is vhost_blk_setup_vqs() (ioctl thread, under dev.mutex); the reader is the vhost worker (a separate kernel
  thread, no mutex). Side by side:

  CPU0: ioctl VHOST_BLK_SET_BACKEND             CPU1: vhost worker
        vhost_blk_setup_vqs()                         vhost_blk_handle_guest_kick()
  ------------------------------------------    ------------------------------------------
  blk->vqs[i].req = kvmalloc_array(...);
                      /* store 1: req array */
  mutex_lock(&vq->mutex);
  vhost_vq_set_backend(vq, blk->backend);
                      /* store 2: publish
                         the backend */
  mutex_unlock(&vq->mutex);
                                                f = vhost_vq_get_backend(vq);
                                                                  /* load 1: backend */
                                                if (!f)
                                                        return;
                                                ...
                                                head = vhost_get_vq_desc(...);
                                                ...
                                                req = &blk_vq->req[head];
                                                                  /* load 2: req array
                                                                     pointer, then
                                                                     dereference */

  Correctness rests on the invariant: if the reader observes backend != NULL, it must also observe the already-written
  blk_vq->req pointer. For that to hold, two things are required: (a) the writer's stores must become visible in program
  order (req first, backend second), and (b) the reader's loads must be performed in program order (backend first, req
  second).

  The writer-side mutex does not help here: a lock orders memory only between critical sections of that same lock. The
  reader never takes it, so there is no happens-before edge between the writer's mutex_unlock() and the worker's loads.
  Worse, mutex_lock() is an acquire operation, and acquire is one-directional: it forbids later accesses from moving up
  above it, but it does not forbid an earlier store (the req write) from sinking down below it. So even within the writer,
  store 1 and store 2 are formally unordered with respect to each other as far as an external observer is concerned.

  The failure interleaving on a weakly ordered CPU (e.g. arm64) then looks like this:

  CPU0                                          CPU1
  ------------------------------------------    ------------------------------------------
  store 2 becomes globally visible
     (backend != NULL)
                                                load 1: sees the new backend -> proceeds
                                                load 2: sees the *old* blk_vq->req
                                                        (NULL on first SET_BACKEND, or a
                                                         freed pointer after a -1/fd cycle)
                                                req = &NULL[head]   -> NULL-offset oops
                                                (or use-after-free)
  store 1 becomes globally visible
     (req array pointer) - too late

  On x86-64 this cannot happen, because TSO forbids both store-store reordering on CPU0 and load-load reordering on CPU1 -
  which is the only reason the current code works in practice.


Suggested fix:

  --- a/drivers/vhost/blk.c
  +++ b/drivers/vhost/blk.c
  @@ static void vhost_blk_handle_guest_kick(struct vhost_work *work)
        vq = container_of(work, struct vhost_virtqueue, poll.work);
        blk = container_of(vq->dev, struct vhost_blk, dev);
        blk_vq = container_of(vq, struct vhost_blk_vq, vq);

  +     mutex_lock(&vq->mutex);
  +
        f = vhost_vq_get_backend(vq);
        if (!f)
  -             return;
  +             goto out;

        vhost_disable_notify(&blk->dev, vq);
        for (;;) {
                head = vhost_get_vq_desc(vq, vq->iov,
                                         ARRAY_SIZE(vq->iov),
                                         &out, &in, NULL, NULL);
  @@
                if (!llist_empty(&blk_vq->llhead)) {
                        vhost_poll_queue(&vq->poll);
                        break;
                }
        }
  +out:
  +     mutex_unlock(&vq->mutex);
   }
  @@ static void vhost_blk_handle_host_kick(struct vhost_work *work)
        blk_vq = container_of(work, struct vhost_blk_vq, work);
        vq = &blk_vq->vq;
  +
  +     mutex_lock(&vq->mutex);
        llnode = llist_del_all(&blk_vq->llhead);
        added = false;
        while (llnode) {
  @@
                forget_request(req);
        }

        if (likely(added))
                vhost_signal(&blk->dev, vq);
  +     mutex_unlock(&vq->mutex);
   }



--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team