BPF Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] Revert "virtio-net: ethtool configurable RXCSUM"
@ 2020-10-18 10:31 Michael S. Tsirkin
  2020-10-18 10:57 ` [PATCH net repost] " Michael S. Tsirkin
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2020-10-18 10:31 UTC (permalink / raw)
  To: linux-kernel
  Cc: Tonghao Zhang, Willem de Bruijn, kernel test robot, Jason Wang,
	David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
	virtualization, netdev, bpf

This reverts commit 3618ad2a7c0e78e4258386394d5d5f92a3dbccf8.

When control vq is not negotiated, that commit causes a crash:

[   72.229171] kernel BUG at drivers/net/virtio_net.c:1667!
[   72.230266] invalid opcode: 0000 [#1] PREEMPT SMP
[   72.231172] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc8-02934-g3618ad2a7c0e7 #1
[   72.231172] EIP: virtnet_send_command+0x120/0x140
[   72.231172] Code: 00 0f 94 c0 8b 7d f0 65 33 3d 14 00 00 00 75 1c 8d 65 f4 5b 5e 5f 5d c3 66 90 be 01 00 00 00 e9 6e ff ff ff 8d b6 00
+00 00 00 <0f> 0b e8 d9 bb 82 00 eb 17 8d b4 26 00 00 00 00 8d b4 26 00 00 00
[   72.231172] EAX: 0000000d EBX: f72895c0 ECX: 00000017 EDX: 00000011
[   72.231172] ESI: f7197800 EDI: ed69bd00 EBP: ed69bcf4 ESP: ed69bc98
[   72.231172] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010246
[   72.231172] CR0: 80050033 CR2: 00000000 CR3: 02c84000 CR4: 000406f0
[   72.231172] Call Trace:
[   72.231172]  ? __virt_addr_valid+0x45/0x60
[   72.231172]  ? ___cache_free+0x51f/0x760
[   72.231172]  ? kobject_uevent_env+0xf4/0x560
[   72.231172]  virtnet_set_guest_offloads+0x4d/0x80
[   72.231172]  virtnet_set_features+0x85/0x120
[   72.231172]  ? virtnet_set_guest_offloads+0x80/0x80
[   72.231172]  __netdev_update_features+0x27a/0x8e0
[   72.231172]  ? kobject_uevent+0xa/0x20
[   72.231172]  ? netdev_register_kobject+0x12c/0x160
[   72.231172]  register_netdevice+0x4fe/0x740
[   72.231172]  register_netdev+0x1c/0x40
[   72.231172]  virtnet_probe+0x728/0xb60
[   72.231172]  ? _raw_spin_unlock+0x1d/0x40
[   72.231172]  ? virtio_vdpa_get_status+0x1c/0x20
[   72.231172]  virtio_dev_probe+0x1c6/0x271
[   72.231172]  really_probe+0x195/0x2e0
[   72.231172]  driver_probe_device+0x26/0x60
[   72.231172]  device_driver_attach+0x49/0x60
[   72.231172]  __driver_attach+0x46/0xc0
[   72.231172]  ? device_driver_attach+0x60/0x60
[   72.231172]  bus_add_driver+0x197/0x1c0
[   72.231172]  driver_register+0x66/0xc0
[   72.231172]  register_virtio_driver+0x1b/0x40
[   72.231172]  virtio_net_driver_init+0x61/0x86
[   72.231172]  ? veth_init+0x14/0x14
[   72.231172]  do_one_initcall+0x76/0x2e4
[   72.231172]  ? rdinit_setup+0x2a/0x2a
[   72.231172]  do_initcalls+0xb2/0xd5
[   72.231172]  kernel_init_freeable+0x14f/0x179
[   72.231172]  ? rest_init+0x100/0x100
[   72.231172]  kernel_init+0xd/0xe0
[   72.231172]  ret_from_fork+0x1c/0x30
[   72.231172] Modules linked in:
[   72.269563] ---[ end trace a6ebc4afea0e6cb1 ]---

The reason is that virtnet_set_features now calls virtnet_set_guest_offloads
unconditionally, it used to only call it when there is something
to configure.

If device does not have a control vq, everything breaks.

Looking at this some more, I noticed that it's not really checking the
hardware too much. E.g.

        if ((dev->features ^ features) & NETIF_F_LRO) {
                if (features & NETIF_F_LRO)
                        offloads |= GUEST_OFFLOAD_LRO_MASK &
                                    vi->guest_offloads_capable;
                else
                        offloads &= ~GUEST_OFFLOAD_LRO_MASK;
        }

and

                                (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
                                (1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
                                (1ULL << VIRTIO_NET_F_GUEST_UFO))

But there's no guarantee that e.g. VIRTIO_NET_F_GUEST_TSO6 is set.

If it isn't command should not send it.

Further

static int virtnet_set_features(struct net_device *dev,
                                netdev_features_t features)
{
        struct virtnet_info *vi = netdev_priv(dev);
        u64 offloads = vi->guest_offloads;

seems wrong since guest_offloads is zero initialized,
it does not reflect the state after reset which comes from
the features.

Revert the original commit for now.

Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Cc: Willem de Bruijn <willemb@google.com>
Fixes: 3618ad2a7c0e7 ("virtio-net: ethtool configurable RXCSUM")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/net/virtio_net.c | 50 +++++++++++-----------------------------
 1 file changed, 13 insertions(+), 37 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d2d2c4a53cf2..21b71148c532 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -68,8 +68,6 @@ static const unsigned long guest_offloads[] = {
 				(1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
 				(1ULL << VIRTIO_NET_F_GUEST_UFO))
 
-#define GUEST_OFFLOAD_CSUM_MASK (1ULL << VIRTIO_NET_F_GUEST_CSUM)
-
 struct virtnet_stat_desc {
 	char desc[ETH_GSTRING_LEN];
 	size_t offset;
@@ -2524,48 +2522,29 @@ static int virtnet_get_phys_port_name(struct net_device *dev, char *buf,
 	return 0;
 }
 
-static netdev_features_t virtnet_fix_features(struct net_device *netdev,
-					      netdev_features_t features)
-{
-	/* If Rx checksum is disabled, LRO should also be disabled. */
-	if (!(features & NETIF_F_RXCSUM))
-		features &= ~NETIF_F_LRO;
-
-	return features;
-}
-
 static int virtnet_set_features(struct net_device *dev,
 				netdev_features_t features)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
-	u64 offloads = vi->guest_offloads;
+	u64 offloads;
 	int err;
 
-	/* Don't allow configuration while XDP is active. */
-	if (vi->xdp_queue_pairs)
-		return -EBUSY;
-
 	if ((dev->features ^ features) & NETIF_F_LRO) {
+		if (vi->xdp_queue_pairs)
+			return -EBUSY;
+
 		if (features & NETIF_F_LRO)
-			offloads |= GUEST_OFFLOAD_LRO_MASK &
-				    vi->guest_offloads_capable;
+			offloads = vi->guest_offloads_capable;
 		else
-			offloads &= ~GUEST_OFFLOAD_LRO_MASK;
+			offloads = vi->guest_offloads_capable &
+				   ~GUEST_OFFLOAD_LRO_MASK;
+
+		err = virtnet_set_guest_offloads(vi, offloads);
+		if (err)
+			return err;
+		vi->guest_offloads = offloads;
 	}
 
-	if ((dev->features ^ features) & NETIF_F_RXCSUM) {
-		if (features & NETIF_F_RXCSUM)
-			offloads |= GUEST_OFFLOAD_CSUM_MASK &
-				    vi->guest_offloads_capable;
-		else
-			offloads &= ~GUEST_OFFLOAD_CSUM_MASK;
-	}
-
-	err = virtnet_set_guest_offloads(vi, offloads);
-	if (err)
-		return err;
-
-	vi->guest_offloads = offloads;
 	return 0;
 }
 
@@ -2584,7 +2563,6 @@ static const struct net_device_ops virtnet_netdev = {
 	.ndo_features_check	= passthru_features_check,
 	.ndo_get_phys_port_name	= virtnet_get_phys_port_name,
 	.ndo_set_features	= virtnet_set_features,
-	.ndo_fix_features	= virtnet_fix_features,
 };
 
 static void virtnet_config_changed_work(struct work_struct *work)
@@ -3035,10 +3013,8 @@ static int virtnet_probe(struct virtio_device *vdev)
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
 	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
 		dev->features |= NETIF_F_LRO;
-	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
-		dev->hw_features |= NETIF_F_RXCSUM;
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
 		dev->hw_features |= NETIF_F_LRO;
-	}
 
 	dev->vlan_features = dev->features;
 
-- 
MST


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net repost] Revert "virtio-net: ethtool configurable RXCSUM"
  2020-10-18 10:31 [PATCH] Revert "virtio-net: ethtool configurable RXCSUM" Michael S. Tsirkin
@ 2020-10-18 10:57 ` Michael S. Tsirkin
  2020-10-19 17:32 ` [PATCH net v2] " Michael S. Tsirkin
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2020-10-18 10:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Tonghao Zhang, Willem de Bruijn, kernel test robot, Jason Wang,
	David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
	virtualization, netdev, bpf

This reverts commit 3618ad2a7c0e78e4258386394d5d5f92a3dbccf8.

When control vq is not negotiated, that commit causes a crash:

[   72.229171] kernel BUG at drivers/net/virtio_net.c:1667!
[   72.230266] invalid opcode: 0000 [#1] PREEMPT SMP
[   72.231172] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc8-02934-g3618ad2a7c0e7 #1
[   72.231172] EIP: virtnet_send_command+0x120/0x140
[   72.231172] Code: 00 0f 94 c0 8b 7d f0 65 33 3d 14 00 00 00 75 1c 8d 65 f4 5b 5e 5f 5d c3 66 90 be 01 00 00 00 e9 6e ff ff ff 8d b6 00
+00 00 00 <0f> 0b e8 d9 bb 82 00 eb 17 8d b4 26 00 00 00 00 8d b4 26 00 00 00
[   72.231172] EAX: 0000000d EBX: f72895c0 ECX: 00000017 EDX: 00000011
[   72.231172] ESI: f7197800 EDI: ed69bd00 EBP: ed69bcf4 ESP: ed69bc98
[   72.231172] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010246
[   72.231172] CR0: 80050033 CR2: 00000000 CR3: 02c84000 CR4: 000406f0
[   72.231172] Call Trace:
[   72.231172]  ? __virt_addr_valid+0x45/0x60
[   72.231172]  ? ___cache_free+0x51f/0x760
[   72.231172]  ? kobject_uevent_env+0xf4/0x560
[   72.231172]  virtnet_set_guest_offloads+0x4d/0x80
[   72.231172]  virtnet_set_features+0x85/0x120
[   72.231172]  ? virtnet_set_guest_offloads+0x80/0x80
[   72.231172]  __netdev_update_features+0x27a/0x8e0
[   72.231172]  ? kobject_uevent+0xa/0x20
[   72.231172]  ? netdev_register_kobject+0x12c/0x160
[   72.231172]  register_netdevice+0x4fe/0x740
[   72.231172]  register_netdev+0x1c/0x40
[   72.231172]  virtnet_probe+0x728/0xb60
[   72.231172]  ? _raw_spin_unlock+0x1d/0x40
[   72.231172]  ? virtio_vdpa_get_status+0x1c/0x20
[   72.231172]  virtio_dev_probe+0x1c6/0x271
[   72.231172]  really_probe+0x195/0x2e0
[   72.231172]  driver_probe_device+0x26/0x60
[   72.231172]  device_driver_attach+0x49/0x60
[   72.231172]  __driver_attach+0x46/0xc0
[   72.231172]  ? device_driver_attach+0x60/0x60
[   72.231172]  bus_add_driver+0x197/0x1c0
[   72.231172]  driver_register+0x66/0xc0
[   72.231172]  register_virtio_driver+0x1b/0x40
[   72.231172]  virtio_net_driver_init+0x61/0x86
[   72.231172]  ? veth_init+0x14/0x14
[   72.231172]  do_one_initcall+0x76/0x2e4
[   72.231172]  ? rdinit_setup+0x2a/0x2a
[   72.231172]  do_initcalls+0xb2/0xd5
[   72.231172]  kernel_init_freeable+0x14f/0x179
[   72.231172]  ? rest_init+0x100/0x100
[   72.231172]  kernel_init+0xd/0xe0
[   72.231172]  ret_from_fork+0x1c/0x30
[   72.231172] Modules linked in:
[   72.269563] ---[ end trace a6ebc4afea0e6cb1 ]---

The reason is that virtnet_set_features now calls virtnet_set_guest_offloads
unconditionally, it used to only call it when there is something
to configure.

If device does not have a control vq, everything breaks.

Looking at this some more, I noticed that it's not really checking the
hardware too much. E.g.

        if ((dev->features ^ features) & NETIF_F_LRO) {
                if (features & NETIF_F_LRO)
                        offloads |= GUEST_OFFLOAD_LRO_MASK &
                                    vi->guest_offloads_capable;
                else
                        offloads &= ~GUEST_OFFLOAD_LRO_MASK;
        }

and

                                (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
                                (1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
                                (1ULL << VIRTIO_NET_F_GUEST_UFO))

But there's no guarantee that e.g. VIRTIO_NET_F_GUEST_TSO6 is set.

If it isn't command should not send it.

Further

static int virtnet_set_features(struct net_device *dev,
                                netdev_features_t features)
{
        struct virtnet_info *vi = netdev_priv(dev);
        u64 offloads = vi->guest_offloads;

seems wrong since guest_offloads is zero initialized,
it does not reflect the state after reset which comes from
the features.

Revert the original commit for now.

Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Cc: Willem de Bruijn <willemb@google.com>
Fixes: 3618ad2a7c0e7 ("virtio-net: ethtool configurable RXCSUM")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Reposting with net tag


 drivers/net/virtio_net.c | 50 +++++++++++-----------------------------
 1 file changed, 13 insertions(+), 37 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d2d2c4a53cf2..21b71148c532 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -68,8 +68,6 @@ static const unsigned long guest_offloads[] = {
 				(1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
 				(1ULL << VIRTIO_NET_F_GUEST_UFO))
 
-#define GUEST_OFFLOAD_CSUM_MASK (1ULL << VIRTIO_NET_F_GUEST_CSUM)
-
 struct virtnet_stat_desc {
 	char desc[ETH_GSTRING_LEN];
 	size_t offset;
@@ -2524,48 +2522,29 @@ static int virtnet_get_phys_port_name(struct net_device *dev, char *buf,
 	return 0;
 }
 
-static netdev_features_t virtnet_fix_features(struct net_device *netdev,
-					      netdev_features_t features)
-{
-	/* If Rx checksum is disabled, LRO should also be disabled. */
-	if (!(features & NETIF_F_RXCSUM))
-		features &= ~NETIF_F_LRO;
-
-	return features;
-}
-
 static int virtnet_set_features(struct net_device *dev,
 				netdev_features_t features)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
-	u64 offloads = vi->guest_offloads;
+	u64 offloads;
 	int err;
 
-	/* Don't allow configuration while XDP is active. */
-	if (vi->xdp_queue_pairs)
-		return -EBUSY;
-
 	if ((dev->features ^ features) & NETIF_F_LRO) {
+		if (vi->xdp_queue_pairs)
+			return -EBUSY;
+
 		if (features & NETIF_F_LRO)
-			offloads |= GUEST_OFFLOAD_LRO_MASK &
-				    vi->guest_offloads_capable;
+			offloads = vi->guest_offloads_capable;
 		else
-			offloads &= ~GUEST_OFFLOAD_LRO_MASK;
+			offloads = vi->guest_offloads_capable &
+				   ~GUEST_OFFLOAD_LRO_MASK;
+
+		err = virtnet_set_guest_offloads(vi, offloads);
+		if (err)
+			return err;
+		vi->guest_offloads = offloads;
 	}
 
-	if ((dev->features ^ features) & NETIF_F_RXCSUM) {
-		if (features & NETIF_F_RXCSUM)
-			offloads |= GUEST_OFFLOAD_CSUM_MASK &
-				    vi->guest_offloads_capable;
-		else
-			offloads &= ~GUEST_OFFLOAD_CSUM_MASK;
-	}
-
-	err = virtnet_set_guest_offloads(vi, offloads);
-	if (err)
-		return err;
-
-	vi->guest_offloads = offloads;
 	return 0;
 }
 
@@ -2584,7 +2563,6 @@ static const struct net_device_ops virtnet_netdev = {
 	.ndo_features_check	= passthru_features_check,
 	.ndo_get_phys_port_name	= virtnet_get_phys_port_name,
 	.ndo_set_features	= virtnet_set_features,
-	.ndo_fix_features	= virtnet_fix_features,
 };
 
 static void virtnet_config_changed_work(struct work_struct *work)
@@ -3035,10 +3013,8 @@ static int virtnet_probe(struct virtio_device *vdev)
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
 	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
 		dev->features |= NETIF_F_LRO;
-	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
-		dev->hw_features |= NETIF_F_RXCSUM;
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
 		dev->hw_features |= NETIF_F_LRO;
-	}
 
 	dev->vlan_features = dev->features;
 
-- 
MST


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net v2] Revert "virtio-net: ethtool configurable RXCSUM"
  2020-10-18 10:31 [PATCH] Revert "virtio-net: ethtool configurable RXCSUM" Michael S. Tsirkin
  2020-10-18 10:57 ` [PATCH net repost] " Michael S. Tsirkin
@ 2020-10-19 17:32 ` Michael S. Tsirkin
  2020-10-19 19:15 ` Jakub Kicinski
  2020-10-20  6:13 ` Jason Wang
  3 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2020-10-19 17:32 UTC (permalink / raw)
  To: linux-kernel
  Cc: Tonghao Zhang, Willem de Bruijn, kernel test robot, Jason Wang,
	David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
	virtualization, netdev, bpf

This reverts commit 3618ad2a7c0e78e4258386394d5d5f92a3dbccf8.

When the device does not have a control vq (e.g. when using a
version of QEMU based on upstream v0.10 or older, or when specifying
ctrl_vq=off,ctrl_rx=off,ctrl_vlan=off,ctrl_rx_extra=off,ctrl_mac_addr=off
for the device on the QEMU command line), that commit causes a crash:

[   72.229171] kernel BUG at drivers/net/virtio_net.c:1667!
[   72.230266] invalid opcode: 0000 [#1] PREEMPT SMP
[   72.231172] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc8-02934-g3618ad2a7c0e7 #1
[   72.231172] EIP: virtnet_send_command+0x120/0x140
[   72.231172] Code: 00 0f 94 c0 8b 7d f0 65 33 3d 14 00 00 00 75 1c 8d 65 f4 5b 5e 5f 5d c3 66 90 be 01 00 00 00 e9 6e ff ff ff 8d b6 00
+00 00 00 <0f> 0b e8 d9 bb 82 00 eb 17 8d b4 26 00 00 00 00 8d b4 26 00 00 00
[   72.231172] EAX: 0000000d EBX: f72895c0 ECX: 00000017 EDX: 00000011
[   72.231172] ESI: f7197800 EDI: ed69bd00 EBP: ed69bcf4 ESP: ed69bc98
[   72.231172] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010246
[   72.231172] CR0: 80050033 CR2: 00000000 CR3: 02c84000 CR4: 000406f0
[   72.231172] Call Trace:
[   72.231172]  ? __virt_addr_valid+0x45/0x60
[   72.231172]  ? ___cache_free+0x51f/0x760
[   72.231172]  ? kobject_uevent_env+0xf4/0x560
[   72.231172]  virtnet_set_guest_offloads+0x4d/0x80
[   72.231172]  virtnet_set_features+0x85/0x120
[   72.231172]  ? virtnet_set_guest_offloads+0x80/0x80
[   72.231172]  __netdev_update_features+0x27a/0x8e0
[   72.231172]  ? kobject_uevent+0xa/0x20
[   72.231172]  ? netdev_register_kobject+0x12c/0x160
[   72.231172]  register_netdevice+0x4fe/0x740
[   72.231172]  register_netdev+0x1c/0x40
[   72.231172]  virtnet_probe+0x728/0xb60
[   72.231172]  ? _raw_spin_unlock+0x1d/0x40
[   72.231172]  ? virtio_vdpa_get_status+0x1c/0x20
[   72.231172]  virtio_dev_probe+0x1c6/0x271
[   72.231172]  really_probe+0x195/0x2e0
[   72.231172]  driver_probe_device+0x26/0x60
[   72.231172]  device_driver_attach+0x49/0x60
[   72.231172]  __driver_attach+0x46/0xc0
[   72.231172]  ? device_driver_attach+0x60/0x60
[   72.231172]  bus_add_driver+0x197/0x1c0
[   72.231172]  driver_register+0x66/0xc0
[   72.231172]  register_virtio_driver+0x1b/0x40
[   72.231172]  virtio_net_driver_init+0x61/0x86
[   72.231172]  ? veth_init+0x14/0x14
[   72.231172]  do_one_initcall+0x76/0x2e4
[   72.231172]  ? rdinit_setup+0x2a/0x2a
[   72.231172]  do_initcalls+0xb2/0xd5
[   72.231172]  kernel_init_freeable+0x14f/0x179
[   72.231172]  ? rest_init+0x100/0x100
[   72.231172]  kernel_init+0xd/0xe0
[   72.231172]  ret_from_fork+0x1c/0x30
[   72.231172] Modules linked in:
[   72.269563] ---[ end trace a6ebc4afea0e6cb1 ]---

The reason is that virtnet_set_features now calls virtnet_set_guest_offloads
unconditionally, it used to only call it when there is something
to configure.

If device does not have a control vq, everything breaks.

Looking at this some more, I noticed that it's not really checking the
hardware too much. E.g.

        if ((dev->features ^ features) & NETIF_F_LRO) {
                if (features & NETIF_F_LRO)
                        offloads |= GUEST_OFFLOAD_LRO_MASK &
                                    vi->guest_offloads_capable;
                else
                        offloads &= ~GUEST_OFFLOAD_LRO_MASK;
        }

and

                                (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
                                (1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
                                (1ULL << VIRTIO_NET_F_GUEST_UFO))

But there's no guarantee that e.g. VIRTIO_NET_F_GUEST_TSO6 is set.

If it isn't command should not send it.

Further

static int virtnet_set_features(struct net_device *dev,
                                netdev_features_t features)
{
        struct virtnet_info *vi = netdev_priv(dev);
        u64 offloads = vi->guest_offloads;

seems wrong since guest_offloads is zero initialized,
it does not reflect the state after reset which comes from
the features.

Revert the original commit for now.

Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Cc: Willem de Bruijn <willemb@google.com>
Fixes: 3618ad2a7c0e7 ("virtio-net: ethtool configurable RXCSUM")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

changes from v1:
	- clarify how to reproduce the bug in the log


 drivers/net/virtio_net.c | 50 +++++++++++-----------------------------
 1 file changed, 13 insertions(+), 37 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d2d2c4a53cf2..21b71148c532 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -68,8 +68,6 @@ static const unsigned long guest_offloads[] = {
 				(1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
 				(1ULL << VIRTIO_NET_F_GUEST_UFO))
 
-#define GUEST_OFFLOAD_CSUM_MASK (1ULL << VIRTIO_NET_F_GUEST_CSUM)
-
 struct virtnet_stat_desc {
 	char desc[ETH_GSTRING_LEN];
 	size_t offset;
@@ -2524,48 +2522,29 @@ static int virtnet_get_phys_port_name(struct net_device *dev, char *buf,
 	return 0;
 }
 
-static netdev_features_t virtnet_fix_features(struct net_device *netdev,
-					      netdev_features_t features)
-{
-	/* If Rx checksum is disabled, LRO should also be disabled. */
-	if (!(features & NETIF_F_RXCSUM))
-		features &= ~NETIF_F_LRO;
-
-	return features;
-}
-
 static int virtnet_set_features(struct net_device *dev,
 				netdev_features_t features)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
-	u64 offloads = vi->guest_offloads;
+	u64 offloads;
 	int err;
 
-	/* Don't allow configuration while XDP is active. */
-	if (vi->xdp_queue_pairs)
-		return -EBUSY;
-
 	if ((dev->features ^ features) & NETIF_F_LRO) {
+		if (vi->xdp_queue_pairs)
+			return -EBUSY;
+
 		if (features & NETIF_F_LRO)
-			offloads |= GUEST_OFFLOAD_LRO_MASK &
-				    vi->guest_offloads_capable;
+			offloads = vi->guest_offloads_capable;
 		else
-			offloads &= ~GUEST_OFFLOAD_LRO_MASK;
+			offloads = vi->guest_offloads_capable &
+				   ~GUEST_OFFLOAD_LRO_MASK;
+
+		err = virtnet_set_guest_offloads(vi, offloads);
+		if (err)
+			return err;
+		vi->guest_offloads = offloads;
 	}
 
-	if ((dev->features ^ features) & NETIF_F_RXCSUM) {
-		if (features & NETIF_F_RXCSUM)
-			offloads |= GUEST_OFFLOAD_CSUM_MASK &
-				    vi->guest_offloads_capable;
-		else
-			offloads &= ~GUEST_OFFLOAD_CSUM_MASK;
-	}
-
-	err = virtnet_set_guest_offloads(vi, offloads);
-	if (err)
-		return err;
-
-	vi->guest_offloads = offloads;
 	return 0;
 }
 
@@ -2584,7 +2563,6 @@ static const struct net_device_ops virtnet_netdev = {
 	.ndo_features_check	= passthru_features_check,
 	.ndo_get_phys_port_name	= virtnet_get_phys_port_name,
 	.ndo_set_features	= virtnet_set_features,
-	.ndo_fix_features	= virtnet_fix_features,
 };
 
 static void virtnet_config_changed_work(struct work_struct *work)
@@ -3035,10 +3013,8 @@ static int virtnet_probe(struct virtio_device *vdev)
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
 	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
 		dev->features |= NETIF_F_LRO;
-	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
-		dev->hw_features |= NETIF_F_RXCSUM;
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
 		dev->hw_features |= NETIF_F_LRO;
-	}
 
 	dev->vlan_features = dev->features;
 
-- 
MST


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net v2] Revert "virtio-net: ethtool configurable RXCSUM"
  2020-10-18 10:31 [PATCH] Revert "virtio-net: ethtool configurable RXCSUM" Michael S. Tsirkin
  2020-10-18 10:57 ` [PATCH net repost] " Michael S. Tsirkin
  2020-10-19 17:32 ` [PATCH net v2] " Michael S. Tsirkin
@ 2020-10-19 19:15 ` Jakub Kicinski
  2020-10-20 11:45   ` Michael S. Tsirkin
  2020-10-20  6:13 ` Jason Wang
  3 siblings, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2020-10-19 19:15 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, Tonghao Zhang, Willem de Bruijn, kernel test robot,
	Jason Wang, David S. Miller, Alexei Starovoitov, Daniel Borkmann,
	Jesper Dangaard Brouer, John Fastabend, virtualization, netdev,
	bpf

On Mon, 19 Oct 2020 13:32:12 -0400 Michael S. Tsirkin wrote:
> This reverts commit 3618ad2a7c0e78e4258386394d5d5f92a3dbccf8.
> 
> When the device does not have a control vq (e.g. when using a
> version of QEMU based on upstream v0.10 or older, or when specifying
> ctrl_vq=off,ctrl_rx=off,ctrl_vlan=off,ctrl_rx_extra=off,ctrl_mac_addr=off
> for the device on the QEMU command line), that commit causes a crash:

Hi Michael!

Only our very first (non-resend) version got into patchwork:

https://patchwork.ozlabs.org/project/netdev/list/?submitter=2235&state=*

Any ideas why?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net v2] Revert "virtio-net: ethtool configurable RXCSUM"
  2020-10-18 10:31 [PATCH] Revert "virtio-net: ethtool configurable RXCSUM" Michael S. Tsirkin
                   ` (2 preceding siblings ...)
  2020-10-19 19:15 ` Jakub Kicinski
@ 2020-10-20  6:13 ` Jason Wang
  2020-10-20 11:36   ` Michael S. Tsirkin
  3 siblings, 1 reply; 8+ messages in thread
From: Jason Wang @ 2020-10-20  6:13 UTC (permalink / raw)
  To: Michael S. Tsirkin, linux-kernel
  Cc: Tonghao Zhang, Willem de Bruijn, kernel test robot,
	David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
	virtualization, netdev, bpf


On 2020/10/20 上午1:32, Michael S. Tsirkin wrote:
> This reverts commit 3618ad2a7c0e78e4258386394d5d5f92a3dbccf8.
>
> When the device does not have a control vq (e.g. when using a
> version of QEMU based on upstream v0.10 or older, or when specifying
> ctrl_vq=off,ctrl_rx=off,ctrl_vlan=off,ctrl_rx_extra=off,ctrl_mac_addr=off
> for the device on the QEMU command line), that commit causes a crash:
>
> [   72.229171] kernel BUG at drivers/net/virtio_net.c:1667!
> [   72.230266] invalid opcode: 0000 [#1] PREEMPT SMP
> [   72.231172] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc8-02934-g3618ad2a7c0e7 #1
> [   72.231172] EIP: virtnet_send_command+0x120/0x140
> [   72.231172] Code: 00 0f 94 c0 8b 7d f0 65 33 3d 14 00 00 00 75 1c 8d 65 f4 5b 5e 5f 5d c3 66 90 be 01 00 00 00 e9 6e ff ff ff 8d b6 00
> +00 00 00 <0f> 0b e8 d9 bb 82 00 eb 17 8d b4 26 00 00 00 00 8d b4 26 00 00 00
> [   72.231172] EAX: 0000000d EBX: f72895c0 ECX: 00000017 EDX: 00000011
> [   72.231172] ESI: f7197800 EDI: ed69bd00 EBP: ed69bcf4 ESP: ed69bc98
> [   72.231172] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010246
> [   72.231172] CR0: 80050033 CR2: 00000000 CR3: 02c84000 CR4: 000406f0
> [   72.231172] Call Trace:
> [   72.231172]  ? __virt_addr_valid+0x45/0x60
> [   72.231172]  ? ___cache_free+0x51f/0x760
> [   72.231172]  ? kobject_uevent_env+0xf4/0x560
> [   72.231172]  virtnet_set_guest_offloads+0x4d/0x80
> [   72.231172]  virtnet_set_features+0x85/0x120
> [   72.231172]  ? virtnet_set_guest_offloads+0x80/0x80
> [   72.231172]  __netdev_update_features+0x27a/0x8e0
> [   72.231172]  ? kobject_uevent+0xa/0x20
> [   72.231172]  ? netdev_register_kobject+0x12c/0x160
> [   72.231172]  register_netdevice+0x4fe/0x740
> [   72.231172]  register_netdev+0x1c/0x40
> [   72.231172]  virtnet_probe+0x728/0xb60
> [   72.231172]  ? _raw_spin_unlock+0x1d/0x40
> [   72.231172]  ? virtio_vdpa_get_status+0x1c/0x20
> [   72.231172]  virtio_dev_probe+0x1c6/0x271
> [   72.231172]  really_probe+0x195/0x2e0
> [   72.231172]  driver_probe_device+0x26/0x60
> [   72.231172]  device_driver_attach+0x49/0x60
> [   72.231172]  __driver_attach+0x46/0xc0
> [   72.231172]  ? device_driver_attach+0x60/0x60
> [   72.231172]  bus_add_driver+0x197/0x1c0
> [   72.231172]  driver_register+0x66/0xc0
> [   72.231172]  register_virtio_driver+0x1b/0x40
> [   72.231172]  virtio_net_driver_init+0x61/0x86
> [   72.231172]  ? veth_init+0x14/0x14
> [   72.231172]  do_one_initcall+0x76/0x2e4
> [   72.231172]  ? rdinit_setup+0x2a/0x2a
> [   72.231172]  do_initcalls+0xb2/0xd5
> [   72.231172]  kernel_init_freeable+0x14f/0x179
> [   72.231172]  ? rest_init+0x100/0x100
> [   72.231172]  kernel_init+0xd/0xe0
> [   72.231172]  ret_from_fork+0x1c/0x30
> [   72.231172] Modules linked in:
> [   72.269563] ---[ end trace a6ebc4afea0e6cb1 ]---
>
> The reason is that virtnet_set_features now calls virtnet_set_guest_offloads
> unconditionally, it used to only call it when there is something
> to configure.
>
> If device does not have a control vq, everything breaks.
>
> Looking at this some more, I noticed that it's not really checking the
> hardware too much. E.g.
>
>          if ((dev->features ^ features) & NETIF_F_LRO) {
>                  if (features & NETIF_F_LRO)
>                          offloads |= GUEST_OFFLOAD_LRO_MASK &
>                                      vi->guest_offloads_capable;
>                  else
>                          offloads &= ~GUEST_OFFLOAD_LRO_MASK;
>          }
>
> and
>
>                                  (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
>                                  (1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
>                                  (1ULL << VIRTIO_NET_F_GUEST_UFO))
>
> But there's no guarantee that e.g. VIRTIO_NET_F_GUEST_TSO6 is set.
>
> If it isn't command should not send it.
>
> Further
>
> static int virtnet_set_features(struct net_device *dev,
>                                  netdev_features_t features)
> {
>          struct virtnet_info *vi = netdev_priv(dev);
>          u64 offloads = vi->guest_offloads;
>
> seems wrong since guest_offloads is zero initialized,


I'm not sure I get here.

Did you mean vi->guest_offloads?

We initialize it during probe

     for (i = 0; i < ARRAY_SIZE(guest_offloads); i++)
         if (virtio_has_feature(vi->vdev, guest_offloads[i]))
             set_bit(guest_offloads[i], &vi->guest_offloads);


> it does not reflect the state after reset which comes from
> the features.
>
> Revert the original commit for now.
>
> Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Fixes: 3618ad2a7c0e7 ("virtio-net: ethtool configurable RXCSUM")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> changes from v1:
> 	- clarify how to reproduce the bug in the log
>
>
>   drivers/net/virtio_net.c | 50 +++++++++++-----------------------------
>   1 file changed, 13 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index d2d2c4a53cf2..21b71148c532 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -68,8 +68,6 @@ static const unsigned long guest_offloads[] = {
>   				(1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
>   				(1ULL << VIRTIO_NET_F_GUEST_UFO))
>   
> -#define GUEST_OFFLOAD_CSUM_MASK (1ULL << VIRTIO_NET_F_GUEST_CSUM)
> -
>   struct virtnet_stat_desc {
>   	char desc[ETH_GSTRING_LEN];
>   	size_t offset;
> @@ -2524,48 +2522,29 @@ static int virtnet_get_phys_port_name(struct net_device *dev, char *buf,
>   	return 0;
>   }
>   
> -static netdev_features_t virtnet_fix_features(struct net_device *netdev,
> -					      netdev_features_t features)
> -{
> -	/* If Rx checksum is disabled, LRO should also be disabled. */
> -	if (!(features & NETIF_F_RXCSUM))
> -		features &= ~NETIF_F_LRO;
> -
> -	return features;
> -}
> -
>   static int virtnet_set_features(struct net_device *dev,
>   				netdev_features_t features)
>   {
>   	struct virtnet_info *vi = netdev_priv(dev);
> -	u64 offloads = vi->guest_offloads;
> +	u64 offloads;
>   	int err;
>   
> -	/* Don't allow configuration while XDP is active. */
> -	if (vi->xdp_queue_pairs)
> -		return -EBUSY;
> -
>   	if ((dev->features ^ features) & NETIF_F_LRO) {
> +		if (vi->xdp_queue_pairs)
> +			return -EBUSY;
> +
>   		if (features & NETIF_F_LRO)
> -			offloads |= GUEST_OFFLOAD_LRO_MASK &
> -				    vi->guest_offloads_capable;
> +			offloads = vi->guest_offloads_capable;
>   		else
> -			offloads &= ~GUEST_OFFLOAD_LRO_MASK;
> +			offloads = vi->guest_offloads_capable &
> +				   ~GUEST_OFFLOAD_LRO_MASK;
> +
> +		err = virtnet_set_guest_offloads(vi, offloads);
> +		if (err)
> +			return err;
> +		vi->guest_offloads = offloads;
>   	}
>   
> -	if ((dev->features ^ features) & NETIF_F_RXCSUM) {
> -		if (features & NETIF_F_RXCSUM)
> -			offloads |= GUEST_OFFLOAD_CSUM_MASK &
> -				    vi->guest_offloads_capable;
> -		else
> -			offloads &= ~GUEST_OFFLOAD_CSUM_MASK;
> -	}
> -
> -	err = virtnet_set_guest_offloads(vi, offloads);
> -	if (err)
> -		return err;
> -
> -	vi->guest_offloads = offloads;
>   	return 0;
>   }
>   
> @@ -2584,7 +2563,6 @@ static const struct net_device_ops virtnet_netdev = {
>   	.ndo_features_check	= passthru_features_check,
>   	.ndo_get_phys_port_name	= virtnet_get_phys_port_name,
>   	.ndo_set_features	= virtnet_set_features,
> -	.ndo_fix_features	= virtnet_fix_features,
>   };
>   
>   static void virtnet_config_changed_work(struct work_struct *work)
> @@ -3035,10 +3013,8 @@ static int virtnet_probe(struct virtio_device *vdev)
>   	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
>   	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
>   		dev->features |= NETIF_F_LRO;
> -	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
> -		dev->hw_features |= NETIF_F_RXCSUM;
> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
>   		dev->hw_features |= NETIF_F_LRO;
> -	}
>   
>   	dev->vlan_features = dev->features;
>   


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net v2] Revert "virtio-net: ethtool configurable RXCSUM"
  2020-10-20  6:13 ` Jason Wang
@ 2020-10-20 11:36   ` Michael S. Tsirkin
  0 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2020-10-20 11:36 UTC (permalink / raw)
  To: Jason Wang
  Cc: linux-kernel, Tonghao Zhang, Willem de Bruijn, kernel test robot,
	David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
	virtualization, netdev, bpf

On Tue, Oct 20, 2020 at 02:13:06PM +0800, Jason Wang wrote:
> 
> On 2020/10/20 上午1:32, Michael S. Tsirkin wrote:
> > This reverts commit 3618ad2a7c0e78e4258386394d5d5f92a3dbccf8.
> > 
> > When the device does not have a control vq (e.g. when using a
> > version of QEMU based on upstream v0.10 or older, or when specifying
> > ctrl_vq=off,ctrl_rx=off,ctrl_vlan=off,ctrl_rx_extra=off,ctrl_mac_addr=off
> > for the device on the QEMU command line), that commit causes a crash:
> > 
> > [   72.229171] kernel BUG at drivers/net/virtio_net.c:1667!
> > [   72.230266] invalid opcode: 0000 [#1] PREEMPT SMP
> > [   72.231172] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc8-02934-g3618ad2a7c0e7 #1
> > [   72.231172] EIP: virtnet_send_command+0x120/0x140
> > [   72.231172] Code: 00 0f 94 c0 8b 7d f0 65 33 3d 14 00 00 00 75 1c 8d 65 f4 5b 5e 5f 5d c3 66 90 be 01 00 00 00 e9 6e ff ff ff 8d b6 00
> > +00 00 00 <0f> 0b e8 d9 bb 82 00 eb 17 8d b4 26 00 00 00 00 8d b4 26 00 00 00
> > [   72.231172] EAX: 0000000d EBX: f72895c0 ECX: 00000017 EDX: 00000011
> > [   72.231172] ESI: f7197800 EDI: ed69bd00 EBP: ed69bcf4 ESP: ed69bc98
> > [   72.231172] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010246
> > [   72.231172] CR0: 80050033 CR2: 00000000 CR3: 02c84000 CR4: 000406f0
> > [   72.231172] Call Trace:
> > [   72.231172]  ? __virt_addr_valid+0x45/0x60
> > [   72.231172]  ? ___cache_free+0x51f/0x760
> > [   72.231172]  ? kobject_uevent_env+0xf4/0x560
> > [   72.231172]  virtnet_set_guest_offloads+0x4d/0x80
> > [   72.231172]  virtnet_set_features+0x85/0x120
> > [   72.231172]  ? virtnet_set_guest_offloads+0x80/0x80
> > [   72.231172]  __netdev_update_features+0x27a/0x8e0
> > [   72.231172]  ? kobject_uevent+0xa/0x20
> > [   72.231172]  ? netdev_register_kobject+0x12c/0x160
> > [   72.231172]  register_netdevice+0x4fe/0x740
> > [   72.231172]  register_netdev+0x1c/0x40
> > [   72.231172]  virtnet_probe+0x728/0xb60
> > [   72.231172]  ? _raw_spin_unlock+0x1d/0x40
> > [   72.231172]  ? virtio_vdpa_get_status+0x1c/0x20
> > [   72.231172]  virtio_dev_probe+0x1c6/0x271
> > [   72.231172]  really_probe+0x195/0x2e0
> > [   72.231172]  driver_probe_device+0x26/0x60
> > [   72.231172]  device_driver_attach+0x49/0x60
> > [   72.231172]  __driver_attach+0x46/0xc0
> > [   72.231172]  ? device_driver_attach+0x60/0x60
> > [   72.231172]  bus_add_driver+0x197/0x1c0
> > [   72.231172]  driver_register+0x66/0xc0
> > [   72.231172]  register_virtio_driver+0x1b/0x40
> > [   72.231172]  virtio_net_driver_init+0x61/0x86
> > [   72.231172]  ? veth_init+0x14/0x14
> > [   72.231172]  do_one_initcall+0x76/0x2e4
> > [   72.231172]  ? rdinit_setup+0x2a/0x2a
> > [   72.231172]  do_initcalls+0xb2/0xd5
> > [   72.231172]  kernel_init_freeable+0x14f/0x179
> > [   72.231172]  ? rest_init+0x100/0x100
> > [   72.231172]  kernel_init+0xd/0xe0
> > [   72.231172]  ret_from_fork+0x1c/0x30
> > [   72.231172] Modules linked in:
> > [   72.269563] ---[ end trace a6ebc4afea0e6cb1 ]---
> > 
> > The reason is that virtnet_set_features now calls virtnet_set_guest_offloads
> > unconditionally, it used to only call it when there is something
> > to configure.
> > 
> > If device does not have a control vq, everything breaks.
> > 
> > Looking at this some more, I noticed that it's not really checking the
> > hardware too much. E.g.
> > 
> >          if ((dev->features ^ features) & NETIF_F_LRO) {
> >                  if (features & NETIF_F_LRO)
> >                          offloads |= GUEST_OFFLOAD_LRO_MASK &
> >                                      vi->guest_offloads_capable;
> >                  else
> >                          offloads &= ~GUEST_OFFLOAD_LRO_MASK;
> >          }
> > 
> > and
> > 
> >                                  (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> >                                  (1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
> >                                  (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > 
> > But there's no guarantee that e.g. VIRTIO_NET_F_GUEST_TSO6 is set.
> > 
> > If it isn't command should not send it.
> > 
> > Further
> > 
> > static int virtnet_set_features(struct net_device *dev,
> >                                  netdev_features_t features)
> > {
> >          struct virtnet_info *vi = netdev_priv(dev);
> >          u64 offloads = vi->guest_offloads;
> > 
> > seems wrong since guest_offloads is zero initialized,
> 
> 
> I'm not sure I get here.
> 
> Did you mean vi->guest_offloads?
> 
> We initialize it during probe
> 
>     for (i = 0; i < ARRAY_SIZE(guest_offloads); i++)
>         if (virtio_has_feature(vi->vdev, guest_offloads[i]))
>             set_bit(guest_offloads[i], &vi->guest_offloads);
> 

Good point, will drop this part.


> > it does not reflect the state after reset which comes from
> > the features.
> > 
> > Revert the original commit for now.
> > 
> > Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > Cc: Willem de Bruijn <willemb@google.com>
> > Fixes: 3618ad2a7c0e7 ("virtio-net: ethtool configurable RXCSUM")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > 
> > changes from v1:
> > 	- clarify how to reproduce the bug in the log
> > 
> > 
> >   drivers/net/virtio_net.c | 50 +++++++++++-----------------------------
> >   1 file changed, 13 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index d2d2c4a53cf2..21b71148c532 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -68,8 +68,6 @@ static const unsigned long guest_offloads[] = {
> >   				(1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
> >   				(1ULL << VIRTIO_NET_F_GUEST_UFO))
> > -#define GUEST_OFFLOAD_CSUM_MASK (1ULL << VIRTIO_NET_F_GUEST_CSUM)
> > -
> >   struct virtnet_stat_desc {
> >   	char desc[ETH_GSTRING_LEN];
> >   	size_t offset;
> > @@ -2524,48 +2522,29 @@ static int virtnet_get_phys_port_name(struct net_device *dev, char *buf,
> >   	return 0;
> >   }
> > -static netdev_features_t virtnet_fix_features(struct net_device *netdev,
> > -					      netdev_features_t features)
> > -{
> > -	/* If Rx checksum is disabled, LRO should also be disabled. */
> > -	if (!(features & NETIF_F_RXCSUM))
> > -		features &= ~NETIF_F_LRO;
> > -
> > -	return features;
> > -}
> > -
> >   static int virtnet_set_features(struct net_device *dev,
> >   				netdev_features_t features)
> >   {
> >   	struct virtnet_info *vi = netdev_priv(dev);
> > -	u64 offloads = vi->guest_offloads;
> > +	u64 offloads;
> >   	int err;
> > -	/* Don't allow configuration while XDP is active. */
> > -	if (vi->xdp_queue_pairs)
> > -		return -EBUSY;
> > -
> >   	if ((dev->features ^ features) & NETIF_F_LRO) {
> > +		if (vi->xdp_queue_pairs)
> > +			return -EBUSY;
> > +
> >   		if (features & NETIF_F_LRO)
> > -			offloads |= GUEST_OFFLOAD_LRO_MASK &
> > -				    vi->guest_offloads_capable;
> > +			offloads = vi->guest_offloads_capable;
> >   		else
> > -			offloads &= ~GUEST_OFFLOAD_LRO_MASK;
> > +			offloads = vi->guest_offloads_capable &
> > +				   ~GUEST_OFFLOAD_LRO_MASK;
> > +
> > +		err = virtnet_set_guest_offloads(vi, offloads);
> > +		if (err)
> > +			return err;
> > +		vi->guest_offloads = offloads;
> >   	}
> > -	if ((dev->features ^ features) & NETIF_F_RXCSUM) {
> > -		if (features & NETIF_F_RXCSUM)
> > -			offloads |= GUEST_OFFLOAD_CSUM_MASK &
> > -				    vi->guest_offloads_capable;
> > -		else
> > -			offloads &= ~GUEST_OFFLOAD_CSUM_MASK;
> > -	}
> > -
> > -	err = virtnet_set_guest_offloads(vi, offloads);
> > -	if (err)
> > -		return err;
> > -
> > -	vi->guest_offloads = offloads;
> >   	return 0;
> >   }
> > @@ -2584,7 +2563,6 @@ static const struct net_device_ops virtnet_netdev = {
> >   	.ndo_features_check	= passthru_features_check,
> >   	.ndo_get_phys_port_name	= virtnet_get_phys_port_name,
> >   	.ndo_set_features	= virtnet_set_features,
> > -	.ndo_fix_features	= virtnet_fix_features,
> >   };
> >   static void virtnet_config_changed_work(struct work_struct *work)
> > @@ -3035,10 +3013,8 @@ static int virtnet_probe(struct virtio_device *vdev)
> >   	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> >   	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
> >   		dev->features |= NETIF_F_LRO;
> > -	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
> > -		dev->hw_features |= NETIF_F_RXCSUM;
> > +	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> >   		dev->hw_features |= NETIF_F_LRO;
> > -	}
> >   	dev->vlan_features = dev->features;


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net v2] Revert "virtio-net: ethtool configurable RXCSUM"
  2020-10-19 19:15 ` Jakub Kicinski
@ 2020-10-20 11:45   ` Michael S. Tsirkin
  2020-10-20 21:43     ` Jakub Kicinski
  0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2020-10-20 11:45 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: linux-kernel, Tonghao Zhang, Willem de Bruijn, kernel test robot,
	Jason Wang, David S. Miller, Alexei Starovoitov, Daniel Borkmann,
	Jesper Dangaard Brouer, John Fastabend, virtualization, netdev,
	bpf

On Mon, Oct 19, 2020 at 12:15:00PM -0700, Jakub Kicinski wrote:
> On Mon, 19 Oct 2020 13:32:12 -0400 Michael S. Tsirkin wrote:
> > This reverts commit 3618ad2a7c0e78e4258386394d5d5f92a3dbccf8.
> > 
> > When the device does not have a control vq (e.g. when using a
> > version of QEMU based on upstream v0.10 or older, or when specifying
> > ctrl_vq=off,ctrl_rx=off,ctrl_vlan=off,ctrl_rx_extra=off,ctrl_mac_addr=off
> > for the device on the QEMU command line), that commit causes a crash:
> 
> Hi Michael!
> 
> Only our very first (non-resend) version got into patchwork:
> 
> https://patchwork.ozlabs.org/project/netdev/list/?submitter=2235&state=*
> 
> Any ideas why?

I really don't! Any ideas?

-- 
MST


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net v2] Revert "virtio-net: ethtool configurable RXCSUM"
  2020-10-20 11:45   ` Michael S. Tsirkin
@ 2020-10-20 21:43     ` Jakub Kicinski
  0 siblings, 0 replies; 8+ messages in thread
From: Jakub Kicinski @ 2020-10-20 21:43 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, Tonghao Zhang, Willem de Bruijn, kernel test robot,
	Jason Wang, David S. Miller, Alexei Starovoitov, Daniel Borkmann,
	Jesper Dangaard Brouer, John Fastabend, virtualization, netdev,
	bpf

On Tue, 20 Oct 2020 07:45:09 -0400 Michael S. Tsirkin wrote:
> On Mon, Oct 19, 2020 at 12:15:00PM -0700, Jakub Kicinski wrote:
> > On Mon, 19 Oct 2020 13:32:12 -0400 Michael S. Tsirkin wrote:  
> > > This reverts commit 3618ad2a7c0e78e4258386394d5d5f92a3dbccf8.
> > > 
> > > When the device does not have a control vq (e.g. when using a
> > > version of QEMU based on upstream v0.10 or older, or when specifying
> > > ctrl_vq=off,ctrl_rx=off,ctrl_vlan=off,ctrl_rx_extra=off,ctrl_mac_addr=off
> > > for the device on the QEMU command line), that commit causes a crash:  
> > 
> > Hi Michael!
> > 
> > Only our very first (non-resend) version got into patchwork:
> > 
> > https://patchwork.ozlabs.org/project/netdev/list/?submitter=2235&state=*
> > 
> > Any ideas why?  
> 
> I really don't! Any ideas?

No idea. I have a local instance of patchwork to test things, and it
didn't pick your patch up either. Weird.

Looks like there will be v3, let's not worry about it for this single
patch, worst case I'll pick it up from inbox or lore.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-18 10:31 [PATCH] Revert "virtio-net: ethtool configurable RXCSUM" Michael S. Tsirkin
2020-10-18 10:57 ` [PATCH net repost] " Michael S. Tsirkin
2020-10-19 17:32 ` [PATCH net v2] " Michael S. Tsirkin
2020-10-19 19:15 ` Jakub Kicinski
2020-10-20 11:45   ` Michael S. Tsirkin
2020-10-20 21:43     ` Jakub Kicinski
2020-10-20  6:13 ` Jason Wang
2020-10-20 11:36   ` Michael S. Tsirkin

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git