| Message ID | 20230809031329.251362-1-jasowang@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] virtio-net: set queues after driver_ok | expand |
On Tue, 2023-08-08 at 23:13 -0400, Jason Wang wrote: > 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 <jasowang@redhat.com> > --- > 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? Thanks, Dragos
On Tue, Aug 08, 2023 at 11:13:29PM -0400, Jason Wang wrote: > 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 <jasowang@redhat.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > 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 > -- > 2.39.3
On Wed, Aug 9, 2023 at 2:23 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > On Tue, 2023-08-08 at 23:13 -0400, Jason Wang wrote: > > 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 <jasowang@redhat.com> > > --- > > 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 > > Thanks, > Dragos
On Wed, Aug 09, 2023 at 02:45:17PM +0800, Jason Wang wrote: > On Wed, Aug 9, 2023 at 2:23 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > On Tue, 2023-08-08 at 23:13 -0400, Jason Wang wrote: > > > 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 <jasowang@redhat.com> > > > --- > > > 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. > > > > Thanks, > > Dragos
On Wed, Aug 9, 2023 at 2:51 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Aug 09, 2023 at 02:45:17PM +0800, Jason Wang wrote: > > On Wed, Aug 9, 2023 at 2:23 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > On Tue, 2023-08-08 at 23:13 -0400, Jason Wang wrote: > > > > 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 <jasowang@redhat.com> > > > > --- > > > > 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 > > > > > > > > Thanks, > > > Dragos >
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
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 <jasowang@redhat.com> --- The patch is needed for -stable. --- drivers/net/virtio_net.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)