[PATCH 0/2 v3] fsnotify: Generate FS_CREATE event before FS_OPEN

[PATCH 0/2 v3] fsnotify: Generate FS_CREATE event before FS_OPEN

[Jan Kara]

Hello,

this is a third revision of the series Neil started making sure FS_CREATE event
is generated before FS_OPEN event even for filesystems using atomic_open.

Changes since v2:
* Don't generate FS_OPEN events for O_PATH fd opens
* Move generation of FS_OPEN event for atomic_open() into a more logical place

Previous version:
v2: https://lore.kernel.org/all/171817619547.14261.975798725161704336@noble.neil.brown.name/

								Honza

[PATCH 1/2] fsnotify: Do not generate events for O_PATH file descriptors

[Jan Kara]

Currently we will not generate FS_OPEN events for O_PATH file
descriptors but we will generate FS_CLOSE events for them. This is
asymmetry is confusing. Arguably no fsnotify events should be generated
for O_PATH file descriptors as they cannot be used to access or modify
file content, they are just convenient handles to file objects like
paths. So fix the asymmetry by stopping to generate FS_CLOSE for O_PATH
file descriptors.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 include/linux/fsnotify.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 4da80e92f804..278620e063ab 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -112,7 +112,13 @@ static inline int fsnotify_file(struct file *file, __u32 mask)
 {
 	const struct path *path;
 
-	if (file->f_mode & FMODE_NONOTIFY)
+	/*
+	 * FMODE_NONOTIFY are fds generated by fanotify itself which should not
+	 * generate new events. We also don't want to generate events for
+	 * FMODE_PATH fds (involves open & close events) as they are just
+	 * handle creation / destruction events and not "real" file events.
+	 */
+	if (file->f_mode & (FMODE_NONOTIFY | FMODE_PATH))
 		return 0;
 
 	path = &file->f_path;
-- 
2.35.3

Re: [PATCH 1/2] fsnotify: Do not generate events for O_PATH file descriptors

[Amir Goldstein]

On Mon, Jun 17, 2024 at 7:23 PM Jan Kara <jack@suse.cz> wrote:
>
> Currently we will not generate FS_OPEN events for O_PATH file
> descriptors but we will generate FS_CLOSE events for them. This is
> asymmetry is confusing. Arguably no fsnotify events should be generated
> for O_PATH file descriptors as they cannot be used to access or modify
> file content, they are just convenient handles to file objects like
> paths. So fix the asymmetry by stopping to generate FS_CLOSE for O_PATH
> file descriptors.
>
> Signed-off-by: Jan Kara <jack@suse.cz>

Looks good.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  include/linux/fsnotify.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index 4da80e92f804..278620e063ab 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -112,7 +112,13 @@ static inline int fsnotify_file(struct file *file, __u32 mask)
>  {
>         const struct path *path;
>
> -       if (file->f_mode & FMODE_NONOTIFY)
> +       /*
> +        * FMODE_NONOTIFY are fds generated by fanotify itself which should not
> +        * generate new events. We also don't want to generate events for
> +        * FMODE_PATH fds (involves open & close events) as they are just
> +        * handle creation / destruction events and not "real" file events.
> +        */
> +       if (file->f_mode & (FMODE_NONOTIFY | FMODE_PATH))
>                 return 0;
>
>         path = &file->f_path;
> --
> 2.35.3
>

Re: [PATCH 1/2] fsnotify: Do not generate events for O_PATH file descriptors

[Christian Brauner]

On Mon, 17 Jun 2024 18:23:00 +0200, Jan Kara wrote:
> Currently we will not generate FS_OPEN events for O_PATH file
> descriptors but we will generate FS_CLOSE events for them. This is
> asymmetry is confusing. Arguably no fsnotify events should be generated
> for O_PATH file descriptors as they cannot be used to access or modify
> file content, they are just convenient handles to file objects like
> paths. So fix the asymmetry by stopping to generate FS_CLOSE for O_PATH
> file descriptors.
> 
> [...]

I added a Cc stable to the first patch because this seems like a bugfix
per the mail discussion.

---

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/2] fsnotify: Do not generate events for O_PATH file descriptors
      https://git.kernel.org/vfs/vfs/c/702eb71fd650
[2/2] vfs: generate FS_CREATE before FS_OPEN when ->atomic_open used.
      https://git.kernel.org/vfs/vfs/c/7d1cf5e624ef

Re: [PATCH 1/2] fsnotify: Do not generate events for O_PATH file descriptors

[Jan Kara]

On Tue 18-06-24 16:28:01, Christian Brauner wrote:
> On Mon, 17 Jun 2024 18:23:00 +0200, Jan Kara wrote:
> > Currently we will not generate FS_OPEN events for O_PATH file
> > descriptors but we will generate FS_CLOSE events for them. This is
> > asymmetry is confusing. Arguably no fsnotify events should be generated
> > for O_PATH file descriptors as they cannot be used to access or modify
> > file content, they are just convenient handles to file objects like
> > paths. So fix the asymmetry by stopping to generate FS_CLOSE for O_PATH
> > file descriptors.
> > 
> > [...]
> 
> I added a Cc stable to the first patch because this seems like a bugfix
> per the mail discussion.

Yes, makes sense. Thanks!

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/2] vfs: generate FS_CREATE before FS_OPEN when ->atomic_open used.

[Jan Kara]

From: NeilBrown <neilb@suse.de>

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@arm.com>
Closes: https://lore.kernel.org/all/01c3bf2e-eb1f-4b7f-a54f-d2a05dd3d8c8@arm.com
Signed-off-by: NeilBrown <neilb@suse.de>
Link: https://lore.kernel.org/r/171817619547.14261.975798725161704336@noble.neil.brown.name
Fixes: 7b8c9d7bb457 ("fsnotify: move fsnotify_open() hook into do_dentry_open()")
Tested-by: James Clark <james.clark@arm.com>
Signed-off-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 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

Re: [PATCH 2/2] vfs: generate FS_CREATE before FS_OPEN when ->atomic_open used.

[Amir Goldstein]

On Mon, Jun 17, 2024 at 7:23 PM Jan Kara <jack@suse.cz> wrote:
>
> From: NeilBrown <neilb@suse.de>
>
> 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@arm.com>
> Closes: https://lore.kernel.org/all/01c3bf2e-eb1f-4b7f-a54f-d2a05dd3d8c8@arm.com
> Signed-off-by: NeilBrown <neilb@suse.de>
> Link: https://lore.kernel.org/r/171817619547.14261.975798725161704336@noble.neil.brown.name
> Fixes: 7b8c9d7bb457 ("fsnotify: move fsnotify_open() hook into do_dentry_open()")
> Tested-by: James Clark <james.clark@arm.com>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Jan Kara <jack@suse.cz>

Looks good.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  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
>