Revert "fs: don't block i_writecount during exec"

This reverts commit 2a010c41285345da60cece35575b4e0af7e7bf44.

Rui Ueyama <[email protected]> writes:

> I'm the creator and the maintainer of the mold linker
> (https://github.com/rui314/mold). Recently, we discovered that mold
> started causing process crashes in certain situations due to a change
> in the Linux kernel. Here are the details:
>
> - In general, overwriting an existing file is much faster than
> creating an empty file and writing to it on Linux, so mold attempts to
> reuse an existing executable file if it exists.
>
> - If a program is running, opening the executable file for writing
> previously failed with ETXTBSY. If that happens, mold falls back to
> creating a new file.
>
> - However, the Linux kernel recently changed the behavior so that
> writing to an executable file is now always permitted
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2a010c412853).
>
> That caused mold to write to an executable file even if there's a
> process running that file. Since changes to mmap'ed files are
> immediately visible to other processes, any processes running that
> file would almost certainly crash in a very mysterious way.
> Identifying the cause of these random crashes took us a few days.
>
> Rejecting writes to an executable file that is currently running is a
> well-known behavior, and Linux had operated that way for a very long
> time. So, I don’t believe relying on this behavior was our mistake;
> rather, I see this as a regression in the Linux kernel.

Quoting myself from commit 2a010c412853 ("fs: don't block i_writecount during exec")

> Yes, someone in userspace could potentially be relying on this. It's not
> completely out of the realm of possibility but let's find out if that's
> actually the case and not guess.

It seems we found out that someone is relying on this obscure behavior.
So revert the change.

Link: https://github.com/rui314/mold/issues/1361
Link: https://lore.kernel.org/r/[email protected]
Cc: <[email protected]>
Signed-off-by: Christian Brauner <[email protected]>

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 3039a6b7aba4bd38f26e21b626b579cc03f3a03e..106f0e8af17799ffc54a673e77e60477a05c34dd 100644 (file)
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1257,6 +1257,7 @@ out_free_interp:
                }
                reloc_func_desc = interp_load_addr;
 
+               allow_write_access(interpreter);
                fput(interpreter);
 
                kfree(interp_elf_ex);
@@ -1353,6 +1354,7 @@ out_free_dentry:
        kfree(interp_elf_ex);
        kfree(interp_elf_phdata);
 out_free_file:
+       allow_write_access(interpreter);
        if (interpreter)
                fput(interpreter);
 out_free_ph:

diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index 31d253bd3961a8679678c600f4346bba23502598..f1a7c4875c4aa7401f4f02c01d7a1edce4e6ab50 100644 (file)
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -394,6 +394,7 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
                        goto error;
                }
 
+               allow_write_access(interpreter);
                fput(interpreter);
                interpreter = NULL;
        }
@@ -465,8 +466,10 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
        retval = 0;
 
 error:
-       if (interpreter)
+       if (interpreter) {
+               allow_write_access(interpreter);
                fput(interpreter);
+       }
        kfree(interpreter_name);
        kfree(exec_params.phdrs);
        kfree(exec_params.loadmap);

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 31660d8cc2c610bd42f00f1de7ed6c39618cc5db..6a3a16f910516cb70f0c84e9147c5f13f8ad25a5 100644 (file)
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -247,10 +247,13 @@ static int load_misc_binary(struct linux_binprm *bprm)
        if (retval < 0)
                goto ret;
 
-       if (fmt->flags & MISC_FMT_OPEN_FILE)
+       if (fmt->flags & MISC_FMT_OPEN_FILE) {
                interp_file = file_clone_open(fmt->interp_file);
-       else
+               if (!IS_ERR(interp_file))
+                       deny_write_access(interp_file);
+       } else {
                interp_file = open_exec(fmt->interpreter);
+       }
        retval = PTR_ERR(interp_file);
        if (IS_ERR(interp_file))
                goto ret;

diff --git a/fs/exec.c b/fs/exec.c
index da51ca70489aca5ade88aa8f8ed74b94f6b3a67e..98cb7ba9983c7f55017e01a2d53185ef35d87c7c 100644 (file)
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -883,7 +883,8 @@ EXPORT_SYMBOL(transfer_args_to_stack);
  */
 static struct file *do_open_execat(int fd, struct filename *name, int flags)
 {
-       struct file *file;
+       int err;
+       struct file *file __free(fput) = NULL;
        struct open_flags open_exec_flags = {
                .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
                .acc_mode = MAY_EXEC,
@@ -908,12 +909,14 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
         * an invariant that all non-regular files error out before we get here.
         */
        if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode)) ||
-           path_noexec(&file->f_path)) {
-               fput(file);
+           path_noexec(&file->f_path))
                return ERR_PTR(-EACCES);
-       }
 
-       return file;
+       err = deny_write_access(file);
+       if (err)
+               return ERR_PTR(err);
+
+       return no_free_ptr(file);
 }
 
 /**
@@ -923,7 +926,8 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
  *
  * Returns ERR_PTR on failure or allocated struct file on success.
  *
- * As this is a wrapper for the internal do_open_execat(). Also see
+ * As this is a wrapper for the internal do_open_execat(), callers
+ * must call allow_write_access() before fput() on release. Also see
  * do_close_execat().
  */
 struct file *open_exec(const char *name)
@@ -1465,8 +1469,10 @@ static int prepare_bprm_creds(struct linux_binprm *bprm)
 /* Matches do_open_execat() */
 static void do_close_execat(struct file *file)
 {
-       if (file)
-               fput(file);
+       if (!file)
+               return;
+       allow_write_access(file);
+       fput(file);
 }
 
 static void free_bprm(struct linux_binprm *bprm)
@@ -1791,6 +1797,7 @@ static int exec_binprm(struct linux_binprm *bprm)
                bprm->file = bprm->interpreter;
                bprm->interpreter = NULL;
 
+               allow_write_access(exec);
                if (unlikely(bprm->have_execfd)) {
                        if (bprm->executable) {
                                fput(exec);

diff --git a/kernel/fork.c b/kernel/fork.c
index f253e81d0c28e8c8c0272fb8f14c969606faa2d2..1450b461d196a1efee0e120780467a96f6c7d491 100644 (file)
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -621,6 +621,12 @@ static void dup_mm_exe_file(struct mm_struct *mm, struct mm_struct *oldmm)
 
        exe_file = get_mm_exe_file(oldmm);
        RCU_INIT_POINTER(mm->exe_file, exe_file);
+       /*
+        * We depend on the oldmm having properly denied write access to the
+        * exe_file already.
+        */
+       if (exe_file && deny_write_access(exe_file))
+               pr_warn_once("deny_write_access() failed in %s\n", __func__);
 }
 
 #ifdef CONFIG_MMU
@@ -1413,11 +1419,20 @@ int set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file)
         */
        old_exe_file = rcu_dereference_raw(mm->exe_file);
 
-       if (new_exe_file)
+       if (new_exe_file) {
+               /*
+                * We expect the caller (i.e., sys_execve) to already denied
+                * write access, so this is unlikely to fail.
+                */
+               if (unlikely(deny_write_access(new_exe_file)))
+                       return -EACCES;
                get_file(new_exe_file);
+       }
        rcu_assign_pointer(mm->exe_file, new_exe_file);
-       if (old_exe_file)
+       if (old_exe_file) {
+               allow_write_access(old_exe_file);
                fput(old_exe_file);
+       }
        return 0;
 }
 
@@ -1456,6 +1471,9 @@ int replace_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file)
                        return ret;
        }
 
+       ret = deny_write_access(new_exe_file);
+       if (ret)
+               return -EACCES;
        get_file(new_exe_file);
 
        /* set the new file */
@@ -1464,8 +1482,10 @@ int replace_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file)
        rcu_assign_pointer(mm->exe_file, new_exe_file);
        mmap_write_unlock(mm);
 
-       if (old_exe_file)
+       if (old_exe_file) {
+               allow_write_access(old_exe_file);
                fput(old_exe_file);
+       }
        return 0;
 }