| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 15 Aug 2022 16:36:18 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: linux-kernel@...r.kernel.org,
Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
Jason Wang <jasowang@...hat.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
Linus Torvalds <torvalds@...ux-foundation.org>,
Jens Axboe <axboe@...nel.dk>,
James Bottomley <James.Bottomley@...senpartnership.com>,
"Martin K. Petersen" <martin.petersen@...cle.com>,
Greg KH <gregkh@...uxfoundation.org>,
Andres Freund <andres@...razel.de>
Subject: Re: [PATCH v3 1/5] virtio_net: Revert "virtio_net: set the default
max ring size by find_vqs()"
On Mon, Aug 15, 2022 at 06:00:25PM -0400, Michael S. Tsirkin wrote:
> This reverts commit 762faee5a2678559d3dc09d95f8f2c54cd0466a7.
>
> This has been reported to trip up guests on GCP (Google Cloud).
> The reason is that virtio_find_vqs_ctx_size is broken on legacy
> devices. We can in theory fix virtio_find_vqs_ctx_size but
> in fact the patch itself has several other issues:
>
> - It treats unknown speed as < 10G
> - It leaves userspace no way to find out the ring size set by hypervisor
> - It tests speed when link is down
> - It ignores the virtio spec advice:
> Both \field{speed} and \field{duplex} can change, thus the driver
> is expected to re-read these values after receiving a
> configuration change notification.
> - It is not clear the performance impact has been tested properly
>
> Revert the patch for now.
>
> Reported-by: Andres Freund <andres@...razel.de>
> Link: https://lore.kernel.org/r/20220814212610.GA3690074%40roeck-us.net
> Link: https://lore.kernel.org/r/20220815070203.plwjx7b3cyugpdt7%40awork3.anarazel.de
> Link: https://lore.kernel.org/r/3df6bb82-1951-455d-a768-e9e1513eb667%40www.fastmail.com
> Link: https://lore.kernel.org/r/FCDC5DDE-3CDD-4B8A-916F-CA7D87B547CE%40anarazel.de
> Fixes: 762faee5a267 ("virtio_net: set the default max ring size by find_vqs()")
> Cc: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
> Cc: Jason Wang <jasowang@...hat.com>
> Signed-off-by: Michael S. Tsirkin <mst@...hat.com>
> Tested-by: Andres Freund <andres@...razel.de>
> Tested-by: From: Guenter Roeck <linux@...ck-us.net>
s/From: //
> ---
> drivers/net/virtio_net.c | 42 ++++------------------------------------
> 1 file changed, 4 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index d934774e9733..ece00b84e3a7 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3432,29 +3432,6 @@ static unsigned int mergeable_min_buf_len(struct virtnet_info *vi, struct virtqu
> (unsigned int)GOOD_PACKET_LEN);
> }
>
> -static void virtnet_config_sizes(struct virtnet_info *vi, u32 *sizes)
> -{
> - u32 i, rx_size, tx_size;
> -
> - if (vi->speed == SPEED_UNKNOWN || vi->speed < SPEED_10000) {
> - rx_size = 1024;
> - tx_size = 1024;
> -
> - } else if (vi->speed < SPEED_40000) {
> - rx_size = 1024 * 4;
> - tx_size = 1024 * 4;
> -
> - } else {
> - rx_size = 1024 * 8;
> - tx_size = 1024 * 8;
> - }
> -
> - for (i = 0; i < vi->max_queue_pairs; i++) {
> - sizes[rxq2vq(i)] = rx_size;
> - sizes[txq2vq(i)] = tx_size;
> - }
> -}
> -
> static int virtnet_find_vqs(struct virtnet_info *vi)
> {
> vq_callback_t **callbacks;
> @@ -3462,7 +3439,6 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> int ret = -ENOMEM;
> int i, total_vqs;
> const char **names;
> - u32 *sizes;
> bool *ctx;
>
> /* We expect 1 RX virtqueue followed by 1 TX virtqueue, followed by
> @@ -3490,15 +3466,10 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> ctx = NULL;
> }
>
> - sizes = kmalloc_array(total_vqs, sizeof(*sizes), GFP_KERNEL);
> - if (!sizes)
> - goto err_sizes;
> -
> /* Parameters for control virtqueue, if any */
> if (vi->has_cvq) {
> callbacks[total_vqs - 1] = NULL;
> names[total_vqs - 1] = "control";
> - sizes[total_vqs - 1] = 64;
> }
>
> /* Allocate/initialize parameters for send/receive virtqueues */
> @@ -3513,10 +3484,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> ctx[rxq2vq(i)] = true;
> }
>
> - virtnet_config_sizes(vi, sizes);
> -
> - ret = virtio_find_vqs_ctx_size(vi->vdev, total_vqs, vqs, callbacks,
> - names, sizes, ctx, NULL);
> + ret = virtio_find_vqs_ctx(vi->vdev, total_vqs, vqs, callbacks,
> + names, ctx, NULL);
> if (ret)
> goto err_find;
>
> @@ -3536,8 +3505,6 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>
>
> err_find:
> - kfree(sizes);
> -err_sizes:
> kfree(ctx);
> err_ctx:
> kfree(names);
> @@ -3897,9 +3864,6 @@ static int virtnet_probe(struct virtio_device *vdev)
> vi->curr_queue_pairs = num_online_cpus();
> vi->max_queue_pairs = max_queue_pairs;
>
> - virtnet_init_settings(dev);
> - virtnet_update_settings(vi);
> -
> /* Allocate/initialize the rx/tx queues, and invoke find_vqs */
> err = init_vqs(vi);
> if (err)
> @@ -3912,6 +3876,8 @@ static int virtnet_probe(struct virtio_device *vdev)
> netif_set_real_num_tx_queues(dev, vi->curr_queue_pairs);
> netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
>
> + virtnet_init_settings(dev);
> +
> if (virtio_has_feature(vdev, VIRTIO_NET_F_STANDBY)) {
> vi->failover = net_failover_create(vi->dev);
> if (IS_ERR(vi->failover)) {
> --
> MST
>
Powered by blists - more mailing lists