[PATCH net 0/2] virtio_net: fix race on control_buf

[PATCH net 0/2] virtio_net: fix race on control_buf

[Heng Qi]

Patch 1 did a simple rename, leaving 'ret' for patch 2.
Patch 2 fixed a race between reading the device response and the
new command submission.

Heng Qi (2):
  virtio_net: rename ret to err
  virtio_net: fix missing lock protection on control_buf access

 drivers/net/virtio_net.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

-- 
2.32.0.3.g01195cf9f

[PATCH net 1/2] virtio_net: rename ret to err

[Heng Qi]

The integer variable 'ret', denoting the return code, is mismatched with
the boolean return type of virtnet_send_command_reply(); hence, it is
renamed to 'err'.

The usage of 'ret' is deferred to the next patch.

Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4a802c0ea2cb..6b0512a628e0 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2686,7 +2686,7 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
 {
 	struct scatterlist *sgs[5], hdr, stat;
 	u32 out_num = 0, tmp, in_num = 0;
-	int ret;
+	int err;
 
 	/* Caller should know better */
 	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
@@ -2710,10 +2710,10 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
 		sgs[out_num + in_num++] = in;
 
 	BUG_ON(out_num + in_num > ARRAY_SIZE(sgs));
-	ret = virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi, GFP_ATOMIC);
-	if (ret < 0) {
+	err = virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi, GFP_ATOMIC);
+	if (err < 0) {
 		dev_warn(&vi->vdev->dev,
-			 "Failed to add sgs for command vq: %d\n.", ret);
+			 "Failed to add sgs for command vq: %d\n.", err);
 		mutex_unlock(&vi->cvq_lock);
 		return false;
 	}
-- 
2.32.0.3.g01195cf9f

[PATCH net 2/2] virtio_net: fix missing lock protection on control_buf access

[Heng Qi]

Refactored the handling of control_buf to be within the cvq_lock
critical section, mitigating race conditions between reading device
responses and new command submissions.

Fixes: 6f45ab3e0409 ("virtio_net: Add a lock for the command VQ.")
Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6b0512a628e0..3d8407d9e3d2 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2686,6 +2686,7 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
 {
 	struct scatterlist *sgs[5], hdr, stat;
 	u32 out_num = 0, tmp, in_num = 0;
+	bool ret;
 	int err;
 
 	/* Caller should know better */
@@ -2731,8 +2732,9 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
 	}
 
 unlock:
+	ret = vi->ctrl->status == VIRTIO_NET_OK;
 	mutex_unlock(&vi->cvq_lock);
-	return vi->ctrl->status == VIRTIO_NET_OK;
+	return ret;
 }
 
 static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
-- 
2.32.0.3.g01195cf9f

[PATCH net 1/2] virtio_net: rename ret to err

[Hariprasad Kelam]

> -----Original Message-----
> From: Heng Qi <hengqi@linux.alibaba.com>
> Sent: Tuesday, May 28, 2024 1:22 PM
> To: netdev@vger.kernel.org; virtualization@lists.linux.dev
> Cc: Michael S. Tsirkin <mst@redhat.com>; Jason Wang
> <jasowang@redhat.com>; Xuan Zhuo <xuanzhuo@linux.alibaba.com>;
> Eugenio Pérez <eperezma@redhat.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Jiri Pirko
> <jiri@resnulli.us>; Daniel Jurgens <danielj@nvidia.com>
> Subject: [EXTERNAL] [PATCH net 1/2] virtio_net: rename ret to err
> 
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
> 
> ----------------------------------------------------------------------
> The integer variable 'ret', denoting the return code, is mismatched with the
> boolean return type of virtnet_send_command_reply(); hence, it is renamed
> to 'err'.
> 
> The usage of 'ret' is deferred to the next patch.
> 
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> ---
>  drivers/net/virtio_net.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index
> 4a802c0ea2cb..6b0512a628e0 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2686,7 +2686,7 @@ static bool virtnet_send_command_reply(struct
> virtnet_info *vi, u8 class, u8 cmd  {
>  	struct scatterlist *sgs[5], hdr, stat;
>  	u32 out_num = 0, tmp, in_num = 0;
> -	int ret;
> +	int err;
> 
>  	/* Caller should know better */
>  	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
> @@ -2710,10 +2710,10 @@ static bool virtnet_send_command_reply(struct
> virtnet_info *vi, u8 class, u8 cmd
>  		sgs[out_num + in_num++] = in;
> 
>  	BUG_ON(out_num + in_num > ARRAY_SIZE(sgs));
> -	ret = virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi,
> GFP_ATOMIC);
> -	if (ret < 0) {
> +	err = virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi,
> GFP_ATOMIC);
> +	if (err < 0) {
>  		dev_warn(&vi->vdev->dev,
> -			 "Failed to add sgs for command vq: %d\n.", ret);
> +			 "Failed to add sgs for command vq: %d\n.", err);
>  		mutex_unlock(&vi->cvq_lock);
>  		return false;
>  	}
> --
> 2.32.0.3.g01195cf9f
> 
Reviewed-by: Hariprasad Kelam <hkelam@marvell.com>

[PATCH net 2/2] virtio_net: fix missing lock protection on control_buf access

[Hariprasad Kelam]

> Refactored the handling of control_buf to be within the cvq_lock critical
> section, mitigating race conditions between reading device responses and
> new command submissions.
> 
> Fixes: 6f45ab3e0409 ("virtio_net: Add a lock for the command VQ.")
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> ---
>  drivers/net/virtio_net.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index
> 6b0512a628e0..3d8407d9e3d2 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2686,6 +2686,7 @@ static bool virtnet_send_command_reply(struct
> virtnet_info *vi, u8 class, u8 cmd  {
>  	struct scatterlist *sgs[5], hdr, stat;
>  	u32 out_num = 0, tmp, in_num = 0;
> +	bool ret;
>  	int err;
> 
>  	/* Caller should know better */
> @@ -2731,8 +2732,9 @@ static bool virtnet_send_command_reply(struct
> virtnet_info *vi, u8 class, u8 cmd
>  	}
> 
>  unlock:
> +	ret = vi->ctrl->status == VIRTIO_NET_OK;
>  	mutex_unlock(&vi->cvq_lock);
> -	return vi->ctrl->status == VIRTIO_NET_OK;
> +	return ret;
>  }
> 
>  static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> --
> 2.32.0.3.g01195cf9f
> 
Reviewed-by: Hariprasad Kelam <hkelam@marvell.com>

Re: [PATCH net 2/2] virtio_net: fix missing lock protection on control_buf access

[Michael S. Tsirkin]

On Tue, May 28, 2024 at 03:52:26PM +0800, Heng Qi wrote:
> Refactored the handling of control_buf to be within the cvq_lock
> critical section, mitigating race conditions between reading device
> responses and new command submissions.
> 
> Fixes: 6f45ab3e0409 ("virtio_net: Add a lock for the command VQ.")
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>


I don't get what does this change. status can change immediately
after you drop the mutex, can it not? what exactly is the
race conditions you are worried about?

> ---
>  drivers/net/virtio_net.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6b0512a628e0..3d8407d9e3d2 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2686,6 +2686,7 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
>  {
>  	struct scatterlist *sgs[5], hdr, stat;
>  	u32 out_num = 0, tmp, in_num = 0;
> +	bool ret;
>  	int err;
>  
>  	/* Caller should know better */
> @@ -2731,8 +2732,9 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
>  	}
>  
>  unlock:
> +	ret = vi->ctrl->status == VIRTIO_NET_OK;
>  	mutex_unlock(&vi->cvq_lock);
> -	return vi->ctrl->status == VIRTIO_NET_OK;
> +	return ret;
>  }
>  
>  static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> -- 
> 2.32.0.3.g01195cf9f

Re: [PATCH net 2/2] virtio_net: fix missing lock protection on control_buf access

[Heng Qi]

On Tue, 28 May 2024 11:46:28 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, May 28, 2024 at 03:52:26PM +0800, Heng Qi wrote:
> > Refactored the handling of control_buf to be within the cvq_lock
> > critical section, mitigating race conditions between reading device
> > responses and new command submissions.
> > 
> > Fixes: 6f45ab3e0409 ("virtio_net: Add a lock for the command VQ.")
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> 
> 
> I don't get what does this change. status can change immediately
> after you drop the mutex, can it not? what exactly is the
> race conditions you are worried about?

See the following case:

1. Command A is acknowledged and successfully executed by the device.
2. After releasing the mutex (mutex_unlock), process P1 gets preempted before
   it can read vi->ctrl->status, *which should be VIRTIO_NET_OK*.
3. A new command B (like the DIM command) is issued.
4. Post vi->ctrl->status being set to VIRTIO_NET_ERR by
   virtnet_send_command_reply(), process P2 gets preempted.
5. Process P1 resumes, reads *vi->ctrl->status as VIRTIO_NET_ERR*, and reports
   this error back for Command A. <-- Race causes incorrect results to be read.

Thanks.

> 
> > ---
> >  drivers/net/virtio_net.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 6b0512a628e0..3d8407d9e3d2 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2686,6 +2686,7 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
> >  {
> >  	struct scatterlist *sgs[5], hdr, stat;
> >  	u32 out_num = 0, tmp, in_num = 0;
> > +	bool ret;
> >  	int err;
> >  
> >  	/* Caller should know better */
> > @@ -2731,8 +2732,9 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
> >  	}
> >  
> >  unlock:
> > +	ret = vi->ctrl->status == VIRTIO_NET_OK;
> >  	mutex_unlock(&vi->cvq_lock);
> > -	return vi->ctrl->status == VIRTIO_NET_OK;
> > +	return ret;
> >  }
> >  
> >  static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > -- 
> > 2.32.0.3.g01195cf9f
> 

Re: [PATCH net 2/2] virtio_net: fix missing lock protection on control_buf access

[Michael S. Tsirkin]

On Wed, May 29, 2024 at 12:01:45AM +0800, Heng Qi wrote:
> On Tue, 28 May 2024 11:46:28 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Tue, May 28, 2024 at 03:52:26PM +0800, Heng Qi wrote:
> > > Refactored the handling of control_buf to be within the cvq_lock
> > > critical section, mitigating race conditions between reading device
> > > responses and new command submissions.
> > > 
> > > Fixes: 6f45ab3e0409 ("virtio_net: Add a lock for the command VQ.")
> > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > 
> > 
> > I don't get what does this change. status can change immediately
> > after you drop the mutex, can it not? what exactly is the
> > race conditions you are worried about?
> 
> See the following case:
> 
> 1. Command A is acknowledged and successfully executed by the device.
> 2. After releasing the mutex (mutex_unlock), process P1 gets preempted before
>    it can read vi->ctrl->status, *which should be VIRTIO_NET_OK*.
> 3. A new command B (like the DIM command) is issued.
> 4. Post vi->ctrl->status being set to VIRTIO_NET_ERR by
>    virtnet_send_command_reply(), process P2 gets preempted.
> 5. Process P1 resumes, reads *vi->ctrl->status as VIRTIO_NET_ERR*, and reports
>    this error back for Command A. <-- Race causes incorrect results to be read.
> 
> Thanks.


Why is it important that P1 gets VIRTIO_NET_OK?
After all it is no longer the state.

> > 
> > > ---
> > >  drivers/net/virtio_net.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 6b0512a628e0..3d8407d9e3d2 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -2686,6 +2686,7 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
> > >  {
> > >  	struct scatterlist *sgs[5], hdr, stat;
> > >  	u32 out_num = 0, tmp, in_num = 0;
> > > +	bool ret;
> > >  	int err;
> > >  
> > >  	/* Caller should know better */
> > > @@ -2731,8 +2732,9 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
> > >  	}
> > >  
> > >  unlock:
> > > +	ret = vi->ctrl->status == VIRTIO_NET_OK;
> > >  	mutex_unlock(&vi->cvq_lock);
> > > -	return vi->ctrl->status == VIRTIO_NET_OK;
> > > +	return ret;
> > >  }
> > >  
> > >  static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > -- 
> > > 2.32.0.3.g01195cf9f
> > 

Re: [PATCH net 2/2] virtio_net: fix missing lock protection on control_buf access

[Heng Qi]

On Tue, 28 May 2024 12:45:32 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, May 29, 2024 at 12:01:45AM +0800, Heng Qi wrote:
> > On Tue, 28 May 2024 11:46:28 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Tue, May 28, 2024 at 03:52:26PM +0800, Heng Qi wrote:
> > > > Refactored the handling of control_buf to be within the cvq_lock
> > > > critical section, mitigating race conditions between reading device
> > > > responses and new command submissions.
> > > > 
> > > > Fixes: 6f45ab3e0409 ("virtio_net: Add a lock for the command VQ.")
> > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > 
> > > 
> > > I don't get what does this change. status can change immediately
> > > after you drop the mutex, can it not? what exactly is the
> > > race conditions you are worried about?
> > 
> > See the following case:
> > 
> > 1. Command A is acknowledged and successfully executed by the device.
> > 2. After releasing the mutex (mutex_unlock), process P1 gets preempted before
> >    it can read vi->ctrl->status, *which should be VIRTIO_NET_OK*.
> > 3. A new command B (like the DIM command) is issued.
> > 4. Post vi->ctrl->status being set to VIRTIO_NET_ERR by
> >    virtnet_send_command_reply(), process P2 gets preempted.
> > 5. Process P1 resumes, reads *vi->ctrl->status as VIRTIO_NET_ERR*, and reports
> >    this error back for Command A. <-- Race causes incorrect results to be read.
> > 
> > Thanks.
> 
> 
> Why is it important that P1 gets VIRTIO_NET_OK?
> After all it is no longer the state.

The driver needs to know whether the command actually executed success.

Thanks.

> 
> > > 
> > > > ---
> > > >  drivers/net/virtio_net.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 6b0512a628e0..3d8407d9e3d2 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -2686,6 +2686,7 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
> > > >  {
> > > >  	struct scatterlist *sgs[5], hdr, stat;
> > > >  	u32 out_num = 0, tmp, in_num = 0;
> > > > +	bool ret;
> > > >  	int err;
> > > >  
> > > >  	/* Caller should know better */
> > > > @@ -2731,8 +2732,9 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
> > > >  	}
> > > >  
> > > >  unlock:
> > > > +	ret = vi->ctrl->status == VIRTIO_NET_OK;
> > > >  	mutex_unlock(&vi->cvq_lock);
> > > > -	return vi->ctrl->status == VIRTIO_NET_OK;
> > > > +	return ret;
> > > >  }
> > > >  
> > > >  static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > -- 
> > > > 2.32.0.3.g01195cf9f
> > > 
> 

Re: [PATCH net 1/2] virtio_net: rename ret to err

[Jakub Kicinski]

On Tue, 28 May 2024 15:52:25 +0800 Heng Qi wrote:
> The integer variable 'ret', denoting the return code, is mismatched with
> the boolean return type of virtnet_send_command_reply(); hence, it is
> renamed to 'err'.
> 
> The usage of 'ret' is deferred to the next patch.

That seems a bit much. Can you call the variable in patch 2 "ok"
instead?
-- 
pw-bot: cr

Re: [PATCH net 1/2] virtio_net: rename ret to err

[Heng Qi]

On Wed, 29 May 2024 17:10:53 -0700, Jakub Kicinski <kuba@kernel.org> wrote:
> On Tue, 28 May 2024 15:52:25 +0800 Heng Qi wrote:
> > The integer variable 'ret', denoting the return code, is mismatched with
> > the boolean return type of virtnet_send_command_reply(); hence, it is
> > renamed to 'err'.
> > 
> > The usage of 'ret' is deferred to the next patch.
> 
> That seems a bit much. Can you call the variable in patch 2 "ok"
> instead?

Sure.

Thanks.

> -- 
> pw-bot: cr

Re: [PATCH net 0/2] virtio_net: fix race on control_buf

[Michael S. Tsirkin]

On Tue, May 28, 2024 at 03:52:24PM +0800, Heng Qi wrote:
> Patch 1 did a simple rename, leaving 'ret' for patch 2.
> Patch 2 fixed a race between reading the device response and the
> new command submission.


Acked-by: Michael S. Tsirkin <mst@redhat.com>


> Heng Qi (2):
>   virtio_net: rename ret to err
>   virtio_net: fix missing lock protection on control_buf access
> 
>  drivers/net/virtio_net.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> -- 
> 2.32.0.3.g01195cf9f