drivers/net/virtio_net.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)
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));
--
2.43.0
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, 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.
在 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, 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/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.
© 2016 - 2024 Red Hat, Inc.