[PATCH RFC 0/5] file: minor fixes

[PATCH RFC 0/5] file: minor fixes

[Christian Brauner]

* reduce number of helpers
* rename close_fd_get_file() helprs to reflect the fact that they don't
  take a refcount
* rename rcu_head struct back to callback_head now that we only use it
  for task work and not rcu anymore


---
base-commit: 9da5933ccd0f0cc5ed235f99e1211d4ad7cd2013
change-id: 20231129-vfs-files-fixes-07348f1bc50a

[PATCH RFC 1/5] file: s/close_fd_get_file()/file_close_fd()/g

[Christian Brauner]

That really shouldn't have "get" in there as that implies we're bumping
the reference count which we don't do at all. We used to but not anmore.
Now we're just closing the fd and pick that file from the fdtable
without bumping the reference count. Update the wrong documentation
while at it.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 drivers/android/binder.c |  2 +-
 fs/file.c                | 14 +++++++++-----
 fs/open.c                |  2 +-
 include/linux/fdtable.h  |  2 +-
 4 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 92128aae2d06..7658103ba760 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1921,7 +1921,7 @@ static void binder_deferred_fd_close(int fd)
 	if (!twcb)
 		return;
 	init_task_work(&twcb->twork, binder_do_fd_close);
-	twcb->file = close_fd_get_file(fd);
+	twcb->file = file_close_fd(fd);
 	if (twcb->file) {
 		// pin it until binder_do_fd_close(); see comments there
 		get_file(twcb->file);
diff --git a/fs/file.c b/fs/file.c
index 50df31e104a5..66f04442a384 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -796,7 +796,7 @@ int __close_range(unsigned fd, unsigned max_fd, unsigned int flags)
 }
 
 /*
- * See close_fd_get_file() below, this variant assumes current->files->file_lock
+ * See file_close_fd() below, this variant assumes current->files->file_lock
  * is held.
  */
 struct file *__close_fd_get_file(unsigned int fd)
@@ -804,11 +804,15 @@ struct file *__close_fd_get_file(unsigned int fd)
 	return pick_file(current->files, fd);
 }
 
-/*
- * variant of close_fd that gets a ref on the file for later fput.
- * The caller must ensure that filp_close() called on the file.
+/**
+ * file_close_fd - return file associated with fd
+ * @fd: file descriptor to retrieve file for
+ *
+ * Doesn't take a separate reference count.
+ *
+ * Returns: The file associated with @fd (NULL if @fd is not open)
  */
-struct file *close_fd_get_file(unsigned int fd)
+struct file *file_close_fd(unsigned int fd)
 {
 	struct files_struct *files = current->files;
 	struct file *file;
diff --git a/fs/open.c b/fs/open.c
index 0bd7fce21cbf..328dc6ef1883 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1578,7 +1578,7 @@ SYSCALL_DEFINE1(close, unsigned int, fd)
 	int retval;
 	struct file *file;
 
-	file = close_fd_get_file(fd);
+	file = file_close_fd(fd);
 	if (!file)
 		return -EBADF;
 
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 80bd7789bab1..78c8326d74ae 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -119,7 +119,7 @@ int iterate_fd(struct files_struct *, unsigned,
 
 extern int close_fd(unsigned int fd);
 extern int __close_range(unsigned int fd, unsigned int max_fd, unsigned int flags);
-extern struct file *close_fd_get_file(unsigned int fd);
+extern struct file *file_close_fd(unsigned int fd);
 extern int unshare_fd(unsigned long unshare_flags, unsigned int max_fds,
 		      struct files_struct **new_fdp);
 

-- 
2.42.0

[PATCH RFC 2/5] file: remove pointless wrapper

[Christian Brauner]

Only io_uring uses __close_fd_get_file(). All it does is hide
current->files but io_uring accesses files_struct directly right now
anyway so it's a bit pointless. Just rename pick_file() to
file_close_fd_locked() and let io_uring use it. Add a lockdep assert in
there that we expect the caller to hold file_lock while we're at it.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/file.c            | 23 +++++++++--------------
 fs/internal.h        |  2 +-
 io_uring/openclose.c |  2 +-
 3 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 66f04442a384..c8eaa0b29a08 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -629,19 +629,23 @@ void fd_install(unsigned int fd, struct file *file)
 EXPORT_SYMBOL(fd_install);
 
 /**
- * pick_file - return file associatd with fd
+ * file_close_fd_locked - return file associated with fd
  * @files: file struct to retrieve file from
  * @fd: file descriptor to retrieve file for
  *
+ * Doesn't take a separate reference count.
+ *
  * Context: files_lock must be held.
  *
  * Returns: The file associated with @fd (NULL if @fd is not open)
  */
-static struct file *pick_file(struct files_struct *files, unsigned fd)
+struct file *file_close_fd_locked(struct files_struct *files, unsigned fd)
 {
 	struct fdtable *fdt = files_fdtable(files);
 	struct file *file;
 
+	lockdep_assert_held(&files->file_lock);
+
 	if (fd >= fdt->max_fds)
 		return NULL;
 
@@ -660,7 +664,7 @@ int close_fd(unsigned fd)
 	struct file *file;
 
 	spin_lock(&files->file_lock);
-	file = pick_file(files, fd);
+	file = file_close_fd_locked(files, fd);
 	spin_unlock(&files->file_lock);
 	if (!file)
 		return -EBADF;
@@ -707,7 +711,7 @@ static inline void __range_close(struct files_struct *files, unsigned int fd,
 	max_fd = min(max_fd, n);
 
 	for (; fd <= max_fd; fd++) {
-		file = pick_file(files, fd);
+		file = file_close_fd_locked(files, fd);
 		if (file) {
 			spin_unlock(&files->file_lock);
 			filp_close(file, files);
@@ -795,15 +799,6 @@ int __close_range(unsigned fd, unsigned max_fd, unsigned int flags)
 	return 0;
 }
 
-/*
- * See file_close_fd() below, this variant assumes current->files->file_lock
- * is held.
- */
-struct file *__close_fd_get_file(unsigned int fd)
-{
-	return pick_file(current->files, fd);
-}
-
 /**
  * file_close_fd - return file associated with fd
  * @fd: file descriptor to retrieve file for
@@ -818,7 +813,7 @@ struct file *file_close_fd(unsigned int fd)
 	struct file *file;
 
 	spin_lock(&files->file_lock);
-	file = pick_file(files, fd);
+	file = file_close_fd_locked(files, fd);
 	spin_unlock(&files->file_lock);
 
 	return file;
diff --git a/fs/internal.h b/fs/internal.h
index 273e6fd40d1b..a7469ddba9b6 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -179,7 +179,7 @@ extern struct file *do_file_open_root(const struct path *,
 		const char *, const struct open_flags *);
 extern struct open_how build_open_how(int flags, umode_t mode);
 extern int build_open_flags(const struct open_how *how, struct open_flags *op);
-extern struct file *__close_fd_get_file(unsigned int fd);
+struct file *file_close_fd_locked(struct files_struct *files, unsigned fd);
 
 long do_sys_ftruncate(unsigned int fd, loff_t length, int small);
 int chmod_common(const struct path *path, umode_t mode);
diff --git a/io_uring/openclose.c b/io_uring/openclose.c
index fb73adb89067..74fc22461f48 100644
--- a/io_uring/openclose.c
+++ b/io_uring/openclose.c
@@ -241,7 +241,7 @@ int io_close(struct io_kiocb *req, unsigned int issue_flags)
 		return -EAGAIN;
 	}
 
-	file = __close_fd_get_file(close->fd);
+	file = file_close_fd_locked(files, close->fd);
 	spin_unlock(&files->file_lock);
 	if (!file)
 		goto err;

-- 
2.42.0

[PATCH RFC 3/5] fs: replace f_rcuhead with f_tw

[Christian Brauner]

The naming is actively misleading since we switched to
SLAB_TYPESAFE_BY_RCU. rcu_head is #define callback_head. Use
callback_head directly and rename f_rcuhead to f_tw.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/file_table.c    | 6 +++---
 include/linux/fs.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index 6deac386486d..78614204ef2c 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -407,7 +407,7 @@ static void delayed_fput(struct work_struct *unused)
 
 static void ____fput(struct callback_head *work)
 {
-	__fput(container_of(work, struct file, f_rcuhead));
+	__fput(container_of(work, struct file, f_tw));
 }
 
 /*
@@ -438,8 +438,8 @@ void fput(struct file *file)
 			return;
 		}
 		if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) {
-			init_task_work(&file->f_rcuhead, ____fput);
-			if (!task_work_add(task, &file->f_rcuhead, TWA_RESUME))
+			init_task_work(&file->f_tw, ____fput);
+			if (!task_work_add(task, &file->f_tw, TWA_RESUME))
 				return;
 			/*
 			 * After this task has run exit_task_work(),
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f171505940ff..d23a886df8fa 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -992,7 +992,7 @@ static inline int ra_has_index(struct file_ra_state *ra, pgoff_t index)
 struct file {
 	union {
 		struct llist_node	f_llist;
-		struct rcu_head 	f_rcuhead;
+		struct callback_head 	f_tw;
 		unsigned int 		f_iocb_flags;
 	};
 

-- 
2.42.0

[PATCH RFC 4/5] file: stop exposing receive_fd_user()

[Christian Brauner]

Not every subsystem needs to have their own specialized helper.
Just us the __receive_fd() helper.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 include/linux/file.h | 7 -------
 include/net/scm.h    | 9 +++++++++
 net/compat.c         | 2 +-
 net/core/scm.c       | 2 +-
 4 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/include/linux/file.h b/include/linux/file.h
index 6e9099d29343..c0d5219c2852 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -101,13 +101,6 @@ extern int __receive_fd(struct file *file, int __user *ufd,
 
 extern int receive_fd(struct file *file, unsigned int o_flags);
 
-static inline int receive_fd_user(struct file *file, int __user *ufd,
-				  unsigned int o_flags)
-{
-	if (ufd == NULL)
-		return -EFAULT;
-	return __receive_fd(file, ufd, o_flags);
-}
 int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags);
 
 extern void flush_delayed_fput(void);
diff --git a/include/net/scm.h b/include/net/scm.h
index e8c76b4be2fe..8aae2468bae0 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -5,6 +5,7 @@
 #include <linux/limits.h>
 #include <linux/net.h>
 #include <linux/cred.h>
+#include <linux/file.h>
 #include <linux/security.h>
 #include <linux/pid.h>
 #include <linux/nsproxy.h>
@@ -208,5 +209,13 @@ static inline void scm_recv_unix(struct socket *sock, struct msghdr *msg,
 	scm_destroy_cred(scm);
 }
 
+static inline int scm_recv_one_fd(struct file *f, int __user *ufd,
+				  unsigned int flags)
+{
+	if (!ufd)
+		return -EFAULT;
+	return __receive_fd(f, ufd, flags);
+}
+
 #endif /* __LINUX_NET_SCM_H */
 
diff --git a/net/compat.c b/net/compat.c
index 6564720f32b7..485db8ee9b28 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -297,7 +297,7 @@ void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm)
 	int err = 0, i;
 
 	for (i = 0; i < fdmax; i++) {
-		err = receive_fd_user(scm->fp->fp[i], cmsg_data + i, o_flags);
+		err = scm_recv_one_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
 		if (err < 0)
 			break;
 	}
diff --git a/net/core/scm.c b/net/core/scm.c
index 880027ecf516..eec78e312550 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -319,7 +319,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
 	}
 
 	for (i = 0; i < fdmax; i++) {
-		err = receive_fd_user(scm->fp->fp[i], cmsg_data + i, o_flags);
+		err = scm_recv_one_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
 		if (err < 0)
 			break;
 	}

-- 
2.42.0

[PATCH RFC 5/5] file: remove __receive_fd()

[Christian Brauner]

Honestly, there's little value in having a helper with and without that
int __user *ufd argument. It's just messy and doesn't really give us
anything. Just expose receive_fd() with that argument and get rid of
that helper.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 drivers/vdpa/vdpa_user/vduse_dev.c |  2 +-
 fs/file.c                          | 11 +++--------
 include/linux/file.h               |  5 +----
 include/net/scm.h                  |  2 +-
 kernel/pid.c                       |  2 +-
 kernel/seccomp.c                   |  2 +-
 6 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 6cb5ce4a8b9a..1d24da79c399 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1157,7 +1157,7 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 			fput(f);
 			break;
 		}
-		ret = receive_fd(f, perm_to_file_flags(entry.perm));
+		ret = receive_fd(f, NULL, perm_to_file_flags(entry.perm));
 		fput(f);
 		break;
 	}
diff --git a/fs/file.c b/fs/file.c
index c8eaa0b29a08..3b683b9101d8 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1296,7 +1296,7 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
 }
 
 /**
- * __receive_fd() - Install received file into file descriptor table
+ * receive_fd() - Install received file into file descriptor table
  * @file: struct file that was received from another process
  * @ufd: __user pointer to write new fd number to
  * @o_flags: the O_* flags to apply to the new fd entry
@@ -1310,7 +1310,7 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
  *
  * Returns newly install fd or -ve on error.
  */
-int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)
+int receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)
 {
 	int new_fd;
 	int error;
@@ -1335,6 +1335,7 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)
 	__receive_sock(file);
 	return new_fd;
 }
+EXPORT_SYMBOL_GPL(receive_fd);
 
 int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags)
 {
@@ -1350,12 +1351,6 @@ int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags)
 	return new_fd;
 }
 
-int receive_fd(struct file *file, unsigned int o_flags)
-{
-	return __receive_fd(file, NULL, o_flags);
-}
-EXPORT_SYMBOL_GPL(receive_fd);
-
 static int ksys_dup3(unsigned int oldfd, unsigned int newfd, int flags)
 {
 	int err = -EBADF;
diff --git a/include/linux/file.h b/include/linux/file.h
index c0d5219c2852..a50545ef1197 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -96,10 +96,7 @@ DEFINE_CLASS(get_unused_fd, int, if (_T >= 0) put_unused_fd(_T),
 
 extern void fd_install(unsigned int fd, struct file *file);
 
-extern int __receive_fd(struct file *file, int __user *ufd,
-			unsigned int o_flags);
-
-extern int receive_fd(struct file *file, unsigned int o_flags);
+extern int receive_fd(struct file *file, int __user *ufd, unsigned int o_flags);
 
 int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags);
 
diff --git a/include/net/scm.h b/include/net/scm.h
index 8aae2468bae0..cf68acec4d70 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -214,7 +214,7 @@ static inline int scm_recv_one_fd(struct file *f, int __user *ufd,
 {
 	if (!ufd)
 		return -EFAULT;
-	return __receive_fd(f, ufd, flags);
+	return receive_fd(f, ufd, flags);
 }
 
 #endif /* __LINUX_NET_SCM_H */
diff --git a/kernel/pid.c b/kernel/pid.c
index 6500ef956f2f..b52b10865454 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -700,7 +700,7 @@ static int pidfd_getfd(struct pid *pid, int fd)
 	if (IS_ERR(file))
 		return PTR_ERR(file);
 
-	ret = receive_fd(file, O_CLOEXEC);
+	ret = receive_fd(file, NULL, O_CLOEXEC);
 	fput(file);
 
 	return ret;
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 255999ba9190..aca7b437882e 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1072,7 +1072,7 @@ static void seccomp_handle_addfd(struct seccomp_kaddfd *addfd, struct seccomp_kn
 	 */
 	list_del_init(&addfd->list);
 	if (!addfd->setfd)
-		fd = receive_fd(addfd->file, addfd->flags);
+		fd = receive_fd(addfd->file, NULL, addfd->flags);
 	else
 		fd = receive_fd_replace(addfd->fd, addfd->file, addfd->flags);
 	addfd->ret = fd;

-- 
2.42.0

Re: [PATCH RFC 5/5] file: remove __receive_fd()

[Matthew Wilcox]

On Thu, Nov 30, 2023 at 01:49:11PM +0100, Christian Brauner wrote:
> +++ b/include/linux/file.h
> @@ -96,10 +96,7 @@ DEFINE_CLASS(get_unused_fd, int, if (_T >= 0) put_unused_fd(_T),
>  
>  extern void fd_install(unsigned int fd, struct file *file);
>  
> -extern int __receive_fd(struct file *file, int __user *ufd,
> -			unsigned int o_flags);
> -
> -extern int receive_fd(struct file *file, unsigned int o_flags);
> +extern int receive_fd(struct file *file, int __user *ufd, unsigned int o_flags);

You could drop the 'extern' while you're at it.

Re: [PATCH RFC 2/5] file: remove pointless wrapper

[Jens Axboe]

On 11/30/23 5:49 AM, Christian Brauner wrote:
> Only io_uring uses __close_fd_get_file(). All it does is hide
> current->files but io_uring accesses files_struct directly right now
> anyway so it's a bit pointless. Just rename pick_file() to
> file_close_fd_locked() and let io_uring use it. Add a lockdep assert in
> there that we expect the caller to hold file_lock while we're at it.

LGTM.

Reviewed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe

Re: [PATCH RFC 4/5] file: stop exposing receive_fd_user()

[Jens Axboe]

On 11/30/23 5:49 AM, Christian Brauner wrote:
> Not every subsystem needs to have their own specialized helper.
> Just us the __receive_fd() helper.

s/us/use

-- 
Jens Axboe

Re: [PATCH RFC 0/5] file: minor fixes

[Jens Axboe]

On 11/30/23 5:49 AM, Christian Brauner wrote:
> * reduce number of helpers
> * rename close_fd_get_file() helprs to reflect the fact that they don't
>   take a refcount
> * rename rcu_head struct back to callback_head now that we only use it
>   for task work and not rcu anymore

Series looks good to me:

Reviewed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe

Re: [PATCH RFC 1/5] file: s/close_fd_get_file()/file_close_fd()/g

[Jan Kara]

On Thu 30-11-23 13:49:07, Christian Brauner wrote:
> That really shouldn't have "get" in there as that implies we're bumping
> the reference count which we don't do at all. We used to but not anmore.
> Now we're just closing the fd and pick that file from the fdtable
> without bumping the reference count. Update the wrong documentation
> while at it.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  drivers/android/binder.c |  2 +-
>  fs/file.c                | 14 +++++++++-----
>  fs/open.c                |  2 +-
>  include/linux/fdtable.h  |  2 +-
>  4 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 92128aae2d06..7658103ba760 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -1921,7 +1921,7 @@ static void binder_deferred_fd_close(int fd)
>  	if (!twcb)
>  		return;
>  	init_task_work(&twcb->twork, binder_do_fd_close);
> -	twcb->file = close_fd_get_file(fd);
> +	twcb->file = file_close_fd(fd);
>  	if (twcb->file) {
>  		// pin it until binder_do_fd_close(); see comments there
>  		get_file(twcb->file);
> diff --git a/fs/file.c b/fs/file.c
> index 50df31e104a5..66f04442a384 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -796,7 +796,7 @@ int __close_range(unsigned fd, unsigned max_fd, unsigned int flags)
>  }
>  
>  /*
> - * See close_fd_get_file() below, this variant assumes current->files->file_lock
> + * See file_close_fd() below, this variant assumes current->files->file_lock
>   * is held.
>   */
>  struct file *__close_fd_get_file(unsigned int fd)
> @@ -804,11 +804,15 @@ struct file *__close_fd_get_file(unsigned int fd)
>  	return pick_file(current->files, fd);
>  }
>  
> -/*
> - * variant of close_fd that gets a ref on the file for later fput.
> - * The caller must ensure that filp_close() called on the file.
> +/**
> + * file_close_fd - return file associated with fd
> + * @fd: file descriptor to retrieve file for
> + *
> + * Doesn't take a separate reference count.
> + *
> + * Returns: The file associated with @fd (NULL if @fd is not open)
>   */
> -struct file *close_fd_get_file(unsigned int fd)
> +struct file *file_close_fd(unsigned int fd)
>  {
>  	struct files_struct *files = current->files;
>  	struct file *file;
> diff --git a/fs/open.c b/fs/open.c
> index 0bd7fce21cbf..328dc6ef1883 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -1578,7 +1578,7 @@ SYSCALL_DEFINE1(close, unsigned int, fd)
>  	int retval;
>  	struct file *file;
>  
> -	file = close_fd_get_file(fd);
> +	file = file_close_fd(fd);
>  	if (!file)
>  		return -EBADF;
>  
> diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
> index 80bd7789bab1..78c8326d74ae 100644
> --- a/include/linux/fdtable.h
> +++ b/include/linux/fdtable.h
> @@ -119,7 +119,7 @@ int iterate_fd(struct files_struct *, unsigned,
>  
>  extern int close_fd(unsigned int fd);
>  extern int __close_range(unsigned int fd, unsigned int max_fd, unsigned int flags);
> -extern struct file *close_fd_get_file(unsigned int fd);
> +extern struct file *file_close_fd(unsigned int fd);
>  extern int unshare_fd(unsigned long unshare_flags, unsigned int max_fds,
>  		      struct files_struct **new_fdp);
>  
> 
> -- 
> 2.42.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RFC 2/5] file: remove pointless wrapper

[Jan Kara]

On Thu 30-11-23 13:49:08, Christian Brauner wrote:
> Only io_uring uses __close_fd_get_file(). All it does is hide
> current->files but io_uring accesses files_struct directly right now
> anyway so it's a bit pointless. Just rename pick_file() to
> file_close_fd_locked() and let io_uring use it. Add a lockdep assert in
> there that we expect the caller to hold file_lock while we're at it.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/file.c            | 23 +++++++++--------------
>  fs/internal.h        |  2 +-
>  io_uring/openclose.c |  2 +-
>  3 files changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 66f04442a384..c8eaa0b29a08 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -629,19 +629,23 @@ void fd_install(unsigned int fd, struct file *file)
>  EXPORT_SYMBOL(fd_install);
>  
>  /**
> - * pick_file - return file associatd with fd
> + * file_close_fd_locked - return file associated with fd
>   * @files: file struct to retrieve file from
>   * @fd: file descriptor to retrieve file for
>   *
> + * Doesn't take a separate reference count.
> + *
>   * Context: files_lock must be held.
>   *
>   * Returns: The file associated with @fd (NULL if @fd is not open)
>   */
> -static struct file *pick_file(struct files_struct *files, unsigned fd)
> +struct file *file_close_fd_locked(struct files_struct *files, unsigned fd)
>  {
>  	struct fdtable *fdt = files_fdtable(files);
>  	struct file *file;
>  
> +	lockdep_assert_held(&files->file_lock);
> +
>  	if (fd >= fdt->max_fds)
>  		return NULL;
>  
> @@ -660,7 +664,7 @@ int close_fd(unsigned fd)
>  	struct file *file;
>  
>  	spin_lock(&files->file_lock);
> -	file = pick_file(files, fd);
> +	file = file_close_fd_locked(files, fd);
>  	spin_unlock(&files->file_lock);
>  	if (!file)
>  		return -EBADF;
> @@ -707,7 +711,7 @@ static inline void __range_close(struct files_struct *files, unsigned int fd,
>  	max_fd = min(max_fd, n);
>  
>  	for (; fd <= max_fd; fd++) {
> -		file = pick_file(files, fd);
> +		file = file_close_fd_locked(files, fd);
>  		if (file) {
>  			spin_unlock(&files->file_lock);
>  			filp_close(file, files);
> @@ -795,15 +799,6 @@ int __close_range(unsigned fd, unsigned max_fd, unsigned int flags)
>  	return 0;
>  }
>  
> -/*
> - * See file_close_fd() below, this variant assumes current->files->file_lock
> - * is held.
> - */
> -struct file *__close_fd_get_file(unsigned int fd)
> -{
> -	return pick_file(current->files, fd);
> -}
> -
>  /**
>   * file_close_fd - return file associated with fd
>   * @fd: file descriptor to retrieve file for
> @@ -818,7 +813,7 @@ struct file *file_close_fd(unsigned int fd)
>  	struct file *file;
>  
>  	spin_lock(&files->file_lock);
> -	file = pick_file(files, fd);
> +	file = file_close_fd_locked(files, fd);
>  	spin_unlock(&files->file_lock);
>  
>  	return file;
> diff --git a/fs/internal.h b/fs/internal.h
> index 273e6fd40d1b..a7469ddba9b6 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -179,7 +179,7 @@ extern struct file *do_file_open_root(const struct path *,
>  		const char *, const struct open_flags *);
>  extern struct open_how build_open_how(int flags, umode_t mode);
>  extern int build_open_flags(const struct open_how *how, struct open_flags *op);
> -extern struct file *__close_fd_get_file(unsigned int fd);
> +struct file *file_close_fd_locked(struct files_struct *files, unsigned fd);
>  
>  long do_sys_ftruncate(unsigned int fd, loff_t length, int small);
>  int chmod_common(const struct path *path, umode_t mode);
> diff --git a/io_uring/openclose.c b/io_uring/openclose.c
> index fb73adb89067..74fc22461f48 100644
> --- a/io_uring/openclose.c
> +++ b/io_uring/openclose.c
> @@ -241,7 +241,7 @@ int io_close(struct io_kiocb *req, unsigned int issue_flags)
>  		return -EAGAIN;
>  	}
>  
> -	file = __close_fd_get_file(close->fd);
> +	file = file_close_fd_locked(files, close->fd);
>  	spin_unlock(&files->file_lock);
>  	if (!file)
>  		goto err;
> 
> -- 
> 2.42.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RFC 3/5] fs: replace f_rcuhead with f_tw

[Jan Kara]

On Thu 30-11-23 13:49:09, Christian Brauner wrote:
> The naming is actively misleading since we switched to
> SLAB_TYPESAFE_BY_RCU. rcu_head is #define callback_head. Use
> callback_head directly and rename f_rcuhead to f_tw.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Nice. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/file_table.c    | 6 +++---
>  include/linux/fs.h | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 6deac386486d..78614204ef2c 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -407,7 +407,7 @@ static void delayed_fput(struct work_struct *unused)
>  
>  static void ____fput(struct callback_head *work)
>  {
> -	__fput(container_of(work, struct file, f_rcuhead));
> +	__fput(container_of(work, struct file, f_tw));
>  }
>  
>  /*
> @@ -438,8 +438,8 @@ void fput(struct file *file)
>  			return;
>  		}
>  		if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) {
> -			init_task_work(&file->f_rcuhead, ____fput);
> -			if (!task_work_add(task, &file->f_rcuhead, TWA_RESUME))
> +			init_task_work(&file->f_tw, ____fput);
> +			if (!task_work_add(task, &file->f_tw, TWA_RESUME))
>  				return;
>  			/*
>  			 * After this task has run exit_task_work(),
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index f171505940ff..d23a886df8fa 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -992,7 +992,7 @@ static inline int ra_has_index(struct file_ra_state *ra, pgoff_t index)
>  struct file {
>  	union {
>  		struct llist_node	f_llist;
> -		struct rcu_head 	f_rcuhead;
> +		struct callback_head 	f_tw;
>  		unsigned int 		f_iocb_flags;
>  	};
>  
> 
> -- 
> 2.42.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RFC 4/5] file: stop exposing receive_fd_user()

[Jan Kara]

On Thu 30-11-23 13:49:10, Christian Brauner wrote:
> Not every subsystem needs to have their own specialized helper.
> Just us the __receive_fd() helper.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Modulo the typo Jens pointed out, feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/linux/file.h | 7 -------
>  include/net/scm.h    | 9 +++++++++
>  net/compat.c         | 2 +-
>  net/core/scm.c       | 2 +-
>  4 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/file.h b/include/linux/file.h
> index 6e9099d29343..c0d5219c2852 100644
> --- a/include/linux/file.h
> +++ b/include/linux/file.h
> @@ -101,13 +101,6 @@ extern int __receive_fd(struct file *file, int __user *ufd,
>  
>  extern int receive_fd(struct file *file, unsigned int o_flags);
>  
> -static inline int receive_fd_user(struct file *file, int __user *ufd,
> -				  unsigned int o_flags)
> -{
> -	if (ufd == NULL)
> -		return -EFAULT;
> -	return __receive_fd(file, ufd, o_flags);
> -}
>  int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags);
>  
>  extern void flush_delayed_fput(void);
> diff --git a/include/net/scm.h b/include/net/scm.h
> index e8c76b4be2fe..8aae2468bae0 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -5,6 +5,7 @@
>  #include <linux/limits.h>
>  #include <linux/net.h>
>  #include <linux/cred.h>
> +#include <linux/file.h>
>  #include <linux/security.h>
>  #include <linux/pid.h>
>  #include <linux/nsproxy.h>
> @@ -208,5 +209,13 @@ static inline void scm_recv_unix(struct socket *sock, struct msghdr *msg,
>  	scm_destroy_cred(scm);
>  }
>  
> +static inline int scm_recv_one_fd(struct file *f, int __user *ufd,
> +				  unsigned int flags)
> +{
> +	if (!ufd)
> +		return -EFAULT;
> +	return __receive_fd(f, ufd, flags);
> +}
> +
>  #endif /* __LINUX_NET_SCM_H */
>  
> diff --git a/net/compat.c b/net/compat.c
> index 6564720f32b7..485db8ee9b28 100644
> --- a/net/compat.c
> +++ b/net/compat.c
> @@ -297,7 +297,7 @@ void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm)
>  	int err = 0, i;
>  
>  	for (i = 0; i < fdmax; i++) {
> -		err = receive_fd_user(scm->fp->fp[i], cmsg_data + i, o_flags);
> +		err = scm_recv_one_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
>  		if (err < 0)
>  			break;
>  	}
> diff --git a/net/core/scm.c b/net/core/scm.c
> index 880027ecf516..eec78e312550 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -319,7 +319,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
>  	}
>  
>  	for (i = 0; i < fdmax; i++) {
> -		err = receive_fd_user(scm->fp->fp[i], cmsg_data + i, o_flags);
> +		err = scm_recv_one_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
>  		if (err < 0)
>  			break;
>  	}
> 
> -- 
> 2.42.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RFC 5/5] file: remove __receive_fd()

[Jan Kara]

On Thu 30-11-23 13:49:11, Christian Brauner wrote:
> Honestly, there's little value in having a helper with and without that
> int __user *ufd argument. It's just messy and doesn't really give us
> anything. Just expose receive_fd() with that argument and get rid of
> that helper.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c |  2 +-
>  fs/file.c                          | 11 +++--------
>  include/linux/file.h               |  5 +----
>  include/net/scm.h                  |  2 +-
>  kernel/pid.c                       |  2 +-
>  kernel/seccomp.c                   |  2 +-
>  6 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 6cb5ce4a8b9a..1d24da79c399 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1157,7 +1157,7 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>  			fput(f);
>  			break;
>  		}
> -		ret = receive_fd(f, perm_to_file_flags(entry.perm));
> +		ret = receive_fd(f, NULL, perm_to_file_flags(entry.perm));
>  		fput(f);
>  		break;
>  	}
> diff --git a/fs/file.c b/fs/file.c
> index c8eaa0b29a08..3b683b9101d8 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -1296,7 +1296,7 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
>  }
>  
>  /**
> - * __receive_fd() - Install received file into file descriptor table
> + * receive_fd() - Install received file into file descriptor table
>   * @file: struct file that was received from another process
>   * @ufd: __user pointer to write new fd number to
>   * @o_flags: the O_* flags to apply to the new fd entry
> @@ -1310,7 +1310,7 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
>   *
>   * Returns newly install fd or -ve on error.
>   */
> -int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)
> +int receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)
>  {
>  	int new_fd;
>  	int error;
> @@ -1335,6 +1335,7 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)
>  	__receive_sock(file);
>  	return new_fd;
>  }
> +EXPORT_SYMBOL_GPL(receive_fd);
>  
>  int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags)
>  {
> @@ -1350,12 +1351,6 @@ int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags)
>  	return new_fd;
>  }
>  
> -int receive_fd(struct file *file, unsigned int o_flags)
> -{
> -	return __receive_fd(file, NULL, o_flags);
> -}
> -EXPORT_SYMBOL_GPL(receive_fd);
> -
>  static int ksys_dup3(unsigned int oldfd, unsigned int newfd, int flags)
>  {
>  	int err = -EBADF;
> diff --git a/include/linux/file.h b/include/linux/file.h
> index c0d5219c2852..a50545ef1197 100644
> --- a/include/linux/file.h
> +++ b/include/linux/file.h
> @@ -96,10 +96,7 @@ DEFINE_CLASS(get_unused_fd, int, if (_T >= 0) put_unused_fd(_T),
>  
>  extern void fd_install(unsigned int fd, struct file *file);
>  
> -extern int __receive_fd(struct file *file, int __user *ufd,
> -			unsigned int o_flags);
> -
> -extern int receive_fd(struct file *file, unsigned int o_flags);
> +extern int receive_fd(struct file *file, int __user *ufd, unsigned int o_flags);
>  
>  int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags);
>  
> diff --git a/include/net/scm.h b/include/net/scm.h
> index 8aae2468bae0..cf68acec4d70 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -214,7 +214,7 @@ static inline int scm_recv_one_fd(struct file *f, int __user *ufd,
>  {
>  	if (!ufd)
>  		return -EFAULT;
> -	return __receive_fd(f, ufd, flags);
> +	return receive_fd(f, ufd, flags);
>  }
>  
>  #endif /* __LINUX_NET_SCM_H */
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 6500ef956f2f..b52b10865454 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -700,7 +700,7 @@ static int pidfd_getfd(struct pid *pid, int fd)
>  	if (IS_ERR(file))
>  		return PTR_ERR(file);
>  
> -	ret = receive_fd(file, O_CLOEXEC);
> +	ret = receive_fd(file, NULL, O_CLOEXEC);
>  	fput(file);
>  
>  	return ret;
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 255999ba9190..aca7b437882e 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1072,7 +1072,7 @@ static void seccomp_handle_addfd(struct seccomp_kaddfd *addfd, struct seccomp_kn
>  	 */
>  	list_del_init(&addfd->list);
>  	if (!addfd->setfd)
> -		fd = receive_fd(addfd->file, addfd->flags);
> +		fd = receive_fd(addfd->file, NULL, addfd->flags);
>  	else
>  		fd = receive_fd_replace(addfd->fd, addfd->file, addfd->flags);
>  	addfd->ret = fd;
> 
> -- 
> 2.42.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RFC 3/5] fs: replace f_rcuhead with f_tw

[Linus Torvalds]

On Thu, 30 Nov 2023 at 21:49, Christian Brauner <brauner@kernel.org> wrote:
>
> The naming is actively misleading since we switched to
> SLAB_TYPESAFE_BY_RCU. rcu_head is #define callback_head. Use
> callback_head directly and rename f_rcuhead to f_tw.

Please just write it out, ie "f_taskwork" or something. Using random
letter combinations is very illegible. It may make sense at the time
(when you are literally editing a line about "init_task_work()") but
it's literally just line noise in that structure definition that makes
me go "f_tw is wtf backwards, wtf?".

It might even be a good idea to say *what* it is a task work for.

        Linus

Re: [PATCH RFC 5/5] file: remove __receive_fd()

[Christian Brauner]

> > +extern int receive_fd(struct file *file, int __user *ufd, unsigned int o_flags);
> 
> You could drop the 'extern' while you're at it.

Done.

Re: [PATCH RFC 3/5] fs: replace f_rcuhead with f_tw

[Christian Brauner]

On Fri, Dec 01, 2023 at 08:37:30AM +0900, Linus Torvalds wrote:
> On Thu, 30 Nov 2023 at 21:49, Christian Brauner <brauner@kernel.org> wrote:
> >
> > The naming is actively misleading since we switched to
> > SLAB_TYPESAFE_BY_RCU. rcu_head is #define callback_head. Use
> > callback_head directly and rename f_rcuhead to f_tw.
> 
> Please just write it out, ie "f_taskwork" or something. Using random
> letter combinations is very illegible. It may make sense at the time
> (when you are literally editing a line about "init_task_work()") but
> it's literally just line noise in that structure definition that makes
> me go "f_tw is wtf backwards, wtf?".

Oh, I didn't even think about this.

> It might even be a good idea to say *what* it is a task work for.

Ok, I've added a comment about for both f_llist and f_task_work.

Re: [PATCH RFC 0/5] file: minor fixes

[Christian Brauner]

On Thu, 30 Nov 2023 13:49:06 +0100, Christian Brauner wrote:
> * reduce number of helpers
> * rename close_fd_get_file() helprs to reflect the fact that they don't
>   take a refcount
> * rename rcu_head struct back to callback_head now that we only use it
>   for task work and not rcu anymore
> 

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc 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.misc

[1/5] file: s/close_fd_get_file()/file_close_fd()/g
      https://git.kernel.org/vfs/vfs/c/3c66314bc279
[2/5] file: remove pointless wrapper
      https://git.kernel.org/vfs/vfs/c/316e9855e905
[3/5] fs: replace f_rcuhead with f_tw
      https://git.kernel.org/vfs/vfs/c/a057c426045d
[4/5] file: stop exposing receive_fd_user()
      https://git.kernel.org/vfs/vfs/c/64d002ef7cbf
[5/5] file: remove __receive_fd()
      https://git.kernel.org/vfs/vfs/c/e333c8549ed7