[PATCH 0/7] namei: clean up retry logic in various do_* functions

[PATCH 0/7] namei: clean up retry logic in various do_* functions

[Dmitry Kadashev]

Suggested by Linus in https://lore.kernel.org/io-uring/CAHk-=wh=cpt_tQCirzFZRPawRpbuFTZ2MxNpXiyUF+eBXF=+sw@mail.gmail.com/

This patchset does all the do_* functions one by one. The idea is to
move the main logic to a helper function and handle stale retries /
struct filename cleanups outside, which makes the logic easier to
follow.

There is one minor change in the behavior: filename_lookup() /
filename_parentat() / filename_create() do their own retries on ESTALE
(regardless of flags), and previously they were exempt from retries in
the do_* functions (but they *were* called on retry - it's just the
return code wasn't checked for ESTALE). And now the retry is done on
the upper level, and so technically it could be called a behavior
change. Hopefully it's an edge case where an additional check does not
matter.

On top of https://lore.kernel.org/io-uring/20210708063447.3556403-1-dkadashev@gmail.com/

Dmitry Kadashev (7):
  namei: clean up do_rmdir retry logic
  namei: clean up do_unlinkat retry logic
  namei: clean up do_mkdirat retry logic
  namei: clean up do_mknodat retry logic
  namei: clean up do_symlinkat retry logic
  namei: clean up do_linkat retry logic
  namei: clean up do_renameat retry logic

 fs/namei.c | 283 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 163 insertions(+), 120 deletions(-)

-- 
2.30.2

[PATCH 1/7] namei: clean up do_rmdir retry logic

[Dmitry Kadashev]

Moving the main logic to a helper function makes the whole thing much
easier to follow.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/io-uring/CAHk-=wh=cpt_tQCirzFZRPawRpbuFTZ2MxNpXiyUF+eBXF=+sw@mail.gmail.com/
Signed-off-by: Dmitry Kadashev <dkadashev@gmail.com>
---
 fs/namei.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index b5adfd4f7de6..ae6cde7dc91e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3947,7 +3947,8 @@ int vfs_rmdir(struct user_namespace *mnt_userns, struct inode *dir,
 }
 EXPORT_SYMBOL(vfs_rmdir);
 
-int do_rmdir(int dfd, struct filename *name)
+static int rmdir_helper(int dfd, struct filename *name,
+			unsigned int lookup_flags)
 {
 	struct user_namespace *mnt_userns;
 	int error;
@@ -3955,54 +3956,59 @@ int do_rmdir(int dfd, struct filename *name)
 	struct path path;
 	struct qstr last;
 	int type;
-	unsigned int lookup_flags = 0;
-retry:
+
 	error = __filename_parentat(dfd, name, lookup_flags, &path, &last, &type);
 	if (error)
-		goto exit1;
+		return error;
 
 	switch (type) {
 	case LAST_DOTDOT:
 		error = -ENOTEMPTY;
-		goto exit2;
+		goto exit1;
 	case LAST_DOT:
 		error = -EINVAL;
-		goto exit2;
+		goto exit1;
 	case LAST_ROOT:
 		error = -EBUSY;
-		goto exit2;
+		goto exit1;
 	}
 
 	error = mnt_want_write(path.mnt);
 	if (error)
-		goto exit2;
+		goto exit1;
 
 	inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
 	dentry = __lookup_hash(&last, path.dentry, lookup_flags);
 	error = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
-		goto exit3;
+		goto exit2;
 	if (!dentry->d_inode) {
 		error = -ENOENT;
-		goto exit4;
+		goto exit3;
 	}
 	error = security_path_rmdir(&path, dentry);
 	if (error)
-		goto exit4;
+		goto exit3;
 	mnt_userns = mnt_user_ns(path.mnt);
 	error = vfs_rmdir(mnt_userns, path.dentry->d_inode, dentry);
-exit4:
-	dput(dentry);
 exit3:
+	dput(dentry);
+exit2:
 	inode_unlock(path.dentry->d_inode);
 	mnt_drop_write(path.mnt);
-exit2:
-	path_put(&path);
-	if (retry_estale(error, lookup_flags)) {
-		lookup_flags |= LOOKUP_REVAL;
-		goto retry;
-	}
 exit1:
+	path_put(&path);
+	return error;
+}
+
+int do_rmdir(int dfd, struct filename *name)
+{
+	int error;
+
+	error = rmdir_helper(dfd, name, 0);
+	if (retry_estale(error, 0))
+		error = rmdir_helper(dfd, name, LOOKUP_REVAL);
+
 	putname(name);
 	return error;
 }
-- 
2.30.2

Re: [PATCH 1/7] namei: clean up do_rmdir retry logic

[Christian Brauner]

On Mon, Jul 12, 2021 at 07:36:43PM +0700, Dmitry Kadashev wrote:
> Moving the main logic to a helper function makes the whole thing much
> easier to follow.
> 
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Christian Brauner <christian.brauner@ubuntu.com>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Link: https://lore.kernel.org/io-uring/CAHk-=wh=cpt_tQCirzFZRPawRpbuFTZ2MxNpXiyUF+eBXF=+sw@mail.gmail.com/
> Signed-off-by: Dmitry Kadashev <dkadashev@gmail.com>
> ---
>  fs/namei.c | 44 +++++++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index b5adfd4f7de6..ae6cde7dc91e 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3947,7 +3947,8 @@ int vfs_rmdir(struct user_namespace *mnt_userns, struct inode *dir,
>  }
>  EXPORT_SYMBOL(vfs_rmdir);
>  
> -int do_rmdir(int dfd, struct filename *name)
> +static int rmdir_helper(int dfd, struct filename *name,
> +			unsigned int lookup_flags)
>  {
>  	struct user_namespace *mnt_userns;
>  	int error;
> @@ -3955,54 +3956,59 @@ int do_rmdir(int dfd, struct filename *name)
>  	struct path path;
>  	struct qstr last;
>  	int type;
> -	unsigned int lookup_flags = 0;
> -retry:
> +
>  	error = __filename_parentat(dfd, name, lookup_flags, &path, &last, &type);
>  	if (error)
> -		goto exit1;
> +		return error;
>  
>  	switch (type) {
>  	case LAST_DOTDOT:
>  		error = -ENOTEMPTY;
> -		goto exit2;
> +		goto exit1;
>  	case LAST_DOT:
>  		error = -EINVAL;
> -		goto exit2;
> +		goto exit1;
>  	case LAST_ROOT:
>  		error = -EBUSY;
> -		goto exit2;
> +		goto exit1;
>  	}
>  
>  	error = mnt_want_write(path.mnt);
>  	if (error)
> -		goto exit2;
> +		goto exit1;
>  
>  	inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
>  	dentry = __lookup_hash(&last, path.dentry, lookup_flags);
>  	error = PTR_ERR(dentry);
>  	if (IS_ERR(dentry))
> -		goto exit3;
> +		goto exit2;
>  	if (!dentry->d_inode) {
>  		error = -ENOENT;
> -		goto exit4;
> +		goto exit3;
>  	}
>  	error = security_path_rmdir(&path, dentry);
>  	if (error)
> -		goto exit4;
> +		goto exit3;
>  	mnt_userns = mnt_user_ns(path.mnt);
>  	error = vfs_rmdir(mnt_userns, path.dentry->d_inode, dentry);
> -exit4:
> -	dput(dentry);
>  exit3:
> +	dput(dentry);
> +exit2:
>  	inode_unlock(path.dentry->d_inode);
>  	mnt_drop_write(path.mnt);
> -exit2:
> -	path_put(&path);
> -	if (retry_estale(error, lookup_flags)) {
> -		lookup_flags |= LOOKUP_REVAL;
> -		goto retry;
> -	}
>  exit1:
> +	path_put(&path);
> +	return error;
> +}
> +
> +int do_rmdir(int dfd, struct filename *name)
> +{
> +	int error;
> +
> +	error = rmdir_helper(dfd, name, 0);
> +	if (retry_estale(error, 0))
> +		error = rmdir_helper(dfd, name, LOOKUP_REVAL);

Instead of naming all these $something_helper I would follow the
underscore naming pattern we usually do, i.e. instead of e.g.
rmdir_helper do __rmdir() or __do_rmdir().

Re: [PATCH 1/7] namei: clean up do_rmdir retry logic

[Linus Torvalds]

On Tue, Jul 13, 2021 at 7:53 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> Instead of naming all these $something_helper I would follow the
> underscore naming pattern we usually do, i.e. instead of e.g.
> rmdir_helper do __rmdir() or __do_rmdir().

That's certainly a pattern we have, but I don't necessarily love it.

It would be even better if we'd have names that actually explain
what/why the abstraction exists. In this case, it's the "possibly
retry due to ESTALE", but I have no idea how to sanely name that.
Making it "try_rmdir()" or something like that is the best I can come
up with right now.

On  a similar note, the existing "do_rmdir()" and friends aren't
wonderful names either, but we expose that name out so changing it is
probably not worth it. But right now we have "vfs_rmdir()" and
"do_rmdir()", and they are just different levels of the "rmdir stack",
without the name really describing where in the stack they are.

Naming is hard, and I don't think the double underscores have been
wonderful either.

            Linus

Re: [PATCH 1/7] namei: clean up do_rmdir retry logic

[Dmitry Kadashev]

On Tue, Jul 13, 2021 at 11:58 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, Jul 13, 2021 at 7:53 AM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > Instead of naming all these $something_helper I would follow the
> > underscore naming pattern we usually do, i.e. instead of e.g.
> > rmdir_helper do __rmdir() or __do_rmdir().
>
> That's certainly a pattern we have, but I don't necessarily love it.
>
> It would be even better if we'd have names that actually explain
> what/why the abstraction exists. In this case, it's the "possibly
> retry due to ESTALE", but I have no idea how to sanely name that.
> Making it "try_rmdir()" or something like that is the best I can come
> up with right now.
>
> On  a similar note, the existing "do_rmdir()" and friends aren't
> wonderful names either, but we expose that name out so changing it is
> probably not worth it. But right now we have "vfs_rmdir()" and
> "do_rmdir()", and they are just different levels of the "rmdir stack",
> without the name really describing where in the stack they are.
>
> Naming is hard, and I don't think the double underscores have been
> wonderful either.

Naming *is* hard, I do not have any good ideas here, so I just went with
try_rmdir(). Christian, Linus, let me know if that is not good enough.

-- 
Dmitry Kadashev

[PATCH 2/7] namei: clean up do_unlinkat retry logic

[Dmitry Kadashev]

Moving the main logic to a helper function makes the whole thing much
easier to follow.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/io-uring/CAHk-=wh=cpt_tQCirzFZRPawRpbuFTZ2MxNpXiyUF+eBXF=+sw@mail.gmail.com/
Signed-off-by: Dmitry Kadashev <dkadashev@gmail.com>
---
 fs/namei.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index ae6cde7dc91e..bb18b1adfea5 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4091,7 +4091,8 @@ EXPORT_SYMBOL(vfs_unlink);
  * writeout happening, and we don't want to prevent access to the directory
  * while waiting on the I/O.
  */
-int do_unlinkat(int dfd, struct filename *name)
+static int unlinkat_helper(int dfd, struct filename *name,
+			   unsigned int lookup_flags)
 {
 	int error;
 	struct dentry *dentry;
@@ -4100,19 +4101,18 @@ int do_unlinkat(int dfd, struct filename *name)
 	int type;
 	struct inode *inode = NULL;
 	struct inode *delegated_inode = NULL;
-	unsigned int lookup_flags = 0;
-retry:
+
 	error = __filename_parentat(dfd, name, lookup_flags, &path, &last, &type);
 	if (error)
-		goto exit1;
+		return error;
 
 	error = -EISDIR;
 	if (type != LAST_NORM)
-		goto exit2;
+		goto exit1;
 
 	error = mnt_want_write(path.mnt);
 	if (error)
-		goto exit2;
+		goto exit1;
 retry_deleg:
 	inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
 	dentry = __lookup_hash(&last, path.dentry, lookup_flags);
@@ -4129,11 +4129,11 @@ int do_unlinkat(int dfd, struct filename *name)
 		ihold(inode);
 		error = security_path_unlink(&path, dentry);
 		if (error)
-			goto exit3;
+			goto exit2;
 		mnt_userns = mnt_user_ns(path.mnt);
 		error = vfs_unlink(mnt_userns, path.dentry->d_inode, dentry,
 				   &delegated_inode);
-exit3:
+exit2:
 		dput(dentry);
 	}
 	inode_unlock(path.dentry->d_inode);
@@ -4146,15 +4146,8 @@ int do_unlinkat(int dfd, struct filename *name)
 			goto retry_deleg;
 	}
 	mnt_drop_write(path.mnt);
-exit2:
-	path_put(&path);
-	if (retry_estale(error, lookup_flags)) {
-		lookup_flags |= LOOKUP_REVAL;
-		inode = NULL;
-		goto retry;
-	}
 exit1:
-	putname(name);
+	path_put(&path);
 	return error;
 
 slashes:
@@ -4164,7 +4157,19 @@ int do_unlinkat(int dfd, struct filename *name)
 		error = -EISDIR;
 	else
 		error = -ENOTDIR;
-	goto exit3;
+	goto exit2;
+}
+
+int do_unlinkat(int dfd, struct filename *name)
+{
+	int error;
+
+	error = unlinkat_helper(dfd, name, 0);
+	if (retry_estale(error, 0))
+		error = unlinkat_helper(dfd, name, LOOKUP_REVAL);
+
+	putname(name);
+	return error;
 }
 
 SYSCALL_DEFINE3(unlinkat, int, dfd, const char __user *, pathname, int, flag)
-- 
2.30.2

[PATCH 3/7] namei: clean up do_mkdirat retry logic

[Dmitry Kadashev]

Moving the main logic to a helper function makes the whole thing much
easier to follow.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/io-uring/CAHk-=wijsw1QSsQHFu_6dEoZEr_zvT7++WJWohcuEkLqqXBGrQ@mail.gmail.com/
Signed-off-by: Dmitry Kadashev <dkadashev@gmail.com>
---
 fs/namei.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index bb18b1adfea5..b9762e2cf3b9 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3850,18 +3850,16 @@ int vfs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
 }
 EXPORT_SYMBOL(vfs_mkdir);
 
-int do_mkdirat(int dfd, struct filename *name, umode_t mode)
+static int mkdirat_helper(int dfd, struct filename *name, umode_t mode,
+			  unsigned int lookup_flags)
 {
 	struct dentry *dentry;
 	struct path path;
 	int error;
-	unsigned int lookup_flags = LOOKUP_DIRECTORY;
 
-retry:
 	dentry = __filename_create(dfd, name, &path, lookup_flags);
-	error = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
-		goto out_putname;
+		return PTR_ERR(dentry);
 
 	if (!IS_POSIXACL(path.dentry->d_inode))
 		mode &= ~current_umask();
@@ -3873,11 +3871,21 @@ int do_mkdirat(int dfd, struct filename *name, umode_t mode)
 				  mode);
 	}
 	done_path_create(&path, dentry);
+
+	return error;
+}
+
+int do_mkdirat(int dfd, struct filename *name, umode_t mode)
+{
+	unsigned int lookup_flags = LOOKUP_DIRECTORY;
+	int error;
+
+	error = mkdirat_helper(dfd, name, mode, lookup_flags);
 	if (retry_estale(error, lookup_flags)) {
 		lookup_flags |= LOOKUP_REVAL;
-		goto retry;
+		error = mkdirat_helper(dfd, name, mode, lookup_flags);
 	}
-out_putname:
+
 	putname(name);
 	return error;
 }
-- 
2.30.2

[PATCH 4/7] namei: clean up do_mknodat retry logic

[Dmitry Kadashev]

Moving the main logic to a helper function makes the whole thing much
easier to follow.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/io-uring/CAHk-=wiG+sN+2zSoAOggKCGue2kOJvw3rQySvQXsZstRQFTN+g@mail.gmail.com/
Signed-off-by: Dmitry Kadashev <dkadashev@gmail.com>
---
 fs/namei.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index b9762e2cf3b9..7bf7a9f38ce2 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3745,29 +3745,27 @@ static int may_mknod(umode_t mode)
 	}
 }
 
-static int do_mknodat(int dfd, struct filename *name, umode_t mode,
-		unsigned int dev)
+static int mknodat_helper(int dfd, struct filename *name, umode_t mode,
+			  unsigned int dev, unsigned int lookup_flags)
 {
 	struct user_namespace *mnt_userns;
 	struct dentry *dentry;
 	struct path path;
 	int error;
-	unsigned int lookup_flags = 0;
 
 	error = may_mknod(mode);
 	if (error)
-		goto out1;
-retry:
+		return error;
 	dentry = __filename_create(dfd, name, &path, lookup_flags);
 	error = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
-		goto out1;
+		return error;
 
 	if (!IS_POSIXACL(path.dentry->d_inode))
 		mode &= ~current_umask();
 	error = security_path_mknod(&path, dentry, mode, dev);
 	if (error)
-		goto out2;
+		goto out;
 
 	mnt_userns = mnt_user_ns(path.mnt);
 	switch (mode & S_IFMT) {
@@ -3786,13 +3784,20 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
 					  dentry, mode, 0);
 			break;
 	}
-out2:
+out:
 	done_path_create(&path, dentry);
-	if (retry_estale(error, lookup_flags)) {
-		lookup_flags |= LOOKUP_REVAL;
-		goto retry;
-	}
-out1:
+	return error;
+}
+
+static int do_mknodat(int dfd, struct filename *name, umode_t mode,
+		unsigned int dev)
+{
+	int error;
+
+	error = mknodat_helper(dfd, name, mode, dev, 0);
+	if (retry_estale(error, 0))
+		error = mknodat_helper(dfd, name, mode, dev, LOOKUP_REVAL);
+
 	putname(name);
 	return error;
 }
-- 
2.30.2

Re: [PATCH 4/7] namei: clean up do_mknodat retry logic

[Linus Torvalds]

On Mon, Jul 12, 2021 at 5:37 AM Dmitry Kadashev <dkadashev@gmail.com> wrote:
>
> Moving the main logic to a helper function makes the whole thing much
> easier to follow.

This patch works, but the conversion is not one-to-one.

> -static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> -               unsigned int dev)
> +static int mknodat_helper(int dfd, struct filename *name, umode_t mode,
> +                         unsigned int dev, unsigned int lookup_flags)
>  {
>         struct user_namespace *mnt_userns;
>         struct dentry *dentry;
>         struct path path;
>         int error;
> -       unsigned int lookup_flags = 0;
>
>         error = may_mknod(mode);
>         if (error)
> -               goto out1;
> -retry:

Note how "may_mknod()" was _outside_ the retry before, and now it's inside:

> +static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> +               unsigned int dev)
> +{
> +       int error;
> +
> +       error = mknodat_helper(dfd, name, mode, dev, 0);
> +       if (retry_estale(error, 0))
> +               error = mknodat_helper(dfd, name, mode, dev, LOOKUP_REVAL);
> +
>         putname(name);
>         return error;

which happens to not be fatal - doing the may_mknod() twice will not
break anything - but it doesn't match what it used to do.

A few options options:

 (a) a separate patch to move the "may_mknod()" to the two callers first

 (b) a separate patch to move the "may_mknod()" first into the repeat
loop, with the comment being that it's ok.

 (c) keep the logic the same, with the code something like

  static int do_mknodat(int dfd, struct filename *name, umode_t mode,
                unsigned int dev)
  {
        int error;

        error = may_mknod(mode);
        if (!error) {
                error = mknodat_helper(dfd, name, mode, dev, 0);
                if (retry_estale(error, 0))
                        error = mknodat_helper(dfd, name, mode, dev,
LOOKUP_REVAL);
        }

        putname(name);
        return error;
  }

or

 (d) keep the patch as-is, but with an added commit message note about
how it's not one-to-one and why it's ok.

Hmm?

So this patch could be fine, but it really wants to note how it
changes the logic and why that's fine. Or, the patch should be
modified.

                Linus

[PATCH 5/7] namei: clean up do_symlinkat retry logic

[Dmitry Kadashev]

Moving the main logic to a helper function makes the whole thing much
easier to follow.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/io-uring/CAHk-=wiG+sN+2zSoAOggKCGue2kOJvw3rQySvQXsZstRQFTN+g@mail.gmail.com/
Signed-off-by: Dmitry Kadashev <dkadashev@gmail.com>
---
 fs/namei.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 7bf7a9f38ce2..c9110ac83ccb 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4237,22 +4237,17 @@ int vfs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
 }
 EXPORT_SYMBOL(vfs_symlink);
 
-int do_symlinkat(struct filename *from, int newdfd, struct filename *to)
+static int symlinkat_helper(struct filename *from, int newdfd,
+			    struct filename *to, unsigned int lookup_flags)
 {
 	int error;
 	struct dentry *dentry;
 	struct path path;
-	unsigned int lookup_flags = 0;
 
-	if (IS_ERR(from)) {
-		error = PTR_ERR(from);
-		goto out_putnames;
-	}
-retry:
 	dentry = __filename_create(newdfd, to, &path, lookup_flags);
 	error = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
-		goto out_putnames;
+		return error;
 
 	error = security_path_symlink(&path, dentry, from->name);
 	if (!error) {
@@ -4263,11 +4258,23 @@ int do_symlinkat(struct filename *from, int newdfd, struct filename *to)
 				    from->name);
 	}
 	done_path_create(&path, dentry);
-	if (retry_estale(error, lookup_flags)) {
-		lookup_flags |= LOOKUP_REVAL;
-		goto retry;
+	return error;
+}
+
+int do_symlinkat(struct filename *from, int newdfd, struct filename *to)
+{
+	int error;
+
+	if (IS_ERR(from)) {
+		error = PTR_ERR(from);
+		goto out;
 	}
-out_putnames:
+
+	error = symlinkat_helper(from, newdfd, to, 0);
+	if (retry_estale(error, 0))
+		error = symlinkat_helper(from, newdfd, to, LOOKUP_REVAL);
+
+out:
 	putname(to);
 	putname(from);
 	return error;
-- 
2.30.2

Re: [PATCH 5/7] namei: clean up do_symlinkat retry logic

[Linus Torvalds]

On Mon, Jul 12, 2021 at 5:37 AM Dmitry Kadashev <dkadashev@gmail.com> wrote:
>
> +
> +int do_symlinkat(struct filename *from, int newdfd, struct filename *to)
> +{
> +       int error;
> +
> +       if (IS_ERR(from)) {
> +               error = PTR_ERR(from);
> +               goto out;
>         }
> -out_putnames:
> +
> +       error = symlinkat_helper(from, newdfd, to, 0);
> +       if (retry_estale(error, 0))
> +               error = symlinkat_helper(from, newdfd, to, LOOKUP_REVAL);
> +
> +out:
>         putname(to);
>         putname(from);
>         return error;

So here you moved that part that was outside the retry loop into the
caller. Except it's very ugly and keeps the goto mess.

So I'd suggest either keep it as a nested if - avoiding the goto - or
like in the previous patch, do that "we can do this test twice" with a
big commit message note about why it's ok.

Because it _is_ ok to repeat the test inside the retry_estale, and
'from' won't have changed (and won't have been -ESTALE in the first
place).

Looking at the pattern of this and the previous one, I think just
repeating the test is what generates the cleanest end result.

               Linus

[PATCH 6/7] namei: clean up do_linkat retry logic

[Dmitry Kadashev]

Moving the main logic to a helper function makes the whole thing much
easier to follow.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/io-uring/CAHk-=wiG+sN+2zSoAOggKCGue2kOJvw3rQySvQXsZstRQFTN+g@mail.gmail.com/
Signed-off-by: Dmitry Kadashev <dkadashev@gmail.com>
---
 fs/namei.c | 80 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 44 insertions(+), 36 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index c9110ac83ccb..5e4fa8b65a8d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4380,48 +4380,22 @@ int vfs_link(struct dentry *old_dentry, struct user_namespace *mnt_userns,
 }
 EXPORT_SYMBOL(vfs_link);
 
-/*
- * Hardlinks are often used in delicate situations.  We avoid
- * security-related surprises by not following symlinks on the
- * newname.  --KAB
- *
- * We don't follow them on the oldname either to be compatible
- * with linux 2.0, and to avoid hard-linking to directories
- * and other special files.  --ADM
- */
-int do_linkat(int olddfd, struct filename *old, int newdfd,
-	      struct filename *new, int flags)
+static int linkat_helper(int olddfd, struct filename *old, int newdfd,
+			 struct filename *new, unsigned int lookup_flags)
 {
 	struct user_namespace *mnt_userns;
 	struct dentry *new_dentry;
 	struct path old_path, new_path;
 	struct inode *delegated_inode = NULL;
-	int how = 0;
 	int error;
 
-	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 && !capable(CAP_DAC_READ_SEARCH)) {
-		error = -ENOENT;
-		goto out_putnames;
-	}
-
-	if (flags & AT_SYMLINK_FOLLOW)
-		how |= LOOKUP_FOLLOW;
 retry:
-	error = __filename_lookup(olddfd, old, how, &old_path, NULL);
+	error = __filename_lookup(olddfd, old, lookup_flags, &old_path, NULL);
 	if (error)
-		goto out_putnames;
+		return error;
 
 	new_dentry = __filename_create(newdfd, new, &new_path,
-					(how & LOOKUP_REVAL));
+					(lookup_flags & LOOKUP_REVAL));
 	error = PTR_ERR(new_dentry);
 	if (IS_ERR(new_dentry))
 		goto out_putpath;
@@ -4447,14 +4421,48 @@ int do_linkat(int olddfd, struct filename *old, int newdfd,
 			goto retry;
 		}
 	}
+out_putpath:
+	path_put(&old_path);
+	return error;
+}
+
+/*
+ * Hardlinks are often used in delicate situations.  We avoid
+ * security-related surprises by not following symlinks on the
+ * newname.  --KAB
+ *
+ * We don't follow them on the oldname either to be compatible
+ * with linux 2.0, and to avoid hard-linking to directories
+ * and other special files.  --ADM
+ */
+int do_linkat(int olddfd, struct filename *old, int newdfd,
+	      struct filename *new, int flags)
+{
+	int error, how = 0;
+
+	if ((flags & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0) {
+		error = -EINVAL;
+		goto out;
+	}
+	/*
+	 * 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 && !capable(CAP_DAC_READ_SEARCH)) {
+		error = -ENOENT;
+		goto out;
+	}
+
+	if (flags & AT_SYMLINK_FOLLOW)
+		how |= LOOKUP_FOLLOW;
+
+	error = linkat_helper(olddfd, old, newdfd, new, how);
 	if (retry_estale(error, how)) {
-		path_put(&old_path);
 		how |= LOOKUP_REVAL;
-		goto retry;
+		error = linkat_helper(olddfd, old, newdfd, new, how);
 	}
-out_putpath:
-	path_put(&old_path);
-out_putnames:
+out:
 	putname(old);
 	putname(new);
 
-- 
2.30.2

Re: [PATCH 6/7] namei: clean up do_linkat retry logic

[Linus Torvalds]

On Mon, Jul 12, 2021 at 5:37 AM Dmitry Kadashev <dkadashev@gmail.com> wrote:
>
> Moving the main logic to a helper function makes the whole thing much
> easier to follow.

Ok, this has the same thing as the previous patches had.

I see why the old code tried to avoid the repeat of some tests, but
honestly, that "retry_estale()" might as well be marked "unlikely()",
and we might as well do the test again if it triggers.

That said, in this case we actually end up doing other things too in
"do_linkat()", so I guess it could go either way.

              Linus

[PATCH 7/7] namei: clean up do_renameat retry logic

[Dmitry Kadashev]

Moving the main logic to a helper function makes the whole thing much
easier to follow.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/io-uring/CAHk-=wiG+sN+2zSoAOggKCGue2kOJvw3rQySvQXsZstRQFTN+g@mail.gmail.com/
Signed-off-by: Dmitry Kadashev <dkadashev@gmail.com>
---
 fs/namei.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 5e4fa8b65a8d..023ee19aa5ed 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4658,8 +4658,9 @@ int vfs_rename(struct renamedata *rd)
 }
 EXPORT_SYMBOL(vfs_rename);
 
-int do_renameat2(int olddfd, struct filename *from, int newdfd,
-		 struct filename *to, unsigned int flags)
+static int renameat_helper(int olddfd, struct filename *from, int newdfd,
+			   struct filename *to, unsigned int flags,
+			   unsigned int lookup_flags)
 {
 	struct renamedata rd;
 	struct dentry *old_dentry, *new_dentry;
@@ -4668,25 +4669,23 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
 	struct qstr old_last, new_last;
 	int old_type, new_type;
 	struct inode *delegated_inode = NULL;
-	unsigned int lookup_flags = 0, target_flags = LOOKUP_RENAME_TARGET;
-	bool should_retry = false;
+	unsigned int target_flags = LOOKUP_RENAME_TARGET;
 	int error = -EINVAL;
 
 	if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE | RENAME_WHITEOUT))
-		goto put_names;
+		return error;
 
 	if ((flags & (RENAME_NOREPLACE | RENAME_WHITEOUT)) &&
 	    (flags & RENAME_EXCHANGE))
-		goto put_names;
+		return error;
 
 	if (flags & RENAME_EXCHANGE)
 		target_flags = 0;
 
-retry:
 	error = __filename_parentat(olddfd, from, lookup_flags, &old_path,
 					&old_last, &old_type);
 	if (error)
-		goto put_names;
+		return error;
 
 	error = __filename_parentat(newdfd, to, lookup_flags, &new_path, &new_last,
 				&new_type);
@@ -4784,17 +4783,22 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
 	}
 	mnt_drop_write(old_path.mnt);
 exit2:
-	if (retry_estale(error, lookup_flags))
-		should_retry = true;
 	path_put(&new_path);
 exit1:
 	path_put(&old_path);
-	if (should_retry) {
-		should_retry = false;
-		lookup_flags |= LOOKUP_REVAL;
-		goto retry;
-	}
-put_names:
+	return error;
+}
+
+int do_renameat2(int olddfd, struct filename *from, int newdfd,
+		 struct filename *to, unsigned int flags)
+{
+	int error;
+
+	error = renameat_helper(olddfd, from, newdfd, to, flags, 0);
+	if (retry_estale(error, 0))
+		error = renameat_helper(olddfd, from, newdfd, to, flags,
+					LOOKUP_REVAL);
+
 	putname(from);
 	putname(to);
 	return error;
-- 
2.30.2

Re: [PATCH 0/7] namei: clean up retry logic in various do_* functions

[Dmitry Kadashev]

On Mon, Jul 12, 2021 at 7:37 PM Dmitry Kadashev <dkadashev@gmail.com> wrote:
>
> Suggested by Linus in https://lore.kernel.org/io-uring/CAHk-=wh=cpt_tQCirzFZRPawRpbuFTZ2MxNpXiyUF+eBXF=+sw@mail.gmail.com/
>
> This patchset does all the do_* functions one by one. The idea is to
> move the main logic to a helper function and handle stale retries /
> struct filename cleanups outside, which makes the logic easier to
> follow.
>
> There is one minor change in the behavior: filename_lookup() /
> filename_parentat() / filename_create() do their own retries on ESTALE
> (regardless of flags), and previously they were exempt from retries in
> the do_* functions (but they *were* called on retry - it's just the
> return code wasn't checked for ESTALE). And now the retry is done on
> the upper level, and so technically it could be called a behavior
> change. Hopefully it's an edge case where an additional check does not
> matter.
>
> On top of https://lore.kernel.org/io-uring/20210708063447.3556403-1-dkadashev@gmail.com/

Since this is on top of the stuff that is going to be in the Jens' tree
only until the 5.15 merge window, I'm assuming this series should go
there as well.

-- 
Dmitry Kadashev

Re: [PATCH 0/7] namei: clean up retry logic in various do_* functions

[Linus Torvalds]

On Mon, Jul 12, 2021 at 5:41 AM Dmitry Kadashev <dkadashev@gmail.com> wrote:
>
> Since this is on top of the stuff that is going to be in the Jens' tree
> only until the 5.15 merge window, I'm assuming this series should go
> there as well.

Yeah. Unless Al wants to pick this whole series up.

See my comments about the individual patches - some of them change
code flow, others do. And I think changing code flow as part of
cleanup is ok, but it at the very least needs to be mentioned (and it
might be good to do the "move code that is idempotent inside the
retry" as a separate patch from documentation purposes)

           Linus

Re: [PATCH 0/7] namei: clean up retry logic in various do_* functions

[Al Viro]

On Mon, Jul 12, 2021 at 12:01:52PM -0700, Linus Torvalds wrote:
> On Mon, Jul 12, 2021 at 5:41 AM Dmitry Kadashev <dkadashev@gmail.com> wrote:
> >
> > Since this is on top of the stuff that is going to be in the Jens' tree
> > only until the 5.15 merge window, I'm assuming this series should go
> > there as well.
> 
> Yeah. Unless Al wants to pick this whole series up.
> 
> See my comments about the individual patches - some of them change
> code flow, others do. And I think changing code flow as part of
> cleanup is ok, but it at the very least needs to be mentioned (and it
> might be good to do the "move code that is idempotent inside the
> retry" as a separate patch from documentation purposes)

TBH, my main problem with this is that ESTALE retry logics had never
felt right.  We ask e.g. filename_create() to get us the parent.  We
tell it whether we want it to be maximally suspicious or not.  It
still does the same RCU-normal-LOOKUP_REVAL sequence, only for "trust
no one" variant it's RCU-LOOKUP_REVAL-LOOKUP_REVAL instead.  We are
*not* told how far in that sequence did it have to get.  What's more,
even if we had to get all way up to LOOKUP_REVAL, we ignore that
when we do dcache lookup for the last component - only the argument
of filename_create() is looked at.

It really smells like the calling conventions are wrong.  I agree that
all of that is, by definition, a very slow path - it's just that the
logics makes me go "WTF?" every time I see it... ;-/

Hell knows - perhaps the lookup_flags thing wants to be passed by
reference (all the way into path_parentat()) and have the "we had
to go for LOOKUP_REVAL" returned that way.  Not sure...

Al, still crawling out of the bloody ptrace/asm glue horrors at the moment...

Re: [PATCH 0/7] namei: clean up retry logic in various do_* functions

[Dmitry Kadashev]

On Tue, Jul 13, 2021 at 3:28 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, Jul 12, 2021 at 12:01:52PM -0700, Linus Torvalds wrote:
> > On Mon, Jul 12, 2021 at 5:41 AM Dmitry Kadashev <dkadashev@gmail.com> wrote:
> > >
> > > Since this is on top of the stuff that is going to be in the Jens' tree
> > > only until the 5.15 merge window, I'm assuming this series should go
> > > there as well.
> >
> > Yeah. Unless Al wants to pick this whole series up.
> >
> > See my comments about the individual patches - some of them change
> > code flow, others do. And I think changing code flow as part of
> > cleanup is ok, but it at the very least needs to be mentioned (and it
> > might be good to do the "move code that is idempotent inside the
> > retry" as a separate patch from documentation purposes)
>
> TBH, my main problem with this is that ESTALE retry logics had never
> felt right.  We ask e.g. filename_create() to get us the parent.  We
> tell it whether we want it to be maximally suspicious or not.  It
> still does the same RCU-normal-LOOKUP_REVAL sequence, only for "trust
> no one" variant it's RCU-LOOKUP_REVAL-LOOKUP_REVAL instead.

Regardless of the bigger changes discussed below, should we change
direct comparison to ESTALE to retry_estale(retval, lookup_flags) in
filename_lookup() and filename_parentat() (and probably also
do_filp_open() and do_file_open_root())? At least it won't do two
consecutive LOOKUP_REVAL lookups and the change is trivial.

> We are
> *not* told how far in that sequence did it have to get.  What's more,
> even if we had to get all way up to LOOKUP_REVAL, we ignore that
> when we do dcache lookup for the last component - only the argument
> of filename_create() is looked at.
>
> It really smells like the calling conventions are wrong.  I agree that
> all of that is, by definition, a very slow path - it's just that the
> logics makes me go "WTF?" every time I see it... ;-/

The current series does not make it worse though. I'm happy to work on
further improvements with some guidance, but hopefully in a separate
patchset?

> Hell knows - perhaps the lookup_flags thing wants to be passed by
> reference (all the way into path_parentat()) and have the "we had
> to go for LOOKUP_REVAL" returned that way.  Not sure...

Will that allow to get rid of the retries completely? I'm not sure I
understand all the code paths that can return ESTALE, I'd assume we'd
still have to keep the whole retry logic.

-- 
Dmitry Kadashev

On Tue, Jul 13, 2021 at 3:28 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, Jul 12, 2021 at 12:01:52PM -0700, Linus Torvalds wrote:
> > On Mon, Jul 12, 2021 at 5:41 AM Dmitry Kadashev <dkadashev@gmail.com> wrote:
> > >
> > > Since this is on top of the stuff that is going to be in the Jens' tree
> > > only until the 5.15 merge window, I'm assuming this series should go
> > > there as well.
> >
> > Yeah. Unless Al wants to pick this whole series up.
> >
> > See my comments about the individual patches - some of them change
> > code flow, others do. And I think changing code flow as part of
> > cleanup is ok, but it at the very least needs to be mentioned (and it
> > might be good to do the "move code that is idempotent inside the
> > retry" as a separate patch from documentation purposes)
>
> TBH, my main problem with this is that ESTALE retry logics had never
> felt right.  We ask e.g. filename_create() to get us the parent.  We
> tell it whether we want it to be maximally suspicious or not.  It
> still does the same RCU-normal-LOOKUP_REVAL sequence, only for "trust
> no one" variant it's RCU-LOOKUP_REVAL-LOOKUP_REVAL instead.  We are
> *not* told how far in that sequence did it have to get.  What's more,
> even if we had to get all way up to LOOKUP_REVAL, we ignore that
> when we do dcache lookup for the last component - only the argument
> of filename_create() is looked at.
>
> It really smells like the calling conventions are wrong.  I agree that
> all of that is, by definition, a very slow path - it's just that the
> logics makes me go "WTF?" every time I see it... ;-/
>
> Hell knows - perhaps the lookup_flags thing wants to be passed by
> reference (all the way into path_parentat()) and have the "we had
> to go for LOOKUP_REVAL" returned that way.  Not sure...
>
> Al, still crawling out of the bloody ptrace/asm glue horrors at the moment...

Re: [PATCH 0/7] namei: clean up retry logic in various do_* functions

[Dmitry Kadashev]

On Tue, Jul 13, 2021 at 2:02 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> See my comments about the individual patches - some of them change
> code flow, others do. And I think changing code flow as part of
> cleanup is ok, but it at the very least needs to be mentioned (and it
> might be good to do the "move code that is idempotent inside the
> retry" as a separate patch from documentation purposes)

Indeed, I should have at least included the flow changes into the commit
messages.

I'll send v2 with those comments addressed.

-- 
Dmitry Kadashev