Re: [PATCH v7 07/18] fsnotify: generate pre-content permission event on open

[Amir Goldstein <amir73il@gmail.com>]

On Tue, Nov 12, 2024 at 8:54 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, 12 Nov 2024 at 09:56, Josef Bacik <josef@toxicpanda.com> wrote:
> >
> > +       /*
> > +        * This permission hook is different than fsnotify_open_perm() hook.
> > +        * This is a pre-content hook that is called without sb_writers held
> > +        * and after the file was truncated.
> > +        */
> > +       return fsnotify_file_area_perm(file, MAY_OPEN, &file->f_pos, 0);
> >  }
>
> I still object to this all.
>
> You can't say "permission denied" after you've already truncated the
> file. It's not a sane model. I complained about that earlier, it seems
> that complaint was missed in the other complaints.
>

Not missed.
I answered here:
https://lore.kernel.org/linux-fsdevel/CAOQ4uxg0k4bGz6zOKS+Qt5BjEqDdUhvgG+5pLBPqSCcnQdffig@mail.gmail.com/
Starting with "...I understand why it seems stupid..."

Nevertheless, we can also drop this patch for now,
I don't think the post-open is a must-have hook for HSM.

> Also, this whole "This permission hook is different than
> fsnotify_open_perm() hook" statement is purely because
> fsnotify_open_perm() itself was broken and called from the wrong place
> as mentioned in the other email.
>

You wrote it should be called "in the open path" - that is ambiguous.
pre-content hook must be called without sb_writers held, so current
(in linux-next) location of fsnotify_open_perm() is not good in case of
O_CREATE flag, so I am not sure where a good location is.
Easier is to drop this patch.

> Fix *THAT* first, then unify the two places that should *not* be
> different into one single "this is the fsnotify_open" code. And that
> place explicitly sets that FMODE_NOTIFY_PERM bit, and makes sure that
> it does *not* set it for FMODE_NONOTIFY or FMODE_PATH cases.
>
> And then please work on making sure that that isn't called unless
> actually required.
>
> The actual real "pre-content permission events" should then ONLY test
> the FMODE_NOTIFY_PERM bit. Nothing else. None of this "re-use the
> existing fsnotify_file() logic" stuff. Noe extra tests, no extra
> logic.
>
> Don't make me jump through filve layers of inline functions that all
> test different 'mask' bits, just to verify that the open / read /
> write paths don't do something stupid.
>
> IOW, make it straightforward and obvious what you are doing, and make
> it very clear that you're not pointlessly testing things like
> FMODE_NONOTIFY when the *ONLY* thing that should be tested is whether
> FMODE_NOTIFY_PERM is set.
>
> Please.

Yes. Clear.

Thanks for taking the time to look closer.
and thanks for the feedback,
Amir.