Received-SPF: pass (google.com: domain of linux-kernel+bounces-202863-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 5 Jun 2024 17:48:31 +0200
From: Jan Kara <jack@suse.cz>
To: Mateusz Guzik <mjguzik@gmail.com>
Cc: brauner@kernel.org, viro@zeniv.linux.org.uk, jack@suse.cz,
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 1/3] vfs: stop using user_path_at_empty in do_readlinkat
Message-ID: <20240605154831.6gdqv4uk7o6ufgha@quack3>
References: <20240604155257.109500-1-mjguzik@gmail.com>
 <20240604155257.109500-2-mjguzik@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240604155257.109500-2-mjguzik@gmail.com>

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