Re: [PATCH v2 3/4] ovl: convert ovl_real_fdget_path() callers to ovl_real_file_path()

[Al Viro <viro@zeniv.linux.org.uk>]

On Sun, Oct 06, 2024 at 10:23:58AM +0200, Amir Goldstein wrote:
> Stop using struct fd to return a real file from ovl_real_fdget_path(),
> because we no longer return a temporary file object and the callers
> always get a borrowed file reference.
> 
> Rename the helper to ovl_real_file_path(), return a borrowed reference
> of the real file that is referenced from the overlayfs file or an error.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/file.c | 70 +++++++++++++++++++++++++--------------------
>  1 file changed, 39 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 42f9bbdd65b4..ead805e9f2d6 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c

> +static struct file *ovl_upper_file(const struct file *file, bool data)
>  {
>  	struct dentry *dentry = file_dentry(file);
>  	struct path realpath;
> @@ -193,12 +204,11 @@ static int ovl_upper_fdget(const struct file *file, struct fd *real, bool data)
>  	else
>  		type = ovl_path_real(dentry, &realpath);
>  
> -	real->word = 0;
>  	/* Not interested in lower nor in upper meta if data was requested */
>  	if (!OVL_TYPE_UPPER(type) || (data && OVL_TYPE_MERGE(type)))
> -		return 0;
> +		return NULL;
>  
> -	return ovl_real_fdget_path(file, real, &realpath);
> +	return ovl_real_file_path(file, &realpath);

AFAICS, we should never get NULL from ovl_real_file_path() now.

>  static int ovl_open(struct inode *inode, struct file *file)
> @@ -455,7 +465,7 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
>  
>  static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>  {
> -	struct fd real;
> +	struct file *realfile;
>  	const struct cred *old_cred;
>  	int ret;
>  
> @@ -463,19 +473,17 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>  	if (ret <= 0)
>  		return ret;
>  
> -	ret = ovl_upper_fdget(file, &real, datasync);
> -	if (ret || fd_empty(real))
> -		return ret;
> +	realfile = ovl_upper_file(file, datasync);
> +	if (IS_ERR_OR_NULL(realfile))
> +		return PTR_ERR(realfile);

... if so, the only source of NULL here would be the checks for OVL_TYPE_...
in ovl_upper_file().  Which has no other callers...

>  	/* Don't sync lower file for fear of receiving EROFS error */
> -	if (file_inode(fd_file(real)) == ovl_inode_upper(file_inode(file))) {
> +	if (file_inode(realfile) == ovl_inode_upper(file_inode(file))) {

Can that _not_ be true after the same checks in ovl_upper_file()?