Received-SPF: pass (google.com: domain of linux-kernel+bounces-110868-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <1711072822.882584-1-xuanzhuo@linux.alibaba.com>
Subject: Re: [PATCH] virtio_net: Do not send RSS key if it is not supported
Date: Fri, 22 Mar 2024 10:00:22 +0800
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
To: Breno Leitao <leitao@debian.org>
Cc: rbc@meta.com,
 riel@surriel.com,
 stable@vger.kernel.org,
 qemu-devel@nongnu.org,
 virtualization@lists.linux.dev (open list:VIRTIO CORE AND NET DRIVERS),
 netdev@vger.kernel.org (open list:NETWORKING DRIVERS),
 linux-kernel@vger.kernel.org (open list),
 "Michael S. Tsirkin" <mst@redhat.com>,
 Jason Wang <jasowang@redhat.com>,
 "David S. Miller" <davem@davemloft.net>,
 Eric Dumazet <edumazet@google.com>,
 Jakub Kicinski <kuba@kernel.org>,
 Paolo Abeni <pabeni@redhat.com>,
 Andrew Melnychenko <andrew@daynix.com>
References: <20240321165431.3517868-1-leitao@debian.org>
In-Reply-To: <20240321165431.3517868-1-leitao@debian.org>
Precedence: bulk

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
>