Re: [PATCH v3 3/3] virtio_pci_modern: move avq cleanup from reset to del_vqs

[Michael S. Tsirkin]

On Sat, Sep 12, 2026 at 12:48:54AM +0200, Karl Mehltretter wrote:
> On Fri, Sep 11, 2026 at 05:20:15PM +0100, Michael S. Tsirkin wrote:
> > New in v3. v2 dropped sync from modern vp_reset in one combined
> > patch, leaving avq_cleanup in vp_reset. v3 moves avq_cleanup out
> > of vp_reset entirely into vp_del_vqs, fixing the race. Adds NULL
> > check for admin_vq.info for find_vqs error paths.
> > 
> 
> I think patch 3 introduces a use-after-free in the MSI-X setup error
> path.
> 
> With VP_VQ_VECTOR_POLICY_EACH, vp_setup_vq() stores the admin queue's
> info pointer before request_irq(). If request_irq() fails, vp_del_vq()
> frees info but leaves admin_vq.info set.
> 
> vp_find_vqs_msix() then calls vp_del_vqs(). vp_del_vqs() now calls
> vp_modern_avq_cleanup() first and dereferences the freed pointer:
> 
>     vq = vp_dev->admin_vq.info->vq;
> 
> No interrupt needs to fire.
> 
> I reproduced this on v3 with KASAN by returning -ENOMEM only for the
> admin queue's request_irq():
> 
>     BUG: KASAN: slab-use-after-free in vp_modern_avq_cleanup+0xf4/0x110
> 
> KASAN shows the allocation in vp_setup_vq(), the free in
> vp_find_one_vq_msix(), and the access in vp_modern_avq_cleanup(). Adding
> *p_info = NULL after vp_del_vq() made the same test fall back and boot
> without a KASAN report.
> 
> Thanks,
> Karl

Right. Besides, poking at modern from common is ugly. I'll rework this,
thanks a lot for the testing!

-- 
MST