lkml.org 
[lkml]   [2021]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] scsi: target: tcmu: Fix wrong uio handling causing big memory leak
From
Date
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);

\
 
 \ /
  Last update: 2021-01-14 17:52    [W:0.870 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site