From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konstantin Khorenko Date: Tue, 25 Aug 2026 15:42:57 +0200 Subject: Re: [Devel] [PATCH VZ10 v3 5/5] drivers/vhost/blk: rework queue/backend setup In-Reply-To: <20260825125152.259376-6-andrey.zhadchenko@virtuozzo.com> References: <20260825125152.259376-1-andrey.zhadchenko@virtuozzo.com> <20260825125152.259376-6-andrey.zhadchenko@virtuozzo.com> Message-ID: <0f063fe8-b322-4237-80d3-587a119d948c@virtuozzo.com> List-Id: 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