linux-kernel - Re: [PATCH] vfs: avoid spurious dentry ref/unref cycle on open

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: <20240806155319.GP5334@ZenIV>
Date: Tue, 6 Aug 2024 16:53:19 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: brauner@...nel.org, jack@...e.cz, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] vfs: avoid spurious dentry ref/unref cycle on open

On Tue, Aug 06, 2024 at 04:46:28PM +0200, Mateusz Guzik wrote:

> The flag thing is optional and can be dropped, but I think the general
> direction should be to add *more* asserts and whatnot (even if they are
> to land separately). A debug-only variant would not hurt.

Asserts do *not* clarify anything; if you want your optional flag,
come up with clear description of its semantics.  In terms of state,
_not_ control flow.

> @@ -3683,6 +3685,7 @@ static const char *open_last_lookups(struct nameidata *nd,
>  static int do_open(struct nameidata *nd,
>  		   struct file *file, const struct open_flags *op)
>  {
> +	struct vfsmount *mnt;
>  	struct mnt_idmap *idmap;
>  	int open_flag = op->open_flag;
>  	bool do_truncate;
> @@ -3720,11 +3723,22 @@ static int do_open(struct nameidata *nd,
>  		error = mnt_want_write(nd->path.mnt);
>  		if (error)
>  			return error;
> +		/*
> +		 * We grab an additional reference here because vfs_open_consume()
> +		 * may error out and free the mount from under us, while we need
> +		 * to undo write access below.
> +		 */
> +		mnt = mntget(nd->path.mnt);

It's "after vfs_open_consume() we no longer own the reference in nd->path.mnt",
error or no error...

>  		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 && !(file->f_mode & FMODE_OPENED)) {
> +		BUG_ON(nd->state & ND_PATH_CONSUMED);
> +		error = vfs_open_consume(&nd->path, file);
> +		nd->state |= ND_PATH_CONSUMED;
> +		nd->path.mnt = NULL;
> +		nd->path.dentry = NULL;

Umm...  The thing that feels wrong here is that you get an extra
asymmetry with ->atomic_open() ;-/  We obviously can't do that
kind of thing there (if nothing else, we have the parent directory's
inode to unlock, error or no error).

I don't hate that patch, but it really feels like the calling
conventions are not right.  Let me try to tweak it a bit and
see if anything falls out...