[PATCH] fs: don't block write during exec on pre-content watched files

[PATCH] fs: don't block write during exec on pre-content watched files

[Amir Goldstein]

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@gmail.com>
---

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.

Josef, can you verify that this works as expected.

Also pushed to fsnotify_hsm branch in my github.

Thanks,
Amir.

 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

Re: [PATCH] fs: don't block write during exec on pre-content watched files

[Mateusz Guzik]

On Thu, Nov 28, 2024 at 03:25:32PM +0100, 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.
> 
> +
> +/*
> + * 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);
> +}
> +

so this depends on FMODE_FSNOTIFY_HSM showing up on the file before any
of the above calls and staying there for its lifetime -- does that hold?

I think it would be less error prone down the road to maintain the
counters, except not return the error if HSM is on.

Re: [PATCH] fs: don't block write during exec on pre-content watched files

[Amir Goldstein]

On Thu, Nov 28, 2024 at 3:34 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> On Thu, Nov 28, 2024 at 03:25:32PM +0100, 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.
> >
> > +
> > +/*
> > + * 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);
> > +}
> > +
>
> so this depends on FMODE_FSNOTIFY_HSM showing up on the file before any
> of the above calls and staying there for its lifetime -- does that hold?

Yes!

>
> I think it would be less error prone down the road to maintain the
> counters, except not return the error if HSM is on.

Cannot.
The "deny write counter" and "writers counter" are implemented on the
same counter, so open cannot get_write_access() if we maintain the
negative deny counters from exec.

Also, note that there are still calls to {allow,deny}_write_access() that
are not bypassed even with HSM, so for example, mixing fsverity and
HSM is probably not a good idea...

Thanks,
Amir.

Re: [PATCH] fs: don't block write during exec on pre-content watched files

[Mateusz Guzik]

On Thu, Nov 28, 2024 at 5:57 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Nov 28, 2024 at 3:34 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
> >
> > On Thu, Nov 28, 2024 at 03:25:32PM +0100, 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.
> > >
> > > +
> > > +/*
> > > + * 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);
> > > +}
> > > +
> >
> > so this depends on FMODE_FSNOTIFY_HSM showing up on the file before any
> > of the above calls and staying there for its lifetime -- does that hold?
>
> Yes!
>

ok

In this case the new routines should come with a comment denoting it,
otherwise the code looks incredibly suspicious.

> >
> > I think it would be less error prone down the road to maintain the
> > counters, except not return the error if HSM is on.
>
> Cannot.
> The "deny write counter" and "writers counter" are implemented on the
> same counter, so open cannot get_write_access() if we maintain the
> negative deny counters from exec.
>

I'm aware, in the above suggestion they would have to be split. Not
great by any means but would beat the counter suddenly getting
modified if the above did not hold.

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] fs: don't block write during exec on pre-content watched files

[Amir Goldstein]

On Thu, Nov 28, 2024 at 6:00 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> On Thu, Nov 28, 2024 at 5:57 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Thu, Nov 28, 2024 at 3:34 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
> > >
> > > On Thu, Nov 28, 2024 at 03:25:32PM +0100, 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.
> > > >
> > > > +
> > > > +/*
> > > > + * 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);
> > > > +}
> > > > +
> > >
> > > so this depends on FMODE_FSNOTIFY_HSM showing up on the file before any
> > > of the above calls and staying there for its lifetime -- does that hold?
> >
> > Yes!
> >
>
> ok
>
> In this case the new routines should come with a comment denoting it,
> otherwise the code looks incredibly suspicious.

How's this:

/*
 * Do not prevent write to executable file when watched by pre-content events.
 *
 * (*) FMODE_FSNOTIFY_HSM mode is set depending on pre-content watches at the
 *     time of file open and remains for entire lifetime of the file, so if
 *     pre-content watches are added post execution or removed before the end
 *     of the execution, it will not cause i_writecount reference leak.
 */

Jan, can you add this on commit?

>
> > >
> > > I think it would be less error prone down the road to maintain the
> > > counters, except not return the error if HSM is on.
> >
> > Cannot.
> > The "deny write counter" and "writers counter" are implemented on the
> > same counter, so open cannot get_write_access() if we maintain the
> > negative deny counters from exec.
> >
>
> I'm aware, in the above suggestion they would have to be split. Not
> great by any means but would beat the counter suddenly getting
> modified if the above did not hold.

Well, Christian did propose to add a counter in the i_pipe/i_cdev
union, but this all seems too much.

We are forced to carry the legacy behavior to avoid ABI breakage,
but maintaining a more complex code because of that is not a good
idea IMO.

Hopefully, the suggested solution is a fair compromise.

Thanks,
Amir.

Re: [PATCH] fs: don't block write during exec on pre-content watched files

[Mateusz Guzik]

On Fri, Nov 29, 2024 at 11:57 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Nov 28, 2024 at 6:00 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
> >
> > On Thu, Nov 28, 2024 at 5:57 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Thu, Nov 28, 2024 at 3:34 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
> > > > so this depends on FMODE_FSNOTIFY_HSM showing up on the file before any
> > > > of the above calls and staying there for its lifetime -- does that hold?
> > >
> > > Yes!
> > >
> >
> > ok
> >
> > In this case the new routines should come with a comment denoting it,
> > otherwise the code looks incredibly suspicious.
>
> How's this:
>
> /*
>  * Do not prevent write to executable file when watched by pre-content events.
>  *
>  * (*) FMODE_FSNOTIFY_HSM mode is set depending on pre-content watches at the
>  *     time of file open and remains for entire lifetime of the file, so if
>  *     pre-content watches are added post execution or removed before the end
>  *     of the execution, it will not cause i_writecount reference leak.
>  */
>

Looks good, thank you!

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] fs: don't block write during exec on pre-content watched files

[Jan Kara]

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@gmail.com>
> ---
> 
> 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@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: don't block write during exec on pre-content watched files

[Christian Brauner]

On Tue, Dec 03, 2024 at 06:24:40PM +0100, Jan Kara wrote:
> 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@gmail.com>
> > ---
> > 
> > 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

Thanks! This is fine by me. I already mentioned to Linus that we might
have to accept some unpleasantness in this area but this looks pretty
acceptable to me.

Only one question: The FMODE_FSNOTIFY_HSM(exe_file->f_mode) cannot
change once it's set, right?

The reason I'm asking is that we don't want to end up in a scenario
where we didn't block writecount in exe_file_deny_write_access() but
then unblock someone else's writecount in exe_file_allow_write_access().

Acked-by: Christian Brauner <brauner@kernel.org>

> (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@suse.com>
> SUSE Labs, CR

Re: [PATCH] fs: don't block write during exec on pre-content watched files

[Mateusz Guzik]

On Wed, Dec 04, 2024 at 11:56:39AM +0100, Christian Brauner wrote:
> Only one question: The FMODE_FSNOTIFY_HSM(exe_file->f_mode) cannot
> change once it's set, right?
> 
> The reason I'm asking is that we don't want to end up in a scenario
> where we didn't block writecount in exe_file_deny_write_access() but
> then unblock someone else's writecount in exe_file_allow_write_access().
> 

What's what I brought up, turns out its fine and the code will get a
comment to that extent.

see https://lore.kernel.org/linux-fsdevel/CAGudoHGzfjpukQ1QbuH=0Fot2vAWyrez-aVdO5Fum+330v-hmA@mail.gmail.com/

Re: [PATCH] fs: don't block write during exec on pre-content watched files

[Christian Brauner]

On Wed, Dec 04, 2024 at 12:06:54PM +0100, Mateusz Guzik wrote:
> On Wed, Dec 04, 2024 at 11:56:39AM +0100, Christian Brauner wrote:
> > Only one question: The FMODE_FSNOTIFY_HSM(exe_file->f_mode) cannot
> > change once it's set, right?
> > 
> > The reason I'm asking is that we don't want to end up in a scenario
> > where we didn't block writecount in exe_file_deny_write_access() but
> > then unblock someone else's writecount in exe_file_allow_write_access().
> > 
> 
> What's what I brought up, turns out its fine and the code will get a
> comment to that extent.
> 
> see https://lore.kernel.org/linux-fsdevel/CAGudoHGzfjpukQ1QbuH=0Fot2vAWyrez-aVdO5Fum+330v-hmA@mail.gmail.com/

Thank you! Then it seems we're good to go.

Re: [PATCH] fs: don't block write during exec on pre-content watched files

[Jan Kara]

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@gmail.com>

OK, I've picked up the patch to fsnotify_hsm branch with Christian's ack
and updated comments from Amir. Still waiting for Josef to give this a
final testing from their side but I've pulled the branch into for_next so
that it gets some exposure in linux-next as well.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: don't block write during exec on pre-content watched files

[Amir Goldstein]

On Thu, Dec 5, 2024 at 1:55 PM Jan Kara <jack@suse.cz> wrote:
>
> 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@gmail.com>
>
> OK, I've picked up the patch to fsnotify_hsm branch with Christian's ack
> and updated comments from Amir. Still waiting for Josef to give this a
> final testing from their side but I've pulled the branch into for_next so
> that it gets some exposure in linux-next as well.

Cool. I just pushed a test to my LTP fan_hsm branch for ETXTBSY.

It checks for the expected ETXTBSY with yes or no pre-content watchers,
but it checks failure to execute a file open for write, not actually to open
a file for write in the context of PRE_ACCESS event during execution.

So yeh, a test of the actual use case of large lazy populated
executables from Josef
is required, but at least we have basic sanity test coverage and now we will get
some extra linux-next testing coverage which is great.

Thanks,
Amir.

Re: [PATCH] fs: don't block write during exec on pre-content watched files

[Josef Bacik]

On Thu, Dec 05, 2024 at 02:28:25PM +0100, Amir Goldstein wrote:
> On Thu, Dec 5, 2024 at 1:55 PM Jan Kara <jack@suse.cz> wrote:
> >
> > 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@gmail.com>
> >
> > OK, I've picked up the patch to fsnotify_hsm branch with Christian's ack
> > and updated comments from Amir. Still waiting for Josef to give this a
> > final testing from their side but I've pulled the branch into for_next so
> > that it gets some exposure in linux-next as well.
> 
> Cool. I just pushed a test to my LTP fan_hsm branch for ETXTBSY.
> 
> It checks for the expected ETXTBSY with yes or no pre-content watchers,
> but it checks failure to execute a file open for write, not actually to open
> a file for write in the context of PRE_ACCESS event during execution.
> 
> So yeh, a test of the actual use case of large lazy populated
> executables from Josef
> is required, but at least we have basic sanity test coverage and now we will get
> some extra linux-next testing coverage which is great.

Sorry guys, holidays and then some PTO and other things conspired against me
being useful.  I just built and tested Jan's branch as of 10 minutes ago and
everything works.  Thanks for picking up my slack, I'll be actually responsive
for the next two weeks and then I'll disappear again until Jan 6th,

Josef