[v1] virtio_net: Do not send RSS key if it is not supported

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

Posted by Breno Leitao 1 month, 1 week ago

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(-)

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

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

Posted by Xuan Zhuo 1 month ago

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
>

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

Posted by Breno Leitao 1 month ago

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

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

Posted by Xuan Zhuo 1 month ago

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.

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

Posted by Breno Leitao 1 month ago

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!

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

Posted by Xuan Zhuo 1 month ago

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!

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

Posted by Heng Qi 1 month ago


在 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!