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

[Jan Kara <jack@suse.cz>]

On Thu 24-10-24 18:49:02, Amir Goldstein wrote:
> 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.

OK, this looks reasonably consistent so ack from me.

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