| Message ID | 20240423035746.699466-4-danielj@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Remove RTNL lock protection of CVQ | expand |
On Tue, 2024-04-23 at 06:57 +0300, Daniel Jurgens wrote: > The command VQ will no longer be protected by the RTNL lock. Use a > mutex to protect the control buffer header and the VQ. > > Signed-off-by: Daniel Jurgens <danielj@nvidia.com> > Reviewed-by: Jiri Pirko <jiri@nvidia.com> > --- > drivers/net/virtio_net.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 0ee192b45e1e..d752c8ac5cd3 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -282,6 +282,7 @@ struct virtnet_info { > > /* Has control virtqueue */ > bool has_cvq; > + struct mutex cvq_lock; Minor nit: checkpatch complains this lock needs a comment > > /* Host can handle any s/g split between our header and packet data */ > bool any_header_sg; > @@ -2529,6 +2530,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > /* Caller should know better */ > BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)); > > + mutex_lock(&vi->cvq_lock); > vi->ctrl->status = ~0; > vi->ctrl->hdr.class = class; > vi->ctrl->hdr.cmd = cmd; > @@ -2548,11 +2550,14 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > if (ret < 0) { > dev_warn(&vi->vdev->dev, > "Failed to add sgs for command vq: %d\n.", ret); > + mutex_unlock(&vi->cvq_lock); > return false; > } > > - if (unlikely(!virtqueue_kick(vi->cvq))) > + if (unlikely(!virtqueue_kick(vi->cvq))) { > + mutex_unlock(&vi->cvq_lock); > return vi->ctrl->status == VIRTIO_NET_OK; or: goto unlock; > + } > > /* Spin for a response, the kick causes an ioport write, trapping > * into the hypervisor, so the request should be handled immediately. > @@ -2563,6 +2568,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > cpu_relax(); > } > unlock: > + mutex_unlock(&vi->cvq_lock); > return vi->ctrl->status == VIRTIO_NET_OK; > } > > @@ -4818,8 +4824,10 @@ static int virtnet_probe(struct virtio_device *vdev) > virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) > vi->any_header_sg = true; > > - if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) > + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) { > vi->has_cvq = true; > + mutex_init(&vi->cvq_lock); I'm wondering if syzkaller will be able to touch the lock in some unexpected path? possibly worth always initializing it? Thanks, Paolo
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 0ee192b45e1e..d752c8ac5cd3 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -282,6 +282,7 @@ struct virtnet_info { /* Has control virtqueue */ bool has_cvq; + struct mutex cvq_lock; /* Host can handle any s/g split between our header and packet data */ bool any_header_sg; @@ -2529,6 +2530,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, /* Caller should know better */ BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)); + mutex_lock(&vi->cvq_lock); vi->ctrl->status = ~0; vi->ctrl->hdr.class = class; vi->ctrl->hdr.cmd = cmd; @@ -2548,11 +2550,14 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, if (ret < 0) { dev_warn(&vi->vdev->dev, "Failed to add sgs for command vq: %d\n.", ret); + mutex_unlock(&vi->cvq_lock); return false; } - if (unlikely(!virtqueue_kick(vi->cvq))) + if (unlikely(!virtqueue_kick(vi->cvq))) { + mutex_unlock(&vi->cvq_lock); return vi->ctrl->status == VIRTIO_NET_OK; + } /* Spin for a response, the kick causes an ioport write, trapping * into the hypervisor, so the request should be handled immediately. @@ -2563,6 +2568,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, cpu_relax(); } + mutex_unlock(&vi->cvq_lock); return vi->ctrl->status == VIRTIO_NET_OK; } @@ -4818,8 +4824,10 @@ static int virtnet_probe(struct virtio_device *vdev) virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) vi->any_header_sg = true; - if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) { vi->has_cvq = true; + mutex_init(&vi->cvq_lock); + } if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) { mtu = virtio_cread16(vdev,