Received-SPF: pass (google.com: domain of linux-kernel+bounces-23357-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 11 Jan 2024 10:05:01 +0000
From: Al Viro <viro@zeniv.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Josh Triplett <josh@joshtriplett.org>, Kees Cook <kees@kernel.org>,
	Kees Cook <keescook@chromium.org>, linux-kernel@vger.kernel.org,
	Alexey Dobriyan <adobriyan@gmail.com>
Subject: Re: [GIT PULL] execve updates for v6.8-rc1
Message-ID: <20240111100501.GU1674809@ZenIV>
References: <202401081028.0E908F9E0A@keescook>
 <CAHk-=wgznerM-xs+x+krDfE7eVBiy_HOam35rbsFMMOwvYuEKQ@mail.gmail.com>
 <D01C78AC-830C-4D73-9E9F-7FD38CEF2E82@kernel.org>
 <ZZ2W_xzCSyOgltad@localhost>
 <CAHk-=wi75tFVtZdzFRr4hsDeUKmeACbgD46rLe+2bcd=4mHBBw@mail.gmail.com>
 <ZZ3_Jmb1sb2wQWO_@localhost>
 <CAHk-=whf9qLO8ipps4QhmS0BkM8mtWJhvnuDSdtw5gFjhzvKNA@mail.gmail.com>
 <20240111094711.GT1674809@ZenIV>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240111094711.GT1674809@ZenIV>
Sender: Al Viro <viro@ftp.linux.org.uk>

On Thu, Jan 11, 2024 at 09:47:11AM +0000, Al Viro wrote:
> Doable, but really not pretty, especially since we'd need to massage
> the caller as well...  Less painful variant is
> 	if (error == -ECHILD && (flags & LOOKUP_RCU))
> 		return ERR_PTR(-ECHILD); // keep file for non-rcu pass
> 	*fp = NULL;
> 	fput(file);
> 	...
> on the way out; that won't help with -ESTALE side of things, but if we
> hit *that*, struct file allocation overhead is really noise.

Something like (completely untested) delta below, perhaps?

diff --git a/fs/namei.c b/fs/namei.c
index 5c318d657503..de770be9bb16 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3765,15 +3765,17 @@ static int do_o_path(struct nameidata *nd, unsigned flags, struct file *file)
 	return error;
 }
 
-static struct file *path_openat(struct nameidata *nd,
+static int path_openat(struct nameidata *nd, struct file **fp,
 			const struct open_flags *op, unsigned flags)
 {
-	struct file *file;
+	struct file *file = *fp;
 	int error;
 
-	file = alloc_empty_file(op->open_flag, current_cred());
-	if (IS_ERR(file))
-		return file;
+	if (!file) {
+		file = alloc_empty_file(op->open_flag, current_cred());
+		if (IS_ERR(file))
+			return PTR_ERR(file);
+	}
 
 	if (unlikely(file->f_flags & __O_TMPFILE)) {
 		error = do_tmpfile(nd, flags, op, file);
@@ -3789,11 +3791,17 @@ static struct file *path_openat(struct nameidata *nd,
 		terminate_walk(nd);
 	}
 	if (likely(!error)) {
-		if (likely(file->f_mode & FMODE_OPENED))
-			return file;
+		if (likely(file->f_mode & FMODE_OPENED)) {
+			*fp = file;
+			return 0;
+		}
 		WARN_ON(1);
 		error = -EINVAL;
 	}
+	if (error == -ECHILD && (flags & LOOKUP_RCU)) {
+		*fp = file; // reuse on the next pass
+		return -ECHILD;
+	}
 	fput(file);
 	if (error == -EOPENSTALE) {
 		if (flags & LOOKUP_RCU)
@@ -3801,7 +3809,7 @@ static struct file *path_openat(struct nameidata *nd,
 		else
 			error = -ESTALE;
 	}
-	return ERR_PTR(error);
+	return error;
 }
 
 struct file *do_filp_open(int dfd, struct filename *pathname,
@@ -3809,25 +3817,27 @@ struct file *do_filp_open(int dfd, struct filename *pathname,
 {
 	struct nameidata nd;
 	int flags = op->lookup_flags;
-	struct file *filp;
+	struct file *file = NULL;
+	int err;
 
 	set_nameidata(&nd, dfd, pathname, NULL);
-	filp = path_openat(&nd, op, flags | LOOKUP_RCU);
-	if (unlikely(filp == ERR_PTR(-ECHILD)))
-		filp = path_openat(&nd, op, flags);
-	if (unlikely(filp == ERR_PTR(-ESTALE)))
-		filp = path_openat(&nd, op, flags | LOOKUP_REVAL);
+	err = path_openat(&nd, &file, op, flags | LOOKUP_RCU);
+	if (unlikely(err == -ECHILD))
+		err = path_openat(&nd, &file, op, flags);
+	if (unlikely(err == -ESTALE))
+		err = path_openat(&nd, &file, op, flags | LOOKUP_REVAL);
 	restore_nameidata();
-	return filp;
+	return unlikely(err) ? ERR_PTR(err) : file;
 }
 
 struct file *do_file_open_root(const struct path *root,
 		const char *name, const struct open_flags *op)
 {
 	struct nameidata nd;
-	struct file *file;
+	struct file *file = NULL;
 	struct filename *filename;
 	int flags = op->lookup_flags;
+	int err;
 
 	if (d_is_symlink(root->dentry) && op->intent & LOOKUP_OPEN)
 		return ERR_PTR(-ELOOP);
@@ -3837,14 +3847,14 @@ struct file *do_file_open_root(const struct path *root,
 		return ERR_CAST(filename);
 
 	set_nameidata(&nd, -1, filename, root);
-	file = path_openat(&nd, op, flags | LOOKUP_RCU);
-	if (unlikely(file == ERR_PTR(-ECHILD)))
-		file = path_openat(&nd, op, flags);
-	if (unlikely(file == ERR_PTR(-ESTALE)))
-		file = path_openat(&nd, op, flags | LOOKUP_REVAL);
+	err = path_openat(&nd, &file, op, flags | LOOKUP_RCU);
+	if (unlikely(err == -ECHILD))
+		err = path_openat(&nd, &file, op, flags);
+	if (unlikely(err == -ESTALE))
+		err = path_openat(&nd, &file, op, flags | LOOKUP_REVAL);
 	restore_nameidata();
 	putname(filename);
-	return file;
+	return unlikely(err) ? ERR_PTR(err) : file;
 }
 
 static struct dentry *filename_create(int dfd, struct filename *name,