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

[PATCH] 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: 865a11f("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>
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 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..8add2cf 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;
+	atomic_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);
+
+	atomic_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 (atomic_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;
+	atomic_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] uio/uio_pci_generic: Introduce refcnt on open/release

[Michael S. Tsirkin]

On Thu, Mar 31, 2022 at 10:06:24AM +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.

With two programs poking at the same device, how is this ever
supposed to work even while both are alive?

> It's a common usage for spdk/dpdk to use UIO.

Except people really should just use vfio ...

> So, introduce refcnt
> to avoid such problems.
> 
> Fixes: 865a11f("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>
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  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..8add2cf 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;
> +	atomic_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);
> +
> +	atomic_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 (atomic_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;
> +	atomic_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] uio/uio_pci_generic: Introduce refcnt on open/release

[Greg KH]

On Thu, Mar 31, 2022 at 04:29:23PM -0400, Michael S. Tsirkin wrote:
> On Thu, Mar 31, 2022 at 10:06:24AM +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.
> 
> With two programs poking at the same device, how is this ever
> supposed to work even while both are alive?
> 
> > It's a common usage for spdk/dpdk to use UIO.
> 
> Except people really should just use vfio ...

Yes they should, the kernel should not care if multiple programs open
the same UIO device node, it can not prevent that and userspace is on
it's own here as it _should_ know what it is doing.

thanks,

greg k-h