| Message ID | 20240321165431.3517868-1-leitao@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio_net: Do not send RSS key if it is not supported | expand |
On Thu, 21 Mar 2024 09:54:30 -0700, Breno Leitao <leitao@debian.org> 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; 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)); vi->rss_key_size = virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size)); vi->rss_hash_types_supported = virtio_cread32(vdev, offsetof(struct virtio_net_config, supported_hash_types)); vi->rss_hash_types_supported &= ~(VIRTIO_NET_RSS_HASH_TYPE_IP_EX | VIRTIO_NET_RSS_HASH_TYPE_TCP_EX | VIRTIO_NET_RSS_HASH_TYPE_UDP_EX); dev->hw_features |= NETIF_F_RXHASH; } vi->rss_key_size is initiated here, I wonder if there is something wrong? Thanks. > > 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 the key scatter-gatter key if it is not set. > > Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.") > Signed-off-by: Breno Leitao <leitao@debian.org> > Cc: stable@vger.kernel.org > Cc: qemu-devel@nongnu.org > --- > drivers/net/virtio_net.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index d7ce4a1011ea..5a7700b103f8 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -3041,11 +3041,16 @@ static int virtnet_set_ringparam(struct net_device *dev, > static bool virtnet_commit_rss_command(struct virtnet_info *vi) > { > struct net_device *dev = vi->dev; > + int has_key = vi->rss_key_size; > struct scatterlist sgs[4]; > unsigned int sg_buf_size; > + int nents = 3; > + > + if (has_key) > + nents += 1; > > /* prepare sgs */ > - sg_init_table(sgs, 4); > + sg_init_table(sgs, nents); > > sg_buf_size = offsetof(struct virtio_net_ctrl_rss, indirection_table); > sg_set_buf(&sgs[0], &vi->ctrl->rss, sg_buf_size); > @@ -3057,8 +3062,13 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi) > - offsetof(struct virtio_net_ctrl_rss, max_tx_vq); > sg_set_buf(&sgs[2], &vi->ctrl->rss.max_tx_vq, sg_buf_size); > > - sg_buf_size = vi->rss_key_size; > - sg_set_buf(&sgs[3], vi->ctrl->rss.key, sg_buf_size); > + if (has_key) { > + /* Only populate if key is available, otherwise > + * populating a buffer with zero size breaks virtio > + */ > + sg_buf_size = vi->rss_key_size; > + sg_set_buf(&sgs[3], vi->ctrl->rss.key, sg_buf_size); > + } > > if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ, > vi->has_rss ? VIRTIO_NET_CTRL_MQ_RSS_CONFIG > -- > 2.43.0 >
Hello Xuan, On Fri, Mar 22, 2024 at 10:00:22AM +0800, Xuan Zhuo wrote: > On Thu, 21 Mar 2024 09:54:30 -0700, Breno Leitao <leitao@debian.org> wrote: > > 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; > > > > 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)); > vi->rss_key_size = > virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size)); > > vi->rss_hash_types_supported = > virtio_cread32(vdev, offsetof(struct virtio_net_config, supported_hash_types)); > vi->rss_hash_types_supported &= > ~(VIRTIO_NET_RSS_HASH_TYPE_IP_EX | > VIRTIO_NET_RSS_HASH_TYPE_TCP_EX | > VIRTIO_NET_RSS_HASH_TYPE_UDP_EX); > > dev->hw_features |= NETIF_F_RXHASH; > } > > > vi->rss_key_size is initiated here, I wonder if there is something wrong? Not really, the code above is never executed (in my machines). This is because `vi->has_rss` and `vi->has_rss_hash_report` are both unset. Looking further, vdev does not have the VIRTIO_NET_F_RSS and VIRTIO_NET_F_HASH_REPORT features. Also, when I run `ethtool -x`, I got: # ethtool -x eth0 RX flow hash indirection table for eth0 with 1 RX ring(s): Operation not supported RSS hash key: Operation not supported RSS hash function: toeplitz: on xor: off crc32: off
On Fri, 22 Mar 2024 03:21:21 -0700, Breno Leitao <leitao@debian.org> wrote: > Hello Xuan, > > On Fri, Mar 22, 2024 at 10:00:22AM +0800, Xuan Zhuo wrote: > > On Thu, 21 Mar 2024 09:54:30 -0700, Breno Leitao <leitao@debian.org> wrote: > > > > 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; > > > > > > > > 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)); > > vi->rss_key_size = > > virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size)); > > > > vi->rss_hash_types_supported = > > virtio_cread32(vdev, offsetof(struct virtio_net_config, supported_hash_types)); > > vi->rss_hash_types_supported &= > > ~(VIRTIO_NET_RSS_HASH_TYPE_IP_EX | > > VIRTIO_NET_RSS_HASH_TYPE_TCP_EX | > > VIRTIO_NET_RSS_HASH_TYPE_UDP_EX); > > > > dev->hw_features |= NETIF_F_RXHASH; > > } > > > > > > vi->rss_key_size is initiated here, I wonder if there is something wrong? > > Not really, the code above is never executed (in my machines). This is > because `vi->has_rss` and `vi->has_rss_hash_report` are both unset. > > Looking further, vdev does not have the VIRTIO_NET_F_RSS and > VIRTIO_NET_F_HASH_REPORT features. > > Also, when I run `ethtool -x`, I got: > > # ethtool -x eth0 > RX flow hash indirection table for eth0 with 1 RX ring(s): > Operation not supported > RSS hash key: > Operation not supported > RSS hash function: > toeplitz: on > xor: off > crc32: off The spec saies: Note that if the device offers VIRTIO_NET_F_HASH_REPORT, even if it supports only one pair of virtqueues, it MUST support at least one of commands of VIRTIO_NET_CTRL_MQ class to configure reported hash parameters: If the device offers VIRTIO_NET_F_RSS, it MUST support VIRTIO_NET_CTRL_MQ_RSS_CONFIG command per 5.1.6.5.7.1. Otherwise the device MUST support VIRTIO_NET_CTRL_MQ_HASH_CONFIG command per 5.1.6.5.6.4. So if we have not anyone of `vi->has_rss` and `vi->has_rss_hash_report`, we should return from virtnet_set_rxfh directly. Thanks.
Hello Xuan, On Mon, Mar 25, 2024 at 01:57:53PM +0800, Xuan Zhuo wrote: > On Fri, 22 Mar 2024 03:21:21 -0700, Breno Leitao <leitao@debian.org> wrote: > > Hello Xuan, > > > > On Fri, Mar 22, 2024 at 10:00:22AM +0800, Xuan Zhuo wrote: > > > On Thu, 21 Mar 2024 09:54:30 -0700, Breno Leitao <leitao@debian.org> wrote: > > > > > > 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; > > > > > > > > > > > > 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)); > > > vi->rss_key_size = > > > virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size)); > > > > > > vi->rss_hash_types_supported = > > > virtio_cread32(vdev, offsetof(struct virtio_net_config, supported_hash_types)); > > > vi->rss_hash_types_supported &= > > > ~(VIRTIO_NET_RSS_HASH_TYPE_IP_EX | > > > VIRTIO_NET_RSS_HASH_TYPE_TCP_EX | > > > VIRTIO_NET_RSS_HASH_TYPE_UDP_EX); > > > > > > dev->hw_features |= NETIF_F_RXHASH; > > > } > > > > > > > > > vi->rss_key_size is initiated here, I wonder if there is something wrong? > > > > Not really, the code above is never executed (in my machines). This is > > because `vi->has_rss` and `vi->has_rss_hash_report` are both unset. > > > > Looking further, vdev does not have the VIRTIO_NET_F_RSS and > > VIRTIO_NET_F_HASH_REPORT features. > > > > Also, when I run `ethtool -x`, I got: > > > > # ethtool -x eth0 > > RX flow hash indirection table for eth0 with 1 RX ring(s): > > Operation not supported > > RSS hash key: > > Operation not supported > > RSS hash function: > > toeplitz: on > > xor: off > > crc32: off > > > The spec saies: > Note that if the device offers VIRTIO_NET_F_HASH_REPORT, even if it > supports only one pair of virtqueues, it MUST support at least one of > commands of VIRTIO_NET_CTRL_MQ class to configure reported hash > parameters: > > If the device offers VIRTIO_NET_F_RSS, it MUST support > VIRTIO_NET_CTRL_MQ_RSS_CONFIG command per 5.1.6.5.7.1. > > Otherwise the device MUST support VIRTIO_NET_CTRL_MQ_HASH_CONFIG command > per 5.1.6.5.6.4. > > > So if we have not anyone of `vi->has_rss` and `vi->has_rss_hash_report`, > we should return from virtnet_set_rxfh directly. Makes sense. Although it is not clear to me how vi->has_rss_hash_report is related here, but, I am convinced that we shouldn't do any RSS operation if the device doesn't have the RSS feature, i.e, vi->has_rss is false. That said, I am thinking about something like this. How does it sound? diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 5a7700b103f8..8c1ad7361cf2 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -3780,6 +3780,9 @@ static int virtnet_set_rxfh(struct net_device *dev, struct virtnet_info *vi = netdev_priv(dev); int i; + if (!vi->has_rss) + return -EOPNOTSUPP; + if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE && rxfh->hfunc != ETH_RSS_HASH_TOP) return -EOPNOTSUPP; Thanks!
On Mon, 25 Mar 2024 04:26:08 -0700, Breno Leitao <leitao@debian.org> wrote: > Hello Xuan, > > On Mon, Mar 25, 2024 at 01:57:53PM +0800, Xuan Zhuo wrote: > > On Fri, 22 Mar 2024 03:21:21 -0700, Breno Leitao <leitao@debian.org> wrote: > > > Hello Xuan, > > > > > > On Fri, Mar 22, 2024 at 10:00:22AM +0800, Xuan Zhuo wrote: > > > > On Thu, 21 Mar 2024 09:54:30 -0700, Breno Leitao <leitao@debian.org> wrote: > > > > > > > > 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; > > > > > > > > > > > > > > > > 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)); > > > > vi->rss_key_size = > > > > virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size)); > > > > > > > > vi->rss_hash_types_supported = > > > > virtio_cread32(vdev, offsetof(struct virtio_net_config, supported_hash_types)); > > > > vi->rss_hash_types_supported &= > > > > ~(VIRTIO_NET_RSS_HASH_TYPE_IP_EX | > > > > VIRTIO_NET_RSS_HASH_TYPE_TCP_EX | > > > > VIRTIO_NET_RSS_HASH_TYPE_UDP_EX); > > > > > > > > dev->hw_features |= NETIF_F_RXHASH; > > > > } > > > > > > > > > > > > vi->rss_key_size is initiated here, I wonder if there is something wrong? > > > > > > Not really, the code above is never executed (in my machines). This is > > > because `vi->has_rss` and `vi->has_rss_hash_report` are both unset. > > > > > > Looking further, vdev does not have the VIRTIO_NET_F_RSS and > > > VIRTIO_NET_F_HASH_REPORT features. > > > > > > Also, when I run `ethtool -x`, I got: > > > > > > # ethtool -x eth0 > > > RX flow hash indirection table for eth0 with 1 RX ring(s): > > > Operation not supported > > > RSS hash key: > > > Operation not supported > > > RSS hash function: > > > toeplitz: on > > > xor: off > > > crc32: off > > > > > > The spec saies: > > Note that if the device offers VIRTIO_NET_F_HASH_REPORT, even if it > > supports only one pair of virtqueues, it MUST support at least one of > > commands of VIRTIO_NET_CTRL_MQ class to configure reported hash > > parameters: > > > > If the device offers VIRTIO_NET_F_RSS, it MUST support > > VIRTIO_NET_CTRL_MQ_RSS_CONFIG command per 5.1.6.5.7.1. > > > > Otherwise the device MUST support VIRTIO_NET_CTRL_MQ_HASH_CONFIG command > > per 5.1.6.5.6.4. > > > > > > So if we have not anyone of `vi->has_rss` and `vi->has_rss_hash_report`, > > we should return from virtnet_set_rxfh directly. > > Makes sense. Although it is not clear to me how vi->has_rss_hash_report > is related here, but, I am convinced that we shouldn't do any RSS > operation if the device doesn't have the RSS feature, i.e, vi->has_rss > is false. > > That said, I am thinking about something like this. How does it sound? > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 5a7700b103f8..8c1ad7361cf2 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -3780,6 +3780,9 @@ static int virtnet_set_rxfh(struct net_device *dev, > struct virtnet_info *vi = netdev_priv(dev); > int i; > > + if (!vi->has_rss) > + return -EOPNOTSUPP; > + Should we check has_rss_hash_report? @Heng Qi Could you help us? Thanks. > if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE && > rxfh->hfunc != ETH_RSS_HASH_TOP) > return -EOPNOTSUPP; > > Thanks!
在 2024/3/25 下午7:35, Xuan Zhuo 写道: > On Mon, 25 Mar 2024 04:26:08 -0700, Breno Leitao <leitao@debian.org> wrote: >> Hello Xuan, >> >> On Mon, Mar 25, 2024 at 01:57:53PM +0800, Xuan Zhuo wrote: >>> On Fri, 22 Mar 2024 03:21:21 -0700, Breno Leitao <leitao@debian.org> wrote: >>>> Hello Xuan, >>>> >>>> On Fri, Mar 22, 2024 at 10:00:22AM +0800, Xuan Zhuo wrote: >>>>> On Thu, 21 Mar 2024 09:54:30 -0700, Breno Leitao <leitao@debian.org> wrote: >>>>>> 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; >>>>> >>>>> >>>>> 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)); >>>>> vi->rss_key_size = >>>>> virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size)); >>>>> >>>>> vi->rss_hash_types_supported = >>>>> virtio_cread32(vdev, offsetof(struct virtio_net_config, supported_hash_types)); >>>>> vi->rss_hash_types_supported &= >>>>> ~(VIRTIO_NET_RSS_HASH_TYPE_IP_EX | >>>>> VIRTIO_NET_RSS_HASH_TYPE_TCP_EX | >>>>> VIRTIO_NET_RSS_HASH_TYPE_UDP_EX); >>>>> >>>>> dev->hw_features |= NETIF_F_RXHASH; >>>>> } >>>>> >>>>> >>>>> vi->rss_key_size is initiated here, I wonder if there is something wrong? >>>> Not really, the code above is never executed (in my machines). This is >>>> because `vi->has_rss` and `vi->has_rss_hash_report` are both unset. >>>> >>>> Looking further, vdev does not have the VIRTIO_NET_F_RSS and >>>> VIRTIO_NET_F_HASH_REPORT features. >>>> >>>> Also, when I run `ethtool -x`, I got: >>>> >>>> # ethtool -x eth0 >>>> RX flow hash indirection table for eth0 with 1 RX ring(s): >>>> Operation not supported >>>> RSS hash key: >>>> Operation not supported >>>> RSS hash function: >>>> toeplitz: on >>>> xor: off >>>> crc32: off >>> >>> The spec saies: >>> Note that if the device offers VIRTIO_NET_F_HASH_REPORT, even if it >>> supports only one pair of virtqueues, it MUST support at least one of >>> commands of VIRTIO_NET_CTRL_MQ class to configure reported hash >>> parameters: >>> >>> If the device offers VIRTIO_NET_F_RSS, it MUST support >>> VIRTIO_NET_CTRL_MQ_RSS_CONFIG command per 5.1.6.5.7.1. >>> >>> Otherwise the device MUST support VIRTIO_NET_CTRL_MQ_HASH_CONFIG command >>> per 5.1.6.5.6.4. >>> >>> >>> So if we have not anyone of `vi->has_rss` and `vi->has_rss_hash_report`, >>> we should return from virtnet_set_rxfh directly. >> Makes sense. Although it is not clear to me how vi->has_rss_hash_report >> is related here, but, I am convinced that we shouldn't do any RSS >> operation if the device doesn't have the RSS feature, i.e, vi->has_rss >> is false. >> >> That said, I am thinking about something like this. How does it sound? >> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >> index 5a7700b103f8..8c1ad7361cf2 100644 >> --- a/drivers/net/virtio_net.c >> +++ b/drivers/net/virtio_net.c >> @@ -3780,6 +3780,9 @@ static int virtnet_set_rxfh(struct net_device *dev, >> struct virtnet_info *vi = netdev_priv(dev); >> int i; >> >> + if (!vi->has_rss) >> + return -EOPNOTSUPP; >> + > Should we check has_rss_hash_report? Hi, Breno. You can refer to the following modification. It is worth noting that \field{rss_max_indirection_table_length} should only be accessed if VIRTIO_NET_F_RSS is negotiated, which I have modified below: diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 727c874..fb4c438 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -3836,10 +3836,16 @@ static int virtnet_set_rxfh(struct net_device *dev, struct virtnet_info *vi = netdev_priv(dev); int i; + if (!vi->has_rss && !vi->has_rss_hash_report) + return -EOPNOTSUPP; + if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE && rxfh->hfunc != ETH_RSS_HASH_TOP) return -EOPNOTSUPP; + if (rxfh->indir && !vi->has_rss) + return -EINVAL; + if (rxfh->indir) { for (i = 0; i < vi->rss_indir_table_size; ++i) vi->ctrl->rss.indirection_table[i] = rxfh->indir[i]; @@ -4757,13 +4763,14 @@ 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)); Regards, Heng > > @Heng Qi > > Could you help us? > > Thanks. > > >> if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE && >> rxfh->hfunc != ETH_RSS_HASH_TOP) >> return -EOPNOTSUPP; >> >> Thanks!
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index d7ce4a1011ea..5a7700b103f8 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -3041,11 +3041,16 @@ static int virtnet_set_ringparam(struct net_device *dev, static bool virtnet_commit_rss_command(struct virtnet_info *vi) { struct net_device *dev = vi->dev; + int has_key = vi->rss_key_size; struct scatterlist sgs[4]; unsigned int sg_buf_size; + int nents = 3; + + if (has_key) + nents += 1; /* prepare sgs */ - sg_init_table(sgs, 4); + sg_init_table(sgs, nents); sg_buf_size = offsetof(struct virtio_net_ctrl_rss, indirection_table); sg_set_buf(&sgs[0], &vi->ctrl->rss, sg_buf_size); @@ -3057,8 +3062,13 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi) - offsetof(struct virtio_net_ctrl_rss, max_tx_vq); sg_set_buf(&sgs[2], &vi->ctrl->rss.max_tx_vq, sg_buf_size); - sg_buf_size = vi->rss_key_size; - sg_set_buf(&sgs[3], vi->ctrl->rss.key, sg_buf_size); + if (has_key) { + /* Only populate if key is available, otherwise + * populating a buffer with zero size breaks virtio + */ + sg_buf_size = vi->rss_key_size; + sg_set_buf(&sgs[3], vi->ctrl->rss.key, sg_buf_size); + } if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ, vi->has_rss ? VIRTIO_NET_CTRL_MQ_RSS_CONFIG
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 the key scatter-gatter key if it is not set. Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.") Signed-off-by: Breno Leitao <leitao@debian.org> Cc: stable@vger.kernel.org Cc: qemu-devel@nongnu.org --- drivers/net/virtio_net.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)