Re: fsnotify pre-modify VFS hooks (Was: fanotify and LSM path hooks)

[Jan Kara <jack@suse.cz>]

On Fri 26-06-20 14:06:37, Amir Goldstein wrote:
> On Wed, Apr 17, 2019 at 2:30 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Tue 16-04-19 21:24:44, Amir Goldstein wrote:
> > > > I'm not so sure about directory pre-modification hooks. Given the amount of
> > > > problems we face with applications using fanotify permission events and
> > > > deadlocking the system, I'm not very fond of expanding that API... AFAIU
> > > > you want to use such hooks for recording (and persisting) that some change
> > > > is going to happen and provide crash-consistency guarantees for such
> > > > journal?
> > > >
> > >
> > > That's the general idea.
> > > I have two use cases for pre-modification hooks:
> > > 1. VFS level snapshots
> > > 2. persistent change tracking
> > >
> > > TBH, I did not consider implementing any of the above in userspace,
> > > so I do not have a specific interest in extending the fanotify API.
> > > I am actually interested in pre-modify fsnotify hooks (not fanotify),
> > > that a snapshot or change tracking subsystem can register with.
> > > An in-kernel fsnotify event handler can set a flag in current task
> > > struct to circumvent system deadlocks on nested filesystem access.
> >
> > OK, I'm not opposed to fsnotify pre-modify hooks as such. As long as
> > handlers stay within the kernel, I'm fine with that. After all this is what
> > LSMs are already doing. Just exposing this to userspace for arbitration is
> > what I have a problem with.
> >
> 
> Short update on that.
> 
> I decided to ditch the LSM hooks approach because I realized that for
> the purpose of persistent change tracking, the pre-modify hooks need
> to be called before the caller is taking filesystem locks.
> 
> So I added hooks inside mnt_want_write and file_start_write wrappers:
> https://github.com/amir73il/linux/commits/fsnotify_pre_modify

FWIW I've glanced through the series. I like the choice of mnt_want_write()
and file_start_write() as a place to generate the event. I somewhat dislike
the number of variants you have to introduce and then pass NULL in some
places because you don't have the info available and then it's not
immediately clear what semantics the event consumers can expect... That
would be good to define and then verify in the code.

Also given you have the requirement "no fs locks on event generation", I'm
not sure how reliable this can be. If you don't hold fs locks when
generating event, cannot it happen that actually modified object is
different from the reported one because we raced with some other fs
operations? And can we prove that? So what exactly is the usecase and
guarantees the event needs to provide?

								Honza

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