On Tue, Jun 2, 2020 at 6:23 PM Vivek Goyal <vgoyal@xxxxxxxxxx> 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".
>
> 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@xxxxxxxxxx>
I though I ran this test - I guess not...
Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
> 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
>
