linux-kernel - Re: [PATCH v2] 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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240617093745.nhnc7e7efdldnjzl@quack3>
Date: Mon, 17 Jun 2024 11:37:45 +0200
From: Jan Kara <jack@...e.cz>
To: Christian Brauner <brauner@...nel.org>
Cc: NeilBrown <neilb@...e.de>, 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,
	Alexander Viro <viro@...iv.linux.org.uk>, Jan Kara <jack@...e.cz>
Subject: Re: [PATCH v2] VFS: generate FS_CREATE before FS_OPEN when
 ->atomic_open used.

On Sat 15-06-24 07:35:42, Christian Brauner wrote:
> On Wed, 12 Jun 2024 17:09:55 +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().
> > 
> > [...]
> 
> 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/1] VFS: generate FS_CREATE before FS_OPEN when ->atomic_open used.
>       https://git.kernel.org/vfs/vfs/c/7536b2f06724

I have reviewed the patch you've committed since I wasn't quite sure which
changes you're going to apply after your discussion with Amir. And I have
two comments:

@@ -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;
 }

AFAICT this will have a side-effect that now fsnotify_open() will be
generated even for O_PATH open. It is true that fsnotify_close() is getting
generated for them already and we should strive for symmetry. Conceptually
it doesn't make sense to me to generate fsnotify events for O_PATH
opens/closes but maybe I miss something. Amir, any opinion here?

@@ -3612,6 +3612,9 @@ static int do_open(struct nameidata *nd,
 	int acc_mode;
 	int error;
 
+	if (file->f_mode & FMODE_OPENED)
+		fsnotify_open(file);
+
 	if (!(file->f_mode & (FMODE_OPENED | FMODE_CREATED))) {
 		error = complete_walk(nd);
 		if (error)

Frankly, this works but looks as an odd place to put this notification to.
Why not just placing it just next to where fsnotify_create() is generated
in open_last_lookups()? Like:

        if (open_flag & O_CREAT)
                inode_lock(dir->d_inode);
        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
                inode_unlock_shared(dir->d_inode);

That looks like a place where it is much more obvious this is for
atomic_open() handling? Now I admit I'm not really closely familiar with
the atomic_open() paths so maybe I miss something and do_open() is better.

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