On Wed, Jun 12, 2024 at 4:53 PM Christian Brauner <brauner@xxxxxxxxxx> 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@xxxxxxx>
> > Closes: https://lore.kernel.org/all/01c3bf2e-eb1f-4b7f-a54f-d2a05dd3d8c8@xxxxxxx
> > Signed-off-by: NeilBrown <neilb@xxxxxxx>
> > ---
>
> 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.
