* [PATCH net] virtio-net: Skip set_features on non-cvq devices
@ 2019-12-20 21:22 Alistair Delva
2019-12-20 22:40 ` Stephen Barber
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Alistair Delva @ 2019-12-20 21:22 UTC (permalink / raw)
To: netdev
Cc: stable, Michael S . Tsirkin, Jason Wang, David S . Miller,
kernel-team, virtualization, linux-kernel
On devices without control virtqueue support, such as the virtio_net
implementation in crosvm[1], attempting to configure LRO will panic the
kernel:
kernel BUG at drivers/net/virtio_net.c:1591!
invalid opcode: 0000 [#1] PREEMPT SMP PTI
CPU: 1 PID: 483 Comm: Binder:330_1 Not tainted 5.4.5-01326-g19463e9acaac #1
Hardware name: ChromiumOS crosvm, BIOS 0
RIP: 0010:virtnet_send_command+0x15d/0x170 [virtio_net]
Code: d8 00 00 00 80 78 02 00 0f 94 c0 65 48 8b 0c 25 28 00 00 00 48 3b 4c 24 70 75 11 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b e8 ec a4 12 c8 66 90 66 2e 0f 1f 84 00 00 00 00 00 55 48 89
RSP: 0018:ffffb97940e7bb50 EFLAGS: 00010246
RAX: ffffffffc0596020 RBX: ffffa0e1fc8ea840 RCX: 0000000000000017
RDX: ffffffffc0596110 RSI: 0000000000000011 RDI: 000000000000000d
RBP: ffffb97940e7bbf8 R08: ffffa0e1fc8ea0b0 R09: ffffa0e1fc8ea0b0
R10: ffffffffffffffff R11: ffffffffc0590940 R12: 0000000000000005
R13: ffffa0e1ffad2c00 R14: ffffb97940e7bc08 R15: 0000000000000000
FS: 0000000000000000(0000) GS:ffffa0e1fd100000(006b) knlGS:00000000e5ef7494
CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
CR2: 00000000e5eeb82c CR3: 0000000079b06001 CR4: 0000000000360ee0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
? preempt_count_add+0x58/0xb0
? _raw_spin_lock_irqsave+0x36/0x70
? _raw_spin_unlock_irqrestore+0x1a/0x40
? __wake_up+0x70/0x190
virtnet_set_features+0x90/0xf0 [virtio_net]
__netdev_update_features+0x271/0x980
? nlmsg_notify+0x5b/0xa0
dev_disable_lro+0x2b/0x190
? inet_netconf_notify_devconf+0xe2/0x120
devinet_sysctl_forward+0x176/0x1e0
proc_sys_call_handler+0x1f0/0x250
proc_sys_write+0xf/0x20
__vfs_write+0x3e/0x190
? __sb_start_write+0x6d/0xd0
vfs_write+0xd3/0x190
ksys_write+0x68/0xd0
__ia32_sys_write+0x14/0x20
do_fast_syscall_32+0x86/0xe0
entry_SYSENTER_compat+0x7c/0x8e
This happens because virtio_set_features() does not check the presence
of the control virtqueue feature, which is sanity checked by a BUG_ON
in virtnet_send_command().
Fix this by skipping any feature processing if the control virtqueue is
missing. This should be OK for any future feature that is added, as
presumably all of them would require control virtqueue support to notify
the endpoint that offload etc. should begin.
[1] https://chromium.googlesource.com/chromiumos/platform/crosvm/
Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
Cc: stable@vger.kernel.org [4.20+]
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: kernel-team@android.com
Cc: virtualization@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Alistair Delva <adelva@google.com>
---
drivers/net/virtio_net.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4d7d5434cc5d..709bcd34e485 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2560,6 +2560,9 @@ static int virtnet_set_features(struct net_device *dev,
u64 offloads;
int err;
+ if (!vi->has_cvq)
+ return 0;
+
if ((dev->features ^ features) & NETIF_F_LRO) {
if (vi->xdp_queue_pairs)
return -EBUSY;
--
2.24.1.735.g03f4e72817-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: Skip set_features on non-cvq devices
2019-12-20 21:22 [PATCH net] virtio-net: Skip set_features on non-cvq devices Alistair Delva
@ 2019-12-20 22:40 ` Stephen Barber
2019-12-21 3:08 ` Willem de Bruijn
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Stephen Barber @ 2019-12-20 22:40 UTC (permalink / raw)
To: Alistair Delva
Cc: netdev, stable, Michael S . Tsirkin, Jason Wang,
David S . Miller, kernel-team, virtualization, open list
On Fri, Dec 20, 2019 at 1:22 PM Alistair Delva <adelva@google.com> wrote:
>
> On devices without control virtqueue support, such as the virtio_net
> implementation in crosvm[1], attempting to configure LRO will panic the
> kernel:
If you'd like to try using the control virtqueue, I have a WIP patch
at crrev.com/c/1968200
The downside there is that when enabling IP forwarding the guest
successfully disables LRO,
which AFAIK (please correct me if I'm wrong!) is generally safe when
using virtio-net. My iperf
tests showed a ~90% bandwidth reduction, so we'd need to force LRO
back on after enabling
IP forwarding if the control queue offload patch lands.
>
> kernel BUG at drivers/net/virtio_net.c:1591!
> invalid opcode: 0000 [#1] PREEMPT SMP PTI
> CPU: 1 PID: 483 Comm: Binder:330_1 Not tainted 5.4.5-01326-g19463e9acaac #1
> Hardware name: ChromiumOS crosvm, BIOS 0
> RIP: 0010:virtnet_send_command+0x15d/0x170 [virtio_net]
> Code: d8 00 00 00 80 78 02 00 0f 94 c0 65 48 8b 0c 25 28 00 00 00 48 3b 4c 24 70 75 11 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b e8 ec a4 12 c8 66 90 66 2e 0f 1f 84 00 00 00 00 00 55 48 89
> RSP: 0018:ffffb97940e7bb50 EFLAGS: 00010246
> RAX: ffffffffc0596020 RBX: ffffa0e1fc8ea840 RCX: 0000000000000017
> RDX: ffffffffc0596110 RSI: 0000000000000011 RDI: 000000000000000d
> RBP: ffffb97940e7bbf8 R08: ffffa0e1fc8ea0b0 R09: ffffa0e1fc8ea0b0
> R10: ffffffffffffffff R11: ffffffffc0590940 R12: 0000000000000005
> R13: ffffa0e1ffad2c00 R14: ffffb97940e7bc08 R15: 0000000000000000
> FS: 0000000000000000(0000) GS:ffffa0e1fd100000(006b) knlGS:00000000e5ef7494
> CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
> CR2: 00000000e5eeb82c CR3: 0000000079b06001 CR4: 0000000000360ee0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> ? preempt_count_add+0x58/0xb0
> ? _raw_spin_lock_irqsave+0x36/0x70
> ? _raw_spin_unlock_irqrestore+0x1a/0x40
> ? __wake_up+0x70/0x190
> virtnet_set_features+0x90/0xf0 [virtio_net]
> __netdev_update_features+0x271/0x980
> ? nlmsg_notify+0x5b/0xa0
> dev_disable_lro+0x2b/0x190
> ? inet_netconf_notify_devconf+0xe2/0x120
> devinet_sysctl_forward+0x176/0x1e0
> proc_sys_call_handler+0x1f0/0x250
> proc_sys_write+0xf/0x20
> __vfs_write+0x3e/0x190
> ? __sb_start_write+0x6d/0xd0
> vfs_write+0xd3/0x190
> ksys_write+0x68/0xd0
> __ia32_sys_write+0x14/0x20
> do_fast_syscall_32+0x86/0xe0
> entry_SYSENTER_compat+0x7c/0x8e
>
> This happens because virtio_set_features() does not check the presence
> of the control virtqueue feature, which is sanity checked by a BUG_ON
> in virtnet_send_command().
>
> Fix this by skipping any feature processing if the control virtqueue is
> missing. This should be OK for any future feature that is added, as
> presumably all of them would require control virtqueue support to notify
> the endpoint that offload etc. should begin.
This sounds okay to me. This does end up hitting a netdev_WARN later on though:
[ 1.885957] ------------[ cut here ]------------
[ 1.888398] netdevice: eth0: failed to disable LRO!
[ 1.890369] WARNING: CPU: 0 PID: 163 at
/mnt/host/source/src/third_party/kernel/v5.4/net/core/dev.c:1483
dev_disable_lro+0x91/0x95
[ 1.894761] CPU: 0 PID: 163 Comm: lxd Not tainted 5.4.5 #2
[ 1.896698] Hardware name: ChromiumOS crosvm, BIOS 0
[ 1.898387] RIP: 0010:dev_disable_lro+0x91/0x95
[ 1.899877] Code: a6 4d 0f 44 f7 eb 07 49 c7 c6 af 78 60 a6 4c 89
ff e8 6a 00 00 00 48 c7 c7 dd 73 60 a6 4c 89 f6 48 89 c2 31 c0 e8 36
e2 a5 ff <0f> 0b eb 8a 53 48 83 ec 18 48 89 fb 65 48 8b 04 25 28 00 00
00 48
[ 1.905701] RSP: 0018:ffffae0fc0427d78 EFLAGS: 00010246
[ 1.907282] RAX: fe766977dd14fb00 RBX: 0000000000000001 RCX: ffffffffa683aa50
[ 1.909391] RDX: fffffffffffffe6d RSI: 0000000000000082 RDI: ffffffffa683aa20
[ 1.911503] RBP: ffff990bd3f8e050 R08: ffff0a1000000600 R09: 0000004000000000
[ 1.913613] R10: 0000000000000193 R11: ffffffffa5af56a5 R12: ffffffffa6882690
[ 1.915729] R13: ffff990bd3ec2248 R14: ffff990bd3f8e000 R15: ffff990bd3f8e000
[ 1.918555] FS: 00007ca813fff700(0000) GS:ffff990bd4c00000(0000)
knlGS:0000000000000000
[ 1.920648] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1.922039] CR2: 00005ba0e1789990 CR3: 00000001d1716003 CR4: 00000000003606b0
[ 1.923716] Call Trace:
[ 1.924356] devinet_sysctl_forward+0x15a/0x1b9
[ 1.925444] proc_sys_call_handler+0xc3/0xe0
[ 1.926491] __vfs_write+0x3d/0x19b
[ 1.927351] ? selinux_file_permission+0x8c/0x124
[ 1.928482] vfs_write+0xea/0x17f
[ 1.929329] ksys_write+0x68/0xc9
[ 1.930178] do_syscall_64+0x4a/0x5d
[ 1.931063] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 1.932272] RIP: 0033:0x5ba0e10655c0
[ 1.933159] Code: 8b 7c 24 10 48 8b 74 24 18 48 8b 54 24 20 49 c7
c2 00 00 00 00 49 c7 c0 00 00 00 00 49 c7 c1 00 00 00 00 48 8b 44 24
08 0f 05 <48> 3d 01 f0 ff ff 76 20 48 c7 44 24 28 ff ff ff ff 48 c7 44
24 30
[ 1.937440] RSP: 002b:000000c00041e258 EFLAGS: 00000216 ORIG_RAX:
0000000000000001
[ 1.939224] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00005ba0e10655c0
[ 1.940897] RDX: 0000000000000001 RSI: 000000c00041e4d8 RDI: 0000000000000018
[ 1.942600] RBP: 000000c00041e2a8 R08: 0000000000000000 R09: 0000000000000000
[ 1.944310] R10: 0000000000000000 R11: 0000000000000216 R12: 000000000000000c
[ 1.946007] R13: 0000000000000032 R14: 00005ba0e0fa01e8 R15: 0000000000000000
[ 1.947706] ---[ end trace 9ac0c921383f98c0 ]---
>
> [1] https://chromium.googlesource.com/chromiumos/platform/crosvm/
>
> Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> Cc: stable@vger.kernel.org [4.20+]
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: kernel-team@android.com
> Cc: virtualization@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Alistair Delva <adelva@google.com>
Tested-by: Stephen Barber <smbarber@chromium.org>
> ---
> drivers/net/virtio_net.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4d7d5434cc5d..709bcd34e485 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2560,6 +2560,9 @@ static int virtnet_set_features(struct net_device *dev,
> u64 offloads;
> int err;
>
> + if (!vi->has_cvq)
> + return 0;
> +
> if ((dev->features ^ features) & NETIF_F_LRO) {
> if (vi->xdp_queue_pairs)
> return -EBUSY;
> --
> 2.24.1.735.g03f4e72817-goog
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: Skip set_features on non-cvq devices
2019-12-20 21:22 [PATCH net] virtio-net: Skip set_features on non-cvq devices Alistair Delva
2019-12-20 22:40 ` Stephen Barber
@ 2019-12-21 3:08 ` Willem de Bruijn
2019-12-22 13:11 ` Michael S. Tsirkin
2019-12-22 13:13 ` Michael S. Tsirkin
2020-01-05 13:11 ` Michael S. Tsirkin
3 siblings, 1 reply; 12+ messages in thread
From: Willem de Bruijn @ 2019-12-21 3:08 UTC (permalink / raw)
To: Alistair Delva
Cc: Network Development, stable, Michael S . Tsirkin, Jason Wang,
David S . Miller, kernel-team, virtualization, linux-kernel
On Fri, Dec 20, 2019 at 4:22 PM Alistair Delva <adelva@google.com> wrote:
>
> On devices without control virtqueue support, such as the virtio_net
> implementation in crosvm[1], attempting to configure LRO will panic the
> kernel:
>
> kernel BUG at drivers/net/virtio_net.c:1591!
> invalid opcode: 0000 [#1] PREEMPT SMP PTI
> CPU: 1 PID: 483 Comm: Binder:330_1 Not tainted 5.4.5-01326-g19463e9acaac #1
> Hardware name: ChromiumOS crosvm, BIOS 0
> RIP: 0010:virtnet_send_command+0x15d/0x170 [virtio_net]
> Code: d8 00 00 00 80 78 02 00 0f 94 c0 65 48 8b 0c 25 28 00 00 00 48 3b 4c 24 70 75 11 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b e8 ec a4 12 c8 66 90 66 2e 0f 1f 84 00 00 00 00 00 55 48 89
> RSP: 0018:ffffb97940e7bb50 EFLAGS: 00010246
> RAX: ffffffffc0596020 RBX: ffffa0e1fc8ea840 RCX: 0000000000000017
> RDX: ffffffffc0596110 RSI: 0000000000000011 RDI: 000000000000000d
> RBP: ffffb97940e7bbf8 R08: ffffa0e1fc8ea0b0 R09: ffffa0e1fc8ea0b0
> R10: ffffffffffffffff R11: ffffffffc0590940 R12: 0000000000000005
> R13: ffffa0e1ffad2c00 R14: ffffb97940e7bc08 R15: 0000000000000000
> FS: 0000000000000000(0000) GS:ffffa0e1fd100000(006b) knlGS:00000000e5ef7494
> CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
> CR2: 00000000e5eeb82c CR3: 0000000079b06001 CR4: 0000000000360ee0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> ? preempt_count_add+0x58/0xb0
> ? _raw_spin_lock_irqsave+0x36/0x70
> ? _raw_spin_unlock_irqrestore+0x1a/0x40
> ? __wake_up+0x70/0x190
> virtnet_set_features+0x90/0xf0 [virtio_net]
> __netdev_update_features+0x271/0x980
> ? nlmsg_notify+0x5b/0xa0
> dev_disable_lro+0x2b/0x190
> ? inet_netconf_notify_devconf+0xe2/0x120
> devinet_sysctl_forward+0x176/0x1e0
> proc_sys_call_handler+0x1f0/0x250
> proc_sys_write+0xf/0x20
> __vfs_write+0x3e/0x190
> ? __sb_start_write+0x6d/0xd0
> vfs_write+0xd3/0x190
> ksys_write+0x68/0xd0
> __ia32_sys_write+0x14/0x20
> do_fast_syscall_32+0x86/0xe0
> entry_SYSENTER_compat+0x7c/0x8e
>
> This happens because virtio_set_features() does not check the presence
> of the control virtqueue feature, which is sanity checked by a BUG_ON
> in virtnet_send_command().
>
> Fix this by skipping any feature processing if the control virtqueue is
> missing. This should be OK for any future feature that is added, as
> presumably all of them would require control virtqueue support to notify
> the endpoint that offload etc. should begin.
>
> [1] https://chromium.googlesource.com/chromiumos/platform/crosvm/
>
> Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> Cc: stable@vger.kernel.org [4.20+]
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: kernel-team@android.com
> Cc: virtualization@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Alistair Delva <adelva@google.com>
Thanks for debugging this, Alistair.
> ---
> drivers/net/virtio_net.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4d7d5434cc5d..709bcd34e485 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2560,6 +2560,9 @@ static int virtnet_set_features(struct net_device *dev,
> u64 offloads;
> int err;
>
> + if (!vi->has_cvq)
> + return 0;
> +
Instead of checking for this in virtnet_set_features, how about we
make configurability contingent on cvq in virtnet_probe:
- if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
+ if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) &&
+ virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
dev->hw_features |= NETIF_F_LRO;
Based on this logic a little below in the same function
if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
vi->has_cvq = true;
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: Skip set_features on non-cvq devices
2019-12-21 3:08 ` Willem de Bruijn
@ 2019-12-22 13:11 ` Michael S. Tsirkin
2019-12-22 14:21 ` Willem de Bruijn
0 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2019-12-22 13:11 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Alistair Delva, Network Development, stable, Jason Wang,
David S . Miller, kernel-team, virtualization, linux-kernel
On Fri, Dec 20, 2019 at 10:08:41PM -0500, Willem de Bruijn wrote:
> On Fri, Dec 20, 2019 at 4:22 PM Alistair Delva <adelva@google.com> wrote:
> >
> > On devices without control virtqueue support, such as the virtio_net
> > implementation in crosvm[1], attempting to configure LRO will panic the
> > kernel:
> >
> > kernel BUG at drivers/net/virtio_net.c:1591!
> > invalid opcode: 0000 [#1] PREEMPT SMP PTI
> > CPU: 1 PID: 483 Comm: Binder:330_1 Not tainted 5.4.5-01326-g19463e9acaac #1
> > Hardware name: ChromiumOS crosvm, BIOS 0
> > RIP: 0010:virtnet_send_command+0x15d/0x170 [virtio_net]
> > Code: d8 00 00 00 80 78 02 00 0f 94 c0 65 48 8b 0c 25 28 00 00 00 48 3b 4c 24 70 75 11 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b e8 ec a4 12 c8 66 90 66 2e 0f 1f 84 00 00 00 00 00 55 48 89
> > RSP: 0018:ffffb97940e7bb50 EFLAGS: 00010246
> > RAX: ffffffffc0596020 RBX: ffffa0e1fc8ea840 RCX: 0000000000000017
> > RDX: ffffffffc0596110 RSI: 0000000000000011 RDI: 000000000000000d
> > RBP: ffffb97940e7bbf8 R08: ffffa0e1fc8ea0b0 R09: ffffa0e1fc8ea0b0
> > R10: ffffffffffffffff R11: ffffffffc0590940 R12: 0000000000000005
> > R13: ffffa0e1ffad2c00 R14: ffffb97940e7bc08 R15: 0000000000000000
> > FS: 0000000000000000(0000) GS:ffffa0e1fd100000(006b) knlGS:00000000e5ef7494
> > CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
> > CR2: 00000000e5eeb82c CR3: 0000000079b06001 CR4: 0000000000360ee0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > Call Trace:
> > ? preempt_count_add+0x58/0xb0
> > ? _raw_spin_lock_irqsave+0x36/0x70
> > ? _raw_spin_unlock_irqrestore+0x1a/0x40
> > ? __wake_up+0x70/0x190
> > virtnet_set_features+0x90/0xf0 [virtio_net]
> > __netdev_update_features+0x271/0x980
> > ? nlmsg_notify+0x5b/0xa0
> > dev_disable_lro+0x2b/0x190
> > ? inet_netconf_notify_devconf+0xe2/0x120
> > devinet_sysctl_forward+0x176/0x1e0
> > proc_sys_call_handler+0x1f0/0x250
> > proc_sys_write+0xf/0x20
> > __vfs_write+0x3e/0x190
> > ? __sb_start_write+0x6d/0xd0
> > vfs_write+0xd3/0x190
> > ksys_write+0x68/0xd0
> > __ia32_sys_write+0x14/0x20
> > do_fast_syscall_32+0x86/0xe0
> > entry_SYSENTER_compat+0x7c/0x8e
> >
> > This happens because virtio_set_features() does not check the presence
> > of the control virtqueue feature, which is sanity checked by a BUG_ON
> > in virtnet_send_command().
> >
> > Fix this by skipping any feature processing if the control virtqueue is
> > missing. This should be OK for any future feature that is added, as
> > presumably all of them would require control virtqueue support to notify
> > the endpoint that offload etc. should begin.
> >
> > [1] https://chromium.googlesource.com/chromiumos/platform/crosvm/
> >
> > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > Cc: stable@vger.kernel.org [4.20+]
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Jason Wang <jasowang@redhat.com>
> > Cc: David S. Miller <davem@davemloft.net>
> > Cc: kernel-team@android.com
> > Cc: virtualization@lists.linux-foundation.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Alistair Delva <adelva@google.com>
>
> Thanks for debugging this, Alistair.
>
> > ---
> > drivers/net/virtio_net.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 4d7d5434cc5d..709bcd34e485 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2560,6 +2560,9 @@ static int virtnet_set_features(struct net_device *dev,
> > u64 offloads;
> > int err;
> >
> > + if (!vi->has_cvq)
> > + return 0;
> > +
>
> Instead of checking for this in virtnet_set_features, how about we
> make configurability contingent on cvq in virtnet_probe:
>
> - if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) &&
> + virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> dev->hw_features |= NETIF_F_LRO;
>
> Based on this logic a little below in the same function
>
> if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> vi->has_cvq = true;
This would be a regression on old hypervisors which didn't have
CTL VQ - suddenly they will lose offloads.
--
MST
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: Skip set_features on non-cvq devices
2019-12-20 21:22 [PATCH net] virtio-net: Skip set_features on non-cvq devices Alistair Delva
2019-12-20 22:40 ` Stephen Barber
2019-12-21 3:08 ` Willem de Bruijn
@ 2019-12-22 13:13 ` Michael S. Tsirkin
2020-01-05 13:11 ` Michael S. Tsirkin
3 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2019-12-22 13:13 UTC (permalink / raw)
To: Alistair Delva
Cc: netdev, stable, Jason Wang, David S . Miller, kernel-team,
virtualization, linux-kernel
On Fri, Dec 20, 2019 at 01:22:07PM -0800, Alistair Delva wrote:
> On devices without control virtqueue support, such as the virtio_net
> implementation in crosvm[1], attempting to configure LRO will panic the
> kernel:
>
> kernel BUG at drivers/net/virtio_net.c:1591!
> invalid opcode: 0000 [#1] PREEMPT SMP PTI
> CPU: 1 PID: 483 Comm: Binder:330_1 Not tainted 5.4.5-01326-g19463e9acaac #1
> Hardware name: ChromiumOS crosvm, BIOS 0
> RIP: 0010:virtnet_send_command+0x15d/0x170 [virtio_net]
> Code: d8 00 00 00 80 78 02 00 0f 94 c0 65 48 8b 0c 25 28 00 00 00 48 3b 4c 24 70 75 11 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b e8 ec a4 12 c8 66 90 66 2e 0f 1f 84 00 00 00 00 00 55 48 89
> RSP: 0018:ffffb97940e7bb50 EFLAGS: 00010246
> RAX: ffffffffc0596020 RBX: ffffa0e1fc8ea840 RCX: 0000000000000017
> RDX: ffffffffc0596110 RSI: 0000000000000011 RDI: 000000000000000d
> RBP: ffffb97940e7bbf8 R08: ffffa0e1fc8ea0b0 R09: ffffa0e1fc8ea0b0
> R10: ffffffffffffffff R11: ffffffffc0590940 R12: 0000000000000005
> R13: ffffa0e1ffad2c00 R14: ffffb97940e7bc08 R15: 0000000000000000
> FS: 0000000000000000(0000) GS:ffffa0e1fd100000(006b) knlGS:00000000e5ef7494
> CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
> CR2: 00000000e5eeb82c CR3: 0000000079b06001 CR4: 0000000000360ee0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> ? preempt_count_add+0x58/0xb0
> ? _raw_spin_lock_irqsave+0x36/0x70
> ? _raw_spin_unlock_irqrestore+0x1a/0x40
> ? __wake_up+0x70/0x190
> virtnet_set_features+0x90/0xf0 [virtio_net]
> __netdev_update_features+0x271/0x980
> ? nlmsg_notify+0x5b/0xa0
> dev_disable_lro+0x2b/0x190
> ? inet_netconf_notify_devconf+0xe2/0x120
> devinet_sysctl_forward+0x176/0x1e0
> proc_sys_call_handler+0x1f0/0x250
> proc_sys_write+0xf/0x20
> __vfs_write+0x3e/0x190
> ? __sb_start_write+0x6d/0xd0
> vfs_write+0xd3/0x190
> ksys_write+0x68/0xd0
> __ia32_sys_write+0x14/0x20
> do_fast_syscall_32+0x86/0xe0
> entry_SYSENTER_compat+0x7c/0x8e
>
> This happens because virtio_set_features() does not check the presence
> of the control virtqueue feature, which is sanity checked by a BUG_ON
> in virtnet_send_command().
>
> Fix this by skipping any feature processing if the control virtqueue is
> missing. This should be OK for any future feature that is added, as
> presumably all of them would require control virtqueue support to notify
> the endpoint that offload etc. should begin.
>
> [1] https://chromium.googlesource.com/chromiumos/platform/crosvm/
>
> Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> Cc: stable@vger.kernel.org [4.20+]
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: kernel-team@android.com
> Cc: virtualization@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Alistair Delva <adelva@google.com>
> ---
> drivers/net/virtio_net.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4d7d5434cc5d..709bcd34e485 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2560,6 +2560,9 @@ static int virtnet_set_features(struct net_device *dev,
> u64 offloads;
> int err;
>
> + if (!vi->has_cvq)
> + return 0;
> +
Shouldn't this return failure if the features are actually
being changed?
> if ((dev->features ^ features) & NETIF_F_LRO) {
> if (vi->xdp_queue_pairs)
> return -EBUSY;
> --
> 2.24.1.735.g03f4e72817-goog
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: Skip set_features on non-cvq devices
2019-12-22 13:11 ` Michael S. Tsirkin
@ 2019-12-22 14:21 ` Willem de Bruijn
2019-12-22 14:57 ` Michael S. Tsirkin
0 siblings, 1 reply; 12+ messages in thread
From: Willem de Bruijn @ 2019-12-22 14:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Willem de Bruijn, Alistair Delva, Network Development, stable,
Jason Wang, David S . Miller, kernel-team, virtualization,
linux-kernel
On Sun, Dec 22, 2019 at 8:11 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Dec 20, 2019 at 10:08:41PM -0500, Willem de Bruijn wrote:
> > On Fri, Dec 20, 2019 at 4:22 PM Alistair Delva <adelva@google.com> wrote:
> > >
> > > On devices without control virtqueue support, such as the virtio_net
> > > implementation in crosvm[1], attempting to configure LRO will panic the
> > > kernel:
> > >
> > > kernel BUG at drivers/net/virtio_net.c:1591!
> > > invalid opcode: 0000 [#1] PREEMPT SMP PTI
> > > CPU: 1 PID: 483 Comm: Binder:330_1 Not tainted 5.4.5-01326-g19463e9acaac #1
> > > Hardware name: ChromiumOS crosvm, BIOS 0
> > > RIP: 0010:virtnet_send_command+0x15d/0x170 [virtio_net]
> > > Code: d8 00 00 00 80 78 02 00 0f 94 c0 65 48 8b 0c 25 28 00 00 00 48 3b 4c 24 70 75 11 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b e8 ec a4 12 c8 66 90 66 2e 0f 1f 84 00 00 00 00 00 55 48 89
> > > RSP: 0018:ffffb97940e7bb50 EFLAGS: 00010246
> > > RAX: ffffffffc0596020 RBX: ffffa0e1fc8ea840 RCX: 0000000000000017
> > > RDX: ffffffffc0596110 RSI: 0000000000000011 RDI: 000000000000000d
> > > RBP: ffffb97940e7bbf8 R08: ffffa0e1fc8ea0b0 R09: ffffa0e1fc8ea0b0
> > > R10: ffffffffffffffff R11: ffffffffc0590940 R12: 0000000000000005
> > > R13: ffffa0e1ffad2c00 R14: ffffb97940e7bc08 R15: 0000000000000000
> > > FS: 0000000000000000(0000) GS:ffffa0e1fd100000(006b) knlGS:00000000e5ef7494
> > > CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
> > > CR2: 00000000e5eeb82c CR3: 0000000079b06001 CR4: 0000000000360ee0
> > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > Call Trace:
> > > ? preempt_count_add+0x58/0xb0
> > > ? _raw_spin_lock_irqsave+0x36/0x70
> > > ? _raw_spin_unlock_irqrestore+0x1a/0x40
> > > ? __wake_up+0x70/0x190
> > > virtnet_set_features+0x90/0xf0 [virtio_net]
> > > __netdev_update_features+0x271/0x980
> > > ? nlmsg_notify+0x5b/0xa0
> > > dev_disable_lro+0x2b/0x190
> > > ? inet_netconf_notify_devconf+0xe2/0x120
> > > devinet_sysctl_forward+0x176/0x1e0
> > > proc_sys_call_handler+0x1f0/0x250
> > > proc_sys_write+0xf/0x20
> > > __vfs_write+0x3e/0x190
> > > ? __sb_start_write+0x6d/0xd0
> > > vfs_write+0xd3/0x190
> > > ksys_write+0x68/0xd0
> > > __ia32_sys_write+0x14/0x20
> > > do_fast_syscall_32+0x86/0xe0
> > > entry_SYSENTER_compat+0x7c/0x8e
> > >
> > > This happens because virtio_set_features() does not check the presence
> > > of the control virtqueue feature, which is sanity checked by a BUG_ON
> > > in virtnet_send_command().
> > >
> > > Fix this by skipping any feature processing if the control virtqueue is
> > > missing. This should be OK for any future feature that is added, as
> > > presumably all of them would require control virtqueue support to notify
> > > the endpoint that offload etc. should begin.
> > >
> > > [1] https://chromium.googlesource.com/chromiumos/platform/crosvm/
> > >
> > > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > > Cc: stable@vger.kernel.org [4.20+]
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Jason Wang <jasowang@redhat.com>
> > > Cc: David S. Miller <davem@davemloft.net>
> > > Cc: kernel-team@android.com
> > > Cc: virtualization@lists.linux-foundation.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Signed-off-by: Alistair Delva <adelva@google.com>
> >
> > Thanks for debugging this, Alistair.
> >
> > > ---
> > > drivers/net/virtio_net.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 4d7d5434cc5d..709bcd34e485 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -2560,6 +2560,9 @@ static int virtnet_set_features(struct net_device *dev,
> > > u64 offloads;
> > > int err;
> > >
> > > + if (!vi->has_cvq)
> > > + return 0;
> > > +
> >
> > Instead of checking for this in virtnet_set_features, how about we
> > make configurability contingent on cvq in virtnet_probe:
> >
> > - if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) &&
> > + virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> > dev->hw_features |= NETIF_F_LRO;
> >
> > Based on this logic a little below in the same function
> >
> > if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> > vi->has_cvq = true;
>
>
> This would be a regression on old hypervisors which didn't have
> CTL VQ - suddenly they will lose offloads.
dev->features still correctly displays whether offloads are enabled.
Removing it from dev->hw_features just renders it non-configurable.
Note that before the patch that is being fixed the offloads were
enabled, but ethtool would show them as off.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: Skip set_features on non-cvq devices
2019-12-22 14:21 ` Willem de Bruijn
@ 2019-12-22 14:57 ` Michael S. Tsirkin
2019-12-22 15:54 ` Willem de Bruijn
0 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2019-12-22 14:57 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Alistair Delva, Network Development, stable, Jason Wang,
David S . Miller, kernel-team, virtualization, linux-kernel
On Sun, Dec 22, 2019 at 09:21:43AM -0500, Willem de Bruijn wrote:
> On Sun, Dec 22, 2019 at 8:11 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Dec 20, 2019 at 10:08:41PM -0500, Willem de Bruijn wrote:
> > > On Fri, Dec 20, 2019 at 4:22 PM Alistair Delva <adelva@google.com> wrote:
> > > >
> > > > On devices without control virtqueue support, such as the virtio_net
> > > > implementation in crosvm[1], attempting to configure LRO will panic the
> > > > kernel:
> > > >
> > > > kernel BUG at drivers/net/virtio_net.c:1591!
> > > > invalid opcode: 0000 [#1] PREEMPT SMP PTI
> > > > CPU: 1 PID: 483 Comm: Binder:330_1 Not tainted 5.4.5-01326-g19463e9acaac #1
> > > > Hardware name: ChromiumOS crosvm, BIOS 0
> > > > RIP: 0010:virtnet_send_command+0x15d/0x170 [virtio_net]
> > > > Code: d8 00 00 00 80 78 02 00 0f 94 c0 65 48 8b 0c 25 28 00 00 00 48 3b 4c 24 70 75 11 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b e8 ec a4 12 c8 66 90 66 2e 0f 1f 84 00 00 00 00 00 55 48 89
> > > > RSP: 0018:ffffb97940e7bb50 EFLAGS: 00010246
> > > > RAX: ffffffffc0596020 RBX: ffffa0e1fc8ea840 RCX: 0000000000000017
> > > > RDX: ffffffffc0596110 RSI: 0000000000000011 RDI: 000000000000000d
> > > > RBP: ffffb97940e7bbf8 R08: ffffa0e1fc8ea0b0 R09: ffffa0e1fc8ea0b0
> > > > R10: ffffffffffffffff R11: ffffffffc0590940 R12: 0000000000000005
> > > > R13: ffffa0e1ffad2c00 R14: ffffb97940e7bc08 R15: 0000000000000000
> > > > FS: 0000000000000000(0000) GS:ffffa0e1fd100000(006b) knlGS:00000000e5ef7494
> > > > CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
> > > > CR2: 00000000e5eeb82c CR3: 0000000079b06001 CR4: 0000000000360ee0
> > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > > Call Trace:
> > > > ? preempt_count_add+0x58/0xb0
> > > > ? _raw_spin_lock_irqsave+0x36/0x70
> > > > ? _raw_spin_unlock_irqrestore+0x1a/0x40
> > > > ? __wake_up+0x70/0x190
> > > > virtnet_set_features+0x90/0xf0 [virtio_net]
> > > > __netdev_update_features+0x271/0x980
> > > > ? nlmsg_notify+0x5b/0xa0
> > > > dev_disable_lro+0x2b/0x190
> > > > ? inet_netconf_notify_devconf+0xe2/0x120
> > > > devinet_sysctl_forward+0x176/0x1e0
> > > > proc_sys_call_handler+0x1f0/0x250
> > > > proc_sys_write+0xf/0x20
> > > > __vfs_write+0x3e/0x190
> > > > ? __sb_start_write+0x6d/0xd0
> > > > vfs_write+0xd3/0x190
> > > > ksys_write+0x68/0xd0
> > > > __ia32_sys_write+0x14/0x20
> > > > do_fast_syscall_32+0x86/0xe0
> > > > entry_SYSENTER_compat+0x7c/0x8e
> > > >
> > > > This happens because virtio_set_features() does not check the presence
> > > > of the control virtqueue feature, which is sanity checked by a BUG_ON
> > > > in virtnet_send_command().
> > > >
> > > > Fix this by skipping any feature processing if the control virtqueue is
> > > > missing. This should be OK for any future feature that is added, as
> > > > presumably all of them would require control virtqueue support to notify
> > > > the endpoint that offload etc. should begin.
> > > >
> > > > [1] https://chromium.googlesource.com/chromiumos/platform/crosvm/
> > > >
> > > > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > > > Cc: stable@vger.kernel.org [4.20+]
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Jason Wang <jasowang@redhat.com>
> > > > Cc: David S. Miller <davem@davemloft.net>
> > > > Cc: kernel-team@android.com
> > > > Cc: virtualization@lists.linux-foundation.org
> > > > Cc: linux-kernel@vger.kernel.org
> > > > Signed-off-by: Alistair Delva <adelva@google.com>
> > >
> > > Thanks for debugging this, Alistair.
> > >
> > > > ---
> > > > drivers/net/virtio_net.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 4d7d5434cc5d..709bcd34e485 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -2560,6 +2560,9 @@ static int virtnet_set_features(struct net_device *dev,
> > > > u64 offloads;
> > > > int err;
> > > >
> > > > + if (!vi->has_cvq)
> > > > + return 0;
> > > > +
> > >
> > > Instead of checking for this in virtnet_set_features, how about we
> > > make configurability contingent on cvq in virtnet_probe:
> > >
> > > - if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > > + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) &&
> > > + virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> > > dev->hw_features |= NETIF_F_LRO;
> > >
> > > Based on this logic a little below in the same function
> > >
> > > if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> > > vi->has_cvq = true;
> >
> >
> > This would be a regression on old hypervisors which didn't have
> > CTL VQ - suddenly they will lose offloads.
>
> dev->features still correctly displays whether offloads are enabled.
> Removing it from dev->hw_features just renders it non-configurable.
Oh you are right. I confused it with dev->features.
> Note that before the patch that is being fixed the offloads were
> enabled, but ethtool would show them as off.
So the bug is in spec, it should have said
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS depends on VIRTIO_NET_F_CTRL_VQ, but we
missed that part. We can and I guess should add this as a recommendation
but it's too late to make it a MUST.
Meanwhile I would say it's cleanest to work around
this in virtnet_validate by clearing VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
if VIRTIO_NET_F_CTRL_VQ is off, with a big comment explaining
it's a spec bug.
--
MST
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: Skip set_features on non-cvq devices
2019-12-22 14:57 ` Michael S. Tsirkin
@ 2019-12-22 15:54 ` Willem de Bruijn
2019-12-22 21:12 ` Michael S. Tsirkin
0 siblings, 1 reply; 12+ messages in thread
From: Willem de Bruijn @ 2019-12-22 15:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Willem de Bruijn, Alistair Delva, Network Development, stable,
Jason Wang, David S . Miller, kernel-team, virtualization,
linux-kernel
On Sun, Dec 22, 2019 at 9:57 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Dec 22, 2019 at 09:21:43AM -0500, Willem de Bruijn wrote:
> > On Sun, Dec 22, 2019 at 8:11 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Fri, Dec 20, 2019 at 10:08:41PM -0500, Willem de Bruijn wrote:
> > > > On Fri, Dec 20, 2019 at 4:22 PM Alistair Delva <adelva@google.com> wrote:
> > > > >
> > > > > On devices without control virtqueue support, such as the virtio_net
> > > > > implementation in crosvm[1], attempting to configure LRO will panic the
> > > > > kernel:
> > > > >
> > > > > kernel BUG at drivers/net/virtio_net.c:1591!
> > > > > invalid opcode: 0000 [#1] PREEMPT SMP PTI
> > > > > CPU: 1 PID: 483 Comm: Binder:330_1 Not tainted 5.4.5-01326-g19463e9acaac #1
> > > > > Hardware name: ChromiumOS crosvm, BIOS 0
> > > > > RIP: 0010:virtnet_send_command+0x15d/0x170 [virtio_net]
> > > > > Code: d8 00 00 00 80 78 02 00 0f 94 c0 65 48 8b 0c 25 28 00 00 00 48 3b 4c 24 70 75 11 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b e8 ec a4 12 c8 66 90 66 2e 0f 1f 84 00 00 00 00 00 55 48 89
> > > > > RSP: 0018:ffffb97940e7bb50 EFLAGS: 00010246
> > > > > RAX: ffffffffc0596020 RBX: ffffa0e1fc8ea840 RCX: 0000000000000017
> > > > > RDX: ffffffffc0596110 RSI: 0000000000000011 RDI: 000000000000000d
> > > > > RBP: ffffb97940e7bbf8 R08: ffffa0e1fc8ea0b0 R09: ffffa0e1fc8ea0b0
> > > > > R10: ffffffffffffffff R11: ffffffffc0590940 R12: 0000000000000005
> > > > > R13: ffffa0e1ffad2c00 R14: ffffb97940e7bc08 R15: 0000000000000000
> > > > > FS: 0000000000000000(0000) GS:ffffa0e1fd100000(006b) knlGS:00000000e5ef7494
> > > > > CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
> > > > > CR2: 00000000e5eeb82c CR3: 0000000079b06001 CR4: 0000000000360ee0
> > > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > > > Call Trace:
> > > > > ? preempt_count_add+0x58/0xb0
> > > > > ? _raw_spin_lock_irqsave+0x36/0x70
> > > > > ? _raw_spin_unlock_irqrestore+0x1a/0x40
> > > > > ? __wake_up+0x70/0x190
> > > > > virtnet_set_features+0x90/0xf0 [virtio_net]
> > > > > __netdev_update_features+0x271/0x980
> > > > > ? nlmsg_notify+0x5b/0xa0
> > > > > dev_disable_lro+0x2b/0x190
> > > > > ? inet_netconf_notify_devconf+0xe2/0x120
> > > > > devinet_sysctl_forward+0x176/0x1e0
> > > > > proc_sys_call_handler+0x1f0/0x250
> > > > > proc_sys_write+0xf/0x20
> > > > > __vfs_write+0x3e/0x190
> > > > > ? __sb_start_write+0x6d/0xd0
> > > > > vfs_write+0xd3/0x190
> > > > > ksys_write+0x68/0xd0
> > > > > __ia32_sys_write+0x14/0x20
> > > > > do_fast_syscall_32+0x86/0xe0
> > > > > entry_SYSENTER_compat+0x7c/0x8e
> > > > >
> > > > > This happens because virtio_set_features() does not check the presence
> > > > > of the control virtqueue feature, which is sanity checked by a BUG_ON
> > > > > in virtnet_send_command().
> > > > >
> > > > > Fix this by skipping any feature processing if the control virtqueue is
> > > > > missing. This should be OK for any future feature that is added, as
> > > > > presumably all of them would require control virtqueue support to notify
> > > > > the endpoint that offload etc. should begin.
> > > > >
> > > > > [1] https://chromium.googlesource.com/chromiumos/platform/crosvm/
> > > > >
> > > > > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > > > > Cc: stable@vger.kernel.org [4.20+]
> > > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > > Cc: Jason Wang <jasowang@redhat.com>
> > > > > Cc: David S. Miller <davem@davemloft.net>
> > > > > Cc: kernel-team@android.com
> > > > > Cc: virtualization@lists.linux-foundation.org
> > > > > Cc: linux-kernel@vger.kernel.org
> > > > > Signed-off-by: Alistair Delva <adelva@google.com>
> > > >
> > > > Thanks for debugging this, Alistair.
> > > >
> > > > > ---
> > > > > drivers/net/virtio_net.c | 3 +++
> > > > > 1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > index 4d7d5434cc5d..709bcd34e485 100644
> > > > > --- a/drivers/net/virtio_net.c
> > > > > +++ b/drivers/net/virtio_net.c
> > > > > @@ -2560,6 +2560,9 @@ static int virtnet_set_features(struct net_device *dev,
> > > > > u64 offloads;
> > > > > int err;
> > > > >
> > > > > + if (!vi->has_cvq)
> > > > > + return 0;
> > > > > +
> > > >
> > > > Instead of checking for this in virtnet_set_features, how about we
> > > > make configurability contingent on cvq in virtnet_probe:
> > > >
> > > > - if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > > > + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) &&
> > > > + virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> > > > dev->hw_features |= NETIF_F_LRO;
> > > >
> > > > Based on this logic a little below in the same function
> > > >
> > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> > > > vi->has_cvq = true;
> > >
> > >
> > > This would be a regression on old hypervisors which didn't have
> > > CTL VQ - suddenly they will lose offloads.
> >
> > dev->features still correctly displays whether offloads are enabled.
> > Removing it from dev->hw_features just renders it non-configurable.
>
> Oh you are right. I confused it with dev->features.
>
> > Note that before the patch that is being fixed the offloads were
> > enabled, but ethtool would show them as off.
>
> So the bug is in spec, it should have said
> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS depends on VIRTIO_NET_F_CTRL_VQ, but we
> missed that part. We can and I guess should add this as a recommendation
> but it's too late to make it a MUST.
>
> Meanwhile I would say it's cleanest to work around
> this in virtnet_validate by clearing VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> if VIRTIO_NET_F_CTRL_VQ is off, with a big comment explaining
> it's a spec bug.
Wouldn't that cause precisely the regression you were concerned about?
Workloads may now depend on LRO for cycle efficiency. Reverting to
behavior before this patch (though now displaying the offload state
correctly) is more conservative in that regard.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: Skip set_features on non-cvq devices
2019-12-22 15:54 ` Willem de Bruijn
@ 2019-12-22 21:12 ` Michael S. Tsirkin
2019-12-22 21:44 ` Willem de Bruijn
0 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2019-12-22 21:12 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Alistair Delva, Network Development, stable, Jason Wang,
David S . Miller, kernel-team, virtualization, linux-kernel
On Sun, Dec 22, 2019 at 10:54:23AM -0500, Willem de Bruijn wrote:
> On Sun, Dec 22, 2019 at 9:57 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Sun, Dec 22, 2019 at 09:21:43AM -0500, Willem de Bruijn wrote:
> > > On Sun, Dec 22, 2019 at 8:11 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Fri, Dec 20, 2019 at 10:08:41PM -0500, Willem de Bruijn wrote:
> > > > > On Fri, Dec 20, 2019 at 4:22 PM Alistair Delva <adelva@google.com> wrote:
> > > > > >
> > > > > > On devices without control virtqueue support, such as the virtio_net
> > > > > > implementation in crosvm[1], attempting to configure LRO will panic the
> > > > > > kernel:
> > > > > >
> > > > > > kernel BUG at drivers/net/virtio_net.c:1591!
> > > > > > invalid opcode: 0000 [#1] PREEMPT SMP PTI
> > > > > > CPU: 1 PID: 483 Comm: Binder:330_1 Not tainted 5.4.5-01326-g19463e9acaac #1
> > > > > > Hardware name: ChromiumOS crosvm, BIOS 0
> > > > > > RIP: 0010:virtnet_send_command+0x15d/0x170 [virtio_net]
> > > > > > Code: d8 00 00 00 80 78 02 00 0f 94 c0 65 48 8b 0c 25 28 00 00 00 48 3b 4c 24 70 75 11 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b e8 ec a4 12 c8 66 90 66 2e 0f 1f 84 00 00 00 00 00 55 48 89
> > > > > > RSP: 0018:ffffb97940e7bb50 EFLAGS: 00010246
> > > > > > RAX: ffffffffc0596020 RBX: ffffa0e1fc8ea840 RCX: 0000000000000017
> > > > > > RDX: ffffffffc0596110 RSI: 0000000000000011 RDI: 000000000000000d
> > > > > > RBP: ffffb97940e7bbf8 R08: ffffa0e1fc8ea0b0 R09: ffffa0e1fc8ea0b0
> > > > > > R10: ffffffffffffffff R11: ffffffffc0590940 R12: 0000000000000005
> > > > > > R13: ffffa0e1ffad2c00 R14: ffffb97940e7bc08 R15: 0000000000000000
> > > > > > FS: 0000000000000000(0000) GS:ffffa0e1fd100000(006b) knlGS:00000000e5ef7494
> > > > > > CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
> > > > > > CR2: 00000000e5eeb82c CR3: 0000000079b06001 CR4: 0000000000360ee0
> > > > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > > > > Call Trace:
> > > > > > ? preempt_count_add+0x58/0xb0
> > > > > > ? _raw_spin_lock_irqsave+0x36/0x70
> > > > > > ? _raw_spin_unlock_irqrestore+0x1a/0x40
> > > > > > ? __wake_up+0x70/0x190
> > > > > > virtnet_set_features+0x90/0xf0 [virtio_net]
> > > > > > __netdev_update_features+0x271/0x980
> > > > > > ? nlmsg_notify+0x5b/0xa0
> > > > > > dev_disable_lro+0x2b/0x190
> > > > > > ? inet_netconf_notify_devconf+0xe2/0x120
> > > > > > devinet_sysctl_forward+0x176/0x1e0
> > > > > > proc_sys_call_handler+0x1f0/0x250
> > > > > > proc_sys_write+0xf/0x20
> > > > > > __vfs_write+0x3e/0x190
> > > > > > ? __sb_start_write+0x6d/0xd0
> > > > > > vfs_write+0xd3/0x190
> > > > > > ksys_write+0x68/0xd0
> > > > > > __ia32_sys_write+0x14/0x20
> > > > > > do_fast_syscall_32+0x86/0xe0
> > > > > > entry_SYSENTER_compat+0x7c/0x8e
> > > > > >
> > > > > > This happens because virtio_set_features() does not check the presence
> > > > > > of the control virtqueue feature, which is sanity checked by a BUG_ON
> > > > > > in virtnet_send_command().
> > > > > >
> > > > > > Fix this by skipping any feature processing if the control virtqueue is
> > > > > > missing. This should be OK for any future feature that is added, as
> > > > > > presumably all of them would require control virtqueue support to notify
> > > > > > the endpoint that offload etc. should begin.
> > > > > >
> > > > > > [1] https://chromium.googlesource.com/chromiumos/platform/crosvm/
> > > > > >
> > > > > > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > > > > > Cc: stable@vger.kernel.org [4.20+]
> > > > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > > > Cc: Jason Wang <jasowang@redhat.com>
> > > > > > Cc: David S. Miller <davem@davemloft.net>
> > > > > > Cc: kernel-team@android.com
> > > > > > Cc: virtualization@lists.linux-foundation.org
> > > > > > Cc: linux-kernel@vger.kernel.org
> > > > > > Signed-off-by: Alistair Delva <adelva@google.com>
> > > > >
> > > > > Thanks for debugging this, Alistair.
> > > > >
> > > > > > ---
> > > > > > drivers/net/virtio_net.c | 3 +++
> > > > > > 1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > index 4d7d5434cc5d..709bcd34e485 100644
> > > > > > --- a/drivers/net/virtio_net.c
> > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > @@ -2560,6 +2560,9 @@ static int virtnet_set_features(struct net_device *dev,
> > > > > > u64 offloads;
> > > > > > int err;
> > > > > >
> > > > > > + if (!vi->has_cvq)
> > > > > > + return 0;
> > > > > > +
> > > > >
> > > > > Instead of checking for this in virtnet_set_features, how about we
> > > > > make configurability contingent on cvq in virtnet_probe:
> > > > >
> > > > > - if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > > > > + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) &&
> > > > > + virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> > > > > dev->hw_features |= NETIF_F_LRO;
> > > > >
> > > > > Based on this logic a little below in the same function
> > > > >
> > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> > > > > vi->has_cvq = true;
> > > >
> > > >
> > > > This would be a regression on old hypervisors which didn't have
> > > > CTL VQ - suddenly they will lose offloads.
> > >
> > > dev->features still correctly displays whether offloads are enabled.
> > > Removing it from dev->hw_features just renders it non-configurable.
> >
> > Oh you are right. I confused it with dev->features.
> >
> > > Note that before the patch that is being fixed the offloads were
> > > enabled, but ethtool would show them as off.
> >
> > So the bug is in spec, it should have said
> > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS depends on VIRTIO_NET_F_CTRL_VQ, but we
> > missed that part. We can and I guess should add this as a recommendation
> > but it's too late to make it a MUST.
> >
> > Meanwhile I would say it's cleanest to work around
> > this in virtnet_validate by clearing VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> > if VIRTIO_NET_F_CTRL_VQ is off, with a big comment explaining
> > it's a spec bug.
>
> Wouldn't that cause precisely the regression you were concerned about?
Not sure how do you mean. VIRTIO_NET_F_CTRL_GUEST_OFFLOADS simply can't
work without a ctrl vq. What's the point of keeping it on?
> Workloads may now depend on LRO for cycle efficiency. Reverting to
> behavior before this patch (though now displaying the offload state
> correctly) is more conservative in that regard.
Do you see a problem with the following (untested):
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4d7d5434cc5d..7b8805b47f0d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2971,6 +2971,15 @@ static int virtnet_validate(struct virtio_device *vdev)
if (!virtnet_validate_features(vdev))
return -EINVAL;
+ /* VIRTIO_NET_F_CTRL_GUEST_OFFLOADS does not work without
+ * VIRTIO_NET_F_CTRL_VQ. Unfortunately spec forgot to
+ * specify that VIRTIO_NET_F_CTRL_GUEST_OFFLOADS depends
+ * on VIRTIO_NET_F_CTRL_VQ so devices can set the later but
+ * not the former.
+ */
+ if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
+ __virtio_clear_bit(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS);
+
if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
int mtu = virtio_cread16(vdev,
offsetof(struct virtio_net_config,
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: Skip set_features on non-cvq devices
2019-12-22 21:12 ` Michael S. Tsirkin
@ 2019-12-22 21:44 ` Willem de Bruijn
2019-12-22 23:14 ` Michael S. Tsirkin
0 siblings, 1 reply; 12+ messages in thread
From: Willem de Bruijn @ 2019-12-22 21:44 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Willem de Bruijn, Alistair Delva, Network Development, stable,
Jason Wang, David S . Miller, kernel-team, virtualization,
linux-kernel
On Sun, Dec 22, 2019 at 4:12 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Dec 22, 2019 at 10:54:23AM -0500, Willem de Bruijn wrote:
> > On Sun, Dec 22, 2019 at 9:57 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Sun, Dec 22, 2019 at 09:21:43AM -0500, Willem de Bruijn wrote:
> > > > On Sun, Dec 22, 2019 at 8:11 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Fri, Dec 20, 2019 at 10:08:41PM -0500, Willem de Bruijn wrote:
> > > > > > On Fri, Dec 20, 2019 at 4:22 PM Alistair Delva <adelva@google.com> wrote:
> > > > > > >
> > > > > > > On devices without control virtqueue support, such as the virtio_net
> > > > > > > implementation in crosvm[1], attempting to configure LRO will panic the
> > > > > > > kernel:
> > > > > > >
> > > > > > > kernel BUG at drivers/net/virtio_net.c:1591!
> > > > > > > invalid opcode: 0000 [#1] PREEMPT SMP PTI
> > > > > > > CPU: 1 PID: 483 Comm: Binder:330_1 Not tainted 5.4.5-01326-g19463e9acaac #1
> > > > > > > Hardware name: ChromiumOS crosvm, BIOS 0
> > > > > > > RIP: 0010:virtnet_send_command+0x15d/0x170 [virtio_net]
> > > > > > > Code: d8 00 00 00 80 78 02 00 0f 94 c0 65 48 8b 0c 25 28 00 00 00 48 3b 4c 24 70 75 11 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b e8 ec a4 12 c8 66 90 66 2e 0f 1f 84 00 00 00 00 00 55 48 89
> > > > > > > RSP: 0018:ffffb97940e7bb50 EFLAGS: 00010246
> > > > > > > RAX: ffffffffc0596020 RBX: ffffa0e1fc8ea840 RCX: 0000000000000017
> > > > > > > RDX: ffffffffc0596110 RSI: 0000000000000011 RDI: 000000000000000d
> > > > > > > RBP: ffffb97940e7bbf8 R08: ffffa0e1fc8ea0b0 R09: ffffa0e1fc8ea0b0
> > > > > > > R10: ffffffffffffffff R11: ffffffffc0590940 R12: 0000000000000005
> > > > > > > R13: ffffa0e1ffad2c00 R14: ffffb97940e7bc08 R15: 0000000000000000
> > > > > > > FS: 0000000000000000(0000) GS:ffffa0e1fd100000(006b) knlGS:00000000e5ef7494
> > > > > > > CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
> > > > > > > CR2: 00000000e5eeb82c CR3: 0000000079b06001 CR4: 0000000000360ee0
> > > > > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > > > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > > > > > Call Trace:
> > > > > > > ? preempt_count_add+0x58/0xb0
> > > > > > > ? _raw_spin_lock_irqsave+0x36/0x70
> > > > > > > ? _raw_spin_unlock_irqrestore+0x1a/0x40
> > > > > > > ? __wake_up+0x70/0x190
> > > > > > > virtnet_set_features+0x90/0xf0 [virtio_net]
> > > > > > > __netdev_update_features+0x271/0x980
> > > > > > > ? nlmsg_notify+0x5b/0xa0
> > > > > > > dev_disable_lro+0x2b/0x190
> > > > > > > ? inet_netconf_notify_devconf+0xe2/0x120
> > > > > > > devinet_sysctl_forward+0x176/0x1e0
> > > > > > > proc_sys_call_handler+0x1f0/0x250
> > > > > > > proc_sys_write+0xf/0x20
> > > > > > > __vfs_write+0x3e/0x190
> > > > > > > ? __sb_start_write+0x6d/0xd0
> > > > > > > vfs_write+0xd3/0x190
> > > > > > > ksys_write+0x68/0xd0
> > > > > > > __ia32_sys_write+0x14/0x20
> > > > > > > do_fast_syscall_32+0x86/0xe0
> > > > > > > entry_SYSENTER_compat+0x7c/0x8e
> > > > > > >
> > > > > > > This happens because virtio_set_features() does not check the presence
> > > > > > > of the control virtqueue feature, which is sanity checked by a BUG_ON
> > > > > > > in virtnet_send_command().
> > > > > > >
> > > > > > > Fix this by skipping any feature processing if the control virtqueue is
> > > > > > > missing. This should be OK for any future feature that is added, as
> > > > > > > presumably all of them would require control virtqueue support to notify
> > > > > > > the endpoint that offload etc. should begin.
> > > > > > >
> > > > > > > [1] https://chromium.googlesource.com/chromiumos/platform/crosvm/
> > > > > > >
> > > > > > > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > > > > > > Cc: stable@vger.kernel.org [4.20+]
> > > > > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > Cc: Jason Wang <jasowang@redhat.com>
> > > > > > > Cc: David S. Miller <davem@davemloft.net>
> > > > > > > Cc: kernel-team@android.com
> > > > > > > Cc: virtualization@lists.linux-foundation.org
> > > > > > > Cc: linux-kernel@vger.kernel.org
> > > > > > > Signed-off-by: Alistair Delva <adelva@google.com>
> > > > > >
> > > > > > Thanks for debugging this, Alistair.
> > > > > >
> > > > > > > ---
> > > > > > > drivers/net/virtio_net.c | 3 +++
> > > > > > > 1 file changed, 3 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > index 4d7d5434cc5d..709bcd34e485 100644
> > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > @@ -2560,6 +2560,9 @@ static int virtnet_set_features(struct net_device *dev,
> > > > > > > u64 offloads;
> > > > > > > int err;
> > > > > > >
> > > > > > > + if (!vi->has_cvq)
> > > > > > > + return 0;
> > > > > > > +
> > > > > >
> > > > > > Instead of checking for this in virtnet_set_features, how about we
> > > > > > make configurability contingent on cvq in virtnet_probe:
> > > > > >
> > > > > > - if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > > > > > + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) &&
> > > > > > + virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> > > > > > dev->hw_features |= NETIF_F_LRO;
> > > > > >
> > > > > > Based on this logic a little below in the same function
> > > > > >
> > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> > > > > > vi->has_cvq = true;
> > > > >
> > > > >
> > > > > This would be a regression on old hypervisors which didn't have
> > > > > CTL VQ - suddenly they will lose offloads.
> > > >
> > > > dev->features still correctly displays whether offloads are enabled.
> > > > Removing it from dev->hw_features just renders it non-configurable.
> > >
> > > Oh you are right. I confused it with dev->features.
> > >
> > > > Note that before the patch that is being fixed the offloads were
> > > > enabled, but ethtool would show them as off.
> > >
> > > So the bug is in spec, it should have said
> > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS depends on VIRTIO_NET_F_CTRL_VQ, but we
> > > missed that part. We can and I guess should add this as a recommendation
> > > but it's too late to make it a MUST.
> > >
> > > Meanwhile I would say it's cleanest to work around
> > > this in virtnet_validate by clearing VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> > > if VIRTIO_NET_F_CTRL_VQ is off, with a big comment explaining
> > > it's a spec bug.
> >
> > Wouldn't that cause precisely the regression you were concerned about?
>
> Not sure how do you mean. VIRTIO_NET_F_CTRL_GUEST_OFFLOADS simply can't
> work without a ctrl vq. What's the point of keeping it on?
Ah, now I was mistaken. I thought that
dev->features |= NETIF_F_LRO
was also contingent on VIRTIO_NET_F_CTRL_GUEST_OFFLOADS. But that's
another (pair of) flag(s), of course
if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
dev->features |= NETIF_F_LRO;
I wonder if this bug is then also triggered when enabling XDP, through
virtnet_clear_guest_offloads. That predates LRO, so would deserve
another Fixes tag.
> > Workloads may now depend on LRO for cycle efficiency. Reverting to
> > behavior before this patch (though now displaying the offload state
> > correctly) is more conservative in that regard.
>
> Do you see a problem with the following (untested):
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4d7d5434cc5d..7b8805b47f0d 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2971,6 +2971,15 @@ static int virtnet_validate(struct virtio_device *vdev)
> if (!virtnet_validate_features(vdev))
> return -EINVAL;
>
> + /* VIRTIO_NET_F_CTRL_GUEST_OFFLOADS does not work without
> + * VIRTIO_NET_F_CTRL_VQ. Unfortunately spec forgot to
> + * specify that VIRTIO_NET_F_CTRL_GUEST_OFFLOADS depends
> + * on VIRTIO_NET_F_CTRL_VQ so devices can set the later but
> + * not the former.
> + */
> + if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> + __virtio_clear_bit(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS);
> +
> if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
> int mtu = virtio_cread16(vdev,
> offsetof(struct virtio_net_config,
>
Looks good to me!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: Skip set_features on non-cvq devices
2019-12-22 21:44 ` Willem de Bruijn
@ 2019-12-22 23:14 ` Michael S. Tsirkin
0 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2019-12-22 23:14 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Alistair Delva, Network Development, stable, Jason Wang,
David S . Miller, kernel-team, virtualization, linux-kernel
On Sun, Dec 22, 2019 at 04:44:31PM -0500, Willem de Bruijn wrote:
> On Sun, Dec 22, 2019 at 4:12 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Sun, Dec 22, 2019 at 10:54:23AM -0500, Willem de Bruijn wrote:
> > > On Sun, Dec 22, 2019 at 9:57 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Sun, Dec 22, 2019 at 09:21:43AM -0500, Willem de Bruijn wrote:
> > > > > On Sun, Dec 22, 2019 at 8:11 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > >
> > > > > > On Fri, Dec 20, 2019 at 10:08:41PM -0500, Willem de Bruijn wrote:
> > > > > > > On Fri, Dec 20, 2019 at 4:22 PM Alistair Delva <adelva@google.com> wrote:
> > > > > > > >
> > > > > > > > On devices without control virtqueue support, such as the virtio_net
> > > > > > > > implementation in crosvm[1], attempting to configure LRO will panic the
> > > > > > > > kernel:
> > > > > > > >
> > > > > > > > kernel BUG at drivers/net/virtio_net.c:1591!
> > > > > > > > invalid opcode: 0000 [#1] PREEMPT SMP PTI
> > > > > > > > CPU: 1 PID: 483 Comm: Binder:330_1 Not tainted 5.4.5-01326-g19463e9acaac #1
> > > > > > > > Hardware name: ChromiumOS crosvm, BIOS 0
> > > > > > > > RIP: 0010:virtnet_send_command+0x15d/0x170 [virtio_net]
> > > > > > > > Code: d8 00 00 00 80 78 02 00 0f 94 c0 65 48 8b 0c 25 28 00 00 00 48 3b 4c 24 70 75 11 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b e8 ec a4 12 c8 66 90 66 2e 0f 1f 84 00 00 00 00 00 55 48 89
> > > > > > > > RSP: 0018:ffffb97940e7bb50 EFLAGS: 00010246
> > > > > > > > RAX: ffffffffc0596020 RBX: ffffa0e1fc8ea840 RCX: 0000000000000017
> > > > > > > > RDX: ffffffffc0596110 RSI: 0000000000000011 RDI: 000000000000000d
> > > > > > > > RBP: ffffb97940e7bbf8 R08: ffffa0e1fc8ea0b0 R09: ffffa0e1fc8ea0b0
> > > > > > > > R10: ffffffffffffffff R11: ffffffffc0590940 R12: 0000000000000005
> > > > > > > > R13: ffffa0e1ffad2c00 R14: ffffb97940e7bc08 R15: 0000000000000000
> > > > > > > > FS: 0000000000000000(0000) GS:ffffa0e1fd100000(006b) knlGS:00000000e5ef7494
> > > > > > > > CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
> > > > > > > > CR2: 00000000e5eeb82c CR3: 0000000079b06001 CR4: 0000000000360ee0
> > > > > > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > > > > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > > > > > > Call Trace:
> > > > > > > > ? preempt_count_add+0x58/0xb0
> > > > > > > > ? _raw_spin_lock_irqsave+0x36/0x70
> > > > > > > > ? _raw_spin_unlock_irqrestore+0x1a/0x40
> > > > > > > > ? __wake_up+0x70/0x190
> > > > > > > > virtnet_set_features+0x90/0xf0 [virtio_net]
> > > > > > > > __netdev_update_features+0x271/0x980
> > > > > > > > ? nlmsg_notify+0x5b/0xa0
> > > > > > > > dev_disable_lro+0x2b/0x190
> > > > > > > > ? inet_netconf_notify_devconf+0xe2/0x120
> > > > > > > > devinet_sysctl_forward+0x176/0x1e0
> > > > > > > > proc_sys_call_handler+0x1f0/0x250
> > > > > > > > proc_sys_write+0xf/0x20
> > > > > > > > __vfs_write+0x3e/0x190
> > > > > > > > ? __sb_start_write+0x6d/0xd0
> > > > > > > > vfs_write+0xd3/0x190
> > > > > > > > ksys_write+0x68/0xd0
> > > > > > > > __ia32_sys_write+0x14/0x20
> > > > > > > > do_fast_syscall_32+0x86/0xe0
> > > > > > > > entry_SYSENTER_compat+0x7c/0x8e
> > > > > > > >
> > > > > > > > This happens because virtio_set_features() does not check the presence
> > > > > > > > of the control virtqueue feature, which is sanity checked by a BUG_ON
> > > > > > > > in virtnet_send_command().
> > > > > > > >
> > > > > > > > Fix this by skipping any feature processing if the control virtqueue is
> > > > > > > > missing. This should be OK for any future feature that is added, as
> > > > > > > > presumably all of them would require control virtqueue support to notify
> > > > > > > > the endpoint that offload etc. should begin.
> > > > > > > >
> > > > > > > > [1] https://chromium.googlesource.com/chromiumos/platform/crosvm/
> > > > > > > >
> > > > > > > > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > > > > > > > Cc: stable@vger.kernel.org [4.20+]
> > > > > > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > > Cc: Jason Wang <jasowang@redhat.com>
> > > > > > > > Cc: David S. Miller <davem@davemloft.net>
> > > > > > > > Cc: kernel-team@android.com
> > > > > > > > Cc: virtualization@lists.linux-foundation.org
> > > > > > > > Cc: linux-kernel@vger.kernel.org
> > > > > > > > Signed-off-by: Alistair Delva <adelva@google.com>
> > > > > > >
> > > > > > > Thanks for debugging this, Alistair.
> > > > > > >
> > > > > > > > ---
> > > > > > > > drivers/net/virtio_net.c | 3 +++
> > > > > > > > 1 file changed, 3 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > > index 4d7d5434cc5d..709bcd34e485 100644
> > > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > > @@ -2560,6 +2560,9 @@ static int virtnet_set_features(struct net_device *dev,
> > > > > > > > u64 offloads;
> > > > > > > > int err;
> > > > > > > >
> > > > > > > > + if (!vi->has_cvq)
> > > > > > > > + return 0;
> > > > > > > > +
> > > > > > >
> > > > > > > Instead of checking for this in virtnet_set_features, how about we
> > > > > > > make configurability contingent on cvq in virtnet_probe:
> > > > > > >
> > > > > > > - if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > > > > > > + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) &&
> > > > > > > + virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> > > > > > > dev->hw_features |= NETIF_F_LRO;
> > > > > > >
> > > > > > > Based on this logic a little below in the same function
> > > > > > >
> > > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> > > > > > > vi->has_cvq = true;
> > > > > >
> > > > > >
> > > > > > This would be a regression on old hypervisors which didn't have
> > > > > > CTL VQ - suddenly they will lose offloads.
> > > > >
> > > > > dev->features still correctly displays whether offloads are enabled.
> > > > > Removing it from dev->hw_features just renders it non-configurable.
> > > >
> > > > Oh you are right. I confused it with dev->features.
> > > >
> > > > > Note that before the patch that is being fixed the offloads were
> > > > > enabled, but ethtool would show them as off.
> > > >
> > > > So the bug is in spec, it should have said
> > > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS depends on VIRTIO_NET_F_CTRL_VQ, but we
> > > > missed that part. We can and I guess should add this as a recommendation
> > > > but it's too late to make it a MUST.
> > > >
> > > > Meanwhile I would say it's cleanest to work around
> > > > this in virtnet_validate by clearing VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> > > > if VIRTIO_NET_F_CTRL_VQ is off, with a big comment explaining
> > > > it's a spec bug.
> > >
> > > Wouldn't that cause precisely the regression you were concerned about?
> >
> > Not sure how do you mean. VIRTIO_NET_F_CTRL_GUEST_OFFLOADS simply can't
> > work without a ctrl vq. What's the point of keeping it on?
>
> Ah, now I was mistaken. I thought that
>
> dev->features |= NETIF_F_LRO
>
> was also contingent on VIRTIO_NET_F_CTRL_GUEST_OFFLOADS. But that's
> another (pair of) flag(s), of course
>
> if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
> dev->features |= NETIF_F_LRO;
>
> I wonder if this bug is then also triggered when enabling XDP, through
> virtnet_clear_guest_offloads. That predates LRO, so would deserve
> another Fixes tag.
Are you sure? I thought LRO has been there before xdp...
>
> > > Workloads may now depend on LRO for cycle efficiency. Reverting to
> > > behavior before this patch (though now displaying the offload state
> > > correctly) is more conservative in that regard.
> >
> > Do you see a problem with the following (untested):
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 4d7d5434cc5d..7b8805b47f0d 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2971,6 +2971,15 @@ static int virtnet_validate(struct virtio_device *vdev)
> > if (!virtnet_validate_features(vdev))
> > return -EINVAL;
> >
> > + /* VIRTIO_NET_F_CTRL_GUEST_OFFLOADS does not work without
> > + * VIRTIO_NET_F_CTRL_VQ. Unfortunately spec forgot to
> > + * specify that VIRTIO_NET_F_CTRL_GUEST_OFFLOADS depends
> > + * on VIRTIO_NET_F_CTRL_VQ so devices can set the later but
> > + * not the former.
> > + */
> > + if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> > + __virtio_clear_bit(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS);
> > +
> > if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
> > int mtu = virtio_cread16(vdev,
> > offsetof(struct virtio_net_config,
> >
>
> Looks good to me!
Alstair could you pls try this patch and report?
Thanks!
--
MST
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: Skip set_features on non-cvq devices
2019-12-20 21:22 [PATCH net] virtio-net: Skip set_features on non-cvq devices Alistair Delva
` (2 preceding siblings ...)
2019-12-22 13:13 ` Michael S. Tsirkin
@ 2020-01-05 13:11 ` Michael S. Tsirkin
3 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2020-01-05 13:11 UTC (permalink / raw)
To: Alistair Delva
Cc: netdev, stable, Jason Wang, David S . Miller, kernel-team,
virtualization, linux-kernel
On Fri, Dec 20, 2019 at 01:22:07PM -0800, Alistair Delva wrote:
> On devices without control virtqueue support, such as the virtio_net
> implementation in crosvm[1], attempting to configure LRO will panic the
> kernel:
>
> kernel BUG at drivers/net/virtio_net.c:1591!
> invalid opcode: 0000 [#1] PREEMPT SMP PTI
> CPU: 1 PID: 483 Comm: Binder:330_1 Not tainted 5.4.5-01326-g19463e9acaac #1
> Hardware name: ChromiumOS crosvm, BIOS 0
> RIP: 0010:virtnet_send_command+0x15d/0x170 [virtio_net]
> Code: d8 00 00 00 80 78 02 00 0f 94 c0 65 48 8b 0c 25 28 00 00 00 48 3b 4c 24 70 75 11 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b e8 ec a4 12 c8 66 90 66 2e 0f 1f 84 00 00 00 00 00 55 48 89
> RSP: 0018:ffffb97940e7bb50 EFLAGS: 00010246
> RAX: ffffffffc0596020 RBX: ffffa0e1fc8ea840 RCX: 0000000000000017
> RDX: ffffffffc0596110 RSI: 0000000000000011 RDI: 000000000000000d
> RBP: ffffb97940e7bbf8 R08: ffffa0e1fc8ea0b0 R09: ffffa0e1fc8ea0b0
> R10: ffffffffffffffff R11: ffffffffc0590940 R12: 0000000000000005
> R13: ffffa0e1ffad2c00 R14: ffffb97940e7bc08 R15: 0000000000000000
> FS: 0000000000000000(0000) GS:ffffa0e1fd100000(006b) knlGS:00000000e5ef7494
> CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
> CR2: 00000000e5eeb82c CR3: 0000000079b06001 CR4: 0000000000360ee0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> ? preempt_count_add+0x58/0xb0
> ? _raw_spin_lock_irqsave+0x36/0x70
> ? _raw_spin_unlock_irqrestore+0x1a/0x40
> ? __wake_up+0x70/0x190
> virtnet_set_features+0x90/0xf0 [virtio_net]
> __netdev_update_features+0x271/0x980
> ? nlmsg_notify+0x5b/0xa0
> dev_disable_lro+0x2b/0x190
> ? inet_netconf_notify_devconf+0xe2/0x120
> devinet_sysctl_forward+0x176/0x1e0
> proc_sys_call_handler+0x1f0/0x250
> proc_sys_write+0xf/0x20
> __vfs_write+0x3e/0x190
> ? __sb_start_write+0x6d/0xd0
> vfs_write+0xd3/0x190
> ksys_write+0x68/0xd0
> __ia32_sys_write+0x14/0x20
> do_fast_syscall_32+0x86/0xe0
> entry_SYSENTER_compat+0x7c/0x8e
>
> This happens because virtio_set_features() does not check the presence
> of the control virtqueue feature, which is sanity checked by a BUG_ON
> in virtnet_send_command().
>
> Fix this by skipping any feature processing if the control virtqueue is
> missing. This should be OK for any future feature that is added, as
> presumably all of them would require control virtqueue support to notify
> the endpoint that offload etc. should begin.
>
> [1] https://chromium.googlesource.com/chromiumos/platform/crosvm/
>
> Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> Cc: stable@vger.kernel.org [4.20+]
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: kernel-team@android.com
> Cc: virtualization@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Alistair Delva <adelva@google.com>
> ---
> drivers/net/virtio_net.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4d7d5434cc5d..709bcd34e485 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2560,6 +2560,9 @@ static int virtnet_set_features(struct net_device *dev,
> u64 offloads;
> int err;
>
> + if (!vi->has_cvq)
> + return 0;
> +
So should this return an error then?
> if ((dev->features ^ features) & NETIF_F_LRO) {
> if (vi->xdp_queue_pairs)
> return -EBUSY;
> --
> 2.24.1.735.g03f4e72817-goog
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-01-05 13:11 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-20 21:22 [PATCH net] virtio-net: Skip set_features on non-cvq devices Alistair Delva
2019-12-20 22:40 ` Stephen Barber
2019-12-21 3:08 ` Willem de Bruijn
2019-12-22 13:11 ` Michael S. Tsirkin
2019-12-22 14:21 ` Willem de Bruijn
2019-12-22 14:57 ` Michael S. Tsirkin
2019-12-22 15:54 ` Willem de Bruijn
2019-12-22 21:12 ` Michael S. Tsirkin
2019-12-22 21:44 ` Willem de Bruijn
2019-12-22 23:14 ` Michael S. Tsirkin
2019-12-22 13:13 ` Michael S. Tsirkin
2020-01-05 13:11 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).