vduse_vq_kick() runs in the context of the vdpa .kick_vq callback. With
the virtio_vdpa bus driver that callback is invoked by virtqueue_notify()
from the virtio device driver, which may be an atomic context: virtio-blk
kicks from ->queue_rq(), which blk-mq dispatches under rcu_read_lock()
(the tag set does not use BLK_MQ_F_BLOCKING), and virtio-net kicks from
its xmit path with the tx queue lock held.
Commit b282418bc366 ("vduse: Add suspend") made vduse_vq_kick() take
dev->rwsem for reading in order to check dev->suspended. down_read() may
sleep, so with CONFIG_DEBUG_ATOMIC_SLEEP the first I/O on a VDUSE-backed
virtio-blk device bound to virtio_vdpa now triggers:
BUG: sleeping function called from invalid context at
kernel/locking/rwsem.c:1573
in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 27, name: kworker/1:0H
preempt_count: 0, expected: 0
RCU nest depth: 1, expected: 0
3 locks held by kworker/1:0H/27:
#0: ((wq_completion)kblockd){+.+.}-{0:0}, at: process_one_work+0xac7/0xcf0
#1: ((work_completion)(&(&hctx->run_work)->work)){+.+.}-{0:0}, at:
process_one_work+0x51f/0xcf0
#2: (rcu_read_lock){....}-{1:3}, at: blk_mq_run_work_fn+0x119/0x220
Workqueue: kblockd blk_mq_run_work_fn
Call Trace:
<TASK>
dump_stack_lvl+0x80/0xa0
__might_resched+0x231/0x370
down_read+0x73/0x330
vduse_vq_kick+0x30/0x120
virtio_vdpa_notify+0x63/0x80
virtqueue_notify+0x45/0x70
virtio_queue_rq+0x19d/0x300
blk_mq_dispatch_rq_list+0x269/0xe20
__blk_mq_sched_dispatch_requests+0x761/0xa60
blk_mq_sched_dispatch_requests+0x6b/0xc0
blk_mq_run_work_fn+0x143/0x220
process_one_work+0x581/0xcf0
worker_thread+0x2fc/0x5a0
kthread+0x1cc/0x210
ret_from_fork+0x3c4/0x540
ret_from_fork_asm+0x1a/0x30
</TASK>
Without CONFIG_DEBUG_ATOMIC_SLEEP, a kick that finds the rwsem
write-locked by vduse_dev_reset() or vduse_vdpa_suspend() blocks inside
an RCU read-side critical section. The vhost_vdpa path kicks from the
vhost worker, i.e. process context, which is why this went unnoticed.
Check dev->suspended under vq->kick_lock instead, which the kick path
already takes, and have vduse_vdpa_suspend() cycle every virtqueue's
kick_lock after setting the flag. A kick that observed suspended == false
has thus finished signalling before suspend returns, which is the
guarantee the rwsem used to provide. The flag is now also read outside
the rwsem, so access it with READ_ONCE()/WRITE_ONCE().
Fixes: b282418bc366 ("vduse: Add suspend")
Signed-off-by: Nikhil <[email protected]>
---
drivers/vdpa/vdpa_user/vduse_dev.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c
b/drivers/vdpa/vdpa_user/vduse_dev.c
index 9891cd2cf712..766789a7bbfa 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -506,7 +506,7 @@ static void vduse_dev_reset(struct vduse_dev *dev)
}
scoped_guard(rwsem_write, &dev->rwsem) {
- dev->suspended = false;
+ WRITE_ONCE(dev->suspended, false);
dev->status = 0;
dev->driver_features = 0;
dev->generation++;
@@ -567,11 +567,17 @@ static int vduse_vdpa_set_vq_address(struct vdpa_device
*vdpa, u16 idx,
static void vduse_vq_kick(struct vduse_virtqueue *vq)
{
- guard(rwsem_read)(&vq->dev->rwsem);
- if (vq->dev->suspended)
+ /*
+ * This runs in the context of the vdpa kick_vq op, which may be
+ * atomic (e.g. virtio-blk kicks from blk-mq dispatch under
+ * rcu_read_lock()), so dev->rwsem must not be taken here.
+ * dev->suspended is checked under kick_lock instead and
+ * vduse_vdpa_suspend() cycles every kick_lock after setting it.
+ */
+ guard(spinlock)(&vq->kick_lock);
+ if (READ_ONCE(vq->dev->suspended))
return;
- guard(spinlock)(&vq->kick_lock);
scoped_guard(spinlock_bh, &vq->ready_lock)
if (!vq->ready)
return;
@@ -946,7 +952,17 @@ static int vduse_vdpa_suspend(struct vdpa_device *vdpa)
ret = vduse_dev_msg_sync(dev, &msg);
if (ret == 0) {
scoped_guard(rwsem_write, &dev->rwsem)
- dev->suspended = true;
+ WRITE_ONCE(dev->suspended, true);
+
+ /*
+ * Kicks check dev->suspended under kick_lock without taking
+ * the rwsem: cycle each kick_lock so that no kick that has
+ * already passed the check is still in flight after this.
+ */
+ for (u32 i = 0; i < dev->vq_num; i++) {
+ spin_lock(&dev->vqs[i]->kick_lock);
+ spin_unlock(&dev->vqs[i]->kick_lock);
+ }
cancel_work_sync(&dev->inject);
for (u32 i = 0; i < dev->vq_num; i++)
--
2.43.0
