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

Commit Message

Josef Bacik Nov. 12, 2024, 5:55 p.m. UTC

From: Amir Goldstein <[e-mail]>

Generate pre-content event on open in addition to FS_OPEN_PERM,
but without sb_writers held and after file was truncated
in case file was opened with O_CREAT and/or O_TRUNC.

The event will have a range info of [0..0] to provide an opportunity
to fill entire file content on open.

Signed-off-by: Amir Goldstein <[e-mail]>
---
 fs/namei.c               | 10 +++++++++-
 include/linux/fsnotify.h |  4 +++-
 2 files changed, 12 insertions(+), 2 deletions(-)

Comments

Linus Torvalds Nov. 12, 2024, 7:54 p.m. UTC | #1

On Tue, 12 Nov 2024 at 09:56, Josef Bacik <[e-mail]> 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.

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.

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.

              Linus

Amir Goldstein Nov. 12, 2024, 11:40 p.m. UTC | #2

On Tue, Nov 12, 2024 at 8:54 PM Linus Torvalds
<[e-mail]> wrote:
>
> On Tue, 12 Nov 2024 at 09:56, Josef Bacik <[e-mail]> 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/[e-mail]/
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.

Linus Torvalds Nov. 13, 2024, 12:58 a.m. UTC | #3

On Tue, 12 Nov 2024 at 15:41, Amir Goldstein <[e-mail]> wrote:
>
> 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.

Dropping that patch obviously removes my objection.

But since none of the whole "return errors" is valid with a truncate
or a new file creation anyway, isn't the whole thing kind of moot?

I guess do_open() could do it, but only inside a

        if (!error && !do_truncate && !(file->f_mode & FMODE_CREATED))
                error = fsnotify_opened_old(file);

kind of thing. With a big comment about how this is a pre-read hook,
and not relevant for a new file or a truncate event since then it's
always empty anyway.

But hey, if you don't absolutely need it in the first place, not
having it is *MUCH* preferable.

It sounds like the whole point was to catch reads - not opens. So then
you should catch it at read() time, not at open() time.

                Linus

Amir Goldstein Nov. 13, 2024, 10:12 a.m. UTC | #4

On Wed, Nov 13, 2024 at 1:58 AM Linus Torvalds
<[e-mail]> wrote:
>
> On Tue, 12 Nov 2024 at 15:41, Amir Goldstein <[e-mail]> wrote:
> >
> > 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.
>
> Dropping that patch obviously removes my objection.
>
> But since none of the whole "return errors" is valid with a truncate
> or a new file creation anyway, isn't the whole thing kind of moot?
>

Not moot. It is needed for the case that open with O_CREAT
finds an existing file and that file needs to be filled on open
and anyway do_open() is also taking sb_writers for O_RDWR
and O_WRONLY (not 100% sure why) not only for O_CREAT.

Essentially, this means that the legacy FAN_OPEN_PERM event
is not safe to be used by HSM, to fill file content on open.
and while I can document that fact all over the internet, that won't
stop people from using FAN_OPEN_PERM to implement a simple
HSM.

This is (the only) reason that I wanted to have a noticeable new event
at open time that is documented as safe for use by HSM and inviting
HSM developers to use the correct event.

Very possible that this is not a good enough reason.

> I guess do_open() could do it, but only inside a
>
>         if (!error && !do_truncate && !(file->f_mode & FMODE_CREATED))
>                 error = fsnotify_opened_old(file);
>
> kind of thing. With a big comment about how this is a pre-read hook,
> and not relevant for a new file or a truncate event since then it's
> always empty anyway.

Right. That would be good for what I wanted to achieve.

>
> But hey, if you don't absolutely need it in the first place, not
> having it is *MUCH* preferable.
>
> It sounds like the whole point was to catch reads - not opens. So then
> you should catch it at read() time, not at open() time.

Yeh, for sure.

Will drop this patch.

Thanks,
Amir.

Patch

diff --git a/fs/namei.c b/fs/namei.c
index 9d30c7aa9aa6..a1a5b10893f6 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3836,7 +3836,15 @@  static int do_open(struct nameidata *nd,
 	}
 	if (do_truncate)
 		mnt_drop_write(nd->path.mnt);
-	return error;
+	if (error)
+		return error;
+
+	/*
+	 * 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);
 }
 
 /**
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 7110bc2f5aa7..2d1c13df112c 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -193,6 +193,8 @@  static inline int fsnotify_pre_content(const struct file *file,
 
 /*
  * fsnotify_file_area_perm - permission hook before access of file range
+ *
+ * Called post open with access range [0..0].
  */
 static inline int fsnotify_file_area_perm(struct file *file, int perm_mask,
 					  const loff_t *ppos, size_t count)
@@ -207,7 +209,7 @@  static inline int fsnotify_file_area_perm(struct file *file, int perm_mask,
 	/*
 	 * read()/write and other types of access generate pre-content events.
 	 */
-	if (perm_mask & (MAY_READ | MAY_WRITE | MAY_ACCESS)) {
+	if (perm_mask & (MAY_READ | MAY_WRITE | MAY_ACCESS | MAY_OPEN)) {
 		int ret = fsnotify_pre_content(file, ppos, count);
 
 		if (ret)