Date: Thu, 13 Jun 2024 10:31:00 +0300
From: Amir Goldstein <amir73il@...il.com>
To: Christian Brauner <brauner@...nel.org>, NeilBrown <neilb@...e.de>
Cc: Alexander Viro <viro@...iv.linux.org.uk>, Jan Kara <jack@...e.cz>, 
	James Clark <james.clark@....com>, ltp@...ts.linux.it, linux-nfs@...r.kernel.org, 
	LKML <linux-kernel@...r.kernel.org>, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2] VFS: generate FS_CREATE before FS_OPEN when
 ->atomic_open used.

On Wed, Jun 12, 2024 at 4:53 PM Christian Brauner <brauner@...nel.org> wrote:
>
> On Wed, Jun 12, 2024 at 05:09:55PM +1000, NeilBrown wrote:
> >
> > When a file is opened and created with open(..., O_CREAT) we get
> > both the CREATE and OPEN fsnotify events and would expect them in that
> > order.   For most filesystems we get them in that order because
> > open_last_lookups() calls fsnofify_create() and then do_open() (from
> > path_openat()) calls vfs_open()->do_dentry_open() which calls
> > fsnotify_open().
> >
> > However when ->atomic_open is used, the
> >    do_dentry_open() -> fsnotify_open()
> > call happens from finish_open() which is called from the ->atomic_open
> > handler in lookup_open() which is called *before* open_last_lookups()
> > calls fsnotify_create.  So we get the "open" notification before
> > "create" - which is backwards.  ltp testcase inotify02 tests this and
> > reports the inconsistency.
> >
> > This patch lifts the fsnotify_open() call out of do_dentry_open() and
> > places it higher up the call stack.  There are three callers of
> > do_dentry_open().
> >
> > For vfs_open() and kernel_file_open() the fsnotify_open() is placed
> > directly in that caller so there should be no behavioural change.
> >
> > For finish_open() there are two cases:
> >  - finish_open is used in ->atomic_open handlers.  For these we add a
> >    call to fsnotify_open() at the top of do_open() if FMODE_OPENED is
> >    set - which means do_dentry_open() has been called.
> >  - finish_open is used in ->tmpfile() handlers.  For these a similar
> >    call to fsnotify_open() is added to vfs_tmpfile()
> >
> > With this patch NFSv3 is restored to its previous behaviour (before
> > ->atomic_open support was added) of generating CREATE notifications
> > before OPEN, and NFSv4 now has that same correct ordering that is has
> > not had before.  I haven't tested other filesystems.
> >
> > Fixes: 7c6c5249f061 ("NFS: add atomic_open for NFSv3 to handle O_TRUNC correctly.")

I think it is better to add (also?)
Fixes: 7b8c9d7bb457 ("fsnotify: move fsnotify_open() hook into
do_dentry_open()")
because this is when the test case was regressed for other atomic_open() fs

> > Reported-by: James Clark <james.clark@....com>
> > Closes: https://lore.kernel.org/all/01c3bf2e-eb1f-4b7f-a54f-d2a05dd3d8c8@arm.com
> > Signed-off-by: NeilBrown <neilb@...e.de>
> > ---
>
> We should take this is a bugfix because it doesn't change behavior.
>

I agree.
I would love for this to be backported to at least v6.9.y
because FAN_CREATE events supported on fuse,nfs, (zero f_fsid)
only since v6.8, which triggered my fix to fanotify16 LTP test.

> But then we should follow this up with a patch series that tries to
> rectify the open/close imbalance because I find that pretty ugly. That's
> at least my opinion.
>
> We should aim to only generate an open event when may_open() succeeds
> and don't generate a close event when the open has failed. Maybe:
>
> +#ifdef CONFIG_FSNOTIFY
> +#define file_nonotify(f) ((f)->f_mode |= __FMODE_NONOTIFY)
> +#else
> +#define file_nonotify(f) ((void)(f))
> +#endif
>
> will do.

Why bother with the ifdef? __FMODE_NONOTIFY is always defined.

Maybe something like this (untested partial patch):


+static inline int fsnotify_open_error(struct file *f, int error)
+{
+       /*
+        * Once we return a file with FMODE_OPENED, __fput() will call
+        * fsnotify_close(), so we need to either call fsnotify_open() or
+        * set __FMODE_NONOTIFY to suppress fsnotify_close() for symmetry.
+        */
+       if (error)
+               f->f_mode |= __FMODE_NONOTIFY;
+       else
+               fsnotify_open(f);
+       return error;
+}
+
 static int do_dentry_open(struct file *f,
                          int (*open)(struct inode *, struct file *))
 {
@@ -1004,11 +1018,6 @@ static int do_dentry_open(struct file *f,
                }
        }

-       /*
-        * Once we return a file with FMODE_OPENED, __fput() will call
-        * fsnotify_close(), so we need fsnotify_open() here for symmetry.
-        */
-       fsnotify_open(f);
        return 0;

 cleanup_all:
@@ -1085,8 +1094,11 @@ EXPORT_SYMBOL(file_path);
  */
 int vfs_open(const struct path *path, struct file *file)
 {
+       int error;
+
        file->f_path = *path;
-       return do_dentry_open(file, NULL);
+       error = do_dentry_open(file, NULL);
+       return fsnotify_open_error(file, error);
 }

 struct file *dentry_open(const struct path *path, int flags,
@@ -1175,6 +1187,7 @@ struct file *kernel_file_open(const struct path
*path, int flags,

        f->f_path = *path;
        error = do_dentry_open(f, NULL);
+       fsnotify_open_error(f, error);
        if (error) {
                fput(f);
                f = ERR_PTR(error);


>
> Basic open permissions failing should count as failure to open and thus
> also turn of a close event.
>
> The somewhat ugly part is imho that security hooks introduce another
> layer of complexity. While we do count security_file_permission() as
> a failure to open we wouldn't e.g., count security_file_post_open() as a
> failure to open (Though granted here that "*_post_open()" makes it
> easier.). But it is really ugly that LSMs get to say "no" _after_ the
> file has been opened. I suspect this is some IMA or EVM thing where they
> hash the contents or something but it's royally ugly and I complained
> about this before. But maybe such things should just generate an LSM
> layer event via fsnotify in the future (FSNOTIFY_MAC) or something...
> Then userspace can see "Hey, the VFS said yes but then the MAC stuff
> said no."

Not sure what IMA/EVM needs so cannot comment about this proposal.

Thanks,
Amir.

Powered by blists - more mailing lists