[PATCH 0/3] whack user_path_at_empty, cleanup getname_flags

[PATCH 0/3] whack user_path_at_empty, cleanup getname_flags

[Mateusz Guzik]

I tried to take a stab at the atomic filename refcount thing [1], found
some easy cleanup to do as a soft prerequisite.

user_path_at_empty saddles getname_flags with an int * argument nobody
else uses, so it only results in everyone else having to pass NULL
there. This is trivially avoidable.

Should a need for user_path_at_empty it can probably be implemented in a
nicer manner than it was.

1: https://lore.kernel.org/linux-fsdevel/20240604132448.101183-1-mjguzik@gmail.com/T/#u

Mateusz Guzik (3):
  vfs: stop using user_path_at_empty in do_readlinkat
  vfs: retire user_path_at_empty and drop empty arg from getname_flags
  vfs: shave a branch in getname_flags

 fs/fsopen.c           |  2 +-
 fs/namei.c            | 41 +++++++++++++++++++------------------
 fs/stat.c             | 47 ++++++++++++++++++++++++-------------------
 include/linux/fs.h    |  2 +-
 include/linux/namei.h |  8 +-------
 io_uring/statx.c      |  3 +--
 io_uring/xattr.c      |  4 ++--
 7 files changed, 53 insertions(+), 54 deletions(-)

-- 
2.39.2

[PATCH 1/3] vfs: stop using user_path_at_empty in do_readlinkat

[Mateusz Guzik]

It is the only consumer and it saddles getname_flags with an argument set
to NULL by everyone else.

Instead the routine can do the empty check on its own.

Then user_path_at_empty can get retired and getname_flags can lose the
argument.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
 fs/stat.c | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/fs/stat.c b/fs/stat.c
index 70bd3e888cfa..7f7861544500 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -488,34 +488,39 @@ static int do_readlinkat(int dfd, const char __user *pathname,
 			 char __user *buf, int bufsiz)
 {
 	struct path path;
+	struct filename *name;
 	int error;
-	int empty = 0;
 	unsigned int lookup_flags = LOOKUP_EMPTY;
 
 	if (bufsiz <= 0)
 		return -EINVAL;
 
 retry:
-	error = user_path_at_empty(dfd, pathname, lookup_flags, &path, &empty);
-	if (!error) {
-		struct inode *inode = d_backing_inode(path.dentry);
+	name = getname_flags(pathname, lookup_flags, NULL);
+	error = filename_lookup(dfd, name, lookup_flags, &path, NULL);
+	if (unlikely(error)) {
+		putname(name);
+		return error;
+	}
 
-		error = empty ? -ENOENT : -EINVAL;
-		/*
-		 * AFS mountpoints allow readlink(2) but are not symlinks
-		 */
-		if (d_is_symlink(path.dentry) || inode->i_op->readlink) {
-			error = security_inode_readlink(path.dentry);
-			if (!error) {
-				touch_atime(&path);
-				error = vfs_readlink(path.dentry, buf, bufsiz);
-			}
-		}
-		path_put(&path);
-		if (retry_estale(error, lookup_flags)) {
-			lookup_flags |= LOOKUP_REVAL;
-			goto retry;
+	/*
+	 * AFS mountpoints allow readlink(2) but are not symlinks
+	 */
+	if (d_is_symlink(path.dentry) ||
+	    d_backing_inode(path.dentry)->i_op->readlink) {
+		error = security_inode_readlink(path.dentry);
+		if (!error) {
+			touch_atime(&path);
+			error = vfs_readlink(path.dentry, buf, bufsiz);
 		}
+	} else {
+		error = (name->name[0] == '\0') ? -ENOENT : -EINVAL;
+	}
+	path_put(&path);
+	putname(name);
+	if (retry_estale(error, lookup_flags)) {
+		lookup_flags |= LOOKUP_REVAL;
+		goto retry;
 	}
 	return error;
 }
-- 
2.39.2

Re: [PATCH 1/3] vfs: stop using user_path_at_empty in do_readlinkat

[Jan Kara]

On Tue 04-06-24 17:52:55, Mateusz Guzik wrote:
> It is the only consumer and it saddles getname_flags with an argument set
> to NULL by everyone else.
> 
> Instead the routine can do the empty check on its own.
> 
> Then user_path_at_empty can get retired and getname_flags can lose the
> argument.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/stat.c | 43 ++++++++++++++++++++++++-------------------
>  1 file changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/stat.c b/fs/stat.c
> index 70bd3e888cfa..7f7861544500 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -488,34 +488,39 @@ static int do_readlinkat(int dfd, const char __user *pathname,
>  			 char __user *buf, int bufsiz)
>  {
>  	struct path path;
> +	struct filename *name;
>  	int error;
> -	int empty = 0;
>  	unsigned int lookup_flags = LOOKUP_EMPTY;
>  
>  	if (bufsiz <= 0)
>  		return -EINVAL;
>  
>  retry:
> -	error = user_path_at_empty(dfd, pathname, lookup_flags, &path, &empty);
> -	if (!error) {
> -		struct inode *inode = d_backing_inode(path.dentry);
> +	name = getname_flags(pathname, lookup_flags, NULL);
> +	error = filename_lookup(dfd, name, lookup_flags, &path, NULL);
> +	if (unlikely(error)) {
> +		putname(name);
> +		return error;
> +	}
>  
> -		error = empty ? -ENOENT : -EINVAL;
> -		/*
> -		 * AFS mountpoints allow readlink(2) but are not symlinks
> -		 */
> -		if (d_is_symlink(path.dentry) || inode->i_op->readlink) {
> -			error = security_inode_readlink(path.dentry);
> -			if (!error) {
> -				touch_atime(&path);
> -				error = vfs_readlink(path.dentry, buf, bufsiz);
> -			}
> -		}
> -		path_put(&path);
> -		if (retry_estale(error, lookup_flags)) {
> -			lookup_flags |= LOOKUP_REVAL;
> -			goto retry;
> +	/*
> +	 * AFS mountpoints allow readlink(2) but are not symlinks
> +	 */
> +	if (d_is_symlink(path.dentry) ||
> +	    d_backing_inode(path.dentry)->i_op->readlink) {
> +		error = security_inode_readlink(path.dentry);
> +		if (!error) {
> +			touch_atime(&path);
> +			error = vfs_readlink(path.dentry, buf, bufsiz);
>  		}
> +	} else {
> +		error = (name->name[0] == '\0') ? -ENOENT : -EINVAL;
> +	}
> +	path_put(&path);
> +	putname(name);
> +	if (retry_estale(error, lookup_flags)) {
> +		lookup_flags |= LOOKUP_REVAL;
> +		goto retry;
>  	}
>  	return error;
>  }
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/3] vfs: retire user_path_at_empty and drop empty arg from getname_flags

[Mateusz Guzik]

No users after do_readlinkat started doing the job on its own.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
 fs/fsopen.c           |  2 +-
 fs/namei.c            | 16 +++++++---------
 fs/stat.c             |  6 +++---
 include/linux/fs.h    |  2 +-
 include/linux/namei.h |  8 +-------
 io_uring/statx.c      |  3 +--
 io_uring/xattr.c      |  4 ++--
 7 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/fs/fsopen.c b/fs/fsopen.c
index 6593ae518115..e7d0080c4f8b 100644
--- a/fs/fsopen.c
+++ b/fs/fsopen.c
@@ -451,7 +451,7 @@ SYSCALL_DEFINE5(fsconfig,
 		fallthrough;
 	case FSCONFIG_SET_PATH:
 		param.type = fs_value_is_filename;
-		param.name = getname_flags(_value, lookup_flags, NULL);
+		param.name = getname_flags(_value, lookup_flags);
 		if (IS_ERR(param.name)) {
 			ret = PTR_ERR(param.name);
 			goto out_key;
diff --git a/fs/namei.c b/fs/namei.c
index 37fb0a8aa09a..950ad6bdd9fe 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -126,7 +126,7 @@
 #define EMBEDDED_NAME_MAX	(PATH_MAX - offsetof(struct filename, iname))
 
 struct filename *
-getname_flags(const char __user *filename, int flags, int *empty)
+getname_flags(const char __user *filename, int flags)
 {
 	struct filename *result;
 	char *kname;
@@ -190,8 +190,6 @@ getname_flags(const char __user *filename, int flags, int *empty)
 	atomic_set(&result->refcnt, 1);
 	/* The empty path is special. */
 	if (unlikely(!len)) {
-		if (empty)
-			*empty = 1;
 		if (!(flags & LOOKUP_EMPTY)) {
 			putname(result);
 			return ERR_PTR(-ENOENT);
@@ -209,13 +207,13 @@ getname_uflags(const char __user *filename, int uflags)
 {
 	int flags = (uflags & AT_EMPTY_PATH) ? LOOKUP_EMPTY : 0;
 
-	return getname_flags(filename, flags, NULL);
+	return getname_flags(filename, flags);
 }
 
 struct filename *
 getname(const char __user * filename)
 {
-	return getname_flags(filename, 0, NULL);
+	return getname_flags(filename, 0);
 }
 
 struct filename *
@@ -2922,16 +2920,16 @@ int path_pts(struct path *path)
 }
 #endif
 
-int user_path_at_empty(int dfd, const char __user *name, unsigned flags,
-		 struct path *path, int *empty)
+int user_path_at(int dfd, const char __user *name, unsigned flags,
+		 struct path *path)
 {
-	struct filename *filename = getname_flags(name, flags, empty);
+	struct filename *filename = getname_flags(name, flags);
 	int ret = filename_lookup(dfd, filename, flags, path, NULL);
 
 	putname(filename);
 	return ret;
 }
-EXPORT_SYMBOL(user_path_at_empty);
+EXPORT_SYMBOL(user_path_at);
 
 int __check_sticky(struct mnt_idmap *idmap, struct inode *dir,
 		   struct inode *inode)
diff --git a/fs/stat.c b/fs/stat.c
index 7f7861544500..16aa1f5ceec4 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -300,7 +300,7 @@ int vfs_fstatat(int dfd, const char __user *filename,
 			return vfs_fstat(dfd, stat);
 	}
 
-	name = getname_flags(filename, getname_statx_lookup_flags(statx_flags), NULL);
+	name = getname_flags(filename, getname_statx_lookup_flags(statx_flags));
 	ret = vfs_statx(dfd, name, statx_flags, stat, STATX_BASIC_STATS);
 	putname(name);
 
@@ -496,7 +496,7 @@ static int do_readlinkat(int dfd, const char __user *pathname,
 		return -EINVAL;
 
 retry:
-	name = getname_flags(pathname, lookup_flags, NULL);
+	name = getname_flags(pathname, lookup_flags);
 	error = filename_lookup(dfd, name, lookup_flags, &path, NULL);
 	if (unlikely(error)) {
 		putname(name);
@@ -710,7 +710,7 @@ SYSCALL_DEFINE5(statx,
 	int ret;
 	struct filename *name;
 
-	name = getname_flags(filename, getname_statx_lookup_flags(flags), NULL);
+	name = getname_flags(filename, getname_statx_lookup_flags(flags));
 	ret = do_statx(dfd, name, flags, mask, buffer);
 	putname(name);
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 0283cf366c2a..dfe22a622df6 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2685,7 +2685,7 @@ static inline struct file *file_clone_open(struct file *file)
 }
 extern int filp_close(struct file *, fl_owner_t id);
 
-extern struct filename *getname_flags(const char __user *, int, int *);
+extern struct filename *getname_flags(const char __user *, int);
 extern struct filename *getname_uflags(const char __user *, int);
 extern struct filename *getname(const char __user *);
 extern struct filename *getname_kernel(const char *);
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 967aa9ea9f96..8ec8fed3bce8 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -50,13 +50,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT};
 
 extern int path_pts(struct path *path);
 
-extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
-
-static inline int user_path_at(int dfd, const char __user *name, unsigned flags,
-		 struct path *path)
-{
-	return user_path_at_empty(dfd, name, flags, path, NULL);
-}
+extern int user_path_at(int, const char __user *, unsigned, struct path *);
 
 struct dentry *lookup_one_qstr_excl(const struct qstr *name,
 				    struct dentry *base,
diff --git a/io_uring/statx.c b/io_uring/statx.c
index abb874209caa..f7f9b202eec0 100644
--- a/io_uring/statx.c
+++ b/io_uring/statx.c
@@ -37,8 +37,7 @@ int io_statx_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	sx->flags = READ_ONCE(sqe->statx_flags);
 
 	sx->filename = getname_flags(path,
-				     getname_statx_lookup_flags(sx->flags),
-				     NULL);
+				     getname_statx_lookup_flags(sx->flags));
 
 	if (IS_ERR(sx->filename)) {
 		int ret = PTR_ERR(sx->filename);
diff --git a/io_uring/xattr.c b/io_uring/xattr.c
index 44905b82eea8..6cf41c3bc369 100644
--- a/io_uring/xattr.c
+++ b/io_uring/xattr.c
@@ -96,7 +96,7 @@ int io_getxattr_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 
 	path = u64_to_user_ptr(READ_ONCE(sqe->addr3));
 
-	ix->filename = getname_flags(path, LOOKUP_FOLLOW, NULL);
+	ix->filename = getname_flags(path, LOOKUP_FOLLOW);
 	if (IS_ERR(ix->filename)) {
 		ret = PTR_ERR(ix->filename);
 		ix->filename = NULL;
@@ -189,7 +189,7 @@ int io_setxattr_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 
 	path = u64_to_user_ptr(READ_ONCE(sqe->addr3));
 
-	ix->filename = getname_flags(path, LOOKUP_FOLLOW, NULL);
+	ix->filename = getname_flags(path, LOOKUP_FOLLOW);
 	if (IS_ERR(ix->filename)) {
 		ret = PTR_ERR(ix->filename);
 		ix->filename = NULL;
-- 
2.39.2

Re: [PATCH 2/3] vfs: retire user_path_at_empty and drop empty arg from getname_flags

[Jan Kara]

On Tue 04-06-24 17:52:56, Mateusz Guzik wrote:
> No users after do_readlinkat started doing the job on its own.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/fsopen.c           |  2 +-
>  fs/namei.c            | 16 +++++++---------
>  fs/stat.c             |  6 +++---
>  include/linux/fs.h    |  2 +-
>  include/linux/namei.h |  8 +-------
>  io_uring/statx.c      |  3 +--
>  io_uring/xattr.c      |  4 ++--
>  7 files changed, 16 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/fsopen.c b/fs/fsopen.c
> index 6593ae518115..e7d0080c4f8b 100644
> --- a/fs/fsopen.c
> +++ b/fs/fsopen.c
> @@ -451,7 +451,7 @@ SYSCALL_DEFINE5(fsconfig,
>  		fallthrough;
>  	case FSCONFIG_SET_PATH:
>  		param.type = fs_value_is_filename;
> -		param.name = getname_flags(_value, lookup_flags, NULL);
> +		param.name = getname_flags(_value, lookup_flags);
>  		if (IS_ERR(param.name)) {
>  			ret = PTR_ERR(param.name);
>  			goto out_key;
> diff --git a/fs/namei.c b/fs/namei.c
> index 37fb0a8aa09a..950ad6bdd9fe 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -126,7 +126,7 @@
>  #define EMBEDDED_NAME_MAX	(PATH_MAX - offsetof(struct filename, iname))
>  
>  struct filename *
> -getname_flags(const char __user *filename, int flags, int *empty)
> +getname_flags(const char __user *filename, int flags)
>  {
>  	struct filename *result;
>  	char *kname;
> @@ -190,8 +190,6 @@ getname_flags(const char __user *filename, int flags, int *empty)
>  	atomic_set(&result->refcnt, 1);
>  	/* The empty path is special. */
>  	if (unlikely(!len)) {
> -		if (empty)
> -			*empty = 1;
>  		if (!(flags & LOOKUP_EMPTY)) {
>  			putname(result);
>  			return ERR_PTR(-ENOENT);
> @@ -209,13 +207,13 @@ getname_uflags(const char __user *filename, int uflags)
>  {
>  	int flags = (uflags & AT_EMPTY_PATH) ? LOOKUP_EMPTY : 0;
>  
> -	return getname_flags(filename, flags, NULL);
> +	return getname_flags(filename, flags);
>  }
>  
>  struct filename *
>  getname(const char __user * filename)
>  {
> -	return getname_flags(filename, 0, NULL);
> +	return getname_flags(filename, 0);
>  }
>  
>  struct filename *
> @@ -2922,16 +2920,16 @@ int path_pts(struct path *path)
>  }
>  #endif
>  
> -int user_path_at_empty(int dfd, const char __user *name, unsigned flags,
> -		 struct path *path, int *empty)
> +int user_path_at(int dfd, const char __user *name, unsigned flags,
> +		 struct path *path)
>  {
> -	struct filename *filename = getname_flags(name, flags, empty);
> +	struct filename *filename = getname_flags(name, flags);
>  	int ret = filename_lookup(dfd, filename, flags, path, NULL);
>  
>  	putname(filename);
>  	return ret;
>  }
> -EXPORT_SYMBOL(user_path_at_empty);
> +EXPORT_SYMBOL(user_path_at);
>  
>  int __check_sticky(struct mnt_idmap *idmap, struct inode *dir,
>  		   struct inode *inode)
> diff --git a/fs/stat.c b/fs/stat.c
> index 7f7861544500..16aa1f5ceec4 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -300,7 +300,7 @@ int vfs_fstatat(int dfd, const char __user *filename,
>  			return vfs_fstat(dfd, stat);
>  	}
>  
> -	name = getname_flags(filename, getname_statx_lookup_flags(statx_flags), NULL);
> +	name = getname_flags(filename, getname_statx_lookup_flags(statx_flags));
>  	ret = vfs_statx(dfd, name, statx_flags, stat, STATX_BASIC_STATS);
>  	putname(name);
>  
> @@ -496,7 +496,7 @@ static int do_readlinkat(int dfd, const char __user *pathname,
>  		return -EINVAL;
>  
>  retry:
> -	name = getname_flags(pathname, lookup_flags, NULL);
> +	name = getname_flags(pathname, lookup_flags);
>  	error = filename_lookup(dfd, name, lookup_flags, &path, NULL);
>  	if (unlikely(error)) {
>  		putname(name);
> @@ -710,7 +710,7 @@ SYSCALL_DEFINE5(statx,
>  	int ret;
>  	struct filename *name;
>  
> -	name = getname_flags(filename, getname_statx_lookup_flags(flags), NULL);
> +	name = getname_flags(filename, getname_statx_lookup_flags(flags));
>  	ret = do_statx(dfd, name, flags, mask, buffer);
>  	putname(name);
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 0283cf366c2a..dfe22a622df6 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2685,7 +2685,7 @@ static inline struct file *file_clone_open(struct file *file)
>  }
>  extern int filp_close(struct file *, fl_owner_t id);
>  
> -extern struct filename *getname_flags(const char __user *, int, int *);
> +extern struct filename *getname_flags(const char __user *, int);
>  extern struct filename *getname_uflags(const char __user *, int);
>  extern struct filename *getname(const char __user *);
>  extern struct filename *getname_kernel(const char *);
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 967aa9ea9f96..8ec8fed3bce8 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -50,13 +50,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT};
>  
>  extern int path_pts(struct path *path);
>  
> -extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
> -
> -static inline int user_path_at(int dfd, const char __user *name, unsigned flags,
> -		 struct path *path)
> -{
> -	return user_path_at_empty(dfd, name, flags, path, NULL);
> -}
> +extern int user_path_at(int, const char __user *, unsigned, struct path *);
>  
>  struct dentry *lookup_one_qstr_excl(const struct qstr *name,
>  				    struct dentry *base,
> diff --git a/io_uring/statx.c b/io_uring/statx.c
> index abb874209caa..f7f9b202eec0 100644
> --- a/io_uring/statx.c
> +++ b/io_uring/statx.c
> @@ -37,8 +37,7 @@ int io_statx_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  	sx->flags = READ_ONCE(sqe->statx_flags);
>  
>  	sx->filename = getname_flags(path,
> -				     getname_statx_lookup_flags(sx->flags),
> -				     NULL);
> +				     getname_statx_lookup_flags(sx->flags));
>  
>  	if (IS_ERR(sx->filename)) {
>  		int ret = PTR_ERR(sx->filename);
> diff --git a/io_uring/xattr.c b/io_uring/xattr.c
> index 44905b82eea8..6cf41c3bc369 100644
> --- a/io_uring/xattr.c
> +++ b/io_uring/xattr.c
> @@ -96,7 +96,7 @@ int io_getxattr_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  
>  	path = u64_to_user_ptr(READ_ONCE(sqe->addr3));
>  
> -	ix->filename = getname_flags(path, LOOKUP_FOLLOW, NULL);
> +	ix->filename = getname_flags(path, LOOKUP_FOLLOW);
>  	if (IS_ERR(ix->filename)) {
>  		ret = PTR_ERR(ix->filename);
>  		ix->filename = NULL;
> @@ -189,7 +189,7 @@ int io_setxattr_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  
>  	path = u64_to_user_ptr(READ_ONCE(sqe->addr3));
>  
> -	ix->filename = getname_flags(path, LOOKUP_FOLLOW, NULL);
> +	ix->filename = getname_flags(path, LOOKUP_FOLLOW);
>  	if (IS_ERR(ix->filename)) {
>  		ret = PTR_ERR(ix->filename);
>  		ix->filename = NULL;
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 3/3] vfs: shave a branch in getname_flags

[Mateusz Guzik]

Check for an error while copying and no path in one branch.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
 fs/namei.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 950ad6bdd9fe..f25dcb9077dd 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -148,9 +148,20 @@ getname_flags(const char __user *filename, int flags)
 	result->name = kname;
 
 	len = strncpy_from_user(kname, filename, EMBEDDED_NAME_MAX);
-	if (unlikely(len < 0)) {
-		__putname(result);
-		return ERR_PTR(len);
+	/*
+	 * Handle both empty path and copy failure in one go.
+	 */
+	if (unlikely(len <= 0)) {
+		if (unlikely(len < 0)) {
+			__putname(result);
+			return ERR_PTR(len);
+		}
+
+		/* The empty path is special. */
+		if (!(flags & LOOKUP_EMPTY)) {
+			__putname(result);
+			return ERR_PTR(-ENOENT);
+		}
 	}
 
 	/*
@@ -188,14 +199,6 @@ getname_flags(const char __user *filename, int flags)
 	}
 
 	atomic_set(&result->refcnt, 1);
-	/* The empty path is special. */
-	if (unlikely(!len)) {
-		if (!(flags & LOOKUP_EMPTY)) {
-			putname(result);
-			return ERR_PTR(-ENOENT);
-		}
-	}
-
 	result->uptr = filename;
 	result->aname = NULL;
 	audit_getname(result);
-- 
2.39.2

Re: [PATCH 3/3] vfs: shave a branch in getname_flags

[Christian Brauner]

On Tue, Jun 04, 2024 at 05:52:57PM +0200, Mateusz Guzik wrote:
> Check for an error while copying and no path in one branch.

Fine to move that check up but we need to redo it after we recopied.
It's extremely unlikely but the contents could've changed iirc. I can
fix that up though.

Re: [PATCH 3/3] vfs: shave a branch in getname_flags

[Mateusz Guzik]

On Wed, Jun 5, 2024 at 5:03 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Tue, Jun 04, 2024 at 05:52:57PM +0200, Mateusz Guzik wrote:
> > Check for an error while copying and no path in one branch.
>
> Fine to move that check up but we need to redo it after we recopied.
> It's extremely unlikely but the contents could've changed iirc. I can
> fix that up though.

Oh I see, you mean the buffer might have initially been too big so it
landed in the zmalloc fallback, but by that time userspace might have
played games and slapped a NUL char in there.

Fair, but also trivial to fix up, so I would appreciate if you sorted
it out, especially since you offered :)

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH 3/3] vfs: shave a branch in getname_flags

[Jan Kara]

On Tue 04-06-24 17:52:57, Mateusz Guzik wrote:
> Check for an error while copying and no path in one branch.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>

Looks good to me modulo the need for rechecking Christian already pointed
out. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>


								Honza

> ---
>  fs/namei.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 950ad6bdd9fe..f25dcb9077dd 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -148,9 +148,20 @@ getname_flags(const char __user *filename, int flags)
>  	result->name = kname;
>  
>  	len = strncpy_from_user(kname, filename, EMBEDDED_NAME_MAX);
> -	if (unlikely(len < 0)) {
> -		__putname(result);
> -		return ERR_PTR(len);
> +	/*
> +	 * Handle both empty path and copy failure in one go.
> +	 */
> +	if (unlikely(len <= 0)) {
> +		if (unlikely(len < 0)) {
> +			__putname(result);
> +			return ERR_PTR(len);
> +		}
> +
> +		/* The empty path is special. */
> +		if (!(flags & LOOKUP_EMPTY)) {
> +			__putname(result);
> +			return ERR_PTR(-ENOENT);
> +		}
>  	}
>  
>  	/*
> @@ -188,14 +199,6 @@ getname_flags(const char __user *filename, int flags)
>  	}
>  
>  	atomic_set(&result->refcnt, 1);
> -	/* The empty path is special. */
> -	if (unlikely(!len)) {
> -		if (!(flags & LOOKUP_EMPTY)) {
> -			putname(result);
> -			return ERR_PTR(-ENOENT);
> -		}
> -	}
> -
>  	result->uptr = filename;
>  	result->aname = NULL;
>  	audit_getname(result);
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/3] whack user_path_at_empty, cleanup getname_flags

[Christian Brauner]

On Tue, 04 Jun 2024 17:52:54 +0200, Mateusz Guzik wrote:
> I tried to take a stab at the atomic filename refcount thing [1], found
> some easy cleanup to do as a soft prerequisite.
> 
> user_path_at_empty saddles getname_flags with an int * argument nobody
> else uses, so it only results in everyone else having to pass NULL
> there. This is trivially avoidable.
> 
> [...]

Snatched with some minor changes unless I hear complaints.

---

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc 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.misc

[1/3] vfs: stop using user_path_at_empty in do_readlinkat
      https://git.kernel.org/vfs/vfs/c/d63b3a67520b
[2/3] vfs: retire user_path_at_empty and drop empty arg from getname_flags
      https://git.kernel.org/vfs/vfs/c/a01c264715dc
[3/3] vfs: shave a branch in getname_flags
      https://git.kernel.org/vfs/vfs/c/12a8c8f491b4