On Mon, Jun 17, 2024 at 7:23 PM Jan Kara <jack@xxxxxxx> wrote:
>
> From: NeilBrown <neilb@xxxxxxx>
>
> 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 open_last_lookups() 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.")
> 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>
> Link: https://lore.kernel.org/r/171817619547.14261.975798725161704336@xxxxxxxxxxxxxxxxxxxxx
> Fixes: 7b8c9d7bb457 ("fsnotify: move fsnotify_open() hook into do_dentry_open()")
> Tested-by: James Clark <james.clark@xxxxxxx>
> Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
Looks good.
Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
> fs/namei.c | 10 ++++++++--
> fs/open.c | 22 +++++++++++++++-------
> 2 files changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 37fb0a8aa09a..1e05a0f3f04d 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3572,8 +3572,12 @@ static const char *open_last_lookups(struct nameidata *nd,
> else
> inode_lock_shared(dir->d_inode);
> dentry = lookup_open(nd, file, op, got_write);
> - if (!IS_ERR(dentry) && (file->f_mode & FMODE_CREATED))
> - fsnotify_create(dir->d_inode, dentry);
> + if (!IS_ERR(dentry)) {
> + if (file->f_mode & FMODE_CREATED)
> + fsnotify_create(dir->d_inode, dentry);
> + if (file->f_mode & FMODE_OPENED)
> + fsnotify_open(file);
> + }
> if (open_flag & O_CREAT)
> inode_unlock(dir->d_inode);
> else
> @@ -3700,6 +3704,8 @@ int vfs_tmpfile(struct mnt_idmap *idmap,
> mode = vfs_prepare_mode(idmap, dir, mode, mode, mode);
> error = dir->i_op->tmpfile(idmap, dir, file, mode);
> dput(child);
> + if (file->f_mode & FMODE_OPENED)
> + fsnotify_open(file);
> if (error)
> return error;
> /* Don't check for other permissions, the inode was just created */
> diff --git a/fs/open.c b/fs/open.c
> index 89cafb572061..f1607729acb9 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -1004,11 +1004,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 +1080,19 @@ EXPORT_SYMBOL(file_path);
> */
> int vfs_open(const struct path *path, struct file *file)
> {
> + int ret;
> +
> file->f_path = *path;
> - return do_dentry_open(file, NULL);
> + ret = do_dentry_open(file, NULL);
> + if (!ret) {
> + /*
> + * Once we return a file with FMODE_OPENED, __fput() will call
> + * fsnotify_close(), so we need fsnotify_open() here for
> + * symmetry.
> + */
> + fsnotify_open(file);
> + }
> + return ret;
> }
>
> struct file *dentry_open(const struct path *path, int flags,
> @@ -1177,8 +1183,10 @@ struct file *kernel_file_open(const struct path *path, int flags,
> error = do_dentry_open(f, NULL);
> if (error) {
> fput(f);
> - f = ERR_PTR(error);
> + return ERR_PTR(error);
> }
> +
> + fsnotify_open(f);
> return f;
> }
> EXPORT_SYMBOL_GPL(kernel_file_open);
> --
> 2.35.3
>
