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

[Xuan Zhuo]

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
>