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

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Feb 16, 2023 at 10:55:05AM -0500, Michael S. Tsirkin wrote:
> 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")
> 

It's not really "correct" to leave dma bus mastering enabled, right?

Again, you can't "count" open and close calls, that just does not work
(i.e. the proposed patch will not work), so I don't see a viable
solution at the moment here.

thanks,

greg k-h