Messages in this thread | | | Date | Thu, 16 Feb 2023 10:55:05 -0500 | From | "Michael S. Tsirkin" <> | Subject | Re: [PATCH] uio:uio_pci_generic:Don't clear master bit when the process does not exit |
| |
On Thu, Feb 16, 2023 at 04:07:24PM +0100, Greg KH wrote: > On Thu, Feb 16, 2023 at 10:45:02PM +0800, Suweifeng (Weifeng, EulerOS) wrote: > > 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. > > Did your patch change this functionality? > > > 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. > > Look at the big comment in the release() function in this file. Does > that explain the issues here? > > Just do not open the device multiple times, you have full control over > this, right? > > If not, then perhaps your hardware should not be using the > uio_pci_generic() driver but rather have a real kernel driver for it > instead if it needs to handle this type of functionality? > > thanks, > > greg k-h
Hmm. this code did however work correctly before 865a11f987ab5f03089 ("uio/uio_pci_generic: Disable bus-mastering on release")
| |