Re: [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir

[Vivek Goyal <vgoyal@redhat.com>]

On Mon, Jul 13, 2020 at 01:57:31PM +0300, Amir Goldstein wrote:
> Changes to underlying layers while overlay in mounted result in
> undefined behavior.  Therefore, we can change the behavior to
> invalidate the overlay dentry on dcache lookup if one of the
> underlying dentries was deleted since the dentry was composed.
> 
> Negative underlying dentries are not expected in overlay upper and
> lower dentries.  If they are found it is probably dcache lookup racing
> with an overlay unlink, before d_drop() was called on the overlay dentry.
> IS_DEADDIR directories may be caused by underlying rmdir, so invalidate
> overlay dentry on dcache lookup if we find those.

Can you elaborate a bit more on this race. Doesn't inode_lock_nested(dir)
protect against that. I see that both vfs_rmdir() and vfs_unlink()
happen with parent directory inode mutex held exclusively. And IIUC,
that should mean no further lookup()/->revalidate() must be in progress
on that dentry? I might very well be wrong, hence asking for more
details.

Thanks
Vivek

> 
> We preserve the legacy behaior of returning -ESTALE on invalid cache
> for lower dentries, but we relax this behavior for upper dentries
> that may be invalidated by a race with overlay unlink/rmdir.
> 
> This doesn't make live changes to underlying layers valid, because
> invalid dentry stacks may still be referenced by open files, but it
> reduces the window for possible bugs caused by underlying delete,
> because lookup cannot return those invalid dentry stacks.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/super.c | 41 +++++++++++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 06ec3cb977e6..f2c74387e05b 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -113,21 +113,42 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
>  	return dentry;
>  }
>  
> -static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak)
> +static bool ovl_dentry_is_dead(struct dentry *d)
>  {
> +	return unlikely(!d->d_inode || IS_DEADDIR(d->d_inode));
> +}
> +
> +static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak,
> +			       bool is_upper)
> +{
> +	bool strict = !weak;
>  	int ret = 1;
>  
> -	if (weak) {
> +	/* Invalidate dentry if real was deleted since we found it */
> +	if (ovl_dentry_is_dead(d)) {
> +		ret = 0;
> +		/* Raced with overlay unlink/rmdir? */
> +		if (is_upper)
> +			strict = false;

> +	} else if (weak) {
>  		if (d->d_flags & DCACHE_OP_WEAK_REVALIDATE)
> -			ret =  d->d_op->d_weak_revalidate(d, flags);
> +			ret = d->d_op->d_weak_revalidate(d, flags);
>  	} else if (d->d_flags & DCACHE_OP_REVALIDATE) {
>  		ret = d->d_op->d_revalidate(d, flags);
> -		if (!ret) {
> -			if (!(flags & LOOKUP_RCU))
> -				d_invalidate(d);
> -			ret = -ESTALE;
> -		}
>  	}
> +
> +	/*
> +	 * Legacy overlayfs strict behavior is to return an error to user on
> +	 * non-weak revalidate rather than retry the lookup, because underlying
> +	 * layer changes are not expected. We may want to relax this in the
> +	 * future either for upper only or also for lower.
> +	 */
> +	if (strict && !ret) {
> +		if (!(flags & LOOKUP_RCU))
> +			d_invalidate(d);
> +		ret = -ESTALE;
> +	}
> +
>  	return ret;
>  }
>  
> @@ -141,11 +162,11 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry,
>  
>  	upper = ovl_dentry_upper(dentry);
>  	if (upper)
> -		ret = ovl_revalidate_real(upper, flags, weak);
> +		ret = ovl_revalidate_real(upper, flags, weak, true);
>  
>  	for (i = 0; ret > 0 && i < oe->numlower; i++) {
>  		ret = ovl_revalidate_real(oe->lowerstack[i].dentry, flags,
> -					  weak);
> +					  weak, false);
>  	}
>  	return ret;
>  }
> -- 
> 2.17.1
>