ovl: fix decode of dir file handle with multi lower layers

PATCH In review Amir Goldstein 2 participants 4 messages Nov 5, 2018 – Nov 23, 2018 Overlay filesystem

Share & tools

Amir Goldstein

author maintainer

2018-11-05 05:50 UTC #0 lore raw

PATCH 1/1 ovl: fix decode of dir file handle with multi lower layers

When decoding a lower file handle, we first call ovl_check_origin_fh() with connected=false to get any real lower dentry for overlay inode cache lookup.

If the real dentry is a disconnected dir dentry, ovl_check_origin_fh() is called again with connected=true to get a connected real dentry and find the lower layer the real dentry belongs to.

If the first call returned a connected real dentry, we use it to lookup an overlay connected dentry, but the first ovl_check_origin_fh() call with connected=false did not check that the found dentry is under the root of the layer (see ovl_acceptable()), it only checked that the found dentry super block matches the uuid of the lower file handle.

In case there are multiple lower layers on the same fs and the found dentry is not from the top most lower layer, using the layer index returned from the first ovl_check_origin_fh() is wrong and we end up failing to decode the file handle.

Fix this by always calling ovl_check_origin_fh() with connected=true if we got a directory dentry in the first call.

Fixes 8b58924ad55c ("ovl: lookup in inode cache first when decoding...")

Cc <[email protected]> # v4.17

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

---
 fs/overlayfs/export.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

fs/overlayfs/export.c +3 -3

index 8fa37cd7818a..54e5d17d7f3e 100644
@@ -754,9 +754,8 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
 		goto out;
 	}
 
-	/* Otherwise, get a connected non-upper dir or disconnected non-dir */
-	if (d_is_dir(origin.dentry) &&
-	    (origin.dentry->d_flags & DCACHE_DISCONNECTED)) {
+	/* Find origin.dentry again with ovl_acceptable() layer check */
+	if (d_is_dir(origin.dentry)) {
 		dput(origin.dentry);
 		origin.dentry = NULL;
 		err = ovl_check_origin_fh(ofs, fh, true, NULL, &stack);

@@ -769,6 +768,7 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
 			goto out_err;
 	}
 
+	/* Get a connected non-upper dir or disconnected non-dir */
 	dentry = ovl_get_dentry(sb, NULL, &origin, index);
 
 out:

Signature · 1 line

2.17.1

Miklos Szeredi

maintainer

2018-11-22 14:45 UTC #1 lore raw

Quoted 15 lines · Amir Goldstein

When decoding a lower file handle, we first call ovl_check_origin_fh() with connected=false to get any real lower dentry for overlay inode cache lookup.
If the real dentry is a disconnected dir dentry, ovl_check_origin_fh() is called again with connected=true to get a connected real dentry and find the lower layer the real dentry belongs to.
If the first call returned a connected real dentry, we use it to lookup an overlay connected dentry, but the first ovl_check_origin_fh() call with connected=false did not check that the found dentry is under the root of the layer (see ovl_acceptable()), it only checked that the found dentry super block matches the uuid of the lower file handle.

Quoted 7 lines

In case there are multiple lower layers on the same fs and the found dentry is not from the top most lower layer, using the layer index returned from the first ovl_check_origin_fh() is wrong and we end up failing to decode the file handle.
Fix this by always calling ovl_check_origin_fh() with connected=true if we got a directory dentry in the first call.

Hi Amir,

Does this have a testcase?

Thanks,
Miklos

Amir Goldstein

author maintainer

2018-11-23 01:30 UTC #2 lore raw

Quoted 22 lines · Miklos Szeredi

Quoted 20 lines · Amir Goldstein
When decoding a lower file handle, we first call ovl_check_origin_fh() with connected=false to get any real lower dentry for overlay inode cache lookup.
If the real dentry is a disconnected dir dentry, ovl_check_origin_fh() is called again with connected=true to get a connected real dentry and find the lower layer the real dentry belongs to.
If the first call returned a connected real dentry, we use it to lookup an overlay connected dentry, but the first ovl_check_origin_fh() call with connected=false did not check that the found dentry is under the root of the layer (see ovl_acceptable()), it only checked that the found dentry super block matches the uuid of the lower file handle.
In case there are multiple lower layers on the same fs and the found dentry is not from the top most lower layer, using the layer index returned from the first ovl_check_origin_fh() is wrong and we end up failing to decode the file handle.

Quoted 7 lines

Quoted 2 lines
Fix this by always calling ovl_check_origin_fh() with connected=true if we got a directory dentry in the first call.
Hi Amir,
Does this have a testcase?

xfstest overlay/062
already upstream.

Thanks,
Amir.

Miklos Szeredi

maintainer

2018-11-22 15:32 UTC #3 lore raw

Quoted 27 lines · Amir Goldstein

Quoted 25 lines · Miklos Szeredi
Quoted 22 lines · Amir Goldstein
When decoding a lower file handle, we first call ovl_check_origin_fh() with connected=false to get any real lower dentry for overlay inode cache lookup.
If the real dentry is a disconnected dir dentry, ovl_check_origin_fh() is called again with connected=true to get a connected real dentry and find the lower layer the real dentry belongs to.
If the first call returned a connected real dentry, we use it to lookup an overlay connected dentry, but the first ovl_check_origin_fh() call with connected=false did not check that the found dentry is under the root of the layer (see ovl_acceptable()), it only checked that the found dentry super block matches the uuid of the lower file handle.
In case there are multiple lower layers on the same fs and the found dentry is not from the top most lower layer, using the layer index returned from the first ovl_check_origin_fh() is wrong and we end up failing to decode the file handle.
Fix this by always calling ovl_check_origin_fh() with connected=true if we got a directory dentry in the first call.

Quoted 7 lines

Quoted 4 lines
Hi Amir,
Does this have a testcase?
xfstest overlay/062
already upstream.

Good.

Thanks,
Miklos