ovl: Bunch of ovl_lookup() path fixes

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 <[email protected]>

Reviewed-by Amir Goldstein <[email protected]>

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/namei.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

fs/overlayfs/namei.c +6 -3

index de3e6da1d5a5..78798367c4f0 100644
@@ -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

From: Amir Goldstein <[email protected]>

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 <[email protected]> #v4.10

Signed-off-by Amir Goldstein <[email protected]>

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/namei.c | 8 ++++++++
 1 file changed, 8 insertions(+)

fs/overlayfs/namei.c +0 -0

index de08a67405d0..096d5853af7d 100644

fs/overlayfs/namei.c +1 -1

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

2.13.6

Quoted 9 lines · Vivek Goyal

From: Amir Goldstein <[email protected]>

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.

Quoted 4 lines

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

Quoted 8 lines

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

Quoted 50 lines

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 <[email protected]> #v4.10

Signed-off-by Amir Goldstein <[email protected]>

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/namei.c | 8 ++++++++
 1 file changed, 8 insertions(+)

fs/overlayfs/namei.c +2 -0

index de08a67405d0..096d5853af7d 100644
@@ -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

fs/overlayfs/namei.c static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d, · line 65

Quoted 5 lines

fs/overlayfs/namei.c +5 -0

+                * 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.

Quoted 8 lines

fs/overlayfs/namei.c +1 -1

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

2.13.6

Quoted 10 lines · Amir Goldstein

Quoted 9 lines · Vivek Goyal

From: Amir Goldstein <[email protected]>

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 10 lines

Quoted 8 lines

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.

Quoted 50 lines

Quoted 50 lines

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 <[email protected]> #v4.10

Signed-off-by Amir Goldstein <[email protected]>

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/namei.c | 8 ++++++++
 1 file changed, 8 insertions(+)

fs/overlayfs/namei.c +2 -0

index de08a67405d0..096d5853af7d 100644
@@ -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

Quoted 8 lines

fs/overlayfs/namei.c static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d, · line 65

Quoted 5 lines

fs/overlayfs/namei.c +5 -0

+                * 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).