[PATCH v2] uio/uio_pci_generic: Introduce refcnt on open/release

[PATCH v2] uio/uio_pci_generic: Introduce refcnt on open/release

[Yao Hongbo]

If two userspace programs both open the PCI UIO fd, when one
of the program exits uncleanly, the other will cause IO hang
due to bus-mastering disabled.

It's a common usage for spdk/dpdk to use UIO. So, introduce refcnt
to avoid such problems.

Fixes: 865a11f987ab("uio/uio_pci_generic: Disable bus-mastering on release")
Reported-by: Xiu Yang <yangxiu.yx@alibaba-inc.com>
Signed-off-by: Yao Hongbo <yaohongbo@linux.alibaba.com>
---
Changes for v2:
	Use refcount_t instead of atomic_t to catch overflow/underflows.
---
 drivers/uio/uio_pci_generic.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
index e03f9b5..1a5e1fd 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -31,6 +31,7 @@
 struct uio_pci_generic_dev {
 	struct uio_info info;
 	struct pci_dev *pdev;
+	refcount_t refcnt;
 };
 
 static inline struct uio_pci_generic_dev *
@@ -39,6 +40,14 @@ struct uio_pci_generic_dev {
 	return container_of(info, struct uio_pci_generic_dev, info);
 }
 
+static int open(struct uio_info *info, struct inode *inode)
+{
+	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
+
+	refcount_inc(&gdev->refcnt);
+	return 0;
+}
+
 static int release(struct uio_info *info, struct inode *inode)
 {
 	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
@@ -51,7 +60,9 @@ static int release(struct uio_info *info, struct inode *inode)
 	 * Note that there's a non-zero chance doing this will wedge the device
 	 * at least until reset.
 	 */
-	pci_clear_master(gdev->pdev);
+	if (refcount_dec_and_test(&gdev->refcnt))
+		pci_clear_master(gdev->pdev);
+
 	return 0;
 }
 
@@ -92,8 +103,11 @@ static int probe(struct pci_dev *pdev,
 
 	gdev->info.name = "uio_pci_generic";
 	gdev->info.version = DRIVER_VERSION;
+	gdev->info.open = open;
 	gdev->info.release = release;
 	gdev->pdev = pdev;
+	refcount_set(&gdev->refcnt, 0);
+
 	if (pdev->irq && (pdev->irq != IRQ_NOTCONNECTED)) {
 		gdev->info.irq = pdev->irq;
 		gdev->info.irq_flags = IRQF_SHARED;
-- 
1.8.3.1

Re: [PATCH v2] uio/uio_pci_generic: Introduce refcnt on open/release

[Greg KH]

On Wed, Apr 13, 2022 at 03:01:42PM +0800, Yao Hongbo wrote:
> If two userspace programs both open the PCI UIO fd, when one
> of the program exits uncleanly, the other will cause IO hang
> due to bus-mastering disabled.
> 
> It's a common usage for spdk/dpdk to use UIO. So, introduce refcnt
> to avoid such problems.

Why do you have multiple userspace programs opening the same device?
Shouldn't they coordinate?

> 
> Fixes: 865a11f987ab("uio/uio_pci_generic: Disable bus-mastering on release")
> Reported-by: Xiu Yang <yangxiu.yx@alibaba-inc.com>
> Signed-off-by: Yao Hongbo <yaohongbo@linux.alibaba.com>
> ---
> Changes for v2:
> 	Use refcount_t instead of atomic_t to catch overflow/underflows.
> ---
>  drivers/uio/uio_pci_generic.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
> index e03f9b5..1a5e1fd 100644
> --- a/drivers/uio/uio_pci_generic.c
> +++ b/drivers/uio/uio_pci_generic.c
> @@ -31,6 +31,7 @@
>  struct uio_pci_generic_dev {
>  	struct uio_info info;
>  	struct pci_dev *pdev;
> +	refcount_t refcnt;
>  };
>  
>  static inline struct uio_pci_generic_dev *
> @@ -39,6 +40,14 @@ struct uio_pci_generic_dev {
>  	return container_of(info, struct uio_pci_generic_dev, info);
>  }
>  
> +static int open(struct uio_info *info, struct inode *inode)
> +{
> +	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
> +
> +	refcount_inc(&gdev->refcnt);
> +	return 0;
> +}
> +
>  static int release(struct uio_info *info, struct inode *inode)
>  {
>  	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
> @@ -51,7 +60,9 @@ static int release(struct uio_info *info, struct inode *inode)
>  	 * Note that there's a non-zero chance doing this will wedge the device
>  	 * at least until reset.
>  	 */
> -	pci_clear_master(gdev->pdev);
> +	if (refcount_dec_and_test(&gdev->refcnt))
> +		pci_clear_master(gdev->pdev);

The goal here is to flush things when userspace closes the device, as
the comment says.  So don't you want that to happen for when userspace
closes the file handle no matter who opened it?

As this is a functional change, how is userspace going to "know" this
functionality is now changed or not?

And if userspace really wants to open this multiple times, then properly
switch the code to only create the device-specific structures when open
is called.  Otherwise you are sharing structures here that are not
intended to be shared, shouldn't you have your own private one?

this feels odd.

thanks,

greg k-h

Re: [PATCH v2] uio/uio_pci_generic: Introduce refcnt on open/release

[Michael S. Tsirkin]

On Wed, Apr 13, 2022 at 09:33:17AM +0200, Greg KH wrote:
> On Wed, Apr 13, 2022 at 03:01:42PM +0800, Yao Hongbo wrote:
> > If two userspace programs both open the PCI UIO fd, when one
> > of the program exits uncleanly, the other will cause IO hang
> > due to bus-mastering disabled.
> > 
> > It's a common usage for spdk/dpdk to use UIO. So, introduce refcnt
> > to avoid such problems.
> 
> Why do you have multiple userspace programs opening the same device?
> Shouldn't they coordinate?

Or to restate, I think the question is, why not open the device
once and pass the FD around?


> > 
> > Fixes: 865a11f987ab("uio/uio_pci_generic: Disable bus-mastering on release")


space missing before ( here .

> > Reported-by: Xiu Yang <yangxiu.yx@alibaba-inc.com>
> > Signed-off-by: Yao Hongbo <yaohongbo@linux.alibaba.com>
> > ---
> > Changes for v2:
> > 	Use refcount_t instead of atomic_t to catch overflow/underflows.
> > ---
> >  drivers/uio/uio_pci_generic.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
> > index e03f9b5..1a5e1fd 100644
> > --- a/drivers/uio/uio_pci_generic.c
> > +++ b/drivers/uio/uio_pci_generic.c
> > @@ -31,6 +31,7 @@
> >  struct uio_pci_generic_dev {
> >  	struct uio_info info;
> >  	struct pci_dev *pdev;
> > +	refcount_t refcnt;
> >  };
> >  
> >  static inline struct uio_pci_generic_dev *
> > @@ -39,6 +40,14 @@ struct uio_pci_generic_dev {
> >  	return container_of(info, struct uio_pci_generic_dev, info);
> >  }
> >  
> > +static int open(struct uio_info *info, struct inode *inode)
> > +{
> > +	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
> > +
> > +	refcount_inc(&gdev->refcnt);
> > +	return 0;
> > +}
> > +
> >  static int release(struct uio_info *info, struct inode *inode)
> >  {
> >  	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
> > @@ -51,7 +60,9 @@ static int release(struct uio_info *info, struct inode *inode)
> >  	 * Note that there's a non-zero chance doing this will wedge the device
> >  	 * at least until reset.
> >  	 */
> > -	pci_clear_master(gdev->pdev);
> > +	if (refcount_dec_and_test(&gdev->refcnt))
> > +		pci_clear_master(gdev->pdev);
> 
> The goal here is to flush things when userspace closes the device, as
> the comment says.  So don't you want that to happen for when userspace
> closes the file handle no matter who opened it?
> 
> As this is a functional change, how is userspace going to "know" this
> functionality is now changed or not?
> 
> And if userspace really wants to open this multiple times, then properly
> switch the code to only create the device-specific structures when open
> is called.  Otherwise you are sharing structures here that are not
> intended to be shared, shouldn't you have your own private one?
> 
> this feels odd.
> 
> thanks,
> 
> greg k-h

Sigh. Maybe it was a mistake to do 865a11f987ab to begin with.
Anyone doing DMA with UIO is already on very thin ice.
But oh well.

-- 
MST

Re: [PATCH v2] uio/uio_pci_generic: Introduce refcnt on open/release

[Yao Hongbo]

在 2022/4/13 下午4:51, Michael S. Tsirkin 写道:
> On Wed, Apr 13, 2022 at 09:33:17AM +0200, Greg KH wrote:
>> On Wed, Apr 13, 2022 at 03:01:42PM +0800, Yao Hongbo wrote:
>>> If two userspace programs both open the PCI UIO fd, when one
>>> of the program exits uncleanly, the other will cause IO hang
>>> due to bus-mastering disabled.
>>>
>>> It's a common usage for spdk/dpdk to use UIO. So, introduce refcnt
>>> to avoid such problems.
>> Why do you have multiple userspace programs opening the same device?
>> Shouldn't they coordinate?
> Or to restate, I think the question is, why not open the device
> once and pass the FD around?

Hmm, it will have the same result, no matter  whether opening the same 
device or pass the FD around.

Our expectation is that even if the primary process exits abnormally,  
the second process can still send

or receive data.

The impact of disabling pci bus-master is relatively large, and we 
should make some restrictions on

this behavior.

>
>>> Fixes: 865a11f987ab("uio/uio_pci_generic: Disable bus-mastering on release")
>
> space missing before ( here .
>
>>> Reported-by: Xiu Yang <yangxiu.yx@alibaba-inc.com>
>>> Signed-off-by: Yao Hongbo <yaohongbo@linux.alibaba.com>
>>> ---
>>> Changes for v2:
>>> 	Use refcount_t instead of atomic_t to catch overflow/underflows.
>>> ---
>>>   drivers/uio/uio_pci_generic.c | 16 +++++++++++++++-
>>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
>>> index e03f9b5..1a5e1fd 100644
>>> --- a/drivers/uio/uio_pci_generic.c
>>> +++ b/drivers/uio/uio_pci_generic.c
>>> @@ -31,6 +31,7 @@
>>>   struct uio_pci_generic_dev {
>>>   	struct uio_info info;
>>>   	struct pci_dev *pdev;
>>> +	refcount_t refcnt;
>>>   };
>>>   
>>>   static inline struct uio_pci_generic_dev *
>>> @@ -39,6 +40,14 @@ struct uio_pci_generic_dev {
>>>   	return container_of(info, struct uio_pci_generic_dev, info);
>>>   }
>>>   
>>> +static int open(struct uio_info *info, struct inode *inode)
>>> +{
>>> +	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
>>> +
>>> +	refcount_inc(&gdev->refcnt);
>>> +	return 0;
>>> +}
>>> +
>>>   static int release(struct uio_info *info, struct inode *inode)
>>>   {
>>>   	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
>>> @@ -51,7 +60,9 @@ static int release(struct uio_info *info, struct inode *inode)
>>>   	 * Note that there's a non-zero chance doing this will wedge the device
>>>   	 * at least until reset.
>>>   	 */
>>> -	pci_clear_master(gdev->pdev);
>>> +	if (refcount_dec_and_test(&gdev->refcnt))
>>> +		pci_clear_master(gdev->pdev);
>> The goal here is to flush things when userspace closes the device, as
>> the comment says.  So don't you want that to happen for when userspace
>> closes the file handle no matter who opened it?
>>
>> As this is a functional change, how is userspace going to "know" this
>> functionality is now changed or not?
>>
>> And if userspace really wants to open this multiple times, then properly
>> switch the code to only create the device-specific structures when open
>> is called.  Otherwise you are sharing structures here that are not
>> intended to be shared, shouldn't you have your own private one?
>>
>> this feels odd.
>>
>> thanks,
>>
>> greg k-h
> Sigh. Maybe it was a mistake to do 865a11f987ab to begin with.
> Anyone doing DMA with UIO is already on very thin ice.
> But oh well.
>

Re: [PATCH v2] uio/uio_pci_generic: Introduce refcnt on open/release

[Greg KH]

On Wed, Apr 13, 2022 at 05:25:40PM +0800, Yao Hongbo wrote:
> 
> 在 2022/4/13 下午4:51, Michael S. Tsirkin 写道:
> > On Wed, Apr 13, 2022 at 09:33:17AM +0200, Greg KH wrote:
> > > On Wed, Apr 13, 2022 at 03:01:42PM +0800, Yao Hongbo wrote:
> > > > If two userspace programs both open the PCI UIO fd, when one
> > > > of the program exits uncleanly, the other will cause IO hang
> > > > due to bus-mastering disabled.
> > > > 
> > > > It's a common usage for spdk/dpdk to use UIO. So, introduce refcnt
> > > > to avoid such problems.
> > > Why do you have multiple userspace programs opening the same device?
> > > Shouldn't they coordinate?
> > Or to restate, I think the question is, why not open the device
> > once and pass the FD around?
> 
> Hmm, it will have the same result, no matter  whether opening the same
> device or pass the FD around.

How?  You only open once, and close once.  Where is the multiple closes?

> Our expectation is that even if the primary process exits abnormally,  the
> second process can still send
> 
> or receive data.

Then use the same file descriptor.

> The impact of disabling pci bus-master is relatively large, and we should
> make some restrictions on
> this behavior.

Why?  UIO is "you better really really know what you are doing to use
this interface", right?  Just duplicate the fd and pass it around if you
must have multiple accesses to the same device.

And again, this will be a functional change.  How can you handle your
userspace on older kernels if you make this change?

thanks,

greg k-h

Re: [PATCH v2] uio/uio_pci_generic: Introduce refcnt on open/release

[Yao Hongbo]

在 2022/4/13 下午5:43, Greg KH 写道:
> On Wed, Apr 13, 2022 at 05:25:40PM +0800, Yao Hongbo wrote:
>> 在 2022/4/13 下午4:51, Michael S. Tsirkin 写道:
>>> On Wed, Apr 13, 2022 at 09:33:17AM +0200, Greg KH wrote:
>>>> On Wed, Apr 13, 2022 at 03:01:42PM +0800, Yao Hongbo wrote:
>>>>> If two userspace programs both open the PCI UIO fd, when one
>>>>> of the program exits uncleanly, the other will cause IO hang
>>>>> due to bus-mastering disabled.
>>>>>
>>>>> It's a common usage for spdk/dpdk to use UIO. So, introduce refcnt
>>>>> to avoid such problems.
>>>> Why do you have multiple userspace programs opening the same device?
>>>> Shouldn't they coordinate?
>>> Or to restate, I think the question is, why not open the device
>>> once and pass the FD around?
>> Hmm, it will have the same result, no matter  whether opening the same
>> device or pass the FD around.
> How?  You only open once, and close once.  Where is the multiple closes?
>
>> Our expectation is that even if the primary process exits abnormally,  the
>> second process can still send
>>
>> or receive data.
> Then use the same file descriptor.


Yes, we can use the same file descriptor.

but since the pcie bus-master  has been disabled by the primary process,

the seconday process cannot continue to operate.

>
>> The impact of disabling pci bus-master is relatively large, and we should
>> make some restrictions on
>> this behavior.
> Why?  UIO is "you better really really know what you are doing to use
> this interface", right?  Just duplicate the fd and pass it around if you
> must have multiple accesses to the same device.
>
> And again, this will be a functional change.  How can you handle your
> userspace on older kernels if you make this change?

Without this change, our userspace cannot work properly on older kernels.


Our userspace only use the "multi process mode" feature of the spdk.

The SPDK links:
https://spdk.io/doc/app_overview.html

"Multi process mode
When --shm-id is specified, the application is started in multi-process 
mode.

Applications using the same shm-id share their memory and NVMe devices.

The first app to start with a given id becomes a primary process, with 
the rest,

called secondary processes, only attaching to it. When the primary 
process exits,

the secondary ones continue to operate, but no new processes can be 
attached

at this point. All processes within the same shm-id group must use the 
same --single-file-segments setting."

> thanks,
>
> greg k-h

Re: [PATCH v2] uio/uio_pci_generic: Introduce refcnt on open/release

[Greg KH]

On Wed, Apr 13, 2022 at 07:09:57PM +0800, Yao Hongbo wrote:
> 
> 在 2022/4/13 下午5:43, Greg KH 写道:
> > On Wed, Apr 13, 2022 at 05:25:40PM +0800, Yao Hongbo wrote:
> > > 在 2022/4/13 下午4:51, Michael S. Tsirkin 写道:
> > > > On Wed, Apr 13, 2022 at 09:33:17AM +0200, Greg KH wrote:
> > > > > On Wed, Apr 13, 2022 at 03:01:42PM +0800, Yao Hongbo wrote:
> > > > > > If two userspace programs both open the PCI UIO fd, when one
> > > > > > of the program exits uncleanly, the other will cause IO hang
> > > > > > due to bus-mastering disabled.
> > > > > > 
> > > > > > It's a common usage for spdk/dpdk to use UIO. So, introduce refcnt
> > > > > > to avoid such problems.
> > > > > Why do you have multiple userspace programs opening the same device?
> > > > > Shouldn't they coordinate?
> > > > Or to restate, I think the question is, why not open the device
> > > > once and pass the FD around?
> > > Hmm, it will have the same result, no matter  whether opening the same
> > > device or pass the FD around.
> > How?  You only open once, and close once.  Where is the multiple closes?
> > 
> > > Our expectation is that even if the primary process exits abnormally,  the
> > > second process can still send
> > > 
> > > or receive data.
> > Then use the same file descriptor.
> 
> 
> Yes, we can use the same file descriptor.
> 
> but since the pcie bus-master  has been disabled by the primary process,
> 
> the seconday process cannot continue to operate.

Really?  With the same file descriptor?  Try it and see.  release should
only be called when the file descriptor is closed.

> > > The impact of disabling pci bus-master is relatively large, and we should
> > > make some restrictions on
> > > this behavior.
> > Why?  UIO is "you better really really know what you are doing to use
> > this interface", right?  Just duplicate the fd and pass it around if you
> > must have multiple accesses to the same device.
> > 
> > And again, this will be a functional change.  How can you handle your
> > userspace on older kernels if you make this change?
> 
> Without this change, our userspace cannot work properly on older kernels.

What change broke your userspace?

> Our userspace only use the "multi process mode" feature of the spdk.
> 
> The SPDK links:
> https://spdk.io/doc/app_overview.html
> 
> "Multi process mode
> When --shm-id is specified, the application is started in multi-process
> mode.
> 
> Applications using the same shm-id share their memory and NVMe devices.
> 
> The first app to start with a given id becomes a primary process, with the
> rest,
> 
> called secondary processes, only attaching to it. When the primary process
> exits,
> 
> the secondary ones continue to operate, but no new processes can be attached
> 
> at this point. All processes within the same shm-id group must use the same
> --single-file-segments setting."

Please work with the spdk users, I know nothing about that mess, sorry.

greg k-h

Re: [PATCH v2] uio/uio_pci_generic: Introduce refcnt on open/release

[Christoph Hellwig]

On Wed, Apr 13, 2022 at 09:33:17AM +0200, Greg KH wrote:
> On Wed, Apr 13, 2022 at 03:01:42PM +0800, Yao Hongbo wrote:
> > If two userspace programs both open the PCI UIO fd, when one
> > of the program exits uncleanly, the other will cause IO hang
> > due to bus-mastering disabled.
> > 
> > It's a common usage for spdk/dpdk to use UIO. So, introduce refcnt
> > to avoid such problems.
> 
> Why do you have multiple userspace programs opening the same device?
> Shouldn't they coordinate?

Independent of that (very valid) question I think the current code is
just broken.  Either we need to prohbit multiple opens or do this kind
of refcounting.