[PATCH v2 0/4] ovl: Bunch of ovl_lookup() path fixes

[PATCH v2 0/4] ovl: Bunch of ovl_lookup() path fixes

[Vivek Goyal]

Hi,

Here are bunch of ovl_lookup() related fixes. These are cleanup also
and make some semantics more clear. Which should be useful when non-dir
dentries will have redirects. (metacopy patches).

Thanks
Vivek

Amir Goldstein (1):
  ovl: fix lookup with middle layer opaque dir and absolute path
    redirects

Vivek Goyal (3):
  ovl: Set d->last properly during lookup
  ovl: Do not check for redirect if this is last layer
  ovl: set d->is_dir and d->opaque for last path element

 fs/overlayfs/namei.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

-- 
2.13.6

[PATCH v2 1/4] ovl: Set d->last properly during lookup

[Vivek Goyal]

d->last signifies that this is the last layer we are looking into and there
is no more. And that means this allows for some optimzation opportunities
during lookup. For example, in ovl_lookup_single() we don't have to check
for opaque xattr of a directory is this is the last layer we are looking
into (d->last = true).

But knowing for sure whether we are looking into last layer can be very
tricky. If redirects are not enabled, then we can look at poe->numlower
and figure out if the lookup we are about to is last layer or not. But
if redircts are enabled then it is possible poe->numlower suggests that
we are looking in last layer, but there is an absolute redirect present
in found element and that redirects us to a layer in root and that means
lookup will continue in lower layers further.

For example, consider following.

/upperdir/pure (opaque=y)
/upperdir/pure/foo (opaque=y,redirect=/bar)
/lowerdir/bar

In this case pure is "pure upper". When we look for "foo", that time
poe->numlower=0. But that alone does not mean that we will not search
for a merge candidate in /lowerdir. Absolute redirect changes that.

IOW, d->last should not be set just based on poe->numlower if redirects
are enabled. That can lead to setting d->last while it should not have
and that means we will not check for opaque xattr while we should have.

So do this.

- If redirects are not enabled, then continue to rely on poe->numlower
  information to determine if it is last layer or not.

- If redirects are enabled, then set d->last = true only if this is the
  last layer in root ovl_entry (roe).

Suggested-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/namei.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index de3e6da1d5a5..78798367c4f0 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -815,7 +815,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		.is_dir = false,
 		.opaque = false,
 		.stop = false,
-		.last = !poe->numlower,
+		.last = ofs->config.redirect_follow ? false : !poe->numlower,
 		.redirect = NULL,
 	};
 
@@ -873,7 +873,11 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	for (i = 0; !d.stop && i < poe->numlower; i++) {
 		struct ovl_path lower = poe->lowerstack[i];
 
-		d.last = i == poe->numlower - 1;
+		if (!ofs->config.redirect_follow)
+			d.last = i == poe->numlower - 1;
+		else
+			d.last = lower.layer->idx == roe->numlower;
+
 		err = ovl_lookup_layer(lower.dentry, &d, &this);
 		if (err)
 			goto out_put;
-- 
2.13.6

Re: [PATCH v2 1/4] ovl: Set d->last properly during lookup

[Miklos Szeredi]

On Fri, Mar 9, 2018 at 9:44 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> d->last signifies that this is the last layer we are looking into and there
> is no more. And that means this allows for some optimzation opportunities
> during lookup. For example, in ovl_lookup_single() we don't have to check
> for opaque xattr of a directory is this is the last layer we are looking
> into (d->last = true).
>
> But knowing for sure whether we are looking into last layer can be very
> tricky. If redirects are not enabled, then we can look at poe->numlower
> and figure out if the lookup we are about to is last layer or not. But
> if redircts are enabled then it is possible poe->numlower suggests that
> we are looking in last layer, but there is an absolute redirect present
> in found element and that redirects us to a layer in root and that means
> lookup will continue in lower layers further.
>
> For example, consider following.
>
> /upperdir/pure (opaque=y)
> /upperdir/pure/foo (opaque=y,redirect=/bar)
> /lowerdir/bar
>
> In this case pure is "pure upper". When we look for "foo", that time
> poe->numlower=0. But that alone does not mean that we will not search
> for a merge candidate in /lowerdir. Absolute redirect changes that.
>
> IOW, d->last should not be set just based on poe->numlower if redirects
> are enabled. That can lead to setting d->last while it should not have
> and that means we will not check for opaque xattr while we should have.
>
> So do this.
>
> - If redirects are not enabled, then continue to rely on poe->numlower
>   information to determine if it is last layer or not.
>
> - If redirects are enabled, then set d->last = true only if this is the
>   last layer in root ovl_entry (roe).
>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Added:

Fixes: 02b69b284cd7 ("ovl: lookup redirects")
Cc: <stable@vger.kernel.org> #v4.10


> ---
>  fs/overlayfs/namei.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index de3e6da1d5a5..78798367c4f0 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -815,7 +815,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 .is_dir = false,
>                 .opaque = false,
>                 .stop = false,
> -               .last = !poe->numlower,
> +               .last = ofs->config.redirect_follow ? false : !poe->numlower,
>                 .redirect = NULL,
>         };
>
> @@ -873,7 +873,11 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         for (i = 0; !d.stop && i < poe->numlower; i++) {
>                 struct ovl_path lower = poe->lowerstack[i];
>
> -               d.last = i == poe->numlower - 1;
> +               if (!ofs->config.redirect_follow)
> +                       d.last = i == poe->numlower - 1;
> +               else
> +                       d.last = lower.layer->idx == roe->numlower;
> +
>                 err = ovl_lookup_layer(lower.dentry, &d, &this);
>                 if (err)
>                         goto out_put;
> --
> 2.13.6
>

[PATCH v2 2/4] ovl: Do not check for redirect if this is last layer

[Vivek Goyal]

If we are looking in last layer, then there should not be any need to
process redirect. redirect information is used only for lookup in next
lower layer and there is no more lower layer to look into. So no need
to process redirects.

IOW, ignore redirects on lowest layer.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 78798367c4f0..e27afd8d378a 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -248,7 +248,10 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
 		goto out;
 	}
 	d->is_dir = true;
-	if (!d->last && ovl_is_opaquedir(this)) {
+	if (d->last)
+		goto out;
+
+	if (ovl_is_opaquedir(this)) {
 		d->stop = d->opaque = true;
 		goto out;
 	}
-- 
2.13.6

[PATCH v2 3/4] ovl: set d->is_dir and d->opaque for last path element

[Vivek Goyal]

Certain properties in ovl_lookup_data should be set only for the last
element of the path. IOW, of we are calling ovl_lookup_single() for an
absolute redirect, then d->is_dir and d->opaque do not make much sense
for intermediate path elements. Instead set them only if dentry being
lookup is last path element.

As of now we do not seem to be making use of d->opaque if it is set for
a path/dentry in lower. But just define the semantics so that future code
can make use of this assumption.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/namei.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index e27afd8d378a..de08a67405d0 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -220,6 +220,7 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
 {
 	struct dentry *this;
 	int err;
+	bool last_element = !post[0];
 
 	this = lookup_one_len_unlocked(name, base, namelen);
 	if (IS_ERR(this)) {
@@ -247,12 +248,15 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
 			goto put_and_out;
 		goto out;
 	}
-	d->is_dir = true;
+	if (last_element)
+		d->is_dir = true;
 	if (d->last)
 		goto out;
 
 	if (ovl_is_opaquedir(this)) {
-		d->stop = d->opaque = true;
+		d->stop = true;
+		if (last_element)
+			d->opaque = true;
 		goto out;
 	}
 	err = ovl_check_redirect(this, d, prelen, post);
-- 
2.13.6

[PATCH v2 4/4] ovl: fix lookup with middle layer opaque dir and absolute path redirects

[Vivek Goyal]

From: Amir Goldstein <amir73il@gmail.com>

As of now if we encounter an opaque dir while looking for a dentry, we set
d->last=true. This means that there is no need to look further in any of
the lower layers. This works fine as long as there are no redirets or
relative redircts. But what if there is an absolute redirect on the
children dentry of opaque directory. We still need to continue to look into
next lower layer. This patch fixes it.

Here is first example to demonstrate the issue. Say you have following setup.

upper:  /redirect (redirect=/a/b/c)
lower1: /a/[b]/c       ([b] is opaque) (c has absolute redirect=/a/b/d/foo)
lower0: /a/b/d/foo

Now "redirect" dir should merge with lower1:/a/b/c/ and lower0:/a/b/d. Note,
despite the fact lower1:/a/[b] is opaque, we need to continue to look into
lower0 because children c has an absolute redirect.

Following is second example.

Watch me make foo disappear:

 $ mkdir lower middle upper work work2 merged
 $ mkdir lower/origin
 $ touch lower/origin/foo
 $ mount -t overlay none merged/ \
         -olowerdir=lower,upperdir=middle,workdir=work2
 $ mkdir merged/pure
 $ mv merged/origin merged/pure/redirect
 $ umount merged
 $ mount -t overlay none merged/ \
         -olowerdir=middle:lower,upperdir=upper,workdir=work
 $ mv merged/pure/redirect merged/redirect

Now you see foo inside a twice redirected merged dir:

 $ ls merged/redirect
 foo
 $ umount merged
 $ mount -t overlay none merged/ \
         -olowerdir=middle:lower,upperdir=upper,workdir=work

After mount cycle you don't see foo inside the same dir:

 $ ls merged/redirect

During middle layer lookup, the opaqueness of middle/pure is left in
the lookup state and then middle/pure/redirect is wrongly treated as
opaque.

Fixes: 02b69b284cd7 ("ovl: lookup redirects")
Cc: <stable@vger.kernel.org> #v4.10
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/namei.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index de08a67405d0..096d5853af7d 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -56,6 +56,14 @@ static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
 			if (s == next)
 				goto invalid;
 		}
+		/*
+		 * One of the ancestor path elements in an absolute path
+		 * lookup in ovl_lookup_layer() could have been opaque, but it
+		 * possible for a decendant path element to reset opaqueness in
+		 * case this path element has an absolute redirect to a lower
+		 * layer.
+		 */
+		d->stop = false;
 	} else {
 		if (strchr(buf, '/') != NULL)
 			goto invalid;
-- 
2.13.6

Re: [PATCH v2 4/4] ovl: fix lookup with middle layer opaque dir and absolute path redirects

[Amir Goldstein]

On Fri, Mar 9, 2018 at 10:44 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> From: Amir Goldstein <amir73il@gmail.com>
>
> As of now if we encounter an opaque dir while looking for a dentry, we set
> d->last=true. This means that there is no need to look further in any of
> the lower layers. This works fine as long as there are no redirets or
> relative redircts. But what if there is an absolute redirect on the
> children dentry of opaque directory. We still need to continue to look into
> next lower layer. This patch fixes it.
>
> Here is first example to demonstrate the issue. Say you have following setup.
>
> upper:  /redirect (redirect=/a/b/c)
> lower1: /a/[b]/c       ([b] is opaque) (c has absolute redirect=/a/b/d/foo)

According to the text below you meant redirect=/a/b/d

> lower0: /a/b/d/foo
>
> Now "redirect" dir should merge with lower1:/a/b/c/ and lower0:/a/b/d. Note,
> despite the fact lower1:/a/[b] is opaque, we need to continue to look into
> lower0 because children c has an absolute redirect.
>
> Following is second example.

Let's refer to the below as a reproducer then

>
> Watch me make foo disappear:
>
>  $ mkdir lower middle upper work work2 merged
>  $ mkdir lower/origin
>  $ touch lower/origin/foo
>  $ mount -t overlay none merged/ \
>          -olowerdir=lower,upperdir=middle,workdir=work2
>  $ mkdir merged/pure
>  $ mv merged/origin merged/pure/redirect
>  $ umount merged
>  $ mount -t overlay none merged/ \
>          -olowerdir=middle:lower,upperdir=upper,workdir=work
>  $ mv merged/pure/redirect merged/redirect
>
> Now you see foo inside a twice redirected merged dir:
>
>  $ ls merged/redirect
>  foo
>  $ umount merged
>  $ mount -t overlay none merged/ \
>          -olowerdir=middle:lower,upperdir=upper,workdir=work
>
> After mount cycle you don't see foo inside the same dir:
>
>  $ ls merged/redirect
>
> During middle layer lookup, the opaqueness of middle/pure is left in
> the lookup state and then middle/pure/redirect is wrongly treated as
> opaque.
>
> Fixes: 02b69b284cd7 ("ovl: lookup redirects")
> Cc: <stable@vger.kernel.org> #v4.10
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/namei.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index de08a67405d0..096d5853af7d 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -56,6 +56,14 @@ static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
>                         if (s == next)
>                                 goto invalid;
>                 }
> +               /*
> +                * One of the ancestor path elements in an absolute path
> +                * lookup in ovl_lookup_layer() could have been opaque, but it
> +                * possible for a decendant path element to reset opaqueness in
> +                * case this path element has an absolute redirect to a lower
> +                * layer.
> +                */

Now the comment is out of sync with new semantics, it is not opaquness
that is being reset. it's stop and continue lookup in lower layers.

> +               d->stop = false;
>         } else {
>                 if (strchr(buf, '/') != NULL)
>                         goto invalid;
> --
> 2.13.6
>

Re: [PATCH v2 4/4] ovl: fix lookup with middle layer opaque dir and absolute path redirects

[Vivek Goyal]

On Sat, Mar 10, 2018 at 10:07:07PM +0200, Amir Goldstein wrote:
> On Fri, Mar 9, 2018 at 10:44 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > From: Amir Goldstein <amir73il@gmail.com>
> >
> > As of now if we encounter an opaque dir while looking for a dentry, we set
> > d->last=true. This means that there is no need to look further in any of
> > the lower layers. This works fine as long as there are no redirets or
> > relative redircts. But what if there is an absolute redirect on the
> > children dentry of opaque directory. We still need to continue to look into
> > next lower layer. This patch fixes it.
> >
> > Here is first example to demonstrate the issue. Say you have following setup.
> >
> > upper:  /redirect (redirect=/a/b/c)
> > lower1: /a/[b]/c       ([b] is opaque) (c has absolute redirect=/a/b/d/foo)
> 
> According to the text below you meant redirect=/a/b/d

Will fix.

> 
> > lower0: /a/b/d/foo
> >
> > Now "redirect" dir should merge with lower1:/a/b/c/ and lower0:/a/b/d. Note,
> > despite the fact lower1:/a/[b] is opaque, we need to continue to look into
> > lower0 because children c has an absolute redirect.
> >
> > Following is second example.
> 
> Let's refer to the below as a reproducer then

Ok.

> 
> >
> > Watch me make foo disappear:
> >
> >  $ mkdir lower middle upper work work2 merged
> >  $ mkdir lower/origin
> >  $ touch lower/origin/foo
> >  $ mount -t overlay none merged/ \
> >          -olowerdir=lower,upperdir=middle,workdir=work2
> >  $ mkdir merged/pure
> >  $ mv merged/origin merged/pure/redirect
> >  $ umount merged
> >  $ mount -t overlay none merged/ \
> >          -olowerdir=middle:lower,upperdir=upper,workdir=work
> >  $ mv merged/pure/redirect merged/redirect
> >
> > Now you see foo inside a twice redirected merged dir:
> >
> >  $ ls merged/redirect
> >  foo
> >  $ umount merged
> >  $ mount -t overlay none merged/ \
> >          -olowerdir=middle:lower,upperdir=upper,workdir=work
> >
> > After mount cycle you don't see foo inside the same dir:
> >
> >  $ ls merged/redirect
> >
> > During middle layer lookup, the opaqueness of middle/pure is left in
> > the lookup state and then middle/pure/redirect is wrongly treated as
> > opaque.
> >
> > Fixes: 02b69b284cd7 ("ovl: lookup redirects")
> > Cc: <stable@vger.kernel.org> #v4.10
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/namei.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index de08a67405d0..096d5853af7d 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -56,6 +56,14 @@ static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
> >                         if (s == next)
> >                                 goto invalid;
> >                 }
> > +               /*
> > +                * One of the ancestor path elements in an absolute path
> > +                * lookup in ovl_lookup_layer() could have been opaque, but it
> > +                * possible for a decendant path element to reset opaqueness in
> > +                * case this path element has an absolute redirect to a lower
> > +                * layer.
> > +                */
> 
> Now the comment is out of sync with new semantics, it is not opaquness
> that is being reset. it's stop and continue lookup in lower layers.

Will change. How about following.

One of the ancestor path elements in an absolute path
lookup in ovl_lookup_layer() could have been opaque and that will
stop further lookup in lower layers (d->stop = true). But we have
found an absolute redirect in decendant path element and that should
force continue lookup in lower layers (reset d->stop).

Thanks
Vivek

> 
> > +               d->stop = false;
> >         } else {
> >                 if (strchr(buf, '/') != NULL)
> >                         goto invalid;
> > --
> > 2.13.6
> >

Re: [PATCH v2 4/4] ovl: fix lookup with middle layer opaque dir and absolute path redirects

[Amir Goldstein]

On Mon, Mar 12, 2018 at 2:51 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Sat, Mar 10, 2018 at 10:07:07PM +0200, Amir Goldstein wrote:
>> On Fri, Mar 9, 2018 at 10:44 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > From: Amir Goldstein <amir73il@gmail.com>
>> >
>> > As of now if we encounter an opaque dir while looking for a dentry, we set
>> > d->last=true. This means that there is no need to look further in any of
>> > the lower layers. This works fine as long as there are no redirets or
>> > relative redircts. But what if there is an absolute redirect on the
>> > children dentry of opaque directory. We still need to continue to look into
>> > next lower layer. This patch fixes it.
>> >
>> > Here is first example to demonstrate the issue. Say you have following setup.
>> >
>> > upper:  /redirect (redirect=/a/b/c)
>> > lower1: /a/[b]/c       ([b] is opaque) (c has absolute redirect=/a/b/d/foo)
>>
>> According to the text below you meant redirect=/a/b/d
>
> Will fix.
>
>>
>> > lower0: /a/b/d/foo
>> >
>> > Now "redirect" dir should merge with lower1:/a/b/c/ and lower0:/a/b/d. Note,
>> > despite the fact lower1:/a/[b] is opaque, we need to continue to look into
>> > lower0 because children c has an absolute redirect.
>> >
>> > Following is second example.
>>
>> Let's refer to the below as a reproducer then
>
> Ok.
>
>>
>> >
>> > Watch me make foo disappear:
>> >
>> >  $ mkdir lower middle upper work work2 merged
>> >  $ mkdir lower/origin
>> >  $ touch lower/origin/foo
>> >  $ mount -t overlay none merged/ \
>> >          -olowerdir=lower,upperdir=middle,workdir=work2
>> >  $ mkdir merged/pure
>> >  $ mv merged/origin merged/pure/redirect
>> >  $ umount merged
>> >  $ mount -t overlay none merged/ \
>> >          -olowerdir=middle:lower,upperdir=upper,workdir=work
>> >  $ mv merged/pure/redirect merged/redirect
>> >
>> > Now you see foo inside a twice redirected merged dir:
>> >
>> >  $ ls merged/redirect
>> >  foo
>> >  $ umount merged
>> >  $ mount -t overlay none merged/ \
>> >          -olowerdir=middle:lower,upperdir=upper,workdir=work
>> >
>> > After mount cycle you don't see foo inside the same dir:
>> >
>> >  $ ls merged/redirect
>> >
>> > During middle layer lookup, the opaqueness of middle/pure is left in
>> > the lookup state and then middle/pure/redirect is wrongly treated as
>> > opaque.
>> >
>> > Fixes: 02b69b284cd7 ("ovl: lookup redirects")
>> > Cc: <stable@vger.kernel.org> #v4.10
>> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> > ---
>> >  fs/overlayfs/namei.c | 8 ++++++++
>> >  1 file changed, 8 insertions(+)
>> >
>> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>> > index de08a67405d0..096d5853af7d 100644
>> > --- a/fs/overlayfs/namei.c
>> > +++ b/fs/overlayfs/namei.c
>> > @@ -56,6 +56,14 @@ static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
>> >                         if (s == next)
>> >                                 goto invalid;
>> >                 }
>> > +               /*
>> > +                * One of the ancestor path elements in an absolute path
>> > +                * lookup in ovl_lookup_layer() could have been opaque, but it
>> > +                * possible for a decendant path element to reset opaqueness in
>> > +                * case this path element has an absolute redirect to a lower
>> > +                * layer.
>> > +                */
>>
>> Now the comment is out of sync with new semantics, it is not opaquness
>> that is being reset. it's stop and continue lookup in lower layers.
>
> Will change. How about following.
>
> One of the ancestor path elements in an absolute path
> lookup in ovl_lookup_layer() could have been opaque and that will
> stop further lookup in lower layers (d->stop = true). But we have
> found an absolute redirect in decendant path element and that should
> force continue lookup in lower layers (reset d->stop).
>

Good.
Thanks,
Amir.

[PATCH v2.1 4/4] ovl: fix lookup with middle layer opaque dir and absolute path redirects

[Vivek Goyal]

From: Amir Goldstein <amir73il@gmail.com>
 
As of now if we encounter an opaque dir while looking for a dentry, we set
d->last=true. This means that there is no need to look further in any of
the lower layers. This works fine as long as there are no redirets or
relative redircts. But what if there is an absolute redirect on the
children dentry of opaque directory. We still need to continue to look into
next lower layer. This patch fixes it.

Here is an example to demonstrate the issue. Say you have following setup.

upper:  /redirect (redirect=/a/b/c)
lower1: /a/[b]/c       ([b] is opaque) (c has absolute redirect=/a/b/d/)
lower0: /a/b/d/foo

Now "redirect" dir should merge with lower1:/a/b/c/ and lower0:/a/b/d. Note,
despite the fact lower1:/a/[b] is opaque, we need to continue to look into
lower0 because children c has an absolute redirect.

Following is a reproducer.

Watch me make foo disappear:

 $ mkdir lower middle upper work work2 merged
 $ mkdir lower/origin
 $ touch lower/origin/foo
 $ mount -t overlay none merged/ \
         -olowerdir=lower,upperdir=middle,workdir=work2
 $ mkdir merged/pure
 $ mv merged/origin merged/pure/redirect
 $ umount merged
 $ mount -t overlay none merged/ \
         -olowerdir=middle:lower,upperdir=upper,workdir=work
 $ mv merged/pure/redirect merged/redirect

Now you see foo inside a twice redirected merged dir:

 $ ls merged/redirect
 foo
 $ umount merged
 $ mount -t overlay none merged/ \
         -olowerdir=middle:lower,upperdir=upper,workdir=work

After mount cycle you don't see foo inside the same dir:

 $ ls merged/redirect

During middle layer lookup, the opaqueness of middle/pure is left in
the lookup state and then middle/pure/redirect is wrongly treated as
opaque.

Fixes: 02b69b284cd7 ("ovl: lookup redirects")
Cc: <stable@vger.kernel.org> #v4.10
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/namei.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Index: rhvgoyal-linux/fs/overlayfs/namei.c
===================================================================
--- rhvgoyal-linux.orig/fs/overlayfs/namei.c	2018-03-12 09:09:28.239836991 -0400
+++ rhvgoyal-linux/fs/overlayfs/namei.c	2018-03-12 09:16:48.004836991 -0400
@@ -56,6 +56,15 @@ static int ovl_check_redirect(struct den
 			if (s == next)
 				goto invalid;
 		}
+		/*
+		 * One of the ancestor path elements in an absolute path
+		 * lookup in ovl_lookup_layer() could have been opaque and
+		 * that will stop further lookup in lower layers (d->stop=true)
+		 * But we have found an absolute redirect in decendant path
+		 * element and that should force continue lookup in lower
+		 * layers (reset d->stop).
+		 */
+		d->stop = false;
 	} else {
 		if (strchr(buf, '/') != NULL)
 			goto invalid;