On Thu 28-11-24 15:25:32, Amir Goldstein wrote:
> Commit 2a010c412853 ("fs: don't block i_writecount during exec") removed
> the legacy behavior of getting ETXTBSY on attempt to open and executable
> file for write while it is being executed.
>
> This commit was reverted because an application that depends on this
> legacy behavior was broken by the change.
>
> We need to allow HSM writing into executable files while executed to
> fill their content on-the-fly.
>
> To that end, disable the ETXTBSY legacy behavior for files that are
> watched by pre-content events.
>
> This change is not expected to cause regressions with existing systems
> which do not have any pre-content event listeners.
>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
>
> Jan,
>
> This patch is on top of your fsnotify_hsm rebased branch.
> It passed LTP sanity tests, but did not test filling an executable
> on-the-fly.
Just to be clear: I'm fine with this approach but I'd like to see this
tested and get Christian's ack before pushing the patch into my tree
(and then into linux-next because at this point this is the only
outstanding problem I'm aware of).
Honza
> fs/binfmt_elf.c | 4 ++--
> fs/binfmt_elf_fdpic.c | 4 ++--
> fs/exec.c | 8 ++++----
> include/linux/fs.h | 17 +++++++++++++++++
> kernel/fork.c | 12 ++++++------
> 5 files changed, 31 insertions(+), 14 deletions(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 106f0e8af177..8054f44d39cf 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1257,7 +1257,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
> }
> reloc_func_desc = interp_load_addr;
>
> - allow_write_access(interpreter);
> + exe_file_allow_write_access(interpreter);
> fput(interpreter);
>
> kfree(interp_elf_ex);
> @@ -1354,7 +1354,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
> kfree(interp_elf_ex);
> kfree(interp_elf_phdata);
> out_free_file:
> - allow_write_access(interpreter);
> + exe_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 f1a7c4875c4a..c13ee8180b17 100644
> --- a/fs/binfmt_elf_fdpic.c
> +++ b/fs/binfmt_elf_fdpic.c
> @@ -394,7 +394,7 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
> goto error;
> }
>
> - allow_write_access(interpreter);
> + exe_file_allow_write_access(interpreter);
> fput(interpreter);
> interpreter = NULL;
> }
> @@ -467,7 +467,7 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
>
> error:
> if (interpreter) {
> - allow_write_access(interpreter);
> + exe_file_allow_write_access(interpreter);
> fput(interpreter);
> }
> kfree(interpreter_name);
> diff --git a/fs/exec.c b/fs/exec.c
> index 98cb7ba9983c..c41cfd35c74c 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -912,7 +912,7 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
> path_noexec(&file->f_path))
> return ERR_PTR(-EACCES);
>
> - err = deny_write_access(file);
> + err = exe_file_deny_write_access(file);
> if (err)
> return ERR_PTR(err);
>
> @@ -927,7 +927,7 @@ 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(), callers
> - * must call allow_write_access() before fput() on release. Also see
> + * must call exe_file_allow_write_access() before fput() on release. Also see
> * do_close_execat().
> */
> struct file *open_exec(const char *name)
> @@ -1471,7 +1471,7 @@ static void do_close_execat(struct file *file)
> {
> if (!file)
> return;
> - allow_write_access(file);
> + exe_file_allow_write_access(file);
> fput(file);
> }
>
> @@ -1797,7 +1797,7 @@ static int exec_binprm(struct linux_binprm *bprm)
> bprm->file = bprm->interpreter;
> bprm->interpreter = NULL;
>
> - allow_write_access(exec);
> + exe_file_allow_write_access(exec);
> if (unlikely(bprm->have_execfd)) {
> if (bprm->executable) {
> fput(exec);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index dd6d0eddea9b..2aeab643f1ab 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3089,6 +3089,23 @@ static inline void allow_write_access(struct file *file)
> if (file)
> atomic_inc(&file_inode(file)->i_writecount);
> }
> +
> +/*
> + * Do not prevent write to executable file when watched by pre-content events.
> + */
> +static inline int exe_file_deny_write_access(struct file *exe_file)
> +{
> + if (unlikely(FMODE_FSNOTIFY_HSM(exe_file->f_mode)))
> + return 0;
> + return deny_write_access(exe_file);
> +}
> +static inline void exe_file_allow_write_access(struct file *exe_file)
> +{
> + if (unlikely(FMODE_FSNOTIFY_HSM(exe_file->f_mode)))
> + return;
> + allow_write_access(exe_file);
> +}
> +
> static inline bool inode_is_open_for_write(const struct inode *inode)
> {
> return atomic_read(&inode->i_writecount) > 0;
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 1450b461d196..015c397f47ca 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -625,8 +625,8 @@ static void dup_mm_exe_file(struct mm_struct *mm, struct mm_struct *oldmm)
> * 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__);
> + if (exe_file && exe_file_deny_write_access(exe_file))
> + pr_warn_once("exe_file_deny_write_access() failed in %s\n", __func__);
> }
>
> #ifdef CONFIG_MMU
> @@ -1424,13 +1424,13 @@ int set_mm_exe_file(struct mm_struct *mm, struct file *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)))
> + if (unlikely(exe_file_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) {
> - allow_write_access(old_exe_file);
> + exe_file_allow_write_access(old_exe_file);
> fput(old_exe_file);
> }
> return 0;
> @@ -1471,7 +1471,7 @@ int replace_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file)
> return ret;
> }
>
> - ret = deny_write_access(new_exe_file);
> + ret = exe_file_deny_write_access(new_exe_file);
> if (ret)
> return -EACCES;
> get_file(new_exe_file);
> @@ -1483,7 +1483,7 @@ int replace_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file)
> mmap_write_unlock(mm);
>
> if (old_exe_file) {
> - allow_write_access(old_exe_file);
> + exe_file_allow_write_access(old_exe_file);
> fput(old_exe_file);
> }
> return 0;
> --
> 2.34.1
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
