Re: [PATCH RFC 2/2] ovl: invalidate dentry if lower was renamed

[Vivek Goyal <vgoyal@redhat.com>]

On Mon, Jul 13, 2020 at 01:57:32PM +0300, Amir Goldstein wrote:
> Changes to lower layer 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 dentries in the lowerstack
> was renamed since the lowerstack was composed.
> 
> To be absolute certain that lower dentry was not renamed we would need to
> know the redirect path that lead to it, but that is not necessary.
> Instead, we just store the hash of the parent/name from when we composed
> the stack, which gives a good enough probablity to detect a lower rename
> and is much less complexity.
> 
> We do not provide this protection for upper dentries, because that would
> require updating the hash on overlay initiated renames and that is harder
> to implement with lockless lookup.
> 
> 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 lower rename, because
> lookup cannot return those invalid dentry stacks.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/export.c    |  1 +
>  fs/overlayfs/namei.c     |  4 +++-
>  fs/overlayfs/ovl_entry.h |  2 ++
>  fs/overlayfs/super.c     | 17 ++++++++++-------
>  4 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 0e696f72cf65..7221b6226e26 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -319,6 +319,7 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
>  	if (lower) {
>  		oe->lowerstack->dentry = dget(lower);
>  		oe->lowerstack->layer = lowerpath->layer;
> +		oe->lowerstack->hash = lower->d_name.hash_len;
>  	}
>  	dentry->d_fsdata = oe;
>  	if (upper_alias)
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 3566282a9199..ae1c1216a038 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -375,7 +375,8 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
>  	}
>  	**stackp = (struct ovl_path){
>  		.dentry = origin,
> -		.layer = &ofs->layers[i]
> +		.layer = &ofs->layers[i],
> +		.hash = origin->d_name.hash_len,
>  	};
>  
>  	return 0;
> @@ -968,6 +969,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>  		} else {
>  			stack[ctr].dentry = this;
>  			stack[ctr].layer = lower.layer;
> +			stack[ctr].hash = this->d_name.hash_len;
>  			ctr++;
>  		}
>  
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index b429c80879ee..557f1782f53b 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -42,6 +42,8 @@ struct ovl_layer {
>  struct ovl_path {
>  	const struct ovl_layer *layer;
>  	struct dentry *dentry;
> +	/* Hash of the lower parent/name when we found it */
> +	u64 hash;
>  };
>  
>  /* private information held for overlayfs's superblock */
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index f2c74387e05b..4b7cb2d98203 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -119,13 +119,13 @@ static bool ovl_dentry_is_dead(struct dentry *d)
>  }
>  
>  static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak,
> -			       bool is_upper)
> +			       bool is_upper, u64 hash)
>  {
>  	bool strict = !weak;
>  	int ret = 1;
>  
> -	/* Invalidate dentry if real was deleted since we found it */
> -	if (ovl_dentry_is_dead(d)) {
> +	/* Invalidate dentry if real was deleted/renamed since we found it */
> +	if (ovl_dentry_is_dead(d) || (hash && hash != d->d_name.hash_len)) {

So if lower hash_len changes, on local filesystem we will return -ESTALE?
I am assuming we did that for remote filesystems and now we will do
that for local filesystems as well?

Thanks
Vivek

>  		ret = 0;
>  		/* Raced with overlay unlink/rmdir? */
>  		if (is_upper)
> @@ -156,17 +156,18 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry,
>  					unsigned int flags, bool weak)
>  {
>  	struct ovl_entry *oe = dentry->d_fsdata;
> +	struct ovl_path *lower = oe->lowerstack;
>  	struct dentry *upper;
>  	unsigned int i;
>  	int ret = 1;
>  
>  	upper = ovl_dentry_upper(dentry);
>  	if (upper)
> -		ret = ovl_revalidate_real(upper, flags, weak, true);
> +		ret = ovl_revalidate_real(upper, flags, weak, true, 0);
>  
> -	for (i = 0; ret > 0 && i < oe->numlower; i++) {
> -		ret = ovl_revalidate_real(oe->lowerstack[i].dentry, flags,
> -					  weak, false);
> +	for (i = 0; ret > 0 && i < oe->numlower; i++, lower++) {
> +		ret = ovl_revalidate_real(lower->dentry, flags, weak, false,
> +					  lower->hash);
>  	}
>  	return ret;
>  }
> @@ -1652,6 +1653,8 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>  	for (i = 0; i < numlower; i++) {
>  		oe->lowerstack[i].dentry = dget(stack[i].dentry);
>  		oe->lowerstack[i].layer = &ofs->layers[i+1];
> +		/* layer root should not be invalidated by rename */
> +		oe->lowerstack->hash = 0;
>  	}
>  
>  out:
> -- 
> 2.17.1
>