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 <[email protected]>
Reported-by: Willem de Bruijn <[email protected]>
Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set")
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
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
Hi Michael,
On Mon, Dec 23, 2019 at 6:09 AM Michael S. Tsirkin <[email protected]> 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 <[email protected]>
> Reported-by: Willem de Bruijn <[email protected]>
> Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set")
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
>
> 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
>
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 <[email protected]> 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 <[email protected]>
> > Reported-by: Willem de Bruijn <[email protected]>
> > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set")
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > ---
> >
> > 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
> >
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 <[email protected]>
> > Reported-by: Willem de Bruijn <[email protected]>
> > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set")
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > ---
> >
> > 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.
On Mon, Dec 23, 2019 at 2:56 PM Willem de Bruijn
<[email protected]> 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 <[email protected]>
> > > Reported-by: Willem de Bruijn <[email protected]>
> > > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set")
> > > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > > ---
> > >
> > > 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.
On Mon, Dec 23, 2019 at 12:12 PM Willem de Bruijn
<[email protected]> wrote:
>
> On Mon, Dec 23, 2019 at 2:56 PM Willem de Bruijn
> <[email protected]> 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 <[email protected]>
> > > > Reported-by: Willem de Bruijn <[email protected]>
> > > > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set")
> > > > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > > > ---
> > > >
> > > > 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,
On 2019/12/24 上午4:21, Alistair Delva wrote:
> On Mon, Dec 23, 2019 at 12:12 PM Willem de Bruijn
> <[email protected]> wrote:
>> On Mon, Dec 23, 2019 at 2:56 PM Willem de Bruijn
>> <[email protected]> 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 <[email protected]>
>>>>> Reported-by: Willem de Bruijn <[email protected]>
>>>>> Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set")
>>>>> Signed-off-by: Michael S. Tsirkin <[email protected]>
>>>>> ---
>>>>>
>>>>> 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
On Mon, Dec 23, 2019 at 3:22 PM Alistair Delva <[email protected]> wrote:
>
> On Mon, Dec 23, 2019 at 12:12 PM Willem de Bruijn
> <[email protected]> wrote:
> >
> > On Mon, Dec 23, 2019 at 2:56 PM Willem de Bruijn
> > <[email protected]> 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 <[email protected]>
> > > > > Reported-by: Willem de Bruijn <[email protected]>
> > > > > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set")
> > > > > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > > > > ---
> > > > >
> > > > > 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.
On Wed, Dec 25, 2019 at 8:02 AM Willem de Bruijn
<[email protected]> wrote:
>
> On Mon, Dec 23, 2019 at 3:22 PM Alistair Delva <[email protected]> wrote:
> >
> > On Mon, Dec 23, 2019 at 12:12 PM Willem de Bruijn
> > <[email protected]> wrote:
> > >
> > > On Mon, Dec 23, 2019 at 2:56 PM Willem de Bruijn
> > > <[email protected]> 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 <[email protected]>
> > > > > > Reported-by: Willem de Bruijn <[email protected]>
> > > > > > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set")
> > > > > > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > > > > > ---
> > > > > >
> > > > > > 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.
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
> > <[email protected]> wrote:
> > > On Mon, Dec 23, 2019 at 2:56 PM Willem de Bruijn
> > > <[email protected]> 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 <[email protected]>
> > > > > > Reported-by: Willem de Bruijn <[email protected]>
> > > > > > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set")
> > > > > > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > > > > > ---
> > > > > >
> > > > > > 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
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
> <[email protected]> wrote:
> >
> > On Mon, Dec 23, 2019 at 2:56 PM Willem de Bruijn
> > <[email protected]> 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 <[email protected]>
> > > > > Reported-by: Willem de Bruijn <[email protected]>
> > > > > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set")
> > > > > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > > > > ---
> > > > >
> > > > > 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?