diff mbox series

get rid of close_fd() misuse in cachefiles

Message ID 20240603001128.GG1629371@ZenIV (mailing list archive)
State New, archived
Headers show
Series get rid of close_fd() misuse in cachefiles | expand

Commit Message

Al Viro June 3, 2024, 12:11 a.m. UTC 
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>
---

Comments

Gao Xiang June 3, 2024, 1:53 a.m. UTC | #1 
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
Al Viro June 3, 2024, 2:21 a.m. UTC | #2 
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.
Gao Xiang June 3, 2024, 2:33 a.m. UTC | #3 
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
Al Viro June 3, 2024, 3:40 a.m. UTC | #4 
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
diff mbox series

Patch

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);