* [PATCH net] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ @ 2019-12-23 14:09 Michael S. Tsirkin 2019-12-23 17:40 ` Alistair Delva 0 siblings, 1 reply; 11+ messages in thread From: Michael S. Tsirkin @ 2019-12-23 14:09 UTC (permalink / raw) To: linux-kernel Cc: Alistair Delva, Willem de Bruijn, Jason Wang, David S. Miller, virtualization, netdev The only way for guest to control offloads (as enabled by VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) is by sending commands through CTRL_VQ. So it does not make sense to acknowledge VIRTIO_NET_F_CTRL_GUEST_OFFLOADS without VIRTIO_NET_F_CTRL_VQ. The spec does not outlaw devices with such a configuration, but Linux assumed that with VIRTIO_NET_F_CTRL_GUEST_OFFLOADS control vq is always there, resulting in the following crash when configuring LRO: 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 A similar crash will likely trigger when enabling XDP. Reported-by: Alistair Delva <adelva@google.com> Reported-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set") Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- Lightly tested. Alistair, could you please test and confirm that this resolves the crash for you? Dave, after testing confirms the fix, pls queue up for stable. drivers/net/virtio_net.c | 9 +++++++++ 1 file changed, 9 insertions(+) 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. However the virtio spec does not + * 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, -- MST ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ 2019-12-23 14:09 [PATCH net] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ Michael S. Tsirkin @ 2019-12-23 17:40 ` Alistair Delva 2019-12-23 19:44 ` Michael S. Tsirkin 2019-12-23 19:56 ` Willem de Bruijn 0 siblings, 2 replies; 11+ messages in thread From: Alistair Delva @ 2019-12-23 17:40 UTC (permalink / raw) To: Michael S. Tsirkin Cc: LKML, Willem de Bruijn, Jason Wang, David S. Miller, virtualization, netdev Hi Michael, On Mon, Dec 23, 2019 at 6:09 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > The only way for guest to control offloads (as enabled by > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) is by sending commands > through CTRL_VQ. So it does not make sense to > acknowledge VIRTIO_NET_F_CTRL_GUEST_OFFLOADS without > VIRTIO_NET_F_CTRL_VQ. > > The spec does not outlaw devices with such a configuration, > but Linux assumed that with VIRTIO_NET_F_CTRL_GUEST_OFFLOADS > control vq is always there, resulting in the following crash > when configuring LRO: > > 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 > > A similar crash will likely trigger when enabling XDP. > > Reported-by: Alistair Delva <adelva@google.com> > Reported-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set") > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Lightly tested. > > Alistair, could you please test and confirm that this resolves the > crash for you? This patch doesn't work. The reason is that NETIF_F_LRO is also turned on by TSO4/TSO6, which your patch didn't check for. So it ends up going through the same path and crashing in the same way. if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) || virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6)) dev->features |= NETIF_F_LRO; It sounds like this patch is fixing something slightly differently to my patch fixed. virtnet_set_features() doesn't care about GUEST_OFFLOADS, it only tests against NETIF_F_LRO. Even if "offloads" is zero, it will call virtnet_set_guest_offloads(), which triggers the crash. So either we need to ensure NETIF_F_LRO is never set, or virtnet_set_features needs to be updated to check for GUEST_OFFLOADS, right? > Dave, after testing confirms the fix, pls queue up for stable. > > > drivers/net/virtio_net.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > 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. However the virtio spec does not > + * 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, > -- > MST > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ 2019-12-23 17:40 ` Alistair Delva @ 2019-12-23 19:44 ` Michael S. Tsirkin 2019-12-23 19:56 ` Willem de Bruijn 1 sibling, 0 replies; 11+ messages in thread From: Michael S. Tsirkin @ 2019-12-23 19:44 UTC (permalink / raw) To: Alistair Delva Cc: LKML, Willem de Bruijn, Jason Wang, David S. Miller, virtualization, netdev On Mon, Dec 23, 2019 at 09:40:11AM -0800, Alistair Delva wrote: > Hi Michael, > > On Mon, Dec 23, 2019 at 6:09 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > The only way for guest to control offloads (as enabled by > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) is by sending commands > > through CTRL_VQ. So it does not make sense to > > acknowledge VIRTIO_NET_F_CTRL_GUEST_OFFLOADS without > > VIRTIO_NET_F_CTRL_VQ. > > > > The spec does not outlaw devices with such a configuration, > > but Linux assumed that with VIRTIO_NET_F_CTRL_GUEST_OFFLOADS > > control vq is always there, resulting in the following crash > > when configuring LRO: > > > > 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 > > > > A similar crash will likely trigger when enabling XDP. > > > > Reported-by: Alistair Delva <adelva@google.com> > > Reported-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set") > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > Lightly tested. > > > > Alistair, could you please test and confirm that this resolves the > > crash for you? > > This patch doesn't work. The reason is that NETIF_F_LRO is also turned > on by TSO4/TSO6, which your patch didn't check for. So it ends up > going through the same path and crashing in the same way. > > if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) || > virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6)) > dev->features |= NETIF_F_LRO; > > It sounds like this patch is fixing something slightly differently to > my patch fixed. virtnet_set_features() doesn't care about > GUEST_OFFLOADS, it only tests against NETIF_F_LRO. Even if "offloads" > is zero, it will call virtnet_set_guest_offloads(), which triggers the > crash. Oh wait a second. You mean when we try to clear the offloads, right? > So either we need to ensure NETIF_F_LRO is never set, or > virtnet_set_features needs to be updated to check for GUEST_OFFLOADS, > right? So we have: static int virtnet_set_features(struct net_device *dev, netdev_features_t features) { struct virtnet_info *vi = netdev_priv(dev); u64 offloads; int err; if ((dev->features ^ features) & NETIF_F_LRO) { if (vi->xdp_queue_pairs) return -EBUSY; if (features & NETIF_F_LRO) offloads = vi->guest_offloads_capable; else offloads = 0; err = virtnet_set_guest_offloads(vi, offloads); if (err) return err; vi->guest_offloads = offloads; } return 0; } and static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads) { struct scatterlist sg; vi->ctrl->offloads = cpu_to_virtio64(vi->vdev, offloads); sg_init_one(&sg, &vi->ctrl->offloads, sizeof(vi->ctrl->offloads)); if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_GUEST_OFFLOADS, VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, &sg)) { dev_warn(&vi->dev->dev, "Fail to set guest offload.\n"); return -EINVAL; } return 0; } finally static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, struct scatterlist *out) { struct scatterlist *sgs[4], hdr, stat; unsigned out_num = 0, tmp; /* Caller should know better */ BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)); vi->ctrl->status = ~0; ... } so return false here and we should be good right? > > Dave, after testing confirms the fix, pls queue up for stable. > > > > > > drivers/net/virtio_net.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > 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. However the virtio spec does not > > + * 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, > > -- > > MST > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ 2019-12-23 17:40 ` Alistair Delva 2019-12-23 19:44 ` Michael S. Tsirkin @ 2019-12-23 19:56 ` Willem de Bruijn 2019-12-23 20:12 ` Willem de Bruijn 1 sibling, 1 reply; 11+ messages in thread From: Willem de Bruijn @ 2019-12-23 19:56 UTC (permalink / raw) To: Alistair Delva Cc: Michael S. Tsirkin, LKML, Willem de Bruijn, Jason Wang, David S. Miller, virtualization, Network Development 00fffe0ff0 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 > > > > A similar crash will likely trigger when enabling XDP. > > > > Reported-by: Alistair Delva <adelva@google.com> > > Reported-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set") > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > Lightly tested. > > > > Alistair, could you please test and confirm that this resolves the > > crash for you? > > This patch doesn't work. The reason is that NETIF_F_LRO is also turned > on by TSO4/TSO6, which your patch didn't check for. So it ends up > going through the same path and crashing in the same way. > > if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) || > virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6)) > dev->features |= NETIF_F_LRO; > > It sounds like this patch is fixing something slightly differently to > my patch fixed. virtnet_set_features() doesn't care about > GUEST_OFFLOADS, it only tests against NETIF_F_LRO. Even if "offloads" > is zero, it will call virtnet_set_guest_offloads(), which triggers the > crash. Interesting. It's surprising that it is trying to configure a flag that is not configurable, i.e., absent from dev->hw_features after Michael's change. > So either we need to ensure NETIF_F_LRO is never set, or LRO might be available, just not configurable. Indeed this was what I observed in the past. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ 2019-12-23 19:56 ` Willem de Bruijn @ 2019-12-23 20:12 ` Willem de Bruijn 2019-12-23 20:21 ` Alistair Delva 0 siblings, 1 reply; 11+ messages in thread From: Willem de Bruijn @ 2019-12-23 20:12 UTC (permalink / raw) To: Willem de Bruijn Cc: Alistair Delva, Michael S. Tsirkin, LKML, Jason Wang, David S. Miller, virtualization, Network Development On Mon, Dec 23, 2019 at 2:56 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > 00fffe0ff0 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 > > > > > > A similar crash will likely trigger when enabling XDP. > > > > > > Reported-by: Alistair Delva <adelva@google.com> > > > Reported-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > > > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set") > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > --- > > > > > > Lightly tested. > > > > > > Alistair, could you please test and confirm that this resolves the > > > crash for you? > > > > This patch doesn't work. The reason is that NETIF_F_LRO is also turned > > on by TSO4/TSO6, which your patch didn't check for. So it ends up > > going through the same path and crashing in the same way. > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) || > > virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6)) > > dev->features |= NETIF_F_LRO; > > > > It sounds like this patch is fixing something slightly differently to > > my patch fixed. virtnet_set_features() doesn't care about > > GUEST_OFFLOADS, it only tests against NETIF_F_LRO. Even if "offloads" > > is zero, it will call virtnet_set_guest_offloads(), which triggers the > > crash. > > > Interesting. It's surprising that it is trying to configure a flag > that is not configurable, i.e., absent from dev->hw_features > after Michael's change. > > > So either we need to ensure NETIF_F_LRO is never set, or > > LRO might be available, just not configurable. Indeed this was what I > observed in the past. dev_disable_lro expects that NETIF_F_LRO is always configurable. Which I guess is a reasonable assumption, just not necessarily the case in virtio_net. So I think we need both patches. Correctly mark the feature as fixed by removing from dev->hw_features and also ignore the request from dev_disable_lro, which does not check for this. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ 2019-12-23 20:12 ` Willem de Bruijn @ 2019-12-23 20:21 ` Alistair Delva 2019-12-24 2:49 ` Jason Wang ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Alistair Delva @ 2019-12-23 20:21 UTC (permalink / raw) To: Willem de Bruijn Cc: Michael S. Tsirkin, LKML, Jason Wang, David S. Miller, virtualization, Network Development On Mon, Dec 23, 2019 at 12:12 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Mon, Dec 23, 2019 at 2:56 PM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > 00fffe0ff0 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 > > > > > > > > A similar crash will likely trigger when enabling XDP. > > > > > > > > Reported-by: Alistair Delva <adelva@google.com> > > > > Reported-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > > > > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set") > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > --- > > > > > > > > Lightly tested. > > > > > > > > Alistair, could you please test and confirm that this resolves the > > > > crash for you? > > > > > > This patch doesn't work. The reason is that NETIF_F_LRO is also turned > > > on by TSO4/TSO6, which your patch didn't check for. So it ends up > > > going through the same path and crashing in the same way. > > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) || > > > virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6)) > > > dev->features |= NETIF_F_LRO; > > > > > > It sounds like this patch is fixing something slightly differently to > > > my patch fixed. virtnet_set_features() doesn't care about > > > GUEST_OFFLOADS, it only tests against NETIF_F_LRO. Even if "offloads" > > > is zero, it will call virtnet_set_guest_offloads(), which triggers the > > > crash. > > > > > > Interesting. It's surprising that it is trying to configure a flag > > that is not configurable, i.e., absent from dev->hw_features > > after Michael's change. > > > > > So either we need to ensure NETIF_F_LRO is never set, or > > > > LRO might be available, just not configurable. Indeed this was what I > > observed in the past. > > dev_disable_lro expects that NETIF_F_LRO is always configurable. Which > I guess is a reasonable assumption, just not necessarily the case in > virtio_net. > > So I think we need both patches. Correctly mark the feature as fixed > by removing from dev->hw_features and also ignore the request from > dev_disable_lro, which does not check for this. Something like this maybe: diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 4d7d5434cc5d..0556f42b0fb5 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 (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) + return 0; + if ((dev->features ^ features) & NETIF_F_LRO) { if (vi->xdp_queue_pairs) return -EBUSY; @@ -2971,6 +2974,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. However the virtio spec does not + * 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] 11+ messages in thread
* Re: [PATCH net] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ 2019-12-23 20:21 ` Alistair Delva @ 2019-12-24 2:49 ` Jason Wang 2020-01-05 11:16 ` Michael S. Tsirkin 2019-12-25 16:02 ` Willem de Bruijn 2020-01-05 13:13 ` Michael S. Tsirkin 2 siblings, 1 reply; 11+ messages in thread From: Jason Wang @ 2019-12-24 2:49 UTC (permalink / raw) To: Alistair Delva, Willem de Bruijn Cc: Michael S. Tsirkin, LKML, David S. Miller, virtualization, Network Development On 2019/12/24 上午4:21, Alistair Delva wrote: > On Mon, Dec 23, 2019 at 12:12 PM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: >> On Mon, Dec 23, 2019 at 2:56 PM Willem de Bruijn >> <willemdebruijn.kernel@gmail.com> wrote: >>> 00fffe0ff0 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 >>>>> >>>>> A similar crash will likely trigger when enabling XDP. >>>>> >>>>> Reported-by: Alistair Delva <adelva@google.com> >>>>> Reported-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com> >>>>> Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set") >>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>>> --- >>>>> >>>>> Lightly tested. >>>>> >>>>> Alistair, could you please test and confirm that this resolves the >>>>> crash for you? >>>> This patch doesn't work. The reason is that NETIF_F_LRO is also turned >>>> on by TSO4/TSO6, which your patch didn't check for. So it ends up >>>> going through the same path and crashing in the same way. >>>> >>>> if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) || >>>> virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6)) >>>> dev->features |= NETIF_F_LRO; >>>> >>>> It sounds like this patch is fixing something slightly differently to >>>> my patch fixed. virtnet_set_features() doesn't care about >>>> GUEST_OFFLOADS, it only tests against NETIF_F_LRO. Even if "offloads" >>>> is zero, it will call virtnet_set_guest_offloads(), which triggers the >>>> crash. >>> >>> Interesting. It's surprising that it is trying to configure a flag >>> that is not configurable, i.e., absent from dev->hw_features >>> after Michael's change. >>> >>>> So either we need to ensure NETIF_F_LRO is never set, or >>> LRO might be available, just not configurable. Indeed this was what I >>> observed in the past. >> dev_disable_lro expects that NETIF_F_LRO is always configurable. Which >> I guess is a reasonable assumption, just not necessarily the case in >> virtio_net. >> >> So I think we need both patches. Correctly mark the feature as fixed >> by removing from dev->hw_features and also ignore the request from >> dev_disable_lro, which does not check for this. > Something like this maybe: > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 4d7d5434cc5d..0556f42b0fb5 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 (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) > + return 0; > + > if ((dev->features ^ features) & NETIF_F_LRO) { > if (vi->xdp_queue_pairs) > return -EBUSY; > @@ -2971,6 +2974,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. However the virtio spec does not > + * 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, > We check feature dependency and fail the probe in virtnet_validate_features(). Is it more straightforward to fail the probe there when CTRL_GUEST_OFFLOADS was set but CTRL_VQ wasn't? Thanks ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ 2019-12-24 2:49 ` Jason Wang @ 2020-01-05 11:16 ` Michael S. Tsirkin 0 siblings, 0 replies; 11+ messages in thread From: Michael S. Tsirkin @ 2020-01-05 11:16 UTC (permalink / raw) To: Jason Wang Cc: Alistair Delva, Willem de Bruijn, LKML, David S. Miller, virtualization, Network Development On Tue, Dec 24, 2019 at 10:49:13AM +0800, Jason Wang wrote: > > On 2019/12/24 上午4:21, Alistair Delva wrote: > > On Mon, Dec 23, 2019 at 12:12 PM Willem de Bruijn > > <willemdebruijn.kernel@gmail.com> wrote: > > > On Mon, Dec 23, 2019 at 2:56 PM Willem de Bruijn > > > <willemdebruijn.kernel@gmail.com> wrote: > > > > 00fffe0ff0 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 > > > > > > > > > > > > A similar crash will likely trigger when enabling XDP. > > > > > > > > > > > > Reported-by: Alistair Delva <adelva@google.com> > > > > > > Reported-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > > > > > > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set") > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > > --- > > > > > > > > > > > > Lightly tested. > > > > > > > > > > > > Alistair, could you please test and confirm that this resolves the > > > > > > crash for you? > > > > > This patch doesn't work. The reason is that NETIF_F_LRO is also turned > > > > > on by TSO4/TSO6, which your patch didn't check for. So it ends up > > > > > going through the same path and crashing in the same way. > > > > > > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) || > > > > > virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6)) > > > > > dev->features |= NETIF_F_LRO; > > > > > > > > > > It sounds like this patch is fixing something slightly differently to > > > > > my patch fixed. virtnet_set_features() doesn't care about > > > > > GUEST_OFFLOADS, it only tests against NETIF_F_LRO. Even if "offloads" > > > > > is zero, it will call virtnet_set_guest_offloads(), which triggers the > > > > > crash. > > > > > > > > Interesting. It's surprising that it is trying to configure a flag > > > > that is not configurable, i.e., absent from dev->hw_features > > > > after Michael's change. > > > > > > > > > So either we need to ensure NETIF_F_LRO is never set, or > > > > LRO might be available, just not configurable. Indeed this was what I > > > > observed in the past. > > > dev_disable_lro expects that NETIF_F_LRO is always configurable. Which > > > I guess is a reasonable assumption, just not necessarily the case in > > > virtio_net. > > > > > > So I think we need both patches. Correctly mark the feature as fixed > > > by removing from dev->hw_features and also ignore the request from > > > dev_disable_lro, which does not check for this. > > Something like this maybe: > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 4d7d5434cc5d..0556f42b0fb5 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 (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) > > + return 0; > > + > > if ((dev->features ^ features) & NETIF_F_LRO) { > > if (vi->xdp_queue_pairs) > > return -EBUSY; > > @@ -2971,6 +2974,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. However the virtio spec does not > > + * 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, > > > > We check feature dependency and fail the probe in > virtnet_validate_features(). > > Is it more straightforward to fail the probe there when CTRL_GUEST_OFFLOADS > was set but CTRL_VQ wasn't? > > Thanks Expanding on what the comment above says, we can't fail probe in this configuration without breaking the driver for spec compliant devices. -- MST ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ 2019-12-23 20:21 ` Alistair Delva 2019-12-24 2:49 ` Jason Wang @ 2019-12-25 16:02 ` Willem de Bruijn 2019-12-26 19:12 ` Alistair Delva 2020-01-05 13:13 ` Michael S. Tsirkin 2 siblings, 1 reply; 11+ messages in thread From: Willem de Bruijn @ 2019-12-25 16:02 UTC (permalink / raw) To: Alistair Delva Cc: Willem de Bruijn, Michael S. Tsirkin, LKML, Jason Wang, David S. Miller, virtualization, Network Development On Mon, Dec 23, 2019 at 3:22 PM Alistair Delva <adelva@google.com> wrote: > > On Mon, Dec 23, 2019 at 12:12 PM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > On Mon, Dec 23, 2019 at 2:56 PM Willem de Bruijn > > <willemdebruijn.kernel@gmail.com> wrote: > > > > > > 00fffe0ff0 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 > > > > > > > > > > A similar crash will likely trigger when enabling XDP. > > > > > > > > > > Reported-by: Alistair Delva <adelva@google.com> > > > > > Reported-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > > > > > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set") > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > --- > > > > > > > > > > Lightly tested. > > > > > > > > > > Alistair, could you please test and confirm that this resolves the > > > > > crash for you? > > > > > > > > This patch doesn't work. The reason is that NETIF_F_LRO is also turned > > > > on by TSO4/TSO6, which your patch didn't check for. So it ends up > > > > going through the same path and crashing in the same way. > > > > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) || > > > > virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6)) > > > > dev->features |= NETIF_F_LRO; > > > > > > > > It sounds like this patch is fixing something slightly differently to > > > > my patch fixed. virtnet_set_features() doesn't care about > > > > GUEST_OFFLOADS, it only tests against NETIF_F_LRO. Even if "offloads" > > > > is zero, it will call virtnet_set_guest_offloads(), which triggers the > > > > crash. > > > > > > > > > Interesting. It's surprising that it is trying to configure a flag > > > that is not configurable, i.e., absent from dev->hw_features > > > after Michael's change. > > > > > > > So either we need to ensure NETIF_F_LRO is never set, or > > > > > > LRO might be available, just not configurable. Indeed this was what I > > > observed in the past. > > > > dev_disable_lro expects that NETIF_F_LRO is always configurable. Which > > I guess is a reasonable assumption, just not necessarily the case in > > virtio_net. > > > > So I think we need both patches. Correctly mark the feature as fixed > > by removing from dev->hw_features and also ignore the request from > > dev_disable_lro, which does not check for this. > > Something like this maybe: > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 4d7d5434cc5d..0556f42b0fb5 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 (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) > + return 0; > + > if ((dev->features ^ features) & NETIF_F_LRO) { > if (vi->xdp_queue_pairs) > return -EBUSY; > @@ -2971,6 +2974,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. However the virtio spec does not > + * 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, I think we need three separate patches. - disable guest offloads if VQ is absent - remove LRO from hw_features if guest offloads are absent This should fix the ethtool path. But there is a separate path to disable LRO through dev_disable_lro (from bond enslave, xdp install, sysctl -n net.ipv.ip_forward=1, ..) that assumes LRO is always configurable. So we cannot work around needing a patch to virtset_set_features. After a long detour, I think your original submission is fine for that. Perhaps with a comment in the commit that it is needed even if the feature is absent from hw_features for dev_disable_lro. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ 2019-12-25 16:02 ` Willem de Bruijn @ 2019-12-26 19:12 ` Alistair Delva 0 siblings, 0 replies; 11+ messages in thread From: Alistair Delva @ 2019-12-26 19:12 UTC (permalink / raw) To: Willem de Bruijn Cc: Michael S. Tsirkin, LKML, Jason Wang, David S. Miller, virtualization, Network Development On Wed, Dec 25, 2019 at 8:02 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Mon, Dec 23, 2019 at 3:22 PM Alistair Delva <adelva@google.com> wrote: > > > > On Mon, Dec 23, 2019 at 12:12 PM Willem de Bruijn > > <willemdebruijn.kernel@gmail.com> wrote: > > > > > > On Mon, Dec 23, 2019 at 2:56 PM Willem de Bruijn > > > <willemdebruijn.kernel@gmail.com> wrote: > > > > > > > > 00fffe0ff0 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 > > > > > > > > > > > > A similar crash will likely trigger when enabling XDP. > > > > > > > > > > > > Reported-by: Alistair Delva <adelva@google.com> > > > > > > Reported-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > > > > > > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set") > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > > --- > > > > > > > > > > > > Lightly tested. > > > > > > > > > > > > Alistair, could you please test and confirm that this resolves the > > > > > > crash for you? > > > > > > > > > > This patch doesn't work. The reason is that NETIF_F_LRO is also turned > > > > > on by TSO4/TSO6, which your patch didn't check for. So it ends up > > > > > going through the same path and crashing in the same way. > > > > > > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) || > > > > > virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6)) > > > > > dev->features |= NETIF_F_LRO; > > > > > > > > > > It sounds like this patch is fixing something slightly differently to > > > > > my patch fixed. virtnet_set_features() doesn't care about > > > > > GUEST_OFFLOADS, it only tests against NETIF_F_LRO. Even if "offloads" > > > > > is zero, it will call virtnet_set_guest_offloads(), which triggers the > > > > > crash. > > > > > > > > > > > > Interesting. It's surprising that it is trying to configure a flag > > > > that is not configurable, i.e., absent from dev->hw_features > > > > after Michael's change. > > > > > > > > > So either we need to ensure NETIF_F_LRO is never set, or > > > > > > > > LRO might be available, just not configurable. Indeed this was what I > > > > observed in the past. > > > > > > dev_disable_lro expects that NETIF_F_LRO is always configurable. Which > > > I guess is a reasonable assumption, just not necessarily the case in > > > virtio_net. > > > > > > So I think we need both patches. Correctly mark the feature as fixed > > > by removing from dev->hw_features and also ignore the request from > > > dev_disable_lro, which does not check for this. > > > > Something like this maybe: > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 4d7d5434cc5d..0556f42b0fb5 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 (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) > > + return 0; > > + > > if ((dev->features ^ features) & NETIF_F_LRO) { > > if (vi->xdp_queue_pairs) > > return -EBUSY; > > @@ -2971,6 +2974,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. However the virtio spec does not > > + * 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, > > I think we need three separate patches. > > - disable guest offloads if VQ is absent > - remove LRO from hw_features if guest offloads are absent > > This should fix the ethtool path. But there is a separate path to > disable LRO through dev_disable_lro (from bond enslave, xdp install, > sysctl -n net.ipv.ip_forward=1, ..) that assumes LRO is always > configurable. So we cannot work around needing a patch to > virtset_set_features. > > After a long detour, I think your original submission is fine for > that. Alright. Knowing all the use cases now, and if nobody beats me to it, I'll resubmit this as a series which fixes the three paths. > Perhaps with a comment in the commit that it is needed even > if the feature is absent from hw_features for dev_disable_lro. Ack. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ 2019-12-23 20:21 ` Alistair Delva 2019-12-24 2:49 ` Jason Wang 2019-12-25 16:02 ` Willem de Bruijn @ 2020-01-05 13:13 ` Michael S. Tsirkin 2 siblings, 0 replies; 11+ messages in thread From: Michael S. Tsirkin @ 2020-01-05 13:13 UTC (permalink / raw) To: Alistair Delva Cc: Willem de Bruijn, LKML, Jason Wang, David S. Miller, virtualization, Network Development On Mon, Dec 23, 2019 at 12:21:56PM -0800, Alistair Delva wrote: > On Mon, Dec 23, 2019 at 12:12 PM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > On Mon, Dec 23, 2019 at 2:56 PM Willem de Bruijn > > <willemdebruijn.kernel@gmail.com> wrote: > > > > > > 00fffe0ff0 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 > > > > > > > > > > A similar crash will likely trigger when enabling XDP. > > > > > > > > > > Reported-by: Alistair Delva <adelva@google.com> > > > > > Reported-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > > > > > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set") > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > --- > > > > > > > > > > Lightly tested. > > > > > > > > > > Alistair, could you please test and confirm that this resolves the > > > > > crash for you? > > > > > > > > This patch doesn't work. The reason is that NETIF_F_LRO is also turned > > > > on by TSO4/TSO6, which your patch didn't check for. So it ends up > > > > going through the same path and crashing in the same way. > > > > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) || > > > > virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6)) > > > > dev->features |= NETIF_F_LRO; > > > > > > > > It sounds like this patch is fixing something slightly differently to > > > > my patch fixed. virtnet_set_features() doesn't care about > > > > GUEST_OFFLOADS, it only tests against NETIF_F_LRO. Even if "offloads" > > > > is zero, it will call virtnet_set_guest_offloads(), which triggers the > > > > crash. > > > > > > > > > Interesting. It's surprising that it is trying to configure a flag > > > that is not configurable, i.e., absent from dev->hw_features > > > after Michael's change. > > > > > > > So either we need to ensure NETIF_F_LRO is never set, or > > > > > > LRO might be available, just not configurable. Indeed this was what I > > > observed in the past. > > > > dev_disable_lro expects that NETIF_F_LRO is always configurable. Which > > I guess is a reasonable assumption, just not necessarily the case in > > virtio_net. > > > > So I think we need both patches. Correctly mark the feature as fixed > > by removing from dev->hw_features and also ignore the request from > > dev_disable_lro, which does not check for this. > > Something like this maybe: > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 4d7d5434cc5d..0556f42b0fb5 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 (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) > + return 0; > + > if ((dev->features ^ features) & NETIF_F_LRO) { > if (vi->xdp_queue_pairs) > return -EBUSY; Should this return error here? > @@ -2971,6 +2974,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. However the virtio spec does not > + * 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, This is just my virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ right? ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-01-05 13:14 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-23 14:09 [PATCH net] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ Michael S. Tsirkin 2019-12-23 17:40 ` Alistair Delva 2019-12-23 19:44 ` Michael S. Tsirkin 2019-12-23 19:56 ` Willem de Bruijn 2019-12-23 20:12 ` Willem de Bruijn 2019-12-23 20:21 ` Alistair Delva 2019-12-24 2:49 ` Jason Wang 2020-01-05 11:16 ` Michael S. Tsirkin 2019-12-25 16:02 ` Willem de Bruijn 2019-12-26 19:12 ` Alistair Delva 2020-01-05 13:13 ` 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).