[PATCH 0/2] uio and tcmu: Fix memory leak in tcmu by adding new uio feature

[PATCH 0/2] uio and tcmu: Fix memory leak in tcmu by adding new uio feature

[Bodo Stroesser]

A couple of weeks ago I found a huge memory leak in tcmu:

tcmu needs to keep resources as long as userspace holds the uio
device open or mmap'ed. Therefore tcmu increments and decrements
a refcnt during uio_info::uio_open (tcmu_open) and
uio_info::uio_release (tcmu_release).

If via configFS user tries to destroy a tcmu device, tcmu calls
uio_unregister_device(). If during this call userspace daemon
still holds the uio device open or mmap'ed, uio does not call
tcmu_release when userspace later closes and munmaps the uio
device. So refcnt never drops to 0 and resources are not freed.

My first attempt to fix the problem you can find here:
  https://lore.kernel.org/linux-scsi/20201218141534.9918-1-bostroesser@gmail.com/
That fix delayed calling uio_unregister_device until tcmu_release
was called. To make userspace aware of the device going to be
destroyed without calling uio_unregister_device, the patch
inserted the following code snippet in tcmu:

  /* reset uio_info->irq, so uio will reject read() and write() */
  udev->uio_info.irq = 0;
  /* Set bit, so we can reject later calls to tcmu_open and tcmu_mmap */
  set_bit(TCMU_DEV_BIT_GOING_DOWN, &udev->flags);
  /* wake up possible sleeper in uio_read(), it will return -EIO */
  uio_event_notify(&udev->uio_info);

Especially resetting uio_info::irq on an alive uio device is not
very clean, I think.

Therefore I'm sending a small series of two patches as a second
attempt to fix the memory leak.

Patch 1 adds the new optional callback uio_info::late_release
which is called if userspace closes or munmaps the uio device
after uio_register_device was called.

Patch 2 is a one liner that uses the new feature in tcmu.
No further changes in tcmu are necessary.

I'm wondering whether the new feature in uio can be useful for
other drivers also, e.g. uio_hv_generic?


The patches were made on top of Martin's for-next branch.
But they probably will apply to most other recent trees.


Bode Stroesser (2):
  uio: Add late_release callback to uio_info
  scsi: target: tcmu: Fix memory leak by using new uio callback

 Documentation/driver-api/uio-howto.rst | 10 ++++++++++
 drivers/target/target_core_user.c      |  1 +
 drivers/uio/uio.c                      |  4 ++++
 include/linux/uio_driver.h             |  4 ++++
 4 files changed, 19 insertions(+)

-- 
2.12.3

[PATCH 1/2] uio: Add late_release callback to uio_info

[Bodo Stroesser]

If uio_unregister_device() is called while userspace daemon
still holds the uio device open or mmap'ed, uio will not call
uio_info->release() on later close / munmap.

At least one user of uio (tcmu) should not free resources (pages
allocated by tcmu which are mmap'ed to userspace) while uio
device still is open, because that could cause userspace daemon
to be killed by SIGSEGV or SIGBUS. Therefore tcmu frees the
pages only after it called uio_unregister_device _and_ the device
was closed.
So, uio not calling uio_info->release causes trouble.
tcmu currently leaks memory in that case.

Just waiting for userspace daemon to exit before calling
uio_unregister_device I think is not the right solution, because
daemon would not become aware of kernel code wanting to destroy
the uio device.
After uio_unregister_device was called, reading or writing the
uio device returns -EIO, which normally results in daemon exit.

This patch adds new callback pointer 'late_release' to struct
uio_info. If uio user sets this callback, it will be called by
uio if userspace closes / munmaps the device after
uio_unregister_device was executed.

That way we can use uio_unregister_device() to notify userspace
that we are going to destroy the device, but still get a call
to late_release when uio device is finally closed.

Signed-off-by: Bodo Stroesser <bostroesser@gmail.com>
---
 Documentation/driver-api/uio-howto.rst | 10 ++++++++++
 drivers/uio/uio.c                      |  4 ++++
 include/linux/uio_driver.h             |  4 ++++
 3 files changed, 18 insertions(+)

diff --git a/Documentation/driver-api/uio-howto.rst b/Documentation/driver-api/uio-howto.rst
index 907ffa3b38f5..a2d57a7d623a 100644
--- a/Documentation/driver-api/uio-howto.rst
+++ b/Documentation/driver-api/uio-howto.rst
@@ -265,6 +265,16 @@ the members are required, others are optional.
    function. The parameter ``irq_on`` will be 0 to disable interrupts
    and 1 to enable them.
 
+-  ``int (*late_release)(struct uio_info *info, struct inode *inode)``:
+   Optional. If you define your own :c:func:`open()`, you will
+   in certain cases also want a custom :c:func:`late_release()`
+   function. If uio device is unregistered - by calling
+   :c:func:`uio_unregister_device()` - while it is open or mmap'ed by
+   userspace, the custom :c:func:`release()` function will not be
+   called when userspace later closes the device. An optionally
+   specified :c:func:`late_release()` function will be called in that
+   situation.
+
 Usually, your device will have one or more memory regions that can be
 mapped to user space. For each region, you have to set up a
 ``struct uio_mem`` in the ``mem[]`` array. Here's a description of the
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index ea96e319c8a0..0b2636f8d373 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -532,6 +532,8 @@ static int uio_release(struct inode *inode, struct file *filep)
 	mutex_lock(&idev->info_lock);
 	if (idev->info && idev->info->release)
 		ret = idev->info->release(idev->info, inode);
+	else if (idev->late_info && idev->late_info->late_release)
+		ret = idev->late_info->late_release(idev->late_info, inode);
 	mutex_unlock(&idev->info_lock);
 
 	module_put(idev->owner);
@@ -1057,6 +1059,8 @@ void uio_unregister_device(struct uio_info *info)
 		free_irq(info->irq, idev);
 
 	idev->info = NULL;
+	if (info->late_release)
+		idev->late_info = info;
 	mutex_unlock(&idev->info_lock);
 
 	wake_up_interruptible(&idev->wait);
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 47c5962b876b..5712e149f9ac 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -74,6 +74,7 @@ struct uio_device {
 	struct fasync_struct    *async_queue;
 	wait_queue_head_t       wait;
 	struct uio_info         *info;
+	struct uio_info         *late_info;
 	struct mutex		info_lock;
 	struct kobject          *map_dir;
 	struct kobject          *portio_dir;
@@ -94,6 +95,8 @@ struct uio_device {
  * @open:		open operation for this uio device
  * @release:		release operation for this uio device
  * @irqcontrol:		disable/enable irqs when 0/1 is written to /dev/uioX
+ * @late_release:	optional late release operation for this uio device,
+ *			called if device is released after uio_unregister_device()
  */
 struct uio_info {
 	struct uio_device	*uio_dev;
@@ -109,6 +112,7 @@ struct uio_info {
 	int (*open)(struct uio_info *info, struct inode *inode);
 	int (*release)(struct uio_info *info, struct inode *inode);
 	int (*irqcontrol)(struct uio_info *info, s32 irq_on);
+	int (*late_release)(struct uio_info *info, struct inode *inode);
 };
 
 extern int __must_check
-- 
2.12.3

Re: [PATCH 1/2] uio: Add late_release callback to uio_info

[Greg Kroah-Hartman]

On Wed, Feb 10, 2021 at 08:40:30PM +0100, Bodo Stroesser wrote:
> If uio_unregister_device() is called while userspace daemon
> still holds the uio device open or mmap'ed, uio will not call
> uio_info->release() on later close / munmap.
> 
> At least one user of uio (tcmu) should not free resources (pages
> allocated by tcmu which are mmap'ed to userspace) while uio
> device still is open, because that could cause userspace daemon
> to be killed by SIGSEGV or SIGBUS. Therefore tcmu frees the
> pages only after it called uio_unregister_device _and_ the device
> was closed.
> So, uio not calling uio_info->release causes trouble.
> tcmu currently leaks memory in that case.
> 
> Just waiting for userspace daemon to exit before calling
> uio_unregister_device I think is not the right solution, because
> daemon would not become aware of kernel code wanting to destroy
> the uio device.
> After uio_unregister_device was called, reading or writing the
> uio device returns -EIO, which normally results in daemon exit.
> 
> This patch adds new callback pointer 'late_release' to struct
> uio_info. If uio user sets this callback, it will be called by
> uio if userspace closes / munmaps the device after
> uio_unregister_device was executed.
> 
> That way we can use uio_unregister_device() to notify userspace
> that we are going to destroy the device, but still get a call
> to late_release when uio device is finally closed.
> 
> Signed-off-by: Bodo Stroesser <bostroesser@gmail.com>
> ---
>  Documentation/driver-api/uio-howto.rst | 10 ++++++++++
>  drivers/uio/uio.c                      |  4 ++++
>  include/linux/uio_driver.h             |  4 ++++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/Documentation/driver-api/uio-howto.rst b/Documentation/driver-api/uio-howto.rst
> index 907ffa3b38f5..a2d57a7d623a 100644
> --- a/Documentation/driver-api/uio-howto.rst
> +++ b/Documentation/driver-api/uio-howto.rst
> @@ -265,6 +265,16 @@ the members are required, others are optional.
>     function. The parameter ``irq_on`` will be 0 to disable interrupts
>     and 1 to enable them.
>  
> +-  ``int (*late_release)(struct uio_info *info, struct inode *inode)``:
> +   Optional. If you define your own :c:func:`open()`, you will
> +   in certain cases also want a custom :c:func:`late_release()`
> +   function. If uio device is unregistered - by calling
> +   :c:func:`uio_unregister_device()` - while it is open or mmap'ed by
> +   userspace, the custom :c:func:`release()` function will not be
> +   called when userspace later closes the device. An optionally
> +   specified :c:func:`late_release()` function will be called in that
> +   situation.
> +
>  Usually, your device will have one or more memory regions that can be
>  mapped to user space. For each region, you have to set up a
>  ``struct uio_mem`` in the ``mem[]`` array. Here's a description of the
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index ea96e319c8a0..0b2636f8d373 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -532,6 +532,8 @@ static int uio_release(struct inode *inode, struct file *filep)
>  	mutex_lock(&idev->info_lock);
>  	if (idev->info && idev->info->release)
>  		ret = idev->info->release(idev->info, inode);
> +	else if (idev->late_info && idev->late_info->late_release)
> +		ret = idev->late_info->late_release(idev->late_info, inode);
>  	mutex_unlock(&idev->info_lock);

Why can't release() be called here?  Why doesn't your driver define a
release() if it cares about this information?  Why do we need 2
different callbacks that fire at exactly the same time?

This feels really wrong.

greg k-h

Re: [PATCH 1/2] uio: Add late_release callback to uio_info

[Bodo Stroesser]

On 10.02.21 20:47, Greg Kroah-Hartman wrote:
> On Wed, Feb 10, 2021 at 08:40:30PM +0100, Bodo Stroesser wrote:
>> If uio_unregister_device() is called while userspace daemon
>> still holds the uio device open or mmap'ed, uio will not call
>> uio_info->release() on later close / munmap.
>>
>> At least one user of uio (tcmu) should not free resources (pages
>> allocated by tcmu which are mmap'ed to userspace) while uio
>> device still is open, because that could cause userspace daemon
>> to be killed by SIGSEGV or SIGBUS. Therefore tcmu frees the
>> pages only after it called uio_unregister_device _and_ the device
>> was closed.
>> So, uio not calling uio_info->release causes trouble.
>> tcmu currently leaks memory in that case.
>>
>> Just waiting for userspace daemon to exit before calling
>> uio_unregister_device I think is not the right solution, because
>> daemon would not become aware of kernel code wanting to destroy
>> the uio device.
>> After uio_unregister_device was called, reading or writing the
>> uio device returns -EIO, which normally results in daemon exit.
>>
>> This patch adds new callback pointer 'late_release' to struct
>> uio_info. If uio user sets this callback, it will be called by
>> uio if userspace closes / munmaps the device after
>> uio_unregister_device was executed.
>>
>> That way we can use uio_unregister_device() to notify userspace
>> that we are going to destroy the device, but still get a call
>> to late_release when uio device is finally closed.
>>
>> Signed-off-by: Bodo Stroesser <bostroesser@gmail.com>
>> ---
>>   Documentation/driver-api/uio-howto.rst | 10 ++++++++++
>>   drivers/uio/uio.c                      |  4 ++++
>>   include/linux/uio_driver.h             |  4 ++++
>>   3 files changed, 18 insertions(+)
>>
>> diff --git a/Documentation/driver-api/uio-howto.rst b/Documentation/driver-api/uio-howto.rst
>> index 907ffa3b38f5..a2d57a7d623a 100644
>> --- a/Documentation/driver-api/uio-howto.rst
>> +++ b/Documentation/driver-api/uio-howto.rst
>> @@ -265,6 +265,16 @@ the members are required, others are optional.
>>      function. The parameter ``irq_on`` will be 0 to disable interrupts
>>      and 1 to enable them.
>>   
>> +-  ``int (*late_release)(struct uio_info *info, struct inode *inode)``:
>> +   Optional. If you define your own :c:func:`open()`, you will
>> +   in certain cases also want a custom :c:func:`late_release()`
>> +   function. If uio device is unregistered - by calling
>> +   :c:func:`uio_unregister_device()` - while it is open or mmap'ed by
>> +   userspace, the custom :c:func:`release()` function will not be
>> +   called when userspace later closes the device. An optionally
>> +   specified :c:func:`late_release()` function will be called in that
>> +   situation.
>> +
>>   Usually, your device will have one or more memory regions that can be
>>   mapped to user space. For each region, you have to set up a
>>   ``struct uio_mem`` in the ``mem[]`` array. Here's a description of the
>> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>> index ea96e319c8a0..0b2636f8d373 100644
>> --- a/drivers/uio/uio.c
>> +++ b/drivers/uio/uio.c
>> @@ -532,6 +532,8 @@ static int uio_release(struct inode *inode, struct file *filep)
>>   	mutex_lock(&idev->info_lock);
>>   	if (idev->info && idev->info->release)
>>   		ret = idev->info->release(idev->info, inode);
>> +	else if (idev->late_info && idev->late_info->late_release)
>> +		ret = idev->late_info->late_release(idev->late_info, inode);
>>   	mutex_unlock(&idev->info_lock);
> 
> Why can't release() be called here?  Why doesn't your driver define a
> release() if it cares about this information?  Why do we need 2
> different callbacks that fire at exactly the same time?
> 
> This feels really wrong.
> 
> greg k-h
> 

tcmu has a release callback. But uio can't call it after
uio_unregister_device was executed, because in uio_unregister_device
uio sets the uio_device::info to NULL.

So, uio would never call both callbacks for the same release action,
but would call release before uio_unregister_device is executed, and
late_release after that.

Of course it would be good for tcmu if uio would call uio_info:release 
even after uio_unregister_device, but changing this AFAICS could cause
trouble in other drivers using uio.

Bodo
after uio_unregister_device.

Re: [PATCH 1/2] uio: Add late_release callback to uio_info

[Greg Kroah-Hartman]

On Wed, Feb 10, 2021 at 08:57:11PM +0100, Bodo Stroesser wrote:
> On 10.02.21 20:47, Greg Kroah-Hartman wrote:
> > On Wed, Feb 10, 2021 at 08:40:30PM +0100, Bodo Stroesser wrote:
> > > If uio_unregister_device() is called while userspace daemon
> > > still holds the uio device open or mmap'ed, uio will not call
> > > uio_info->release() on later close / munmap.
> > > 
> > > At least one user of uio (tcmu) should not free resources (pages
> > > allocated by tcmu which are mmap'ed to userspace) while uio
> > > device still is open, because that could cause userspace daemon
> > > to be killed by SIGSEGV or SIGBUS. Therefore tcmu frees the
> > > pages only after it called uio_unregister_device _and_ the device
> > > was closed.
> > > So, uio not calling uio_info->release causes trouble.
> > > tcmu currently leaks memory in that case.
> > > 
> > > Just waiting for userspace daemon to exit before calling
> > > uio_unregister_device I think is not the right solution, because
> > > daemon would not become aware of kernel code wanting to destroy
> > > the uio device.
> > > After uio_unregister_device was called, reading or writing the
> > > uio device returns -EIO, which normally results in daemon exit.
> > > 
> > > This patch adds new callback pointer 'late_release' to struct
> > > uio_info. If uio user sets this callback, it will be called by
> > > uio if userspace closes / munmaps the device after
> > > uio_unregister_device was executed.
> > > 
> > > That way we can use uio_unregister_device() to notify userspace
> > > that we are going to destroy the device, but still get a call
> > > to late_release when uio device is finally closed.
> > > 
> > > Signed-off-by: Bodo Stroesser <bostroesser@gmail.com>
> > > ---
> > >   Documentation/driver-api/uio-howto.rst | 10 ++++++++++
> > >   drivers/uio/uio.c                      |  4 ++++
> > >   include/linux/uio_driver.h             |  4 ++++
> > >   3 files changed, 18 insertions(+)
> > > 
> > > diff --git a/Documentation/driver-api/uio-howto.rst b/Documentation/driver-api/uio-howto.rst
> > > index 907ffa3b38f5..a2d57a7d623a 100644
> > > --- a/Documentation/driver-api/uio-howto.rst
> > > +++ b/Documentation/driver-api/uio-howto.rst
> > > @@ -265,6 +265,16 @@ the members are required, others are optional.
> > >      function. The parameter ``irq_on`` will be 0 to disable interrupts
> > >      and 1 to enable them.
> > > +-  ``int (*late_release)(struct uio_info *info, struct inode *inode)``:
> > > +   Optional. If you define your own :c:func:`open()`, you will
> > > +   in certain cases also want a custom :c:func:`late_release()`
> > > +   function. If uio device is unregistered - by calling
> > > +   :c:func:`uio_unregister_device()` - while it is open or mmap'ed by
> > > +   userspace, the custom :c:func:`release()` function will not be
> > > +   called when userspace later closes the device. An optionally
> > > +   specified :c:func:`late_release()` function will be called in that
> > > +   situation.
> > > +
> > >   Usually, your device will have one or more memory regions that can be
> > >   mapped to user space. For each region, you have to set up a
> > >   ``struct uio_mem`` in the ``mem[]`` array. Here's a description of the
> > > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> > > index ea96e319c8a0..0b2636f8d373 100644
> > > --- a/drivers/uio/uio.c
> > > +++ b/drivers/uio/uio.c
> > > @@ -532,6 +532,8 @@ static int uio_release(struct inode *inode, struct file *filep)
> > >   	mutex_lock(&idev->info_lock);
> > >   	if (idev->info && idev->info->release)
> > >   		ret = idev->info->release(idev->info, inode);
> > > +	else if (idev->late_info && idev->late_info->late_release)
> > > +		ret = idev->late_info->late_release(idev->late_info, inode);
> > >   	mutex_unlock(&idev->info_lock);
> > 
> > Why can't release() be called here?  Why doesn't your driver define a
> > release() if it cares about this information?  Why do we need 2
> > different callbacks that fire at exactly the same time?
> > 
> > This feels really wrong.
> > 
> > greg k-h
> > 
> 
> tcmu has a release callback. But uio can't call it after
> uio_unregister_device was executed, because in uio_unregister_device
> uio sets the uio_device::info to NULL.

As it should because the driver could then be gone.  It should NEVER
call back into it again.

> So, uio would never call both callbacks for the same release action,
> but would call release before uio_unregister_device is executed, and
> late_release after that.

That's not ok.

> Of course it would be good for tcmu if uio would call uio_info:release even
> after uio_unregister_device, but changing this AFAICS could cause
> trouble in other drivers using uio.

You are confusing two different lifetime rules here it seems.  One is
the char device and one is the struct device.  They work independently
as different users affect them.

So if one is removed from the system, do not try to keep a callback to
it, otherwise you will crash.

And why is scsi using the uio driver in the first place?  That feels
really odd to me.  Why not just make a "real" driver if you want to
somehow tie these two lifetimes together?

thanks,

greg k-h

Re: [PATCH 1/2] uio: Add late_release callback to uio_info

[Bodo Stroesser]

On 11.02.21 07:51, Greg Kroah-Hartman wrote:
> On Wed, Feb 10, 2021 at 08:57:11PM +0100, Bodo Stroesser wrote:
>> On 10.02.21 20:47, Greg Kroah-Hartman wrote:
>>> On Wed, Feb 10, 2021 at 08:40:30PM +0100, Bodo Stroesser wrote:
>>>> If uio_unregister_device() is called while userspace daemon
>>>> still holds the uio device open or mmap'ed, uio will not call
>>>> uio_info->release() on later close / munmap.
>>>>
>>>> At least one user of uio (tcmu) should not free resources (pages
>>>> allocated by tcmu which are mmap'ed to userspace) while uio
>>>> device still is open, because that could cause userspace daemon
>>>> to be killed by SIGSEGV or SIGBUS. Therefore tcmu frees the
>>>> pages only after it called uio_unregister_device _and_ the device
>>>> was closed.
>>>> So, uio not calling uio_info->release causes trouble.
>>>> tcmu currently leaks memory in that case.
>>>>
>>>> Just waiting for userspace daemon to exit before calling
>>>> uio_unregister_device I think is not the right solution, because
>>>> daemon would not become aware of kernel code wanting to destroy
>>>> the uio device.
>>>> After uio_unregister_device was called, reading or writing the
>>>> uio device returns -EIO, which normally results in daemon exit.
>>>>
>>>> This patch adds new callback pointer 'late_release' to struct
>>>> uio_info. If uio user sets this callback, it will be called by
>>>> uio if userspace closes / munmaps the device after
>>>> uio_unregister_device was executed.
>>>>
>>>> That way we can use uio_unregister_device() to notify userspace
>>>> that we are going to destroy the device, but still get a call
>>>> to late_release when uio device is finally closed.
>>>>
>>>> Signed-off-by: Bodo Stroesser <bostroesser@gmail.com>
>>>> ---
>>>>    Documentation/driver-api/uio-howto.rst | 10 ++++++++++
>>>>    drivers/uio/uio.c                      |  4 ++++
>>>>    include/linux/uio_driver.h             |  4 ++++
>>>>    3 files changed, 18 insertions(+)
>>>>
>>>> diff --git a/Documentation/driver-api/uio-howto.rst b/Documentation/driver-api/uio-howto.rst
>>>> index 907ffa3b38f5..a2d57a7d623a 100644
>>>> --- a/Documentation/driver-api/uio-howto.rst
>>>> +++ b/Documentation/driver-api/uio-howto.rst
>>>> @@ -265,6 +265,16 @@ the members are required, others are optional.
>>>>       function. The parameter ``irq_on`` will be 0 to disable interrupts
>>>>       and 1 to enable them.
>>>> +-  ``int (*late_release)(struct uio_info *info, struct inode *inode)``:
>>>> +   Optional. If you define your own :c:func:`open()`, you will
>>>> +   in certain cases also want a custom :c:func:`late_release()`
>>>> +   function. If uio device is unregistered - by calling
>>>> +   :c:func:`uio_unregister_device()` - while it is open or mmap'ed by
>>>> +   userspace, the custom :c:func:`release()` function will not be
>>>> +   called when userspace later closes the device. An optionally
>>>> +   specified :c:func:`late_release()` function will be called in that
>>>> +   situation.
>>>> +
>>>>    Usually, your device will have one or more memory regions that can be
>>>>    mapped to user space. For each region, you have to set up a
>>>>    ``struct uio_mem`` in the ``mem[]`` array. Here's a description of the
>>>> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>>>> index ea96e319c8a0..0b2636f8d373 100644
>>>> --- a/drivers/uio/uio.c
>>>> +++ b/drivers/uio/uio.c
>>>> @@ -532,6 +532,8 @@ static int uio_release(struct inode *inode, struct file *filep)
>>>>    	mutex_lock(&idev->info_lock);
>>>>    	if (idev->info && idev->info->release)
>>>>    		ret = idev->info->release(idev->info, inode);
>>>> +	else if (idev->late_info && idev->late_info->late_release)
>>>> +		ret = idev->late_info->late_release(idev->late_info, inode);
>>>>    	mutex_unlock(&idev->info_lock);
>>>
>>> Why can't release() be called here?  Why doesn't your driver define a
>>> release() if it cares about this information?  Why do we need 2
>>> different callbacks that fire at exactly the same time?
>>>
>>> This feels really wrong.
>>>
>>> greg k-h
>>>
>>
>> tcmu has a release callback. But uio can't call it after
>> uio_unregister_device was executed, because in uio_unregister_device
>> uio sets the uio_device::info to NULL.
> 
> As it should because the driver could then be gone.  It should NEVER
> call back into it again.

OTOH, uio does try_module_get(idev->owner) in uio_open before calling
the driver's open callback and module_put(idev_owner) in uio_release
after calling driver's release callback. So driver's release callback
is guaranteed to exist until last release is done.

Apart from that, tcmu also has an uio_info::mmap callback. In that
callback it installs its own vm_operations_struct::fault handler.
This handler also can happen to be called as long as userspace holds
the uio device mmap'ed. I think, this is not a problem due to the
above mentioned mechanism.

tcmu just has to ensure, that the tcmu device, which contains the 
uio_info - is kept until the final release call happens. Unfortunately
this call will not happen if uio device is open during
uio_unregister_device. That's why tcmu sometimes leaks memory.

> 
>> So, uio would never call both callbacks for the same release action,
>> but would call release before uio_unregister_device is executed, and
>> late_release after that.
> 
> That's not ok.
> 
>> Of course it would be good for tcmu if uio would call uio_info:release even
>> after uio_unregister_device, but changing this AFAICS could cause
>> trouble in other drivers using uio.
> 
> You are confusing two different lifetime rules here it seems.  One is
> the char device and one is the struct device.  They work independently
> as different users affect them.
I'm not sure I get your point.

> 
> So if one is removed from the system, do not try to keep a callback to
> it, otherwise you will crash.

That's why I tried to change uio in a compatible way, so other drivers
based on it are not afflicted by the change. I saw, that some drivers
based on uio free their resources directly after calling
uio_unregister_device. Executing their release callback later would
definitely cause trouble.

> 
> And why is scsi using the uio driver in the first place?  That feels
> really odd to me.  Why not just make a "real" driver if you want to
> somehow tie these two lifetimes together?

Why tcmu driver is based on uio I don't know. I inherited the driver as
it is. Maybe it would have been better to not base it on uio, I don't
know. But changing this now would cause an API change for all existing
userspace apps, e.g. tcmu-runner. I think I should avoid that and
therefore have to find an acceptable solution for the tcmu/uio
combination.

> 
> thanks,
> 
> greg k-h
> 

[PATCH 2/2] scsi: target: tcmu: Fix memory leak by using new uio callback

[Bodo Stroesser]

Use new uio_info->late_release callback to fix memory leak.

If userspace daemon still holds uio device open or mmap'ed while
tcmu device is removed from configFS, refcount of the tcmu device
never drops down to 0, because uio does not call
uio_info->release - which is tcmu_release - after tcmu had
executed uio_unregister_device. So tcmu device and its allocated
resources are not freed in this situation.

tcmu now sets uio_info->late_release which is called by uio in
the described situation. That way the memory leak is fixed.

Signed-off-by: Bodo Stroesser <bostroesser@gmail.com>
---
 drivers/target/target_core_user.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index 6b171fff007b..bf61705ca92b 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -2092,6 +2092,7 @@ static int tcmu_configure_device(struct se_device *dev)
 	info->mmap = tcmu_mmap;
 	info->open = tcmu_open;
 	info->release = tcmu_release;
+	info->late_release = tcmu_release;
 
 	ret = uio_register_device(tcmu_root_device, info);
 	if (ret)
-- 
2.12.3