| Message ID | 20240329171641.366520-1-leitao@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] virtio_net: Do not send RSS key if it is not supported | expand |
在 2024/3/30 上午1:16, Breno Leitao 写道: > 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 > 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; > + > 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)); > Reviewed-by: Heng Qi <hengqi@linux.alibaba.com> Thanks.
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
On Sun, 31 Mar 2024 16:20:30 -0400 Michael S. Tsirkin wrote: > > 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. Not any more, FWIW: 1.5.7. Stable tree While it used to be the case that netdev submissions were not supposed to carry explicit CC: stable@vger.kernel.org tags that is no longer the case today. Please follow the standard stable rules in Documentation/process/stable-kernel-rules.rst, and make sure you include appropriate Fixes tags! https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#stable-tree
在 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
On Sun, Mar 31, 2024 at 04:20:30PM -0400, Michael S. Tsirkin wrote: > On Fri, Mar 29, 2024 at 10:16:41AM -0700, Breno Leitao wrote: > > @@ -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. If txfh carries a key, then the device needs to has either has_rss or has_rss_hash_report "features". These are basically virtio features VIRTIO_NET_F_HASH_REPORT and VIRTIO_NET_F_RSS that are set at virtio_probe. I will add the comment and respin the series.
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; + 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));
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 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(-)