Messages in this thread | | | Subject | Re: [PATCH] scsi: target: tcmu: Fix wrong uio handling causing big memory leak | From | Bodo Stroesser <> | Date | Thu, 14 Jan 2021 17:50:03 +0100 |
| |
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);
| |