Re: [PATCH] overlayfs: Fix redirect traversal on metacopy dentries

[Vivek Goyal <vgoyal@redhat.com>]

On Tue, Jun 02, 2020 at 11:23:38AM -0400, Vivek Goyal wrote:
> Amir pointed me to metacopy test cases in unionmount-testsuite and
> I decided to run "./run --ov=10 --meta" and it failed while running
> test "rename-mass-5.py".

Hi Miklos,

This patch applies on top of the patch series I posted previously.

https://marc.info/?l=linux-unionfs&m=159102703820108&w=2

Thanks
Vivek

> 
> Problem is w.r.t absolute redirect traversal on intermediate metacopy
> dentry. We do not store intermediate metacopy dentries and also skip
> current loop/layer and move onto lookup in next layer. But at the end
> of loop, we have logic to reset "poe" and layer index if currnently
> looked up dentry has absolute redirect. We skip all that and that
> means lookup in next layer will fail.
> 
> Following is simple test case to reproduce this.
> 
> - mkdir -p lower upper work merged lower/a lower/b
> - touch lower/a/foo.txt
> - mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work,metacopy=on none merged
> 
> # Following will create absolute redirect "/a/foo.txt" on upper/b/bar.txt.
> - mv merged/a/foo.txt merged/b/bar.txt
> 
> # unmount overlay and use upper as lower layer (lower2) for next mount.
> - umount merged
> - mv upper lower2
> - rm -rf work; mkdir -p upper work
> - mount -t overlay -o lowerdir=lower2:lower,upperdir=upper,workdir=work,metacopy=on none merged
> 
> # Force a metacopy copy-up
> - chown bin:bin merged/b/bar.txt
> 
> # unmount overlay and use upper as lower layer (lower3) for next mount.
> - umount merged
> - mv upper lower3
> - rm -rf work; mkdir -p upper work
> - mount -t overlay -o lowerdir=lower3:lower2:lower,upperdir=upper,workdir=work,metacopy=on none merged
> 
> # ls merged/b/bar.txt
> ls: cannot access 'bar.txt': Input/output error
> 
> Intermediate lower layer (lower2) has metacopy dentry b/bar.txt with absolute
> redirect "/a/foo.txt". We skipped redirect processing at the end of loop
> which sets poe to roe and sets the appropriate next lower layer index. And
> that means lookup failed in next layer.
> 
> Fix this by continuing the loop for any intermediate dentries. We still do not
> save these at lower stack. With this fix applied unionmount-testsuite,
> "./run --ov-10 --meta" now passes.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/namei.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index da05e33db9ce..df81ec0e179f 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -913,15 +913,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>  			goto out_put;
>  		}
>  
> -		/*
> -		 * Do not store intermediate metacopy dentries in chain,
> -		 * except top most lower metacopy dentry
> -		 */
> -		if (d.metacopy && ctr) {
> -			dput(this);
> -			continue;
> -		}
> -
>  		/*
>  		 * If no origin fh is stored in upper of a merge dir, store fh
>  		 * of lower dir and set upper parent "impure".
> @@ -956,9 +947,20 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>  			origin = this;
>  		}
>  
> -		stack[ctr].dentry = this;
> -		stack[ctr].layer = lower.layer;
> -		ctr++;
> +		if (d.metacopy && ctr) {
> +			/*
> +			 * Do not store intermediate metacopy dentries in
> +			 * lower chain, except top most lower metacopy dentry.
> +			 * Continue the loop so that if there is an absolute
> +			 * redirect on this dentry, poe can be reset to roe.
> +			 */
> +			dput(this);
> +			this = NULL;
> +		} else {
> +			stack[ctr].dentry = this;
> +			stack[ctr].layer = lower.layer;
> +			ctr++;
> +		}
>  
>  		/*
>  		 * Following redirects can have security consequences: it's like
> -- 
> 2.25.4
>