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

[Michael S. Tsirkin]

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@xxxxxxxxxx>
> > > > 
> > > > 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@xxxxxxxxxx>
> > > > ---
> > > >   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")