[PATCH]virtio-pci: Check if is_avq is NULL

[PATCH]virtio-pci: Check if is_avq is NULL

[Li Zhang]

[bug]
In the virtio_pci_common.c function vp_del_vqs, vp_dev->is_avq is involved
to determine whether it is admin virtqueue, but this function vp_dev->is_avq
 may be empty. For installations, virtio_pci_legacy does not assign a value
 to vp_dev->is_avq.

[fix]
Check whether it is vp_dev->is_avq before use.

[test]
Test with virsh Attach device
Before this patch, the following command would crash the guest system

After applying the patch, everything seems to be working fine.

Signed-off-by: Li Zhang <zhanglikernel@gmail.com>
---
 drivers/virtio/virtio_pci_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index b655fcc..3c18fc1 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -236,7 +236,7 @@ void vp_del_vqs(struct virtio_device *vdev)
 	int i;
 
 	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
-		if (vp_dev->is_avq(vdev, vq->index))
+		if (vp_dev->is_avq && vp_dev->is_avq(vdev, vq->index))
 			continue;
 
 		if (vp_dev->per_vq_vectors) {
-- 
1.8.3.1

Re: [PATCH]virtio-pci: Check if is_avq is NULL

[li zhang]

Add details about test cases

Create a guest named guest1 in KVM,
In the host machine, run the following command.
#create share-device.xml and write the following text (host)
qemu-img create -f qcow2 /root/share-device.qcow2 20G


#create share-device.xml with following message(hosts)
<disk type='file' device='disk'>
    <driver name='qemu' type='qcow2' cache='writeback' io='threads'/>
    <source file='/root/share-device.qcow2'/>
    <target dev='vdb' bus='virtio'/>
</disk>


#attach device to guest1(hosts)
virsh attach-device guest1 share-device.xml --config --live

#detach device to guest1(hosts)
virsh detach-device guest1 share-device.xml --config --live

Before patch, guest1 crashed
[   64.432236] BUG: kernel NULL pointer dereference, address: 0000000000000000
[   64.432734] #PF: supervisor instruction fetch in kernel mode
[   64.433142] #PF: error_code(0x0010) - not-present page
[   64.433497] PGD 0 P4D 0
[   64.433686] Oops: 0010 [#1] PREEMPT SMP PTI
[   64.433977] CPU: 3 PID: 85 Comm: kworker/u16:1 Not tainted 6.8.0+ #3
[   64.434417] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
[   64.434818] Workqueue: kacpi_hotplug acpi_hotplug_work_fn
[   64.435221] RIP: 0010:0x0
[   64.435427] Code: Unable to access opcode bytes at 0xffffffffffffffd6.
[   64.435916] RSP: 0018:ffffaf80003e3bd8 EFLAGS: 00010206
[   64.436260] RAX: 0000000000000000 RBX: ffff9dd8c26e4800 RCX: 0000000000000000
[   64.436662] RDX: 0000000080000000 RSI: 0000000000000000 RDI: ffff9dd8c26e4800
[   64.437061] RBP: ffff9dd8c4bdc900 R08: 0000000000000000 R09: 0000000000000000
[   64.437459] R10: 0000000000000020 R11: 000000000000000f R12: ffff9dd8c26e4b10
[   64.437860] R13: ffff9dd8c26e4b10 R14: ffff9dd8c26e4810 R15: 0000000000000000
[   64.438263] FS:  0000000000000000(0000) GS:ffff9dd9f7d80000(0000)
knlGS:0000000000000000
[   64.438706] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   64.439033] CR2: ffffffffffffffd6 CR3: 0000000103d96002 CR4: 00000000003706f0
[   64.439430] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   64.439825] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   64.440227] Call Trace:
[   64.440373]  <TASK>
[   64.440499]  ? __die_body+0x1b/0x60
[   64.440705]  ? page_fault_oops+0x158/0x4f0
[   64.440954]  ? __mod_zone_page_state+0x6e/0xb0
[   64.441236]  ? exc_page_fault+0x69/0x150
[   64.441455]  ? asm_exc_page_fault+0x22/0x30
[   64.441721]  vp_del_vqs+0x6b/0x280 [virtio_pci]
[   64.441977]  virtblk_remove+0x57/0x80 [virtio_blk]
[   64.442249]  virtio_dev_remove+0x3a/0x90 [virtio]
[   64.442516]  device_release_driver_internal+0xaa/0x140
[   64.442835]  bus_remove_device+0xbf/0x130
[   64.443062]  device_del+0x156/0x3a0
[   64.443265]  device_unregister+0x13/0x60
[   64.443497]  unregister_virtio_device+0x11/0x20 [virtio]
[   64.443806]  virtio_pci_remove+0x3d/0x80 [virtio_pci]
[   64.444112]  pci_device_remove+0x33/0xa0
[   64.444380]  device_release_driver_internal+0xaa/0x140
[   64.444676]  pci_stop_bus_device+0x6c/0x90
[   64.444914]  pci_stop_and_remove_bus_device+0xe/0x20
[   64.445192]  disable_slot+0x49/0x90
[   64.445421]  acpiphp_disable_and_eject_slot+0x15/0x90
[   64.445749]  acpiphp_hotplug_notify+0x14f/0x2a0
[   64.446017]  ? __pfx_acpiphp_hotplug_notify+0x10/0x10
[   64.446318]  acpi_device_hotplug+0xc1/0x4a0
[   64.446556]  acpi_hotplug_work_fn+0x1a/0x30
[   64.446795]  process_one_work+0x158/0x370
[   64.447023]  ? __pfx_worker_thread+0x10/0x10
[   64.447272]  process_scheduled_works+0x42/0x60
[   64.447527]  worker_thread+0x110/0x270
[   64.447739]  ? __pfx_worker_thread+0x10/0x10
[   64.447985]  kthread+0xeb/0x120
[   64.448174]  ? __pfx_kthread+0x10/0x10
[   64.448388]  ret_from_fork+0x2d/0x50
[   64.448595]  ? __pfx_kthread+0x10/0x10
[   64.448812]  ret_from_fork_asm+0x1a/0x30
[   64.449079]  </TASK>
[   64.449210] Modules linked in: ip6t_rpfilter ip6t_REJECT
nf_reject_ipv6 ipt_REJECT nf_reject_ipv4 xt_conntrack ebtable_nat
ebtable_broute ip6table_nat ip6table_mangle ip6table_security
ip6table_raw iptable_nat nf_nat iptable_mangle iptable_security
iptable_raw nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip_set
nfnetlink rfkill ebtable_filter ebtables ip6table_filter ip6_tables
iptable_filter sunrpc intel_rapl_msr intel_rapl_common
crct10dif_pclmul crc32_pclmul ghash_clmulni_intel sha512_ssse3
aesni_intel crypto_simd virtio_rng cryptd virtio_balloon sg pcspkr
joydev floppy i2c_piix4 ip_tables xfs libcrc32c sr_mod cdrom
virtio_net net_failover ata_generic failover virtio_blk virtio_console
pata_acpi virtio_pci virtio ata_piix virtio_pci_legacy_dev
virtio_pci_modern_dev crc32c_intel libata serio_raw virtio_ring
dm_mirror dm_region_hash dm_log dm_mod fuse
[   64.453398] CR2: 0000000000000000
[   64.453587] ---[ end trace 0000000000000000 ]---
[   64.453869] RIP: 0010:0x0
[   64.454018] Code: Unable to access opcode bytes at 0xffffffffffffffd6.
[   64.454405] RSP: 0018:ffffaf80003e3bd8 EFLAGS: 00010206
[   64.454719] RAX: 0000000000000000 RBX: ffff9dd8c26e4800 RCX: 0000000000000000
[   64.455109] RDX: 0000000080000000 RSI: 0000000000000000 RDI: ffff9dd8c26e4800
[   64.455523] RBP: ffff9dd8c4bdc900 R08: 0000000000000000 R09: 0000000000000000
[   64.455938] R10: 0000000000000020 R11: 000000000000000f R12: ffff9dd8c26e4b10
[   64.456330] R13: ffff9dd8c26e4b10 R14: ffff9dd8c26e4810 R15: 0000000000000000
[   64.456753] FS:  0000000000000000(0000) GS:ffff9dd9f7d80000(0000)
knlGS:0000000000000000
[   64.457222] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   64.457535] CR2: ffffffffffffffd6 CR3: 0000000103d96002 CR4: 00000000003706f0
[   64.457951] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   64.458340] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   64.458761] Kernel panic - not syncing: Fatal exception
[   64.459396] Kernel Offset: 0x22400000 from 0xffffffff81000000
(relocation range: 0xffffffff80000000-0xffffffffbfffffff)
[   64.459988] ---[ end Kernel panic - not syncing: Fatal exception ]---




But after applying the patch, everything seems to be normal.

Li Zhang <zhanglikernel@gmail.com> 于2024年3月16日周六 13:26写道：

>
> [bug]
> In the virtio_pci_common.c function vp_del_vqs, vp_dev->is_avq is involved
> to determine whether it is admin virtqueue, but this function vp_dev->is_avq
>  may be empty. For installations, virtio_pci_legacy does not assign a value
>  to vp_dev->is_avq.
>
> [fix]
> Check whether it is vp_dev->is_avq before use.
>
> [test]
> Test with virsh Attach device
> Before this patch, the following command would crash the guest system
>
> After applying the patch, everything seems to be working fine.
>
> Signed-off-by: Li Zhang <zhanglikernel@gmail.com>
> ---
>  drivers/virtio/virtio_pci_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index b655fcc..3c18fc1 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -236,7 +236,7 @@ void vp_del_vqs(struct virtio_device *vdev)
>         int i;
>
>         list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
> -               if (vp_dev->is_avq(vdev, vq->index))
> +               if (vp_dev->is_avq && vp_dev->is_avq(vdev, vq->index))
>                         continue;
>
>                 if (vp_dev->per_vq_vectors) {
> --
> 1.8.3.1
>

Re: [PATCH]virtio-pci: Check if is_avq is NULL

[Jason Wang]

On Sat, Mar 16, 2024 at 1:26 PM Li Zhang <zhanglikernel@gmail.com> wrote:
>
> [bug]
> In the virtio_pci_common.c function vp_del_vqs, vp_dev->is_avq is involved
> to determine whether it is admin virtqueue, but this function vp_dev->is_avq
>  may be empty. For installations, virtio_pci_legacy does not assign a value
>  to vp_dev->is_avq.
>
> [fix]
> Check whether it is vp_dev->is_avq before use.
>
> [test]
> Test with virsh Attach device
> Before this patch, the following command would crash the guest system
>
> After applying the patch, everything seems to be working fine.
>
> Signed-off-by: Li Zhang <zhanglikernel@gmail.com>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

> ---
>  drivers/virtio/virtio_pci_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index b655fcc..3c18fc1 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -236,7 +236,7 @@ void vp_del_vqs(struct virtio_device *vdev)
>         int i;
>
>         list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
> -               if (vp_dev->is_avq(vdev, vq->index))
> +               if (vp_dev->is_avq && vp_dev->is_avq(vdev, vq->index))
>                         continue;
>
>                 if (vp_dev->per_vq_vectors) {
> --
> 1.8.3.1
>
>

Re: [PATCH]virtio-pci: Check if is_avq is NULL

[li zhang]

Hi,
Two months have passed and this bug seems to have not been fixed.
Sincerely,
Li Zhang

Jason Wang <jasowang@redhat.com> 于2024年3月21日周四 14:19写道：
>
> On Sat, Mar 16, 2024 at 1:26 PM Li Zhang <zhanglikernel@gmail.com> wrote:
> >
> > [bug]
> > In the virtio_pci_common.c function vp_del_vqs, vp_dev->is_avq is involved
> > to determine whether it is admin virtqueue, but this function vp_dev->is_avq
> >  may be empty. For installations, virtio_pci_legacy does not assign a value
> >  to vp_dev->is_avq.
> >
> > [fix]
> > Check whether it is vp_dev->is_avq before use.
> >
> > [test]
> > Test with virsh Attach device
> > Before this patch, the following command would crash the guest system
> >
> > After applying the patch, everything seems to be working fine.
> >
> > Signed-off-by: Li Zhang <zhanglikernel@gmail.com>
>
> Acked-by: Jason Wang <jasowang@redhat.com>
>
> Thanks
>
> > ---
> >  drivers/virtio/virtio_pci_common.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > index b655fcc..3c18fc1 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -236,7 +236,7 @@ void vp_del_vqs(struct virtio_device *vdev)
> >         int i;
> >
> >         list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
> > -               if (vp_dev->is_avq(vdev, vq->index))
> > +               if (vp_dev->is_avq && vp_dev->is_avq(vdev, vq->index))
> >                         continue;
> >
> >                 if (vp_dev->per_vq_vectors) {
> > --
> > 1.8.3.1
> >
> >
>
>

Re: [PATCH]virtio-pci: Check if is_avq is NULL

[Jason Wang]

On Tue, May 14, 2024 at 11:37 PM li zhang <zhanglikernel@gmail.com> wrote:
>
> Hi,
> Two months have passed and this bug seems to have not been fixed.
> Sincerely,
> Li Zhang

Adding Michael and virtualization-list.

Please use get_maintainer.pl to make sure the patch reaches the maintainer.

Thanks

>
> Jason Wang <jasowang@redhat.com> 于2024年3月21日周四 14:19写道：
> >
> > On Sat, Mar 16, 2024 at 1:26 PM Li Zhang <zhanglikernel@gmail.com> wrote:
> > >
> > > [bug]
> > > In the virtio_pci_common.c function vp_del_vqs, vp_dev->is_avq is involved
> > > to determine whether it is admin virtqueue, but this function vp_dev->is_avq
> > >  may be empty. For installations, virtio_pci_legacy does not assign a value
> > >  to vp_dev->is_avq.
> > >
> > > [fix]
> > > Check whether it is vp_dev->is_avq before use.
> > >
> > > [test]
> > > Test with virsh Attach device
> > > Before this patch, the following command would crash the guest system
> > >
> > > After applying the patch, everything seems to be working fine.
> > >
> > > Signed-off-by: Li Zhang <zhanglikernel@gmail.com>
> >
> > Acked-by: Jason Wang <jasowang@redhat.com>
> >
> > Thanks
> >
> > > ---
> > >  drivers/virtio/virtio_pci_common.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > index b655fcc..3c18fc1 100644
> > > --- a/drivers/virtio/virtio_pci_common.c
> > > +++ b/drivers/virtio/virtio_pci_common.c
> > > @@ -236,7 +236,7 @@ void vp_del_vqs(struct virtio_device *vdev)
> > >         int i;
> > >
> > >         list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
> > > -               if (vp_dev->is_avq(vdev, vq->index))
> > > +               if (vp_dev->is_avq && vp_dev->is_avq(vdev, vq->index))
> > >                         continue;
> > >
> > >                 if (vp_dev->per_vq_vectors) {
> > > --
> > > 1.8.3.1
> > >
> > >
> >
> >
>