Re: [PATCH net] virtio-net: set queues after driver_ok

On Wed, Aug 9, 2023 at 2:51 PM Michael S. Tsirkin [off-list ref] wrote:

On Wed, Aug 09, 2023 at 02:45:17PM +0800, Jason Wang wrote:

quoted

On Wed, Aug 9, 2023 at 2:23 PM Dragos Tatulea [off-list ref] wrote:

quoted

On Tue, 2023-08-08 at 23:13 -0400, Jason Wang wrote:

quoted

Commit 25266128fe16 ("virtio-net: fix race between set queues and
probe") tries to fix the race between set queues and probe by calling
_virtnet_set_queues() before DRIVER_OK is set. This violates virtio
spec. Fixing this by setting queues after virtio_device_ready().

Fixes: 25266128fe16 ("virtio-net: fix race between set queues and probe")
Reported-by: Dragos Tatulea <dtatulea@nvidia.com>
Signed-off-by: Jason Wang <redacted>
---
The patch is needed for -stable.
---
 drivers/net/virtio_net.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 1270c8d23463..ff03921e46df 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -4219,8 +4219,6 @@ static int virtnet_probe(struct virtio_device *vdev)
        if (vi->has_rss || vi->has_rss_hash_report)
                virtnet_init_default_rss(vi);

-       _virtnet_set_queues(vi, vi->curr_queue_pairs);
-
        /* serialize netdev register + virtio_device_ready() with ndo_open()

*/
        rtnl_lock();

@@ -4233,6 +4231,8 @@ static int virtnet_probe(struct virtio_device *vdev)

        virtio_device_ready(vdev);

+       _virtnet_set_queues(vi, vi->curr_queue_pairs);
+
        /* a random MAC address has been assigned, notify the device.
         * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not there
         * because many devices work fine without getting MAC explicitly

Thanks for the quick fix. Doesn't this fix though bring back the original race
that was fixed in commit 25266128fe16 ("virtio-net: fix race between set queues
and probe")? Or is being under the same rntl_lock session as register_netdev
enough to avoid the race?

Yes, rtnl needs to be held for userspace requests to change the number
of queues. So we are serialized in this way.

Thanks

maybe post v2 adding this in the commit log.

Ok, v2 will be posted soon.

Thanks

quoted

quoted

Thanks,
Dragos