Re: [PATCH] get rid of close_fd() misuse in cachefiles

[Gao Xiang <hsiangkao@linux.alibaba.com>]

Hi Al,

On 2024/6/3 10:21, Al Viro wrote:
> On Mon, Jun 03, 2024 at 09:53:26AM +0800, Gao Xiang wrote:
>> Hi Al,
>>
>> On 2024/6/3 08:11, Al Viro wrote:
>>> 	fd_install() can't be undone by close_fd().  Just delay it
>>> until the last failure exit - have cachefiles_ondemand_get_fd()
>>> return the file on success (and ERR_PTR() on error) and let the
>>> caller do fd_install() after successful copy_to_user()
>>>
>>> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>>
>> It's a straight-forward fix to me, yet it will have a conflict with
>> https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/fs/cachefiles?h=vfs.fixes&id=4b4391e77a6bf24cba2ef1590e113d9b73b11039
>> https://lore.kernel.org/all/20240522114308.2402121-10-libaokun@huaweicloud.com/
>>
>> It also moves fd_install() to the end of the daemon_read() and tends
>> to fix it for months, does it look good to you?
> 
> Looks sane (and my variant lacks put_unused_fd(), so it leaks the
> descriptor).  OTOH, I suspect that my variant of calling conventions
> makes for less churn - fd is available anyway, so you just need error
> or file reference, and for that struct file * with ERR_PTR() for
> errors works fine.

Yes, I agree with that, but since these patches are already
in the -next queue.  We could clean up these later with
your idea later, otherwise I'm not sure if some other
implicit inter-dependencies show up..

> 
> Anyway, your variant seems to be correct; feel free to slap my
> ACKed-by on it.

Hi Christian, would you mind take Al's ack for this, and
hopefully upstream these patches? Many thanks!

Thanks,
Gao Xiang