diff mbox series

[v7,07/10] fs: make do_linkat() take struct filename

Message ID 20210706124901.1360377-8-dkadashev@gmail.com (mailing list archive)
State New, archived
Headers show
Series io_uring: add mkdir and [sym]linkat support | expand

Commit Message

Dmitry Kadashev July 6, 2021, 12:48 p.m. UTC 
Pass in the struct filename pointers instead of the user string, for
uniformity with do_renameat2, do_unlinkat, do_mknodat, etc.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Link: https://lore.kernel.org/io-uring/20210330071700.kpjoyp5zlni7uejm@wittgenstein/
Signed-off-by: Dmitry Kadashev <dkadashev@gmail.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/namei.c | 57 +++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 18 deletions(-)

Comments

Linus Torvalds July 6, 2021, 6:05 p.m. UTC | #1 
On Tue, Jul 6, 2021 at 5:49 AM Dmitry Kadashev <dkadashev@gmail.com> wrote:
>
> Pass in the struct filename pointers instead of the user string, for
> uniformity with do_renameat2, do_unlinkat, do_mknodat, etc.

This is the only one in the series that I still react fairly negatively at.

I still just don't like how filename_lookup() used to be nice and easy
to understand ("always eat the name"), and while those semantics
remain, the new __filename_lookup() has those odd semantics of only
eating it on failure.

And there is exactly _one_ caller of that new __filename_lookup(), and it does

        error = __filename_lookup(olddfd, old, how, &old_path, NULL);
        if (error)
                goto out_putnew;

and I don't even understand why you'd want to eat it on error, because
if if *didn't* eat it on error, it would just do

        error = __filename_lookup(olddfd, old, how, &old_path, NULL);
        if (error)
                goto out_putnames;

and it would be much easier to understand (and the "out_putnew" label
would go away entirely)

What am I missing? You had some reason for not eating the name
unconditionally, but I look at this patch and I just don't see it.

              Linus
Dmitry Kadashev July 7, 2021, 7:27 a.m. UTC | #2 
On Wed, Jul 7, 2021 at 1:05 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> This is the only one in the series that I still react fairly negatively at.
>
> I still just don't like how filename_lookup() used to be nice and easy
> to understand ("always eat the name"), and while those semantics
> remain, the new __filename_lookup() has those odd semantics of only
> eating it on failure.
>
> And there is exactly _one_ caller of that new __filename_lookup(), and it does
>
>         error = __filename_lookup(olddfd, old, how, &old_path, NULL);
>         if (error)
>                 goto out_putnew;
>
> and I don't even understand why you'd want to eat it on error, because
> if if *didn't* eat it on error, it would just do
>
>         error = __filename_lookup(olddfd, old, how, &old_path, NULL);
>         if (error)
>                 goto out_putnames;
>
> and it would be much easier to understand (and the "out_putnew" label
> would go away entirely)
>
> What am I missing? You had some reason for not eating the name
> unconditionally, but I look at this patch and I just don't see it.

__filename_lookup() does that "eat the name on error" for uniformity
with the __filename_create(), and the latter does that mostly because Al
suggested to do it that way:

https://lore.kernel.org/io-uring/20210201150042.GQ740243@zeniv-ca/

Granted, he did that back when this series was much smaller, only about
mkdirat, and in that case it looked like it makes things a tad simpler,
and even though I found the semantics a bit confusing, I've assumed that
I'm missing something and this is something the FS code does, so people
are used to it.

Anyway, I'll send v8 of this series with yet another preparation patch,
that will change filename_parenat() to return an error code instead of
struct filename *, and split it into two: filename_parenat() that always
eats the name, and __filename_parentat() that never eats the name. And
__filename_lookup() and __filename_create() will never eat the name as
well, so things are nice and uniform and easy to reason about.

And hopefully if Al does not like that approach he can weigh in.
diff mbox series

Patch

diff --git a/fs/namei.c b/fs/namei.c
index 70a3c5124db3..0a2731f7ef71 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2450,7 +2450,7 @@  static int path_lookupat(struct nameidata *nd, unsigned flags, struct path *path
 	return err;
 }
 
-int filename_lookup(int dfd, struct filename *name, unsigned flags,
+static int __filename_lookup(int dfd, struct filename *name, unsigned flags,
 		    struct path *path, struct path *root)
 {
 	int retval;
@@ -2472,7 +2472,18 @@  int filename_lookup(int dfd, struct filename *name, unsigned flags,
 		audit_inode(name, path->dentry,
 			    flags & LOOKUP_MOUNTPOINT ? AUDIT_INODE_NOEVAL : 0);
 	restore_nameidata();
-	putname(name);
+	if (retval)
+		putname(name);
+	return retval;
+}
+
+int filename_lookup(int dfd, struct filename *name, unsigned flags,
+		    struct path *path, struct path *root)
+{
+	int retval = __filename_lookup(dfd, name, flags, path, root);
+
+	if (!retval)
+		putname(name);
 	return retval;
 }
 
@@ -4346,8 +4357,8 @@  EXPORT_SYMBOL(vfs_link);
  * with linux 2.0, and to avoid hard-linking to directories
  * and other special files.  --ADM
  */
-static int do_linkat(int olddfd, const char __user *oldname, int newdfd,
-	      const char __user *newname, int flags)
+static int do_linkat(int olddfd, struct filename *old, int newdfd,
+	      struct filename *new, int flags)
 {
 	struct user_namespace *mnt_userns;
 	struct dentry *new_dentry;
@@ -4356,31 +4367,36 @@  static int do_linkat(int olddfd, const char __user *oldname, int newdfd,
 	int how = 0;
 	int error;
 
-	if ((flags & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
-		return -EINVAL;
+	if ((flags & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0) {
+		error = -EINVAL;
+		goto out_putnames;
+	}
 	/*
 	 * To use null names we require CAP_DAC_READ_SEARCH
 	 * This ensures that not everyone will be able to create
 	 * handlink using the passed filedescriptor.
 	 */
-	if (flags & AT_EMPTY_PATH) {
-		if (!capable(CAP_DAC_READ_SEARCH))
-			return -ENOENT;
-		how = LOOKUP_EMPTY;
+	if (flags & AT_EMPTY_PATH && !capable(CAP_DAC_READ_SEARCH)) {
+		error = -ENOENT;
+		goto out_putnames;
 	}
 
 	if (flags & AT_SYMLINK_FOLLOW)
 		how |= LOOKUP_FOLLOW;
 retry:
-	error = user_path_at(olddfd, oldname, how, &old_path);
+	error = __filename_lookup(olddfd, old, how, &old_path, NULL);
 	if (error)
-		return error;
+		goto out_putnew;
 
-	new_dentry = user_path_create(newdfd, newname, &new_path,
+	new_dentry = __filename_create(newdfd, new, &new_path,
 					(how & LOOKUP_REVAL));
 	error = PTR_ERR(new_dentry);
-	if (IS_ERR(new_dentry))
-		goto out;
+	if (IS_ERR(new_dentry)) {
+		// On error `new` is freed by __filename_create, set to NULL to
+		// signal that we do not have to free it below
+		new = NULL;
+		goto out_putpath;
+	}
 
 	error = -EXDEV;
 	if (old_path.mnt != new_path.mnt)
@@ -4408,8 +4424,12 @@  static int do_linkat(int olddfd, const char __user *oldname, int newdfd,
 		how |= LOOKUP_REVAL;
 		goto retry;
 	}
-out:
+out_putpath:
 	path_put(&old_path);
+out_putnames:
+	putname(old);
+out_putnew:
+	putname(new);
 
 	return error;
 }
@@ -4417,12 +4437,13 @@  static int do_linkat(int olddfd, const char __user *oldname, int newdfd,
 SYSCALL_DEFINE5(linkat, int, olddfd, const char __user *, oldname,
 		int, newdfd, const char __user *, newname, int, flags)
 {
-	return do_linkat(olddfd, oldname, newdfd, newname, flags);
+	return do_linkat(olddfd, getname_uflags(oldname, flags),
+		newdfd, getname(newname), flags);
 }
 
 SYSCALL_DEFINE2(link, const char __user *, oldname, const char __user *, newname)
 {
-	return do_linkat(AT_FDCWD, oldname, AT_FDCWD, newname, 0);
+	return do_linkat(AT_FDCWD, getname(oldname), AT_FDCWD, getname(newname), 0);
 }
 
 /**