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