On 10/16/26 04:54, Mingbing Zhai wrote:
> Since c578123e63da ("virtio_balloon: Treat stats requests as wakeup
> events") the stats_request() virtqueue callback takes vb->wakeup_lock
> via start_wakeup_event(). init_vqs() registers that callback and even
> primes and kicks the stats queue, so the lock is reachable from irq
> context as soon as init_vqs() returns, long before the driver sets
> DRIVER_OK: vring_interrupt() dispatches to the callback without any
> DRIVER_OK check unless CONFIG_VIRTIO_HARDEN_NOTIFICATION is set, and
> that option depends on BROKEN. syzbot hit it from a threaded irq:
>
> INFO: trying to register non-static key in stats_request
> ...
> assign_lock_key
> register_lock_class
> lock_acquire
> rt_spin_lock
> start_wakeup_event drivers/virtio/virtio_balloon.c:150 [inline]
> stats_request drivers/virtio/virtio_balloon.c:435
>
> The lock is only initialized near the end of virtballoon_probe(),
> after init_vqs() and several steps that can still fail and unwind
> through del_vqs(), where a racing interrupt still finds the callback
> registered. Until then it is whatever kzalloc() left in vb: lockdep
> complains about the unregistered lock class, and wakeup_signal_mask
> and processing_wakeup_event get updated from irq context while probe
> is still running.
>
> The rt_spin_lock() in the report is just spin_lock() on PREEMPT_RT;
> any PROVE_LOCKING build takes the same path.
>
> stop_update_lock(), which the same callback also acquires, is already
> initialized before init_vqs(); do the same for wakeup_lock.
>
> Fixes: c578123e63da ("virtio_balloon: Treat stats requests as wakeup events")
> Reported-by: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=13df72e0dc4be5c62215
> Signed-off-by: Mingbing Zhai <[email protected]>
> ---
>
> Hi David,
>
> On 24.08.26 14:57, David Hildenbrand (Arm) wrote:
>> I don't really understand what's happening here, Is the
>> register_lock_class() trying to reguster a non-static key?
>
> It is: the wakeup_lock that stats_request() ends up taking at that
> point hasn't been run through spin_lock_init() yet.
>
>> I don't immediately see how virtio-balloon is doing something odd
>> here, all it does is a spin_lock_irqsave(&vb->wakeup_lock, flags),
>> that was earlier initialized with spin_lock_init(&vb->wakeup_lock),
>> before the device was ready.
>
> Right, the lock is initialized before virtio_device_ready(). But if
> I follow correctly, stats_request() can already fire once init_vqs()
> registers it as the stats virtqueue callback, some 100 lines before
> the spin_lock_init(). I also don't see anything on the guest side
> gating the IRQ: vring_interrupt() calls the callback without any
> DRIVER_OK check unless CONFIG_VIRTIO_HARDEN_NOTIFICATION (which
> depends on BROKEN). So a device raising the interrupt while probe is
> still running would take the zero-initialized lock.
>
>> virtio-balloon in an RT kernel/environment, unlikely in pactice ... :)
>
> I don't think it is RT-specific, though: rt_spin_lock() is just what
> spin_lock() compiles to on PREEMPT_RT, so any PROVE_LOCKING build
> should hit the same register_lock_class() path on that lock.
>
> stop_update_lock(), which the same callback takes first, is already
> initialized before init_vqs(); the patch below does the same for
> wakeup_lock. Does that look reasonable?
>
> drivers/virtio/virtio_balloon.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index ab3e3d887e..8c1b47ec17 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -942,6 +942,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
> INIT_WORK(&vb->update_balloon_stats_work, update_balloon_stats_func);
> INIT_WORK(&vb->update_balloon_size_work, update_balloon_size_func);
> spin_lock_init(&vb->stop_update_lock);
> + spin_lock_init(&vb->wakeup_lock);
> mutex_init(&vb->balloon_lock);
> init_waitqueue_head(&vb->acked);
> vb->vdev = vdev;
> @@ -1049,8 +1050,6 @@ static int virtballoon_probe(struct virtio_device *vdev)
> goto out_unregister_oom;
> }
>
> - spin_lock_init(&vb->wakeup_lock);
> -
> /*
> * The virtio balloon itself can't wake up the device, but it is
> * responsible for processing wakeup events passed up from the transport
That looks reasonable to me!
--
Cheers,
David
