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

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

[Al Viro]

	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>
---
diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index 4ba42f1fa3b4..b5da26ef2d45 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -205,7 +205,7 @@ int cachefiles_ondemand_restore(struct cachefiles_cache *cache, char *args)
 	return 0;
 }
 
-static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
+static struct file *cachefiles_ondemand_get_fd(struct cachefiles_req *req)
 {
 	struct cachefiles_object *object;
 	struct cachefiles_cache *cache;
@@ -238,7 +238,6 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
 	}
 
 	file->f_mode |= FMODE_PWRITE | FMODE_LSEEK;
-	fd_install(fd, file);
 
 	load = (void *)req->msg.data;
 	load->fd = fd;
@@ -246,7 +245,7 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
 
 	cachefiles_get_unbind_pincount(cache);
 	trace_cachefiles_ondemand_open(object, &req->msg, load);
-	return 0;
+	return file;
 
 err_put_fd:
 	put_unused_fd(fd);
@@ -254,7 +253,7 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
 	xa_erase(&cache->ondemand_ids, object_id);
 err:
 	cachefiles_put_object(object, cachefiles_obj_put_ondemand_fd);
-	return ret;
+	return ERR_PTR(ret);
 }
 
 static void ondemand_object_worker(struct work_struct *work)
@@ -299,9 +298,9 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
 {
 	struct cachefiles_req *req;
 	struct cachefiles_msg *msg;
+	struct file *file = NULL;
 	unsigned long id = 0;
 	size_t n;
-	int ret = 0;
 	XA_STATE(xas, &cache->reqs, cache->req_id_next);
 
 	xa_lock(&cache->reqs);
@@ -335,8 +334,8 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
 	id = xas.xa_index;
 
 	if (msg->opcode == CACHEFILES_OP_OPEN) {
-		ret = cachefiles_ondemand_get_fd(req);
-		if (ret) {
+		file = cachefiles_ondemand_get_fd(req);
+		if (IS_ERR(file)) {
 			cachefiles_ondemand_set_object_close(req->object);
 			goto error;
 		}
@@ -346,10 +345,15 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
 	msg->object_id = req->object->ondemand->ondemand_id;
 
 	if (copy_to_user(_buffer, msg, n) != 0) {
-		ret = -EFAULT;
-		goto err_put_fd;
+		if (file)
+			fput(file);
+		file = ERR_PTR(-EFAULT);
+		goto error;
 	}
 
+	if (file)
+		fd_install(((struct cachefiles_open *)msg->data)->fd, file);
+
 	/* CLOSE request has no reply */
 	if (msg->opcode == CACHEFILES_OP_CLOSE) {
 		xa_erase(&cache->reqs, id);
@@ -358,14 +362,11 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
 
 	return n;
 
-err_put_fd:
-	if (msg->opcode == CACHEFILES_OP_OPEN)
-		close_fd(((struct cachefiles_open *)msg->data)->fd);
 error:
 	xa_erase(&cache->reqs, id);
-	req->error = ret;
+	req->error = PTR_ERR(file);
 	complete(&req->done);
-	return ret;
+	return PTR_ERR(file);
 }
 
 typedef int (*init_req_fn)(struct cachefiles_req *req, void *private);

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

[Gao Xiang]

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?

Thanks,
Ga Xiang

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

[Al Viro]

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.

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

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

[Gao Xiang]

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

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

[Al Viro]

On Mon, Jun 03, 2024 at 10:33:38AM +0800, Gao Xiang wrote:

> > 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!

FWIW, another thing that would be nice to have there is
removal of now-pointless include on linux/fdtable.h

[PATCH] cachefiles: remove unneeded include of <linux/fdtable.h>

[Gao Xiang]

close_fd() has been killed, let's get rid of unneeded
<linux/fdtable.h> as Al Viro pointed out [1].

[1] https://lore.kernel.org/r/20240603034055.GI1629371@ZenIV
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
Hi Christian,
If it's possible, please kindly also help pick this
patch along with the original patchset..
https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.fixes&id=a82c13d29985a4d99dacd700b497f0c062fe3625

Thanks,
Gao Xiang


 fs/cachefiles/ondemand.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index 58bd80956c5a..bce005f2b456 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -1,5 +1,4 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
-#include <linux/fdtable.h>
 #include <linux/anon_inodes.h>
 #include <linux/uio.h>
 #include "internal.h"
-- 
2.39.3

Re: (subset) [PATCH] cachefiles: remove unneeded include of <linux/fdtable.h>

[Christian Brauner]

On Mon, 03 Jun 2024 14:23:44 +0800, Gao Xiang wrote:
> close_fd() has been killed, let's get rid of unneeded
> <linux/fdtable.h> as Al Viro pointed out [1].
> 
> [1] https://lore.kernel.org/r/20240603034055.GI1629371@ZenIV
> 
> 

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] cachefiles: remove unneeded include of <linux/fdtable.h>
      https://git.kernel.org/vfs/vfs/c/5ea71848f7b2