Messages in this thread | | | Date | Wed, 3 Apr 2024 15:57:22 +0800 | Subject | Re: [PATCH net v3] virtio_net: Do not send RSS key if it is not supported | From | Heng Qi <> |
| |
在 2024/4/1 上午4:20, Michael S. Tsirkin 写道: > 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.
&&
Hi Breno,
You can add a comment like the following:
If either _F_HASH_REPORT or _F_RSS are negotiated, the device provides hash calculation capabilities, that is, hash_key can be configured.
Regards, Heng
> >> + >> 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
| |