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@xxxxxxxxxx>
> Reviewed-by: Jiri Pirko <jiri@xxxxxxxxxx>
> ---
> 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
