linux-kernel - [PATCH] VFS: generate FS_CREATE before FS_OPEN when ->atomic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-id: <171815791109.14261.10223988071271993465@noble.neil.brown.name>
Date: Wed, 12 Jun 2024 12:05:11 +1000
From: "NeilBrown" <neilb@...e.de>
To: Alexander Viro <viro@...iv.linux.org.uk>,
 Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>
Cc: Amir Goldstein <amir73il@...il.com>, 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:
 [PATCH] VFS: generate FS_CREATE before FS_OPEN when ->atomic_open used.

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 three cases:
 - finish_open is used in ->atomic_open handlers.  For these we add a
   call to fsnotify_open() in do_open() if FMODE_OPENED is set - which
   means do_dentry_open() has been called. This happens after fsnotify_create().
 - finish_open is used in ->tmpfile() handlers.  For these a call to
   fsnotify_open() is added to vfs_tmpfile()
 - finish_open is used in gfs2_create_inode() which is used for
   atomic_open, but also for gfs2_create() which is a ->create handler
   and is not expected to open the file.  So losing the fsnotify_open()
   call in this case seems correct.

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@....com>
Closes: https://lore.kernel.org/all/01c3bf2e-eb1f-4b7f-a54f-d2a05dd3d8c8@arm.com
Signed-off-by: NeilBrown <neilb@...e.de>
---
 fs/namei.c |  9 +++++++--
 fs/open.c  | 19 ++++++++++++-------
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 37fb0a8aa09a..32031feaf6b6 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3646,8 +3646,12 @@ static int do_open(struct nameidata *nd,
 		do_truncate = true;
 	}
 	error = may_open(idmap, &nd->path, acc_mode, open_flag);
-	if (!error && !(file->f_mode & FMODE_OPENED))
-		error = vfs_open(&nd->path, file);
+	if (!error) {
+		if (file->f_mode & FMODE_OPENED)
+			fsnotify_open(file);
+		else
+			error = vfs_open(&nd->path, file);
+	}
 	if (!error)
 		error = security_file_post_open(file, op->acc_mode);
 	if (!error && do_truncate)
@@ -3706,6 +3710,7 @@ int vfs_tmpfile(struct mnt_idmap *idmap,
 	error = may_open(idmap, &file->f_path, 0, file->f_flags);
 	if (error)
 		return error;
+	fsnotify_open(file);
 	inode = file_inode(file);
 	if (!(open_flag & O_EXCL)) {
 		spin_lock(&inode->i_lock);
diff --git a/fs/open.c b/fs/open.c
index 89cafb572061..970f299c0e77 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,17 @@ 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,
@@ -1178,7 +1182,8 @@ struct file *kernel_file_open(const struct path *path, int flags,
 	if (error) {
 		fput(f);
 		f = ERR_PTR(error);
-	}
+	} else
+		fsnotify_open(f);
 	return f;
 }
 EXPORT_SYMBOL_GPL(kernel_file_open);
-- 
2.44.0