Re: [PATCH 1/4] vdpa: Protect vdpa reset with cf_mutex

Re: [PATCH 1/4] vdpa: Protect vdpa reset with cf_mutex

[Si-Wei Liu]

On 1/10/2022 11:22 PM, Eli Cohen wrote:
> Call reset using the wrapper function vdpa_reset() to make sure the
> operation is serialized with cf_mutex.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
>   drivers/vhost/vdpa.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 6e7edaf2472b..fe0bbea4dab6 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -155,7 +155,6 @@ static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8 __user *statusp)
>   static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
>   {
>   	struct vdpa_device *vdpa = v->vdpa;
> -	const struct vdpa_config_ops *ops = vdpa->config;
It is okay to remove this line from the patch, but a follow-up patch to 
remove the locking for get_status would need to reintroduce the same 
line. If possible please try post these two together in a same series.

Reviewed-by: Si-Wei Liu<si-wei.liu@oracle.com>

>   	u8 status, status_old;
>   	int ret, nvqs = v->nvqs;
>   	u16 i;
> @@ -177,7 +176,7 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
>   			vhost_vdpa_unsetup_vq_irq(v, i);
>   
>   	if (status == 0) {
> -		ret = ops->reset(vdpa);
> +		ret = vdpa_reset(vdpa);
>   		if (ret)
>   			return ret;
>   	} else

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/4] vdpa: Protect vdpa reset with cf_mutex

[Michael S. Tsirkin]

On Tue, Jan 11, 2022 at 09:22:17AM +0200, Eli Cohen wrote:
> Call reset using the wrapper function vdpa_reset() to make sure the
> operation is serialized with cf_mutex.
> 
> Signed-off-by: Eli Cohen <elic@nvidia.com>

The motivation is vague here, this does not really explain.
Which operations are we trying to serialize?
Multiple reset requests from userspace?
Is anything broken right now without this patch?

> ---
>  drivers/vhost/vdpa.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 6e7edaf2472b..fe0bbea4dab6 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -155,7 +155,6 @@ static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8 __user *statusp)
>  static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
>  {
>  	struct vdpa_device *vdpa = v->vdpa;
> -	const struct vdpa_config_ops *ops = vdpa->config;
>  	u8 status, status_old;
>  	int ret, nvqs = v->nvqs;
>  	u16 i;
> @@ -177,7 +176,7 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
>  			vhost_vdpa_unsetup_vq_irq(v, i);
>  
>  	if (status == 0) {
> -		ret = ops->reset(vdpa);
> +		ret = vdpa_reset(vdpa);
>  		if (ret)
>  			return ret;
>  	} else
> -- 
> 2.34.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/4] vdpa: Protect vdpa reset with cf_mutex

[Michael S. Tsirkin]

On Tue, Jan 11, 2022 at 06:12:38PM +0200, Eli Cohen wrote:
> On Tue, Jan 11, 2022 at 11:01:01AM -0500, Michael S. Tsirkin wrote:
> > On Tue, Jan 11, 2022 at 09:22:17AM +0200, Eli Cohen wrote:
> > > Call reset using the wrapper function vdpa_reset() to make sure the
> > > operation is serialized with cf_mutex.
> > > 
> > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > 
> > The motivation is vague here, this does not really explain.
> > Which operations are we trying to serialize?
> > Multiple reset requests from userspace?
> > Is anything broken right now without this patch?
> 
> vdpa_reset will reset the features which can be read or even be set
> through devlink (see vdpa_get_config_unlocked()). So this lock ensures
> serialization of the operations to ensure consitent value is read.

My point is that this is the kind of thing that needs to go into commit log.
A good log for a bugfix looks like this:


if XYZ triggers a vdpa reset while ABC is reading the features through
devlink, with the result that EFG.  Similarly for write which can lead
to HIJ.

Fix this by calling reset using the wrapper function vdpa_reset() to make sure the
operation is serialized with cf_mutex.

Fixes: <hash> ("subject")



> > 
> > > ---
> > >  drivers/vhost/vdpa.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > > index 6e7edaf2472b..fe0bbea4dab6 100644
> > > --- a/drivers/vhost/vdpa.c
> > > +++ b/drivers/vhost/vdpa.c
> > > @@ -155,7 +155,6 @@ static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8 __user *statusp)
> > >  static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
> > >  {
> > >  	struct vdpa_device *vdpa = v->vdpa;
> > > -	const struct vdpa_config_ops *ops = vdpa->config;
> > >  	u8 status, status_old;
> > >  	int ret, nvqs = v->nvqs;
> > >  	u16 i;
> > > @@ -177,7 +176,7 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
> > >  			vhost_vdpa_unsetup_vq_irq(v, i);
> > >  
> > >  	if (status == 0) {
> > > -		ret = ops->reset(vdpa);
> > > +		ret = vdpa_reset(vdpa);
> > >  		if (ret)
> > >  			return ret;
> > >  	} else
> > > -- 
> > > 2.34.1
> > 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization