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

[Bodo Stroesser <bostroesser@gmail.com>]

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);