Re: [PATCH net v3] virtio_net: Do not send RSS key if it is not supported

On Fri, Mar 29, 2024 at 10:16:41AM -0700, Breno Leitao wrote:

There is a bug when setting the RSS options in virtio_net that can break the whole machine, getting the kernel into an infinite loop.

Running the following command in any QEMU virtual machine with virtionet will reproduce this problem:

# ethtool -X eth0  hfunc toeplitz

This is how the problem happens:

1. ethtool_set_rxfh() calls virtnet_set_rxfh()

2. virtnet_set_rxfh() calls virtnet_commit_rss_command()

3. virtnet_commit_rss_command() populates 4 entries for the rss

scatter-gather

4. Since the command above does not have a key, then the last

scatter-gatter entry will be zeroed, since rss_key_size == 0. sg_buf_size = vi->rss_key_size;

5. This buffer is passed to qemu, but qemu is not happy with a buffer

with zero length, and do the following in virtqueue_map_desc() (QEMU function):

if (!sz) { virtio_error(vdev, "virtio: zero sized buffers are not allowed");

6. virtio_error() (also QEMU function) set the device as broken

   vdev->broken = true;

7. Qemu bails out, and do not repond this crazy kernel.

8. The kernel is waiting for the response to come back (function

virtnet_send_command())

9. The kernel is waiting doing the following :

   while (!virtqueue_get_buf(vi->cvq, &tmp) && !virtqueue_is_broken(vi->cvq)) cpu_relax();

10. None of the following functions above is true, thus, the kernel

loops here forever. Keeping in mind that virtqueue_is_broken() does not look at the qemu `vdev->broken`, so, it never realizes that the vitio is broken at QEMU side.

Fix it by not sending RSS commands if the feature is not available in the device.

Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.") Cc: stable@vger.kernel.org

net has its own stable process, don't CC stable on net patches.

Cc: qemu-devel@nongnu.org Signed-off-by: Breno Leitao leitao@debian.org

---

Changelog:

V2:

• Moved from creating a valid packet, by rejecting the request completely

V3:

• Got some good feedback from and Xuan Zhuo and Heng Qi, and reworked the rejection path.

---

drivers/net/virtio_net.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index c22d1118a133..c4a21ec51adf 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -3807,6 +3807,7 @@ static int virtnet_set_rxfh(struct net_device *dev, struct netlink_ext_ack *extack) { struct virtnet_info *vi = netdev_priv(dev);

• bool update = false; int i;

if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE && @@ -3814,13 +3815,24 @@ static int virtnet_set_rxfh(struct net_device *dev, return -EOPNOTSUPP; if (rxfh->indir) {

• if (!vi->has_rss)
  

• 	return -EOPNOTSUPP;
  

• for (i = 0; i < vi->rss_indir_table_size; ++i) vi->ctrl->rss.indirection_table[i] = rxfh->indir[i];

• update = true;
  

  }

• if (rxfh->key)

• if (rxfh->key) {

• if (!vi->has_rss && !vi->has_rss_hash_report)
  

• 	return -EOPNOTSUPP;
  

What's the logic here? Is it || or &&? A comment can't hurt.

• memcpy(vi->ctrl->rss.key, rxfh->key, vi->rss_key_size);

• update = true;
  

• }

• virtnet_commit_rss_command(vi);

• if (update)

• virtnet_commit_rss_command(vi);
  

return 0; } @@ -4729,13 +4741,15 @@ static int virtnet_probe(struct virtio_device *vdev) if (virtio_has_feature(vdev, VIRTIO_NET_F_HASH_REPORT)) vi->has_rss_hash_report = true;

• if (virtio_has_feature(vdev, VIRTIO_NET_F_RSS))

• if (virtio_has_feature(vdev, VIRTIO_NET_F_RSS)) { vi->has_rss = true;

• if (vi->has_rss || vi->has_rss_hash_report) { vi->rss_indir_table_size = virtio_cread16(vdev, offsetof(struct virtio_net_config, rss_max_indirection_table_length));

• }
• if (vi->has_rss || vi->has_rss_hash_report) { vi->rss_key_size = virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size));

2.43.0