Re: [PATCH 08/10] fanotify: report file range info with pre-content events

[Amir Goldstein <amir73il@gmail.com>]

On Thu, Oct 24, 2024 at 6:35 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 24-10-24 12:06:35, Amir Goldstein wrote:
> > On Thu, Aug 1, 2024 at 7:38 PM Jan Kara <jack@suse.cz> wrote:
> > > On Thu 25-07-24 14:19:45, Josef Bacik wrote:
> > > > From: Amir Goldstein <amir73il@gmail.com>
> > > >
> > > > With group class FAN_CLASS_PRE_CONTENT, report offset and length info
> > > > along with FAN_PRE_ACCESS and FAN_PRE_MODIFY permission events.
> > > >
> > > > This information is meant to be used by hierarchical storage managers
> > > > that want to fill partial content of files on first access to range.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > ---
> > > >  fs/notify/fanotify/fanotify.h      |  8 +++++++
> > > >  fs/notify/fanotify/fanotify_user.c | 38 ++++++++++++++++++++++++++++++
> > > >  include/uapi/linux/fanotify.h      |  7 ++++++
> > > >  3 files changed, 53 insertions(+)
> > > >
> > > > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> > > > index 93598b7d5952..7f06355afa1f 100644
> > > > --- a/fs/notify/fanotify/fanotify.h
> > > > +++ b/fs/notify/fanotify/fanotify.h
> > > > @@ -448,6 +448,14 @@ static inline bool fanotify_is_perm_event(u32 mask)
> > > >               mask & FANOTIFY_PERM_EVENTS;
> > > >  }
> > > >
> > > > +static inline bool fanotify_event_has_access_range(struct fanotify_event *event)
> > > > +{
> > > > +     if (!(event->mask & FANOTIFY_PRE_CONTENT_EVENTS))
> > > > +             return false;
> > > > +
> > > > +     return FANOTIFY_PERM(event)->ppos;
> > > > +}
> > >
> > > Now I'm a bit confused. Can we have legally NULL ppos for an event from
> > > FANOTIFY_PRE_CONTENT_EVENTS?
> > >
> >
> > Sorry for the very late reply...
> >
> > The short answer is that NULL FANOTIFY_PERM(event)->ppos
> > simply means that fanotify_alloc_perm_event() was called with NULL
> > range, which is the very common case of legacy permission events.
> >
> > The long answer is a bit convoluted, so bare with me.
> > The long answer is to the question whether fsnotify_file_range() can
> > be called with a NULL ppos.
> >
> > This shouldn't be possible AFAIK for regular files and directories,
> > unless some fs that is marked with FS_ALLOW_HSM opens a regular
> > file with FMODE_STREAM, which should not be happening IMO,
> > but then the assertion belongs inside fsnotify_file_range().
> >
> > However, there was another way to get NULL ppos before I added the patch
> > "fsnotify: generate pre-content permission event on open"
> >
> > Which made this "half intentional" change:
> >  static inline int fsnotify_file_perm(struct file *file, int perm_mask)
> >  {
> > -       return fsnotify_file_area_perm(file, perm_mask, NULL, 0);
> > +       return fsnotify_file_area_perm(file, perm_mask, &file->f_pos, 0);
> >  }
> >
> > In order to implement:
> > "The event will have a range info of (0..0) to provide an opportunity
> >  to fill the entire file content on open."
> >
> > The problem is that do_open() was not the only caller of fsnotify_file_perm().
> > There is another call from iterate_dir() and the change above causes
> > FS_PRE_ACCESS events on readdir to report the directory f_pos -
> > Do we want that? I think we do, but HSM should be able to tell the
> > difference between opendir() and readdir(), because my HSM only
> > wants to fill dir content on the latter.
>
> Well, I'm not so sure we want to report fpos on opendir / readdir(). For
> directories fpos is an opaque cookie that is filesystem dependent and you
> are not even allowed to carry it from open to open. It is valid only within
> that one open-close session if I remember right. So userspace HSM cannot do
> much with it and in my opinion reporting it to userspace is a recipe for
> abuse...
>
> I'm undecided whether we want to allow pre-access events without range or
> enforce 0-0 range. I don't think there's a big practical difference.
>

So there is a practical difference.
My HSM wants to fill dir content on readdir() and not on opendir().
Other HSMs may want to fill dir content on opendir().
It could do that if opendir() (as does open()) reports range [0..0]
and readdir() reports no range.

I agree that it is somewhat ugly. I am willing to take other semantics
as long as HSM can tell the difference between opendir() and readdir().

Thanks,
Amir.