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

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.

While here tidy up the docs around fd_install().

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Link: https://lore.kernel.org/r/20250320102637.1924183-1-mjguzik@gmail.com
Signed-off-by: Christian Brauner <brauner@kernel.org>

diff --git a/fs/file.c b/fs/file.c
index 2d66816af1058c8c4ab2dd3d87566307986aa2db..40fed4501aab6d7b468531e334f9a3a0fb66ccdd 100644 (file)
--- a/fs/file.c
+++ b/fs/file.c
@@ -621,22 +621,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;
@@ -1249,8 +1241,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.
         */