On Thu, 21 Mar 2024 09:54:30 -0700, Breno Leitao <lei...@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 <lei...@debian.org>
> Cc: sta...@vger.kernel.org
> Cc: qemu-de...@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
>
