Re: [PATCH v2 3/3] fs: store real path instead of fake path in backing file f_path

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

On Sat, Oct 07, 2023 at 11:44:33AM +0300, Amir Goldstein wrote:

> -		if (real_path->mnt)
> -			mnt_put_write_access(real_path->mnt);
> +		if (user_path->mnt)
> +			mnt_put_write_access(user_path->mnt);
>  	}
>  }

Again, how can the predicates be ever false here?  We should *not*
have struct path with NULL .mnt unless it's {NULL, NULL} pair.

For the record, struct path with NULL .dentry and non-NULL .mnt
*IS* possible, but only in a very narrow area - if, during
an attempt to fall back from rcu pathwalk to normal one we
have __legitimize_path() successfully validate (== grab) the
reference to mount, but fail to validate dentry.  In that
case we need to drop mount, but not dentry when we get to
cleanup (pretty much as soon as we drop rcu_read_lock()).
That gets indicated by clearing path->dentry, and only
while we are propagating the error back to the point where
references would be dropped.  No filesystem code should
ever see struct path instances in that state.

Please, don't make the things more confusing; "incomplete"
struct path like that are very much not normal (and this
variety is flat-out impossible).


> @@ -34,9 +34,18 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
>  	struct dentry *real = NULL, *lower;
>  	int err;
>  
> -	/* It's an overlay file */
> +	/*
> +	 * vfs is only expected to call d_real() with NULL from d_real_inode()
> +	 * and with overlay inode from file_dentry() on an overlay file.
> +	 *
> +	 * TODO: remove @inode argument from d_real() API, remove code in this
> +	 * function that deals with non-NULL @inode and remove d_real() call
> +	 * from file_dentry().
> +	 */
>  	if (inode && d_inode(dentry) == inode)
>  		return dentry;
> +	else
> +		WARN_ON_ONCE(inode);
>  
>  	if (!d_is_reg(dentry)) {
>  		if (!inode || inode == d_inode(dentry))
		   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	BTW, that condition is confusing as hell (both before and
after this patch).  AFAICS, it's a pointlessly obfuscated
		if (!inode)
Look: we get to evaluating that test only if we hadn't buggered
off on
 	if (inode && d_inode(dentry) == inode)
 		return dentry;
above.  Which means that either inode is NULL (in which case the
evaluation yields true as soon as we see that !inode is true) or
it's neither NULL nor equal to d_inode(dentry).  In which case
we see that !inode is false and proceed yield false *after*
comparing inode with d_inode(dentry) and seeing that they
are not equal.

<checks history>
e8c985bace13 "ovl: deal with overlay files in ovl_d_real()"
had introduced the first check, and nobody noticed that the
older check below could've been simplified.  Oh, well...

> -static inline const struct path *file_real_path(struct file *f)
> +static inline const struct path *f_path(struct file *f)
>  {
> -	if (unlikely(f->f_mode & FMODE_BACKING))
> -		return backing_file_real_path(f);
>  	return &f->f_path;
>  }

Bad name, IMO - makes grepping harder and... what the hell do
we need it for, anyway?  You have only one caller, and no
obvious reason why it would be worse off as path = &file->f_path...