Re: [PATCH] scsi: target: tcmu: Fix wrong uio handling causing big memory leak

[Bodo Stroesser]

On 13.01.21 22:04, Mike Christie wrote:
> On 1/13/21 11:59 AM, Bodo Stroesser wrote:
> > On 12.01.21 19:36, Mike Christie wrote:
> > > On 12/18/20 8:15 AM, Bodo Stroesser wrote:
> > > > tcmu calls uio_unregister_device from tcmu_destroy_device.
> > > > After that uio will never call tcmu_release for this device.
> > > > If userspace still had the uio device open and / or mmap'ed
> > > > during uio_unregister_device, tcmu_release will not be called and
> > > > udev->kref will never go down to 0.
> > >
> > > I didn't get why the release function is not called if you call
> > > uio_unregister_device while a device is open. Does the device_destroy call in
> > > uio_unregister_device completely free the device or does it set some bits so
> > > uio_release is not called later?
> >
> > uio_unregister_device() resets the pointer (idev->info) to the struct uio_info which tcmu provided in uio_register_device().
> > The uio device itself AFAICS is kept while it is open / mmap'ed.
> > But no matter what userspace does, uio will not call tcmu's callbacks
> > since info pointer now is NULL.
> >
> > When userspace finally closes the uio device, uio_release is called, but
> > tcmu_release can not be called.
> >
> > > Do other drivers hit this? Should uio have refcounting so uio_release is called
> > > when the last ref (from userspace open/close/mmap calls and from the kernel by
> > > drivers like target_core_user) is done?
> >
> > To be honest I don't know exactly.
> > tcmu seems to be a special case in that is has it's own mmap callback.
> > That allows us to map pages allocated by tcmu.
> > As long as userspace still holds the mapping, we should not unmap those
> > pages, because userspace then could get killed by SIGSEGV.
> > So we have to wait for userspace closing uio before we may unmap and
> > free the pages.
>
>
> If we removed the clearing of idev->info in uio_unregister_device, and
> then moved the idev->info->release call from uio_release to
> uio_device_release it would work like you need right? The release callback
> would always be called and called when userspace has dropped it's refs.
> You need to also fix up the module refcount and some other bits because
> it looks like uio uses the uio->info check to determine if the device is
> being removed.

I fear that would not work, because uio_release must be called always,
no matter whether userspace closes the device before or after
uio_unregister_device.

But we could add a new callback pointer 'late_release' to struct
uio_info and struct uio_device. During uio_register_device we would
copy the pointer from info to idev.

If info == NULL, uio_release calls idev->late_release if != NULL.

tcmu would of course set info->release and ->late_release both to
tcmu_release.

> I don't know if that is the correct approach. It looks like non
> target_core_user drivers could hit a similar issue. It seems like drivers
> like qedi/bnx2i could hit the issue if their port is removed from the
> kernel before their uio daemon closes the device. However, they also
> could do a driver specific fix and handle the issue by adding some extra
> kernel/userspace bits to sync the port removal.

I had a closer look into qedi. I assume there might be a leak also,
because qedi_uio_close calls "qedi_ll2_free_skbs(qedi)".

Unfortunately my above proposal would not work here without adding a
new refcount to qedi_uio_dev, because currently in __qedi_free_uio
the udev is freed shortly after uio_unregister_device. So later calls
of qedi_uio_close(udev) would be harmful.

But I guess the leak can be fixed by adding two lines after the
uio_unregister_device() in __qedi_free_uio:

	if (test_bit(UIO_DEV_OPENED, &udev->qedi->flags)
		qedi_ll2_free_skbs(qedi);