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

Greg Kroah-Hartman
2021-02-10T19:49:13Z

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