On Thu, Nov 28, 2024 at 6:00 PM Mateusz Guzik <mjguzik@xxxxxxxxx> wrote:
>
> On Thu, Nov 28, 2024 at 5:57 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> >
> > On Thu, Nov 28, 2024 at 3:34 PM Mateusz Guzik <mjguzik@xxxxxxxxx> 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.
