fs: sort out fd allocation vs dup2 race commentary, take 2

Commit Message

Mateusz Guzik March 20, 2025, 10:26 a.m. UTC

fd_install() has a questionable comment above it.

While it correctly points out a possible race against dup2(), it states:
> We need to detect this and fput() the struct file we are about to
> overwrite in this case.
>
> It should never happen - if we allow dup2() do it, _really_ bad things
> will follow.

I have difficulty parsing the above. The first sentence would suggest
fd_install() tries to detect and recover from the race (it does not),
the next one claims the race needs to be dealt with (it is, by dup2()).

Given that fd_install() does not suffer the burden, this patch removes
the above and instead expands on the race in dup2() commentary instead.

While here tidy up the docs around fd_install().

Signed-off-by: Mateusz Guzik <[e-mail]>
---

I have this commit in -next (*not* in master):

commit ec052fae814d467d6aa7e591b4b24531b87e65ec
Author: Mateusz Guzik <[e-mail]>
Date:   Thu Dec 5 16:47:43 2024 +0100

    fs: sort out a stale comment about races between fd alloc and dup2

It may make sense to combine these two into one. If you want it that way
I'll submit a combined v2.

 fs/file.c | 40 ++++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 14 deletions(-)

Comments

Christian Brauner March 20, 2025, 2:19 p.m. UTC | #1

On Thu, 20 Mar 2025 11:26:37 +0100, Mateusz Guzik wrote:
> fd_install() has a questionable comment above it.
> 
> While it correctly points out a possible race against dup2(), it states:
> > We need to detect this and fput() the struct file we are about to
> > overwrite in this case.
> >
> > It should never happen - if we allow dup2() do it, _really_ bad things
> > will follow.
> 
> [...]

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

[1/1] fs: sort out fd allocation vs dup2 race commentary, take 2
      https://git.kernel.org/vfs/vfs/c/4dec4f91359c

Patch

diff --git a/fs/file.c b/fs/file.c
index 0e919bed6058..dc3f7e120e3e 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -626,22 +626,14 @@  void put_unused_fd(unsigned int fd)
 
 EXPORT_SYMBOL(put_unused_fd);
 
-/*
- * Install a file pointer in the fd array.
- *
- * The VFS is full of places where we drop the files lock between
- * setting the open_fds bitmap and installing the file in the file
- * array.  At any such point, we are vulnerable to a dup2() race
- * installing a file in the array before us.  We need to detect this and
- * fput() the struct file we are about to overwrite in this case.
- *
- * It should never happen - if we allow dup2() do it, _really_ bad things
- * will follow.
+/**
+ * fd_install - install a file pointer in the fd array
+ * @fd: file descriptor to install the file in
+ * @file: the file to install
  *
  * This consumes the "file" refcount, so callers should treat it
  * as if they had called fput(file).
  */
-
 void fd_install(unsigned int fd, struct file *file)
 {
 	struct files_struct *files = current->files;
@@ -1259,8 +1251,28 @@  __releases(&files->file_lock)
 	struct fdtable *fdt;
 
 	/*
-	 * We need to detect attempts to do dup2() over allocated but still
-	 * not finished descriptor.
+	 * dup2() is expected to close the file installed in the target fd slot
+	 * (if any). However, userspace hand-picking a fd may be racing against
+	 * its own threads which happened to allocate it in open() et al but did
+	 * not populate it yet.
+	 *
+	 * Broadly speaking we may be racing against the following:
+	 * fd = get_unused_fd_flags();     // fd slot reserved, ->fd[fd] == NULL
+	 * file = hard_work_goes_here();
+	 * fd_install(fd, file);           // only now ->fd[fd] == file
+	 *
+	 * It is an invariant that a successfully allocated fd has a NULL entry
+	 * in the array until the matching fd_install().
+	 *
+	 * If we fit the window, we have the fd to populate, yet no target file
+	 * to close. Trying to ignore it and install our new file would violate
+	 * the invariant and make fd_install() overwrite our file.
+	 *
+	 * Things can be done(tm) to handle this. However, the issue does not
+	 * concern legitimate programs and we only need to make sure the kernel
+	 * does not trip over it.
+	 *
+	 * The simplest way out is to return an error if we find ourselves here.
 	 *
 	 * POSIX is silent on the issue, we return -EBUSY.
 	 */