[PATCH v2] uio:uio_pci_generic:Don't clear master bit when the process does not exit

[PATCH v2] uio:uio_pci_generic:Don't clear master bit when the process does not exit

[Su Weifeng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 2577 bytes --]

From: Weifeng Su <suweifeng1@huawei.com>

The /dev/uioX device has concurrent operations in a few scenarios.

For example, when a process using the device exits abnormally,
the management program starts the same process to operate the device.
When the process exits and closes the /dev/uioX device,
the master bit of the device is cleared. In this case, if the
new process is issuing commands, I/Os are suspended and cannot be
automatically recovered.

Therefore, reference counting is added to clear the master bit
only when the last process exits.

Signed-off-by: Weifeng Su <suweifeng1@huawei.com>
---
The difference between the first patch and the first patch is that 
the reference counting operation is performed using the atomic semantics, 
just like other drivers under UIO:
cdfa835c6e5e87d145f("uio_hv_generic: defer opening vmbus until first use").

 drivers/uio/uio_pci_generic.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
index e03f9b532..df436e3d9 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,10 +40,19 @@ to_uio_pci_generic_dev(struct uio_info *info)
 	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);
 
+
 	/*
 	 * This driver is insecure when used with devices doing DMA, but some
 	 * people (mis)use it with such devices.
@@ -51,7 +61,8 @@ 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;
 }
 
@@ -93,7 +104,9 @@ static int probe(struct pci_dev *pdev,
 	gdev->info.name = "uio_pci_generic";
 	gdev->info.version = DRIVER_VERSION;
 	gdev->info.release = release;
+	gdev->info.open = open;
 	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;
-- 
2.33.0

Re: [PATCH v2] uio:uio_pci_generic:Don't clear master bit when the process does not exit

[Greg KH]

On Tue, Feb 21, 2023 at 01:10:44AM +0800, Su Weifeng wrote:
> From: Weifeng Su <suweifeng1@huawei.com>
> 
> The /dev/uioX device has concurrent operations in a few scenarios.
> 
> For example, when a process using the device exits abnormally,
> the management program starts the same process to operate the device.
> When the process exits and closes the /dev/uioX device,
> the master bit of the device is cleared. In this case, if the
> new process is issuing commands, I/Os are suspended and cannot be
> automatically recovered.
> 
> Therefore, reference counting is added to clear the master bit
> only when the last process exits.
> 
> Signed-off-by: Weifeng Su <suweifeng1@huawei.com>
> ---
> The difference between the first patch and the first patch is that 
> the reference counting operation is performed using the atomic semantics, 
> just like other drivers under UIO:
> cdfa835c6e5e87d145f("uio_hv_generic: defer opening vmbus until first use").

And I would claim that that change too is incorrect.

Did you test this with dup()?  Lots of open/close cycles on the same
device node?  Passing around the file descriptor?

Logically, this is identical to your previous change, so why should it
be accepted?

Again, why not just use a real PCI driver for your device?

thanks,

greg k-h

Re: [PATCH v2] uio:uio_pci_generic:Don't clear master bit when the process does not exit

[Weifeng Su]

On 2023/2/21 2:11, Greg KH wrote:
> On Tue, Feb 21, 2023 at 01:10:44AM +0800, Su Weifeng wrote:
>> From: Weifeng Su <suweifeng1@huawei.com>
>>
>> The /dev/uioX device has concurrent operations in a few scenarios.
>>
>> For example, when a process using the device exits abnormally,
>> the management program starts the same process to operate the device.
>> When the process exits and closes the /dev/uioX device,
>> the master bit of the device is cleared. In this case, if the
>> new process is issuing commands, I/Os are suspended and cannot be
>> automatically recovered.
>>
>> Therefore, reference counting is added to clear the master bit
>> only when the last process exits.
>>
>> Signed-off-by: Weifeng Su <suweifeng1@huawei.com>
>> ---
>> The difference between the first patch and the first patch is that
>> the reference counting operation is performed using the atomic semantics,
>> just like other drivers under UIO:
>> cdfa835c6e5e87d145f("uio_hv_generic: defer opening vmbus until first use").
> 
> And I would claim that that change too is incorrect.
> 
> Did you test this with dup()?  Lots of open/close cycles on the same
> device node?  Passing around the file descriptor?
The patch is verified to be able to fix the low-probability problem in 
our scenario. At the same time, we also perform the dup test on the 
latest kernel(6.2.0-rc8-g0f5e65cd8f9b-dirty) based on your suggestions. 
The test code model is as follows:
...
int main()
{
         int fd = open("/dev/uio0", O_RDWR);
         if (fd < 0) {
                 printf("error in open /dev/uio0\n");
                 return -1;
         }
         int dup_fd = dup(fd);
         while (true) {
                 sleep(1);
         }
}

After kill the process, The test results are as follows:
[  335.730916] swf call uio open
[  338.307306] swf call uio release

dup does not cause uio_pci_generic.c:open or uio_pci_generic.c:release 
reference counting exceptions.
PS: In this example, a print is added to the entries of uio_open and 
uio_release of the driver to confirm the function invoking status.

After reading the "dup" code, we confirm that the struct file uses the 
f_count protect the same file in the process. The file opened by the 
"dup" only adds reference counting and does not invoke the open 
operation of the driver. Disabling the fd only decrementes the f_count 
count. The release function of the driver is invoked to clear resources 
only when f_count is 0. Different processes use different struct files 
to open the same file, and f_count cannot enable the constraint 
function. In this case, the driver needs to handle the concurrency 
problem of multiple processes,It's like this patch
cdfa835c6e5e87d145f("uio_hv_generic: defer opening vmbus until first use")
> 
> Logically, this is identical to your previous change, so why should it
> be accepted?
> 
> Again, why not just use a real PCI driver for your device?
We use the uio_pci_generic driver because we use the DPDK, which is a 
user-mode development platform on which you can develop the user-mode 
driver.
> 
> thanks,
> 
> greg k-h

Best regards,
Weifeng Su

Re: [PATCH v2] uio:uio_pci_generic:Don't clear master bit when the process does not exit

[Weifeng Su]

On 2023/2/21 20:48, Weifeng Su wrote:
> On 2023/2/21 2:11, Greg KH wrote:
>> On Tue, Feb 21, 2023 at 01:10:44AM +0800, Su Weifeng wrote:
>>> From: Weifeng Su <suweifeng1@huawei.com>
>>>
>>> The /dev/uioX device has concurrent operations in a few scenarios.
>>>
>>> For example, when a process using the device exits abnormally,
>>> the management program starts the same process to operate the device.
>>> When the process exits and closes the /dev/uioX device,
>>> the master bit of the device is cleared. In this case, if the
>>> new process is issuing commands, I/Os are suspended and cannot be
>>> automatically recovered.
>>>
>>> Therefore, reference counting is added to clear the master bit
>>> only when the last process exits.
>>>
>>> Signed-off-by: Weifeng Su <suweifeng1@huawei.com>
>>> ---
>>> The difference between the first patch and the first patch is that
>>> the reference counting operation is performed using the atomic 
>>> semantics,
>>> just like other drivers under UIO:
>>> cdfa835c6e5e87d145f("uio_hv_generic: defer opening vmbus until first 
>>> use").
>>
>> And I would claim that that change too is incorrect.
>>
>> Did you test this with dup()?  Lots of open/close cycles on the same
>> device node?  Passing around the file descriptor?
> The patch is verified to be able to fix the low-probability problem in 
> our scenario. At the same time, we also perform the dup test on the 
> latest kernel(6.2.0-rc8-g0f5e65cd8f9b-dirty) based on your suggestions. 
> The test code model is as follows:
> ...
> int main()
> {
>          int fd = open("/dev/uio0", O_RDWR);
>          if (fd < 0) {
>                  printf("error in open /dev/uio0\n");
>                  return -1;
>          }
>          int dup_fd = dup(fd);
>          while (true) {
>                  sleep(1);
>          }
> }
> 
> After kill the process, The test results are as follows:
> [  335.730916] swf call uio open
> [  338.307306] swf call uio release
> 
> dup does not cause uio_pci_generic.c:open or uio_pci_generic.c:release 
> reference counting exceptions.
> PS: In this example, a print is added to the entries of uio_open and 
> uio_release of the driver to confirm the function invoking status.
> 
> After reading the "dup" code, we confirm that the struct file uses the 
> f_count protect the same file in the process. The file opened by the 
> "dup" only adds reference counting and does not invoke the open 
> operation of the driver. Disabling the fd only decrementes the f_count 
> count. The release function of the driver is invoked to clear resources 
> only when f_count is 0. Different processes use different struct files 
> to open the same file, and f_count cannot enable the constraint 
> function. In this case, the driver needs to handle the concurrency 
> problem of multiple processes,It's like this patch
> cdfa835c6e5e87d145f("uio_hv_generic: defer opening vmbus until first use")
>>
>> Logically, this is identical to your previous change, so why should it
>> be accepted?
>>
>> Again, why not just use a real PCI driver for your device?
> We use the uio_pci_generic driver because we use the DPDK, which is a 
> user-mode development platform on which you can develop the user-mode 
> driver.
>>
>> thanks,
>>
>> greg k-h
> 
> Best regards,
> Weifeng Su
Hi All,

  Do I need to perform more tests on the patch based on the v2 version to
verify it?

Best regards,
Weifeng Su