Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 1/2] uio: Add late_release callback to uio_info
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     linux-scsi@vger.kernel.org, target-devel@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Mike Christie <michael.christie@oracle.com>
References: <20210210194031.7422-1-bostroesser@gmail.com>
 <20210210194031.7422-2-bostroesser@gmail.com> <YCQ4aEz29P26ZxaL@kroah.com>
From:   Bodo Stroesser <bostroesser@gmail.com>
Message-ID: <7bc9eef9-0a9e-58a9-11f1-2c32010c70f0@gmail.com>
Date:   Wed, 10 Feb 2021 20:57:11 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <YCQ4aEz29P26ZxaL@kroah.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.