Skip to contentSkip to conversation

vhost_net: initialize rx_ring in vhost_net_open()

PATCH In review Alexander Potapenko 4 participants 12 messages Mar 8, 2018 – Mar 9, 2018

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 vhost_net: initialize rx_ring in vhost_net_open()

KMSAN reported a use of uninit memory in vhost_net_buf_unproduce() while trying to access n->vqs[VHOST_NET_VQ_TX].rx_ring:

==================================================================

BUG: KMSAN: use of uninitialized memory in vhost_net_buf_unproduce+0x7bb/0x9a0 drivers/vho
et.c:170
CPU: 0 PID: 3021 Comm: syz-fuzzer Not tainted 4.16.0-rc4+ #3853
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Call Trace:
 __dump_stack lib/dump_stack.c:17 [inline]
 dump_stack+0x185/0x1d0 lib/dump_stack.c:53
 kmsan_report+0x142/0x1f0 mm/kmsan/kmsan.c:1093
 __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:676
 vhost_net_buf_unproduce+0x7bb/0x9a0 drivers/vhost/net.c:170
 vhost_net_stop_vq drivers/vhost/net.c:974 [inline]
 vhost_net_stop+0x146/0x380 drivers/vhost/net.c:982
 vhost_net_release+0xb1/0x4f0 drivers/vhost/net.c:1015
 __fput+0x49f/0xa00 fs/file_table.c:209
 ____fput+0x37/0x40 fs/file_table.c:243
 task_work_run+0x243/0x2c0 kernel/task_work.c:113
 tracehook_notify_resume include/linux/tracehook.h:191 [inline]
 exit_to_usermode_loop arch/x86/entry/common.c:166 [inline]
 prepare_exit_to_usermode+0x349/0x3b0 arch/x86/entry/common.c:196
 syscall_return_slowpath+0xf3/0x6d0 arch/x86/entry/common.c:265
 do_syscall_64+0x34d/0x450 arch/x86/entry/common.c:292
...
origin:
 kmsan_save_stack_with_flags mm/kmsan/kmsan.c:303 [inline]
 kmsan_internal_poison_shadow+0xb8/0x1b0 mm/kmsan/kmsan.c:213
 kmsan_kmalloc_large+0x6f/0xd0 mm/kmsan/kmsan.c:392
 kmalloc_large_node_hook mm/slub.c:1366 [inline]
 kmalloc_large_node mm/slub.c:3808 [inline]
 __kmalloc_node+0x100e/0x1290 mm/slub.c:3818
 kmalloc_node include/linux/slab.h:554 [inline]
 kvmalloc_node+0x1a5/0x2e0 mm/util.c:419
 kvmalloc include/linux/mm.h:541 [inline]
 vhost_net_open+0x64/0x5f0 drivers/vhost/net.c:921
 misc_open+0x7b5/0x8b0 drivers/char/misc.c:154
 chrdev_open+0xc28/0xd90 fs/char_dev.c:417
 do_dentry_open+0xccb/0x1430 fs/open.c:752
 vfs_open+0x272/0x2e0 fs/open.c:866
 do_last fs/namei.c:3378 [inline]
 path_openat+0x49ad/0x6580 fs/namei.c:3519
 do_filp_open+0x267/0x640 fs/namei.c:3553
 do_sys_open+0x6ad/0x9c0 fs/open.c:1059
 SYSC_openat+0xc7/0xe0 fs/open.c:1086
 SyS_openat+0x63/0x90 fs/open.c:1080
 do_syscall_64+0x2f1/0x450 arch/x86/entry/common.c:287
==================================================================
---
 drivers/vhost/net.c | 1 +
 1 file changed, 1 insertion(+)
drivers/vhost/net.c +1 -0
index 610cba276d47..60f1080bffc7 100644
@@ -948,6 +948,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
 		n->vqs[i].done_idx = 0;
 		n->vqs[i].vhost_hlen = 0;
 		n->vqs[i].sock_hlen = 0;
+		n->vqs[i].rx_ring = NULL;
 		vhost_net_buf_init(&n->vqs[i].rxq);
 	}
 	vhost_dev_init(dev, vqs, VHOST_NET_VQ_MAX);
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#1 lore raw
Quoted 7 lines · Alexander Potapenko

KMSAN reported a use of uninit memory in vhost_net_buf_unproduce() while trying to access n->vqs[VHOST_NET_VQ_TX].rx_ring:

==================================================================

Please identify bug origin with a Fixes: tag

This will tremendously help stable teams that are struggling with many backports these days.

Thanks.

Quoted 17 lines
---
  drivers/vhost/net.c | 1 +
  1 file changed, 1 insertion(+)
drivers/vhost/net.c +1 -0
index 610cba276d47..60f1080bffc7 100644
@@ -948,6 +948,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
  		n->vqs[i].done_idx = 0;
  		n->vqs[i].vhost_hlen = 0;
  		n->vqs[i].sock_hlen = 0;
+		n->vqs[i].rx_ring = NULL;
  		vhost_net_buf_init(&n->vqs[i].rxq);
  	}
  	vhost_dev_init(dev, vqs, VHOST_NET_VQ_MAX);
#2 lore raw
Quoted 8 lines · Eric Dumazet
Quoted 5 lines · Alexander Potapenko

KMSAN reported a use of uninit memory in vhost_net_buf_unproduce() while trying to access n->vqs[VHOST_NET_VQ_TX].rx_ring:

Quoted 6 lines
Quoted 3 lines

==================================================================

Please identify bug origin with a Fixes: tag

Fixes 5990a30510ed1 ("tun/tap: use ptr_ring instead of skb_array")

The above patch introduced rx_ring, but the problem existed before.

Quoted 25 lines

This will tremendously help stable teams that are struggling with many backports these days.

Thanks.

Quoted 18 lines
---
  drivers/vhost/net.c | 1 +
  1 file changed, 1 insertion(+)
drivers/vhost/net.c +1 -0
index 610cba276d47..60f1080bffc7 100644
@@ -948,6 +948,7 @@ static int vhost_net_open(struct inode *inode, struct
 file *f)
                n->vqs[i].done_idx = 0;
                n->vqs[i].vhost_hlen = 0;
                n->vqs[i].sock_hlen = 0;
+               n->vqs[i].rx_ring = NULL;
                vhost_net_buf_init(&n->vqs[i].rxq);
        }
        vhost_dev_init(dev, vqs, VHOST_NET_VQ_MAX);
Signature · 10 lines
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
#3 lore raw
Quoted 9 lines · Alexander Potapenko
Quoted 8 lines · Eric Dumazet
Quoted 5 lines · Alexander Potapenko

KMSAN reported a use of uninit memory in vhost_net_buf_unproduce() while trying to access n->vqs[VHOST_NET_VQ_TX].rx_ring:

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

==================================================================

Please identify bug origin with a Fixes: tag

Fixes 5990a30510ed1 ("tun/tap: use ptr_ring instead of skb_array")

Please send a V2 with this added tag. patchwork does not recognize it yet.

David Miller has also a lot on his plate, please everybody be gentle with both stable teams and maintainers.

Thanks.

Quoted 31 lines

The above patch introduced rx_ring, but the problem existed before.

Quoted 25 lines

This will tremendously help stable teams that are struggling with many backports these days.

Thanks.

Quoted 18 lines
---
   drivers/vhost/net.c | 1 +
   1 file changed, 1 insertion(+)
drivers/vhost/net.c +1 -0
index 610cba276d47..60f1080bffc7 100644
@@ -948,6 +948,7 @@ static int vhost_net_open(struct inode *inode, struct
 file *f)
                 n->vqs[i].done_idx = 0;
                 n->vqs[i].vhost_hlen = 0;
                 n->vqs[i].sock_hlen = 0;
+               n->vqs[i].rx_ring = NULL;
                 vhost_net_buf_init(&n->vqs[i].rxq);
         }
         vhost_dev_init(dev, vqs, VHOST_NET_VQ_MAX);
#4 lore raw
Quoted 22 lines · Eric Dumazet
Quoted 19 lines · Alexander Potapenko
Quoted 16 lines · Eric Dumazet
Quoted 9 lines · Alexander Potapenko

KMSAN reported a use of uninit memory in vhost_net_buf_unproduce() while trying to access n->vqs[VHOST_NET_VQ_TX].rx_ring:

==================================================================

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

Please identify bug origin with a Fixes: tag

Fixes 5990a30510ed1 ("tun/tap: use ptr_ring instead of skb_array")

Please send a V2 with this added tag. patchwork does not recognize it yet.

Ok, will do. Thanks for reminding about the tag!

Quoted 40 lines

David Miller has also a lot on his plate, please everybody be gentle with both stable teams and maintainers.

Thanks.

Quoted 32 lines

The above patch introduced rx_ring, but the problem existed before.

Quoted 26 lines

This will tremendously help stable teams that are struggling with many backports these days.

Thanks.

Quoted 19 lines
---
   drivers/vhost/net.c | 1 +
   1 file changed, 1 insertion(+)
drivers/vhost/net.c +0 -0
index 610cba276d47..60f1080bffc7 100644
@@ -948,6 +948,7 @@ static int vhost_net_open(struct inode *inode,

struct
file *f)

                 n->vqs[i].done_idx = 0;
                 n->vqs[i].vhost_hlen = 0;
                 n->vqs[i].sock_hlen = 0;
+               n->vqs[i].rx_ring = NULL;
                 vhost_net_buf_init(&n->vqs[i].rxq);
         }
         vhost_dev_init(dev, vqs, VHOST_NET_VQ_MAX);
Signature · 10 lines
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
#5 lore raw
Quoted 10 lines · Alexander Potapenko
Quoted 9 lines · Eric Dumazet
Quoted 8 lines · Alexander Potapenko
Quoted 6 lines · Eric Dumazet
Quoted 4 lines · Alexander Potapenko

KMSAN reported a use of uninit memory in vhost_net_buf_unproduce() while trying to access n->vqs[VHOST_NET_VQ_TX].rx_ring:

Quoted 8 lines
Quoted 7 lines
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines

==================================================================

Please identify bug origin with a Fixes: tag

Fixes 5990a30510ed1 ("tun/tap: use ptr_ring instead of skb_array")

Please send a V2 with this added tag. patchwork does not recognize it yet.

Ok, will do. Thanks for reminding about the tag!

The commit should be c67df11f6e480 ("vhost_net: try batch dequing from skb array").

Let me squash this patch into my series.

Thanks

#6 lore raw
In reply to Alexander Potapenko
Quoted 69 lines · Alexander Potapenko

KMSAN reported a use of uninit memory in vhost_net_buf_unproduce() while trying to access n->vqs[VHOST_NET_VQ_TX].rx_ring:

==================================================================

BUG: KMSAN: use of uninitialized memory in vhost_net_buf_unproduce+0x7bb/0x9a0 drivers/vho
et.c:170
CPU: 0 PID: 3021 Comm: syz-fuzzer Not tainted 4.16.0-rc4+ #3853
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Call Trace:
 __dump_stack lib/dump_stack.c:17 [inline]
 dump_stack+0x185/0x1d0 lib/dump_stack.c:53
 kmsan_report+0x142/0x1f0 mm/kmsan/kmsan.c:1093
 __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:676
 vhost_net_buf_unproduce+0x7bb/0x9a0 drivers/vhost/net.c:170
 vhost_net_stop_vq drivers/vhost/net.c:974 [inline]
 vhost_net_stop+0x146/0x380 drivers/vhost/net.c:982
 vhost_net_release+0xb1/0x4f0 drivers/vhost/net.c:1015
 __fput+0x49f/0xa00 fs/file_table.c:209
 ____fput+0x37/0x40 fs/file_table.c:243
 task_work_run+0x243/0x2c0 kernel/task_work.c:113
 tracehook_notify_resume include/linux/tracehook.h:191 [inline]
 exit_to_usermode_loop arch/x86/entry/common.c:166 [inline]
 prepare_exit_to_usermode+0x349/0x3b0 arch/x86/entry/common.c:196
 syscall_return_slowpath+0xf3/0x6d0 arch/x86/entry/common.c:265
 do_syscall_64+0x34d/0x450 arch/x86/entry/common.c:292
...
origin:
 kmsan_save_stack_with_flags mm/kmsan/kmsan.c:303 [inline]
 kmsan_internal_poison_shadow+0xb8/0x1b0 mm/kmsan/kmsan.c:213
 kmsan_kmalloc_large+0x6f/0xd0 mm/kmsan/kmsan.c:392
 kmalloc_large_node_hook mm/slub.c:1366 [inline]
 kmalloc_large_node mm/slub.c:3808 [inline]
 __kmalloc_node+0x100e/0x1290 mm/slub.c:3818
 kmalloc_node include/linux/slab.h:554 [inline]
 kvmalloc_node+0x1a5/0x2e0 mm/util.c:419
 kvmalloc include/linux/mm.h:541 [inline]
 vhost_net_open+0x64/0x5f0 drivers/vhost/net.c:921
 misc_open+0x7b5/0x8b0 drivers/char/misc.c:154
 chrdev_open+0xc28/0xd90 fs/char_dev.c:417
 do_dentry_open+0xccb/0x1430 fs/open.c:752
 vfs_open+0x272/0x2e0 fs/open.c:866
 do_last fs/namei.c:3378 [inline]
 path_openat+0x49ad/0x6580 fs/namei.c:3519
 do_filp_open+0x267/0x640 fs/namei.c:3553
 do_sys_open+0x6ad/0x9c0 fs/open.c:1059
 SYSC_openat+0xc7/0xe0 fs/open.c:1086
 SyS_openat+0x63/0x90 fs/open.c:1080
 do_syscall_64+0x2f1/0x450 arch/x86/entry/common.c:287
==================================================================
---
 drivers/vhost/net.c | 1 +
 1 file changed, 1 insertion(+)
drivers/vhost/net.c +1 -0
index 610cba276d47..60f1080bffc7 100644
@@ -948,6 +948,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
 		n->vqs[i].done_idx = 0;
 		n->vqs[i].vhost_hlen = 0;
 		n->vqs[i].sock_hlen = 0;
+		n->vqs[i].rx_ring = NULL;
 		vhost_net_buf_init(&n->vqs[i].rxq);
 	}
 	vhost_dev_init(dev, vqs, VHOST_NET_VQ_MAX);
Signature · 1 line
2.16.2.395.g2e18187dfd-goog

I suspect that's not sufficient. rx ring is tied to the tap device. I think we need to drop it every time we drop the device.

#7 lore raw
Quoted 66 lines · Michael S. Tsirkin
Quoted 65 lines · Alexander Potapenko

KMSAN reported a use of uninit memory in vhost_net_buf_unproduce() while trying to access n->vqs[VHOST_NET_VQ_TX].rx_ring:

==================================================================

BUG: KMSAN: use of uninitialized memory in vhost_net_buf_unproduce+0x7bb/0x9a0 drivers/vho
et.c:170
CPU: 0 PID: 3021 Comm: syz-fuzzer Not tainted 4.16.0-rc4+ #3853
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Call Trace:
 __dump_stack lib/dump_stack.c:17 [inline]
 dump_stack+0x185/0x1d0 lib/dump_stack.c:53
 kmsan_report+0x142/0x1f0 mm/kmsan/kmsan.c:1093
 __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:676
 vhost_net_buf_unproduce+0x7bb/0x9a0 drivers/vhost/net.c:170
 vhost_net_stop_vq drivers/vhost/net.c:974 [inline]
 vhost_net_stop+0x146/0x380 drivers/vhost/net.c:982
 vhost_net_release+0xb1/0x4f0 drivers/vhost/net.c:1015
 __fput+0x49f/0xa00 fs/file_table.c:209
 ____fput+0x37/0x40 fs/file_table.c:243
 task_work_run+0x243/0x2c0 kernel/task_work.c:113
 tracehook_notify_resume include/linux/tracehook.h:191 [inline]
 exit_to_usermode_loop arch/x86/entry/common.c:166 [inline]
 prepare_exit_to_usermode+0x349/0x3b0 arch/x86/entry/common.c:196
 syscall_return_slowpath+0xf3/0x6d0 arch/x86/entry/common.c:265
 do_syscall_64+0x34d/0x450 arch/x86/entry/common.c:292
...
origin:
 kmsan_save_stack_with_flags mm/kmsan/kmsan.c:303 [inline]
 kmsan_internal_poison_shadow+0xb8/0x1b0 mm/kmsan/kmsan.c:213
 kmsan_kmalloc_large+0x6f/0xd0 mm/kmsan/kmsan.c:392
 kmalloc_large_node_hook mm/slub.c:1366 [inline]
 kmalloc_large_node mm/slub.c:3808 [inline]
 __kmalloc_node+0x100e/0x1290 mm/slub.c:3818
 kmalloc_node include/linux/slab.h:554 [inline]
 kvmalloc_node+0x1a5/0x2e0 mm/util.c:419
 kvmalloc include/linux/mm.h:541 [inline]
 vhost_net_open+0x64/0x5f0 drivers/vhost/net.c:921
 misc_open+0x7b5/0x8b0 drivers/char/misc.c:154
 chrdev_open+0xc28/0xd90 fs/char_dev.c:417
 do_dentry_open+0xccb/0x1430 fs/open.c:752
 vfs_open+0x272/0x2e0 fs/open.c:866
 do_last fs/namei.c:3378 [inline]
 path_openat+0x49ad/0x6580 fs/namei.c:3519
 do_filp_open+0x267/0x640 fs/namei.c:3553
 do_sys_open+0x6ad/0x9c0 fs/open.c:1059
 SYSC_openat+0xc7/0xe0 fs/open.c:1086
 SyS_openat+0x63/0x90 fs/open.c:1080
 do_syscall_64+0x2f1/0x450 arch/x86/entry/common.c:287
==================================================================
---
 drivers/vhost/net.c | 1 +
 1 file changed, 1 insertion(+)
drivers/vhost/net.c +1 -0
index 610cba276d47..60f1080bffc7 100644
@@ -948,6 +948,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
              n->vqs[i].done_idx = 0;
              n->vqs[i].vhost_hlen = 0;
              n->vqs[i].sock_hlen = 0;
+             n->vqs[i].rx_ring = NULL;
              vhost_net_buf_init(&n->vqs[i].rxq);
Quoted 8 lines
Quoted 4 lines
drivers/vhost/net.c +0 -1
      }
      vhost_dev_init(dev, vqs, VHOST_NET_VQ_MAX);
--

2.16.2.395.g2e18187dfd-goog

I suspect that's not sufficient. rx ring is tied to the tap device. I think we need to drop it every time we drop the device.

Unfortunately I've no idea where is the device dropped. Are you referring to vhost_net_vq_reset()?
I can fix that part if needed, but won't be able to validate it with KMSAN.

Signature · 10 lines
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
#8 lore raw
Quoted 76 lines · Alexander Potapenko
Quoted 66 lines · Michael S. Tsirkin
Quoted 65 lines · Alexander Potapenko

KMSAN reported a use of uninit memory in vhost_net_buf_unproduce() while trying to access n->vqs[VHOST_NET_VQ_TX].rx_ring:

==================================================================

BUG: KMSAN: use of uninitialized memory in vhost_net_buf_unproduce+0x7bb/0x9a0 drivers/vho
et.c:170
CPU: 0 PID: 3021 Comm: syz-fuzzer Not tainted 4.16.0-rc4+ #3853
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Call Trace:
 __dump_stack lib/dump_stack.c:17 [inline]
 dump_stack+0x185/0x1d0 lib/dump_stack.c:53
 kmsan_report+0x142/0x1f0 mm/kmsan/kmsan.c:1093
 __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:676
 vhost_net_buf_unproduce+0x7bb/0x9a0 drivers/vhost/net.c:170
 vhost_net_stop_vq drivers/vhost/net.c:974 [inline]
 vhost_net_stop+0x146/0x380 drivers/vhost/net.c:982
 vhost_net_release+0xb1/0x4f0 drivers/vhost/net.c:1015
 __fput+0x49f/0xa00 fs/file_table.c:209
 ____fput+0x37/0x40 fs/file_table.c:243
 task_work_run+0x243/0x2c0 kernel/task_work.c:113
 tracehook_notify_resume include/linux/tracehook.h:191 [inline]
 exit_to_usermode_loop arch/x86/entry/common.c:166 [inline]
 prepare_exit_to_usermode+0x349/0x3b0 arch/x86/entry/common.c:196
 syscall_return_slowpath+0xf3/0x6d0 arch/x86/entry/common.c:265
 do_syscall_64+0x34d/0x450 arch/x86/entry/common.c:292
...
origin:
 kmsan_save_stack_with_flags mm/kmsan/kmsan.c:303 [inline]
 kmsan_internal_poison_shadow+0xb8/0x1b0 mm/kmsan/kmsan.c:213
 kmsan_kmalloc_large+0x6f/0xd0 mm/kmsan/kmsan.c:392
 kmalloc_large_node_hook mm/slub.c:1366 [inline]
 kmalloc_large_node mm/slub.c:3808 [inline]
 __kmalloc_node+0x100e/0x1290 mm/slub.c:3818
 kmalloc_node include/linux/slab.h:554 [inline]
 kvmalloc_node+0x1a5/0x2e0 mm/util.c:419
 kvmalloc include/linux/mm.h:541 [inline]
 vhost_net_open+0x64/0x5f0 drivers/vhost/net.c:921
 misc_open+0x7b5/0x8b0 drivers/char/misc.c:154
 chrdev_open+0xc28/0xd90 fs/char_dev.c:417
 do_dentry_open+0xccb/0x1430 fs/open.c:752
 vfs_open+0x272/0x2e0 fs/open.c:866
 do_last fs/namei.c:3378 [inline]
 path_openat+0x49ad/0x6580 fs/namei.c:3519
 do_filp_open+0x267/0x640 fs/namei.c:3553
 do_sys_open+0x6ad/0x9c0 fs/open.c:1059
 SYSC_openat+0xc7/0xe0 fs/open.c:1086
 SyS_openat+0x63/0x90 fs/open.c:1080
 do_syscall_64+0x2f1/0x450 arch/x86/entry/common.c:287
==================================================================
---
 drivers/vhost/net.c | 1 +
 1 file changed, 1 insertion(+)
drivers/vhost/net.c +1 -0
index 610cba276d47..60f1080bffc7 100644
@@ -948,6 +948,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
              n->vqs[i].done_idx = 0;
              n->vqs[i].vhost_hlen = 0;
              n->vqs[i].sock_hlen = 0;
+             n->vqs[i].rx_ring = NULL;
              vhost_net_buf_init(&n->vqs[i].rxq);
Quoted 8 lines
Quoted 4 lines
drivers/vhost/net.c +0 -1
      }
      vhost_dev_init(dev, vqs, VHOST_NET_VQ_MAX);
--

2.16.2.395.g2e18187dfd-goog

I suspect that's not sufficient. rx ring is tied to the tap device. I think we need to drop it every time we drop the device.

Quoted 3 lines

Unfortunately I've no idea where is the device dropped. Are you referring to vhost_net_vq_reset()?
I can fix that part if needed, but won't be able to validate it with KMSAN.

I see several issues. For example in vhost_net_set_backend if there's a value then rx ring will point to the ring of the wrong socket.
Something like the below might help but we really need documentation of when is rx_ring valid. Is it only valid when private-data is valid? If yes need to make sure we reset it with private_data.

Also I see __skb_array_destroy_skb used with ptr_ring which seems suspicious: how do we know the entries are skbs?

Patch below is on top of yours, and

But I really would like Jason to look and come up with a patch to address all these issues.

---

drivers/vhost/net.c +5 -2
index 610cba2..7a65b69 100644
@@ -972,6 +973,7 @@ static struct socket *vhost_net_stop_vq(struct vhost_net *n,
 	vhost_net_disable_vq(n, vq);
 	vq->private_data = NULL;
 	vhost_net_buf_unproduce(nvq);
+	vq->rx_ring = NULL;
 	mutex_unlock(&vq->mutex);
 	return sock;
 }
@@ -1161,8 +1163,6 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
 		vhost_net_disable_vq(n, vq);
 		vq->private_data = sock;
 		vhost_net_buf_unproduce(nvq);
-		if (index == VHOST_NET_VQ_RX)
-			nvq->rx_ring = get_tap_ptr_ring(fd);
 		r = vhost_vq_init_access(vq);
 		if (r)
 			goto err_used;
@@ -1172,6 +1172,10 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
 
 		oldubufs = nvq->ubufs;
 		nvq->ubufs = ubufs;
+		if (index == VHOST_NET_VQ_RX)
+			nvq->rx_ring = get_tap_ptr_ring(fd);
+		else
+			nvq->rx_ring = NULL;
 
 		n->tx_packets = 0;
 		n->tx_zcopy_err = 0;
#9 lore raw
Quoted 78 lines · Michael S. Tsirkin
Quoted 74 lines · Alexander Potapenko
Quoted 65 lines · Michael S. Tsirkin
Quoted 64 lines · Alexander Potapenko

KMSAN reported a use of uninit memory in vhost_net_buf_unproduce() while trying to access n->vqs[VHOST_NET_VQ_TX].rx_ring:

==================================================================

BUG: KMSAN: use of uninitialized memory in vhost_net_buf_unproduce+0x7bb/0x9a0 drivers/vho
et.c:170
CPU: 0 PID: 3021 Comm: syz-fuzzer Not tainted 4.16.0-rc4+ #3853
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Call Trace:
  __dump_stack lib/dump_stack.c:17 [inline]
  dump_stack+0x185/0x1d0 lib/dump_stack.c:53
  kmsan_report+0x142/0x1f0 mm/kmsan/kmsan.c:1093
  __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:676
  vhost_net_buf_unproduce+0x7bb/0x9a0 drivers/vhost/net.c:170
  vhost_net_stop_vq drivers/vhost/net.c:974 [inline]
  vhost_net_stop+0x146/0x380 drivers/vhost/net.c:982
  vhost_net_release+0xb1/0x4f0 drivers/vhost/net.c:1015
  __fput+0x49f/0xa00 fs/file_table.c:209
  ____fput+0x37/0x40 fs/file_table.c:243
  task_work_run+0x243/0x2c0 kernel/task_work.c:113
  tracehook_notify_resume include/linux/tracehook.h:191 [inline]
  exit_to_usermode_loop arch/x86/entry/common.c:166 [inline]
  prepare_exit_to_usermode+0x349/0x3b0 arch/x86/entry/common.c:196
  syscall_return_slowpath+0xf3/0x6d0 arch/x86/entry/common.c:265
  do_syscall_64+0x34d/0x450 arch/x86/entry/common.c:292
...
origin:
  kmsan_save_stack_with_flags mm/kmsan/kmsan.c:303 [inline]
  kmsan_internal_poison_shadow+0xb8/0x1b0 mm/kmsan/kmsan.c:213
  kmsan_kmalloc_large+0x6f/0xd0 mm/kmsan/kmsan.c:392
  kmalloc_large_node_hook mm/slub.c:1366 [inline]
  kmalloc_large_node mm/slub.c:3808 [inline]
  __kmalloc_node+0x100e/0x1290 mm/slub.c:3818
  kmalloc_node include/linux/slab.h:554 [inline]
  kvmalloc_node+0x1a5/0x2e0 mm/util.c:419
  kvmalloc include/linux/mm.h:541 [inline]
  vhost_net_open+0x64/0x5f0 drivers/vhost/net.c:921
  misc_open+0x7b5/0x8b0 drivers/char/misc.c:154
  chrdev_open+0xc28/0xd90 fs/char_dev.c:417
  do_dentry_open+0xccb/0x1430 fs/open.c:752
  vfs_open+0x272/0x2e0 fs/open.c:866
  do_last fs/namei.c:3378 [inline]
  path_openat+0x49ad/0x6580 fs/namei.c:3519
  do_filp_open+0x267/0x640 fs/namei.c:3553
  do_sys_open+0x6ad/0x9c0 fs/open.c:1059
  SYSC_openat+0xc7/0xe0 fs/open.c:1086
  SyS_openat+0x63/0x90 fs/open.c:1080
  do_syscall_64+0x2f1/0x450 arch/x86/entry/common.c:287
==================================================================
---
  drivers/vhost/net.c | 1 +
  1 file changed, 1 insertion(+)
drivers/vhost/net.c +1 -0
index 610cba276d47..60f1080bffc7 100644
@@ -948,6 +948,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
               n->vqs[i].done_idx = 0;
               n->vqs[i].vhost_hlen = 0;
               n->vqs[i].sock_hlen = 0;
+             n->vqs[i].rx_ring = NULL;
Quoted 8 lines
Quoted 5 lines
drivers/vhost/net.c +0 -1
               vhost_net_buf_init(&n->vqs[i].rxq);
       }
       vhost_dev_init(dev, vqs, VHOST_NET_VQ_MAX);
--

2.16.2.395.g2e18187dfd-goog

I suspect that's not sufficient. rx ring is tied to the tap device. I think we need to drop it every time we drop the device.

Quoted 3 lines

Unfortunately I've no idea where is the device dropped. Are you referring to vhost_net_vq_reset()?
I can fix that part if needed, but won't be able to validate it with KMSAN.

Quoted 6 lines

I see several issues. For example in vhost_net_set_backend if there's a value then rx ring will point to the ring of the wrong socket.
Something like the below might help but we really need documentation of when is rx_ring valid. Is it only valid when private-data is valid?

I think so, we need keep rx_ring synced with private_data.

Quoted 5 lines

If yes need to make sure
we reset it with private_data.

Also I see __skb_array_destroy_skb used with ptr_ring which seems suspicious: how do we know the entries are skbs?

Good catch, will post an independent patch to fix this.

Quoted 32 lines

Patch below is on top of yours, and

But I really would like Jason to look and come up with a patch to address all these issues.

---

drivers/vhost/net.c +1 -2
index 610cba2..7a65b69 100644
@@ -972,6 +973,7 @@ static struct socket *vhost_net_stop_vq(struct vhost_net *n,
  	vhost_net_disable_vq(n, vq);
  	vq->private_data = NULL;
  	vhost_net_buf_unproduce(nvq);
+	vq->rx_ring = NULL;
  	mutex_unlock(&vq->mutex);
  	return sock;
  }
@@ -1161,8 +1163,6 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
  		vhost_net_disable_vq(n, vq);
  		vq->private_data = sock;
  		vhost_net_buf_unproduce(nvq);
-		if (index == VHOST_NET_VQ_RX)
-			nvq->rx_ring = get_tap_ptr_ring(fd);
  		r = vhost_vq_init_access(vq);
  		if (r)
  			goto err_used;

drivers/vhost/net.c static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd) · line 1179

Quoted 8 lines
drivers/vhost/net.c +4 -0
@@ -1172,6 +1172,10 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
 
  		oldubufs = nvq->ubufs;
  		nvq->ubufs = ubufs;
+		if (index == VHOST_NET_VQ_RX)
+			nvq->rx_ring = get_tap_ptr_ring(fd);
+		else
+			nvq->rx_ring = NULL;
 

Any reason to move those after vhost_net_enable_vq()? And consider we won't try to assign rx_ring to TX, the "else" part seems unnecessary.

Thanks

Quoted 2 lines
drivers/vhost/net.c +0 -0
  		n->tx_packets = 0;
  		n->tx_zcopy_err = 0;
#10 lore raw
Quoted 130 lines · Jason Wang
Quoted 78 lines · Michael S. Tsirkin
Quoted 74 lines · Alexander Potapenko
Quoted 65 lines · Michael S. Tsirkin
Quoted 64 lines · Alexander Potapenko

KMSAN reported a use of uninit memory in vhost_net_buf_unproduce() while trying to access n->vqs[VHOST_NET_VQ_TX].rx_ring:

==================================================================

BUG: KMSAN: use of uninitialized memory in vhost_net_buf_unproduce+0x7bb/0x9a0 drivers/vho
et.c:170
CPU: 0 PID: 3021 Comm: syz-fuzzer Not tainted 4.16.0-rc4+ #3853
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Call Trace:
  __dump_stack lib/dump_stack.c:17 [inline]
  dump_stack+0x185/0x1d0 lib/dump_stack.c:53
  kmsan_report+0x142/0x1f0 mm/kmsan/kmsan.c:1093
  __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:676
  vhost_net_buf_unproduce+0x7bb/0x9a0 drivers/vhost/net.c:170
  vhost_net_stop_vq drivers/vhost/net.c:974 [inline]
  vhost_net_stop+0x146/0x380 drivers/vhost/net.c:982
  vhost_net_release+0xb1/0x4f0 drivers/vhost/net.c:1015
  __fput+0x49f/0xa00 fs/file_table.c:209
  ____fput+0x37/0x40 fs/file_table.c:243
  task_work_run+0x243/0x2c0 kernel/task_work.c:113
  tracehook_notify_resume include/linux/tracehook.h:191 [inline]
  exit_to_usermode_loop arch/x86/entry/common.c:166 [inline]
  prepare_exit_to_usermode+0x349/0x3b0 arch/x86/entry/common.c:196
  syscall_return_slowpath+0xf3/0x6d0 arch/x86/entry/common.c:265
  do_syscall_64+0x34d/0x450 arch/x86/entry/common.c:292
...
origin:
  kmsan_save_stack_with_flags mm/kmsan/kmsan.c:303 [inline]
  kmsan_internal_poison_shadow+0xb8/0x1b0 mm/kmsan/kmsan.c:213
  kmsan_kmalloc_large+0x6f/0xd0 mm/kmsan/kmsan.c:392
  kmalloc_large_node_hook mm/slub.c:1366 [inline]
  kmalloc_large_node mm/slub.c:3808 [inline]
  __kmalloc_node+0x100e/0x1290 mm/slub.c:3818
  kmalloc_node include/linux/slab.h:554 [inline]
  kvmalloc_node+0x1a5/0x2e0 mm/util.c:419
  kvmalloc include/linux/mm.h:541 [inline]
  vhost_net_open+0x64/0x5f0 drivers/vhost/net.c:921
  misc_open+0x7b5/0x8b0 drivers/char/misc.c:154
  chrdev_open+0xc28/0xd90 fs/char_dev.c:417
  do_dentry_open+0xccb/0x1430 fs/open.c:752
  vfs_open+0x272/0x2e0 fs/open.c:866
  do_last fs/namei.c:3378 [inline]
  path_openat+0x49ad/0x6580 fs/namei.c:3519
  do_filp_open+0x267/0x640 fs/namei.c:3553
  do_sys_open+0x6ad/0x9c0 fs/open.c:1059
  SYSC_openat+0xc7/0xe0 fs/open.c:1086
  SyS_openat+0x63/0x90 fs/open.c:1080
  do_syscall_64+0x2f1/0x450 arch/x86/entry/common.c:287
==================================================================
---
  drivers/vhost/net.c | 1 +
  1 file changed, 1 insertion(+)
drivers/vhost/net.c +1 -0
index 610cba276d47..60f1080bffc7 100644
@@ -948,6 +948,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
               n->vqs[i].done_idx = 0;
               n->vqs[i].vhost_hlen = 0;
               n->vqs[i].sock_hlen = 0;
+             n->vqs[i].rx_ring = NULL;
Quoted 8 lines
Quoted 5 lines
drivers/vhost/net.c +0 -1
               vhost_net_buf_init(&n->vqs[i].rxq);
       }
       vhost_dev_init(dev, vqs, VHOST_NET_VQ_MAX);
--

2.16.2.395.g2e18187dfd-goog

I suspect that's not sufficient. rx ring is tied to the tap device. I think we need to drop it every time we drop the device.

Quoted 3 lines

Unfortunately I've no idea where is the device dropped. Are you referring to vhost_net_vq_reset()?
I can fix that part if needed, but won't be able to validate it with KMSAN.

Quoted 6 lines

I see several issues. For example in vhost_net_set_backend if there's a value then rx ring will point to the ring of the wrong socket.
Something like the below might help but we really need documentation of when is rx_ring valid. Is it only valid when private-data is valid?

I think so, we need keep rx_ring synced with private_data.

Quoted 5 lines

If yes need to make sure
we reset it with private_data.

Also I see __skb_array_destroy_skb used with ptr_ring which seems suspicious: how do we know the entries are skbs?

Good catch, will post an independent patch to fix this.

Quoted 32 lines

Patch below is on top of yours, and

But I really would like Jason to look and come up with a patch to address all these issues.

---

drivers/vhost/net.c +1 -2
index 610cba2..7a65b69 100644
@@ -972,6 +973,7 @@ static struct socket *vhost_net_stop_vq(struct vhost_net *n,
  	vhost_net_disable_vq(n, vq);
  	vq->private_data = NULL;
  	vhost_net_buf_unproduce(nvq);
+	vq->rx_ring = NULL;
  	mutex_unlock(&vq->mutex);
  	return sock;
  }
@@ -1161,8 +1163,6 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
  		vhost_net_disable_vq(n, vq);
  		vq->private_data = sock;
  		vhost_net_buf_unproduce(nvq);
-		if (index == VHOST_NET_VQ_RX)
-			nvq->rx_ring = get_tap_ptr_ring(fd);
  		r = vhost_vq_init_access(vq);
  		if (r)
  			goto err_used;
@@ -1172,6 +1172,10 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
Quoted 8 lines

drivers/vhost/net.c static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd) · line 1178

Quoted 6 lines
drivers/vhost/net.c +4 -0
  		oldubufs = nvq->ubufs;
  		nvq->ubufs = ubufs;
+		if (index == VHOST_NET_VQ_RX)
+			nvq->rx_ring = get_tap_ptr_ring(fd);
+		else
+			nvq->rx_ring = NULL;

Any reason to move those after vhost_net_enable_vq()?

Otherwise I see an issue if there is an error and
we revert the change.

Quoted 4 lines

And consider we won't
try to assign rx_ring to TX, the "else" part seems unnecessary.

Thanks

ok, pls pack up all fixes as you see fit and post
a patchset.

Quoted 2 lines
Quoted 2 lines
drivers/vhost/net.c +0 -0
  		n->tx_packets = 0;
  		n->tx_zcopy_err = 0;
#11 lore raw
Quoted 133 lines · Michael S. Tsirkin
Quoted 124 lines · Jason Wang
Quoted 77 lines · Michael S. Tsirkin
Quoted 73 lines · Alexander Potapenko
Quoted 56 lines · Michael S. Tsirkin
Quoted 55 lines · Alexander Potapenko

KMSAN reported a use of uninit memory in vhost_net_buf_unproduce() while trying to access n->vqs[VHOST_NET_VQ_TX].rx_ring:

==================================================================

BUG: KMSAN: use of uninitialized memory in vhost_net_buf_unproduce+0x7bb/0x9a0 drivers/vho
et.c:170
CPU: 0 PID: 3021 Comm: syz-fuzzer Not tainted 4.16.0-rc4+ #3853
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Call Trace:
   __dump_stack lib/dump_stack.c:17 [inline]
   dump_stack+0x185/0x1d0 lib/dump_stack.c:53
   kmsan_report+0x142/0x1f0 mm/kmsan/kmsan.c:1093
   __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:676
   vhost_net_buf_unproduce+0x7bb/0x9a0 drivers/vhost/net.c:170
   vhost_net_stop_vq drivers/vhost/net.c:974 [inline]
   vhost_net_stop+0x146/0x380 drivers/vhost/net.c:982
   vhost_net_release+0xb1/0x4f0 drivers/vhost/net.c:1015
   __fput+0x49f/0xa00 fs/file_table.c:209
   ____fput+0x37/0x40 fs/file_table.c:243
   task_work_run+0x243/0x2c0 kernel/task_work.c:113
   tracehook_notify_resume include/linux/tracehook.h:191 [inline]
   exit_to_usermode_loop arch/x86/entry/common.c:166 [inline]
   prepare_exit_to_usermode+0x349/0x3b0 arch/x86/entry/common.c:196
   syscall_return_slowpath+0xf3/0x6d0 arch/x86/entry/common.c:265
   do_syscall_64+0x34d/0x450 arch/x86/entry/common.c:292
...
origin:
   kmsan_save_stack_with_flags mm/kmsan/kmsan.c:303 [inline]
   kmsan_internal_poison_shadow+0xb8/0x1b0 mm/kmsan/kmsan.c:213
   kmsan_kmalloc_large+0x6f/0xd0 mm/kmsan/kmsan.c:392
   kmalloc_large_node_hook mm/slub.c:1366 [inline]
   kmalloc_large_node mm/slub.c:3808 [inline]
   __kmalloc_node+0x100e/0x1290 mm/slub.c:3818
   kmalloc_node include/linux/slab.h:554 [inline]
   kvmalloc_node+0x1a5/0x2e0 mm/util.c:419
   kvmalloc include/linux/mm.h:541 [inline]
   vhost_net_open+0x64/0x5f0 drivers/vhost/net.c:921
   misc_open+0x7b5/0x8b0 drivers/char/misc.c:154
   chrdev_open+0xc28/0xd90 fs/char_dev.c:417
   do_dentry_open+0xccb/0x1430 fs/open.c:752
   vfs_open+0x272/0x2e0 fs/open.c:866
   do_last fs/namei.c:3378 [inline]
   path_openat+0x49ad/0x6580 fs/namei.c:3519
   do_filp_open+0x267/0x640 fs/namei.c:3553
   do_sys_open+0x6ad/0x9c0 fs/open.c:1059
   SYSC_openat+0xc7/0xe0 fs/open.c:1086
   SyS_openat+0x63/0x90 fs/open.c:1080
   do_syscall_64+0x2f1/0x450 arch/x86/entry/common.c:287
==================================================================
---
   drivers/vhost/net.c | 1 +
   1 file changed, 1 insertion(+)
Quoted 16 lines
Quoted 13 lines
drivers/vhost/net.c +1 -1
index 610cba276d47..60f1080bffc7 100644
@@ -948,6 +948,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
                n->vqs[i].done_idx = 0;
                n->vqs[i].vhost_hlen = 0;
                n->vqs[i].sock_hlen = 0;
+             n->vqs[i].rx_ring = NULL;
                vhost_net_buf_init(&n->vqs[i].rxq);
        }
        vhost_dev_init(dev, vqs, VHOST_NET_VQ_MAX);
--
Quoted 1 line

2.16.2.395.g2e18187dfd-goog

I suspect that's not sufficient. rx ring is tied to the tap device. I think we need to drop it every time we drop the device.

Quoted 3 lines

Unfortunately I've no idea where is the device dropped. Are you referring to vhost_net_vq_reset()?
I can fix that part if needed, but won't be able to validate it with KMSAN.

Quoted 6 lines

I see several issues. For example in vhost_net_set_backend if there's a value then rx ring will point to the ring of the wrong socket.
Something like the below might help but we really need documentation of when is rx_ring valid. Is it only valid when private-data is valid?

I think so, we need keep rx_ring synced with private_data.

Quoted 5 lines

If yes need to make sure
we reset it with private_data.

Also I see __skb_array_destroy_skb used with ptr_ring which seems suspicious: how do we know the entries are skbs?

Good catch, will post an independent patch to fix this.

Quoted 30 lines

Patch below is on top of yours, and

But I really would like Jason to look and come up with a patch to address all these issues.

---

drivers/vhost/net.c +1 -2
index 610cba2..7a65b69 100644
@@ -972,6 +973,7 @@ static struct socket *vhost_net_stop_vq(struct vhost_net *n,
   	vhost_net_disable_vq(n, vq);
   	vq->private_data = NULL;
   	vhost_net_buf_unproduce(nvq);
+	vq->rx_ring = NULL;
   	mutex_unlock(&vq->mutex);
   	return sock;
   }
@@ -1161,8 +1163,6 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
   		vhost_net_disable_vq(n, vq);
   		vq->private_data = sock;
   		vhost_net_buf_unproduce(nvq);
-		if (index == VHOST_NET_VQ_RX)
-			nvq->rx_ring = get_tap_ptr_ring(fd);
   		r = vhost_vq_init_access(vq);
   		if (r)
   			goto err_used;
Quoted 8 lines

drivers/vhost/net.c static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd) · line 1177

Quoted 7 lines
drivers/vhost/net.c +4 -0
@@ -1172,6 +1172,10 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
   		oldubufs = nvq->ubufs;
   		nvq->ubufs = ubufs;
+		if (index == VHOST_NET_VQ_RX)
+			nvq->rx_ring = get_tap_ptr_ring(fd);
+		else
+			nvq->rx_ring = NULL;

Any reason to move those after vhost_net_enable_vq()?

Quoted 2 lines

Otherwise I see an issue if there is an error and
we revert the change.

I see.

Quoted 7 lines
Quoted 5 lines

And consider we won't
try to assign rx_ring to TX, the "else" part seems unnecessary.

Thanks

ok, pls pack up all fixes as you see fit and post
a patchset.

Ok.

Thanks

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/vhost/net.c +0 -0
   		n->tx_packets = 0;
   		n->tx_zcopy_err = 0;