[PATCH] vfio/mtty: Enforce available_instances

[PATCH] vfio/mtty: Enforce available_instances

[Alex Williamson]

The sample mtty mdev driver doesn't actually enforce the number of
device instances it claims are available.  Implement this properly.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

Applies to vfio next branch + Jason's atomic conversion

 samples/vfio-mdev/mtty.c |   22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index ffbaf07a17ea..8b26fecc4afe 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -144,7 +144,7 @@ struct mdev_state {
 	int nr_ports;
 };
 
-static atomic_t mdev_used_ports;
+static atomic_t mdev_avail_ports = ATOMIC_INIT(MAX_MTTYS);
 
 static const struct file_operations vd_fops = {
 	.owner          = THIS_MODULE,
@@ -707,11 +707,20 @@ static int mtty_probe(struct mdev_device *mdev)
 {
 	struct mdev_state *mdev_state;
 	int nr_ports = mdev_get_type_group_id(mdev) + 1;
+	int avail_ports = atomic_read(&mdev_avail_ports);
 	int ret;
 
+	do {
+		if (avail_ports < nr_ports)
+			return -ENOSPC;
+	} while (!atomic_try_cmpxchg(&mdev_avail_ports,
+				     &avail_ports, avail_ports - nr_ports));
+
 	mdev_state = kzalloc(sizeof(struct mdev_state), GFP_KERNEL);
-	if (mdev_state == NULL)
+	if (mdev_state == NULL) {
+		atomic_add(nr_ports, &mdev_avail_ports);
 		return -ENOMEM;
+	}
 
 	vfio_init_group_dev(&mdev_state->vdev, &mdev->dev, &mtty_dev_ops);
 
@@ -724,6 +733,7 @@ static int mtty_probe(struct mdev_device *mdev)
 
 	if (mdev_state->vconfig == NULL) {
 		kfree(mdev_state);
+		atomic_add(nr_ports, &mdev_avail_ports);
 		return -ENOMEM;
 	}
 
@@ -735,9 +745,9 @@ static int mtty_probe(struct mdev_device *mdev)
 	ret = vfio_register_group_dev(&mdev_state->vdev);
 	if (ret) {
 		kfree(mdev_state);
+		atomic_add(nr_ports, &mdev_avail_ports);
 		return ret;
 	}
-	atomic_add(mdev_state->nr_ports, &mdev_used_ports);
 
 	dev_set_drvdata(&mdev->dev, mdev_state);
 	return 0;
@@ -746,12 +756,13 @@ static int mtty_probe(struct mdev_device *mdev)
 static void mtty_remove(struct mdev_device *mdev)
 {
 	struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev);
+	int nr_ports = mdev_state->nr_ports;
 
-	atomic_sub(mdev_state->nr_ports, &mdev_used_ports);
 	vfio_unregister_group_dev(&mdev_state->vdev);
 
 	kfree(mdev_state->vconfig);
 	kfree(mdev_state);
+	atomic_add(nr_ports, &mdev_avail_ports);
 }
 
 static int mtty_reset(struct mdev_state *mdev_state)
@@ -1271,8 +1282,7 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
 {
 	unsigned int ports = mtype_get_type_group_id(mtype) + 1;
 
-	return sprintf(buf, "%d\n",
-		       (MAX_MTTYS - atomic_read(&mdev_used_ports)) / ports);
+	return sprintf(buf, "%d\n", atomic_read(&mdev_avail_ports) / ports);
 }
 
 static MDEV_TYPE_ATTR_RO(available_instances);

Re: [PATCH] vfio/mtty: Enforce available_instances

[Jason Gunthorpe]

On Fri, Jun 25, 2021 at 03:26:11PM -0600, Alex Williamson wrote:
> The sample mtty mdev driver doesn't actually enforce the number of
> device instances it claims are available.  Implement this properly.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
> Applies to vfio next branch + Jason's atomic conversion
> 
>  samples/vfio-mdev/mtty.c |   22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH] vfio/mtty: Enforce available_instances

[Cornelia Huck]

On Fri, Jun 25 2021, Alex Williamson <alex.williamson@redhat.com> wrote:

> The sample mtty mdev driver doesn't actually enforce the number of
> device instances it claims are available.  Implement this properly.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>
> Applies to vfio next branch + Jason's atomic conversion
>
>  samples/vfio-mdev/mtty.c |   22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
>

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH] vfio/mtty: Enforce available_instances

[Kirti Wankhede]

On 6/26/2021 2:56 AM, Alex Williamson wrote:
> The sample mtty mdev driver doesn't actually enforce the number of
> device instances it claims are available.  Implement this properly.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
> Applies to vfio next branch + Jason's atomic conversion
> 


Does this need to be on top of Jason's patch?
Patch to use mdev_used_ports is reverted here, can it be changed from 
mdev_devices_list to mdev_avail_ports atomic variable?

Change here to use atomic variable looks good to me.

Reviewed by: Kirti Wankhede <kwankhede@nvidia.com>



>   samples/vfio-mdev/mtty.c |   22 ++++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
> index ffbaf07a17ea..8b26fecc4afe 100644
> --- a/samples/vfio-mdev/mtty.c
> +++ b/samples/vfio-mdev/mtty.c
> @@ -144,7 +144,7 @@ struct mdev_state {
>   	int nr_ports;
>   };
>   
> -static atomic_t mdev_used_ports;
> +static atomic_t mdev_avail_ports = ATOMIC_INIT(MAX_MTTYS);
>   
>   static const struct file_operations vd_fops = {
>   	.owner          = THIS_MODULE,
> @@ -707,11 +707,20 @@ static int mtty_probe(struct mdev_device *mdev)
>   {
>   	struct mdev_state *mdev_state;
>   	int nr_ports = mdev_get_type_group_id(mdev) + 1;
> +	int avail_ports = atomic_read(&mdev_avail_ports);
>   	int ret;
>   
> +	do {
> +		if (avail_ports < nr_ports)
> +			return -ENOSPC;
> +	} while (!atomic_try_cmpxchg(&mdev_avail_ports,
> +				     &avail_ports, avail_ports - nr_ports));
> +
>   	mdev_state = kzalloc(sizeof(struct mdev_state), GFP_KERNEL);
> -	if (mdev_state == NULL)
> +	if (mdev_state == NULL) {
> +		atomic_add(nr_ports, &mdev_avail_ports);
>   		return -ENOMEM;
> +	}
>   
>   	vfio_init_group_dev(&mdev_state->vdev, &mdev->dev, &mtty_dev_ops);
>   
> @@ -724,6 +733,7 @@ static int mtty_probe(struct mdev_device *mdev)
>   
>   	if (mdev_state->vconfig == NULL) {
>   		kfree(mdev_state);
> +		atomic_add(nr_ports, &mdev_avail_ports);
>   		return -ENOMEM;
>   	}
>   
> @@ -735,9 +745,9 @@ static int mtty_probe(struct mdev_device *mdev)
>   	ret = vfio_register_group_dev(&mdev_state->vdev);
>   	if (ret) {
>   		kfree(mdev_state);
> +		atomic_add(nr_ports, &mdev_avail_ports);
>   		return ret;
>   	}
> -	atomic_add(mdev_state->nr_ports, &mdev_used_ports);
>   
>   	dev_set_drvdata(&mdev->dev, mdev_state);
>   	return 0;
> @@ -746,12 +756,13 @@ static int mtty_probe(struct mdev_device *mdev)
>   static void mtty_remove(struct mdev_device *mdev)
>   {
>   	struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev);
> +	int nr_ports = mdev_state->nr_ports;
>   
> -	atomic_sub(mdev_state->nr_ports, &mdev_used_ports);
>   	vfio_unregister_group_dev(&mdev_state->vdev);
>   
>   	kfree(mdev_state->vconfig);
>   	kfree(mdev_state);
> +	atomic_add(nr_ports, &mdev_avail_ports);
>   }
>   
>   static int mtty_reset(struct mdev_state *mdev_state)
> @@ -1271,8 +1282,7 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>   {
>   	unsigned int ports = mtype_get_type_group_id(mtype) + 1;
>   
> -	return sprintf(buf, "%d\n",
> -		       (MAX_MTTYS - atomic_read(&mdev_used_ports)) / ports);
> +	return sprintf(buf, "%d\n", atomic_read(&mdev_avail_ports) / ports);
>   }
>   
>   static MDEV_TYPE_ATTR_RO(available_instances);
> 
> 

Re: [PATCH] vfio/mtty: Enforce available_instances

[Alex Williamson]

On Mon, 28 Jun 2021 23:19:54 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 6/26/2021 2:56 AM, Alex Williamson wrote:
> > The sample mtty mdev driver doesn't actually enforce the number of
> > device instances it claims are available.  Implement this properly.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > 
> > Applies to vfio next branch + Jason's atomic conversion
> >   
> 
> 
> Does this need to be on top of Jason's patch?

Yes, see immediately above.

> Patch to use mdev_used_ports is reverted here, can it be changed from 
> mdev_devices_list to mdev_avail_ports atomic variable?

It doesn't revert Jason's change, it builds on it.  The patches could
we squashed, but there's no bug in Jason's patch that we're trying to
avoid exposing, so I don't see why we'd do that.

> Change here to use atomic variable looks good to me.
> 
> Reviewed by: Kirti Wankhede <kwankhede@nvidia.com>

Thanks!  It was Jason's patch[1] that converted to use an atomic
though, so I'm slightly confused if this R-b is for the patch below,
Jason's patch, or both.  Thanks,

Alex

[1]https://lore.kernel.org/kvm/0-v1-0bc56b362ca7+62-mtty_used_ports_jgg@nvidia.com/

> >   samples/vfio-mdev/mtty.c |   22 ++++++++++++++++------
> >   1 file changed, 16 insertions(+), 6 deletions(-)
> > 
> > diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
> > index ffbaf07a17ea..8b26fecc4afe 100644
> > --- a/samples/vfio-mdev/mtty.c
> > +++ b/samples/vfio-mdev/mtty.c
> > @@ -144,7 +144,7 @@ struct mdev_state {
> >   	int nr_ports;
> >   };
> >   
> > -static atomic_t mdev_used_ports;
> > +static atomic_t mdev_avail_ports = ATOMIC_INIT(MAX_MTTYS);
> >   
> >   static const struct file_operations vd_fops = {
> >   	.owner          = THIS_MODULE,
> > @@ -707,11 +707,20 @@ static int mtty_probe(struct mdev_device *mdev)
> >   {
> >   	struct mdev_state *mdev_state;
> >   	int nr_ports = mdev_get_type_group_id(mdev) + 1;
> > +	int avail_ports = atomic_read(&mdev_avail_ports);
> >   	int ret;
> >   
> > +	do {
> > +		if (avail_ports < nr_ports)
> > +			return -ENOSPC;
> > +	} while (!atomic_try_cmpxchg(&mdev_avail_ports,
> > +				     &avail_ports, avail_ports - nr_ports));
> > +
> >   	mdev_state = kzalloc(sizeof(struct mdev_state), GFP_KERNEL);
> > -	if (mdev_state == NULL)
> > +	if (mdev_state == NULL) {
> > +		atomic_add(nr_ports, &mdev_avail_ports);
> >   		return -ENOMEM;
> > +	}
> >   
> >   	vfio_init_group_dev(&mdev_state->vdev, &mdev->dev, &mtty_dev_ops);
> >   
> > @@ -724,6 +733,7 @@ static int mtty_probe(struct mdev_device *mdev)
> >   
> >   	if (mdev_state->vconfig == NULL) {
> >   		kfree(mdev_state);
> > +		atomic_add(nr_ports, &mdev_avail_ports);
> >   		return -ENOMEM;
> >   	}
> >   
> > @@ -735,9 +745,9 @@ static int mtty_probe(struct mdev_device *mdev)
> >   	ret = vfio_register_group_dev(&mdev_state->vdev);
> >   	if (ret) {
> >   		kfree(mdev_state);
> > +		atomic_add(nr_ports, &mdev_avail_ports);
> >   		return ret;
> >   	}
> > -	atomic_add(mdev_state->nr_ports, &mdev_used_ports);
> >   
> >   	dev_set_drvdata(&mdev->dev, mdev_state);
> >   	return 0;
> > @@ -746,12 +756,13 @@ static int mtty_probe(struct mdev_device *mdev)
> >   static void mtty_remove(struct mdev_device *mdev)
> >   {
> >   	struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev);
> > +	int nr_ports = mdev_state->nr_ports;
> >   
> > -	atomic_sub(mdev_state->nr_ports, &mdev_used_ports);
> >   	vfio_unregister_group_dev(&mdev_state->vdev);
> >   
> >   	kfree(mdev_state->vconfig);
> >   	kfree(mdev_state);
> > +	atomic_add(nr_ports, &mdev_avail_ports);
> >   }
> >   
> >   static int mtty_reset(struct mdev_state *mdev_state)
> > @@ -1271,8 +1282,7 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
> >   {
> >   	unsigned int ports = mtype_get_type_group_id(mtype) + 1;
> >   
> > -	return sprintf(buf, "%d\n",
> > -		       (MAX_MTTYS - atomic_read(&mdev_used_ports)) / ports);
> > +	return sprintf(buf, "%d\n", atomic_read(&mdev_avail_ports) / ports);
> >   }
> >   
> >   static MDEV_TYPE_ATTR_RO(available_instances);
> > 
> >   
> 

Re: [PATCH] vfio/mtty: Enforce available_instances

[Kirti Wankhede]

On 6/29/2021 12:26 AM, Alex Williamson wrote:
> On Mon, 28 Jun 2021 23:19:54 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 6/26/2021 2:56 AM, Alex Williamson wrote:
>>> The sample mtty mdev driver doesn't actually enforce the number of
>>> device instances it claims are available.  Implement this properly.
>>>
>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>> ---
>>>
>>> Applies to vfio next branch + Jason's atomic conversion
>>>    
>>
>>
>> Does this need to be on top of Jason's patch?
> 
> Yes, see immediately above.
> 
>> Patch to use mdev_used_ports is reverted here, can it be changed from
>> mdev_devices_list to mdev_avail_ports atomic variable?
> 
> It doesn't revert Jason's change, it builds on it.  The patches could
> we squashed, but there's no bug in Jason's patch that we're trying to
> avoid exposing, so I don't see why we'd do that.
>

'Squashed' is the correct word that 'revert', my bad.

>> Change here to use atomic variable looks good to me.
>>
>> Reviewed by: Kirti Wankhede <kwankhede@nvidia.com>
> 
> Thanks!  It was Jason's patch[1] that converted to use an atomic
> though, so I'm slightly confused if this R-b is for the patch below,
> Jason's patch, or both.  Thanks,

I liked 'mdev_avail_ports' approach than 'mdev_used_ports' approach 
here. This R-b is for below patch.

Thanks,
Kirti

> 
> Alex
> 
> [1]https://lore.kernel.org/kvm/0-v1-0bc56b362ca7+62-mtty_used_ports_jgg@nvidia.com/
> 
>>>    samples/vfio-mdev/mtty.c |   22 ++++++++++++++++------
>>>    1 file changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
>>> index ffbaf07a17ea..8b26fecc4afe 100644
>>> --- a/samples/vfio-mdev/mtty.c
>>> +++ b/samples/vfio-mdev/mtty.c
>>> @@ -144,7 +144,7 @@ struct mdev_state {
>>>    	int nr_ports;
>>>    };
>>>    
>>> -static atomic_t mdev_used_ports;
>>> +static atomic_t mdev_avail_ports = ATOMIC_INIT(MAX_MTTYS);
>>>    
>>>    static const struct file_operations vd_fops = {
>>>    	.owner          = THIS_MODULE,
>>> @@ -707,11 +707,20 @@ static int mtty_probe(struct mdev_device *mdev)
>>>    {
>>>    	struct mdev_state *mdev_state;
>>>    	int nr_ports = mdev_get_type_group_id(mdev) + 1;
>>> +	int avail_ports = atomic_read(&mdev_avail_ports);
>>>    	int ret;
>>>    
>>> +	do {
>>> +		if (avail_ports < nr_ports)
>>> +			return -ENOSPC;
>>> +	} while (!atomic_try_cmpxchg(&mdev_avail_ports,
>>> +				     &avail_ports, avail_ports - nr_ports));
>>> +
>>>    	mdev_state = kzalloc(sizeof(struct mdev_state), GFP_KERNEL);
>>> -	if (mdev_state == NULL)
>>> +	if (mdev_state == NULL) {
>>> +		atomic_add(nr_ports, &mdev_avail_ports);
>>>    		return -ENOMEM;
>>> +	}
>>>    
>>>    	vfio_init_group_dev(&mdev_state->vdev, &mdev->dev, &mtty_dev_ops);
>>>    
>>> @@ -724,6 +733,7 @@ static int mtty_probe(struct mdev_device *mdev)
>>>    
>>>    	if (mdev_state->vconfig == NULL) {
>>>    		kfree(mdev_state);
>>> +		atomic_add(nr_ports, &mdev_avail_ports);
>>>    		return -ENOMEM;
>>>    	}
>>>    
>>> @@ -735,9 +745,9 @@ static int mtty_probe(struct mdev_device *mdev)
>>>    	ret = vfio_register_group_dev(&mdev_state->vdev);
>>>    	if (ret) {
>>>    		kfree(mdev_state);
>>> +		atomic_add(nr_ports, &mdev_avail_ports);
>>>    		return ret;
>>>    	}
>>> -	atomic_add(mdev_state->nr_ports, &mdev_used_ports);
>>>    
>>>    	dev_set_drvdata(&mdev->dev, mdev_state);
>>>    	return 0;
>>> @@ -746,12 +756,13 @@ static int mtty_probe(struct mdev_device *mdev)
>>>    static void mtty_remove(struct mdev_device *mdev)
>>>    {
>>>    	struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev);
>>> +	int nr_ports = mdev_state->nr_ports;
>>>    
>>> -	atomic_sub(mdev_state->nr_ports, &mdev_used_ports);
>>>    	vfio_unregister_group_dev(&mdev_state->vdev);
>>>    
>>>    	kfree(mdev_state->vconfig);
>>>    	kfree(mdev_state);
>>> +	atomic_add(nr_ports, &mdev_avail_ports);
>>>    }
>>>    
>>>    static int mtty_reset(struct mdev_state *mdev_state)
>>> @@ -1271,8 +1282,7 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>>>    {
>>>    	unsigned int ports = mtype_get_type_group_id(mtype) + 1;
>>>    
>>> -	return sprintf(buf, "%d\n",
>>> -		       (MAX_MTTYS - atomic_read(&mdev_used_ports)) / ports);
>>> +	return sprintf(buf, "%d\n", atomic_read(&mdev_avail_ports) / ports);
>>>    }
>>>    
>>>    static MDEV_TYPE_ATTR_RO(available_instances);
>>>
>>>    
>>
> 

Re: [PATCH] vfio/mtty: Enforce available_instances

[Alex Williamson]

On Tue, 29 Jun 2021 01:22:00 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 6/29/2021 12:26 AM, Alex Williamson wrote:
> > On Mon, 28 Jun 2021 23:19:54 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >   
> >> On 6/26/2021 2:56 AM, Alex Williamson wrote:  
> >>> The sample mtty mdev driver doesn't actually enforce the number of
> >>> device instances it claims are available.  Implement this properly.
> >>>
> >>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >>> ---
> >>>
> >>> Applies to vfio next branch + Jason's atomic conversion
> >>>      
> >>
> >>
> >> Does this need to be on top of Jason's patch?  
> > 
> > Yes, see immediately above.
> >   
> >> Patch to use mdev_used_ports is reverted here, can it be changed from
> >> mdev_devices_list to mdev_avail_ports atomic variable?  
> > 
> > It doesn't revert Jason's change, it builds on it.  The patches could
> > we squashed, but there's no bug in Jason's patch that we're trying to
> > avoid exposing, so I don't see why we'd do that.
> >  
> 
> 'Squashed' is the correct word that 'revert', my bad.
> 
> >> Change here to use atomic variable looks good to me.
> >>
> >> Reviewed by: Kirti Wankhede <kwankhede@nvidia.com>  
> > 
> > Thanks!  It was Jason's patch[1] that converted to use an atomic
> > though, so I'm slightly confused if this R-b is for the patch below,
> > Jason's patch, or both.  Thanks,  
> 
> I liked 'mdev_avail_ports' approach than 'mdev_used_ports' approach 
> here. This R-b is for below patch.

Got it, added.  Thanks Kirti!