Messages in this thread | | | Date | Thu, 16 Feb 2023 22:45:02 +0800 | Subject | Re: [PATCH] uio:uio_pci_generic:Don't clear master bit when the process does not exit | From | "Suweifeng (Weifeng, EulerOS)" <> |
| |
On 2023/2/14 21:17, Greg KH wrote: > On Tue, Feb 14, 2023 at 09:21:57PM +0800, Su Weifeng wrote: >> From: Weifeng Su <suweifeng1@huawei.com> >> >> The /dev/uioX device is used by multiple processes. The current behavior >> is to clear the master bit when a process exits. This affects other >> processes that use the device, resulting in command suspension and >> timeout. This behavior cannot be sensed by the process itself. >> The solution is to add the reference counting. The reference count is >> self-incremented and self-decremented each time when the device open and >> close. The master bit is cleared only when the last process exited. >> >> Signed-off-by: Weifeng Su <suweifeng1@huawei.com> >> --- >> drivers/uio/uio_pci_generic.c | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c >> index e03f9b532..d36d3e08e 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 dev_refc; >> }; >> >> static inline struct uio_pci_generic_dev * >> @@ -39,10 +40,22 @@ 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); >> + >> + if (gdev) >> + refcount_inc(&gdev->dev_refc); > > This flat out does not work, sorry. > > You should never rely on trying to count open/release calls, just let > the vfs layer handle that for us as it currently does so. > > Think about what happens if you call dup() in userspace on a > filehandle... > >> + return 0; >> +} >> + >> static int release(struct uio_info *info, struct inode *inode) >> { >> struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info); >> >> + if (gdev && refcount_dec_not_one(&gdev->dev_refc)) > > I don't think you actually tested this as it is impossible for gdev to > ever be NULL. > > sorry, but this patch is not correct. > > greg k-h
First of all, thank you for taking the time to read this patch, your comments are very enlightening, but I do have a strange problem here, I test such programs on kernels 5.10 and 6.2. fd = open("/dev/uio0". O_RDWR); while (true) sleep(1); This program only opens the uio device. After starting multiple such processes, I close one of them. From the added print, it can be seen that the "uio_pci_generic.c:release" function is called and the master bit is cleared, instead of being called when the last process exits as expected. I think the vfs is not protected as it should be. Such a problem cannot be handled in the user-mode program. We have to clear the master bit when the last process exits. Otherwise, user-mode programs (for example, the DPDK process that uses uio_pci_generic) cannot work properly.
Best regards, Weifeng Su
| |