[PATCH] ovl: no direct iteration for dir with copy up origin

[PATCH] ovl: no direct iteration for dir with copy up origin

[Amir Goldstein]

If a non-merge dir in an overlay mount has an overlay.origin xattr, it
means it was once an upper merge dir, which may contain whiteouts and
then the lower dir was removed under it.

Do not iterate real dir directly in this case to avoid exposing whiteouts.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/readdir.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

Yi,

You missed a very big spot and so did I in review.
Your patch doen't hide whiteouts from readdir and it doesn't
even pass you own xfstest...

Anyway, here are the missing parts picked from my impure readdir
patch.

Amir.

diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 0298463cf9c3..3c8796705517 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -256,21 +256,42 @@ static inline int ovl_dir_read(struct path *realpath,
 	return err;
 }
 
+/* Can we iterate real dir directly? */
+static bool ovl_dir_is_real(struct dentry *dir)
+{
+	struct path realpath;
+	enum ovl_path_type type = ovl_path_real(dir, &realpath);
+
+	if (OVL_TYPE_MERGE(type))
+		return false;
+
+	/*
+	 * Non-merge dir may contain whiteouts from a time it was a merge
+	 * upper, before lower dir was removed under it and possibly before
+	 * it was rotated from upper to lower layer.
+	 */
+	return !ovl_check_origin_xattr(realpath.dentry);
+}
+
 static void ovl_dir_reset(struct file *file)
 {
 	struct ovl_dir_file *od = file->private_data;
 	struct ovl_dir_cache *cache = od->cache;
 	struct dentry *dentry = file->f_path.dentry;
-	enum ovl_path_type type = ovl_path_type(dentry);
+	bool is_real;
 
 	if (cache && ovl_dentry_version_get(dentry) != cache->version) {
 		ovl_cache_put(od, dentry);
 		od->cache = NULL;
 		od->cursor = NULL;
 	}
-	WARN_ON(!od->is_real && !OVL_TYPE_MERGE(type));
-	if (od->is_real && OVL_TYPE_MERGE(type))
+	is_real = ovl_dir_is_real(dentry);
+	if (od->is_real != is_real) {
+		/* is_real can only become false when dir is copied up */
+		if (WARN_ON(is_real))
+			return;
 		od->is_real = false;
+	}
 }
 
 static int ovl_dir_read_merged(struct dentry *dentry, struct list_head *list)
@@ -502,7 +523,7 @@ static int ovl_dir_open(struct inode *inode, struct file *file)
 		return PTR_ERR(realfile);
 	}
 	od->realfile = realfile;
-	od->is_real = !OVL_TYPE_MERGE(type);
+	od->is_real = ovl_dir_is_real(file->f_path.dentry);
 	od->is_upper = OVL_TYPE_UPPER(type);
 	file->private_data = od;
 
-- 
2.7.4

Re: [PATCH] ovl: no direct iteration for dir with copy up origin

[zhangyi (F)]

On 2017/6/25 22:14, Amir Goldstein Wrote:
> If a non-merge dir in an overlay mount has an overlay.origin xattr, it
> means it was once an upper merge dir, which may contain whiteouts and
> then the lower dir was removed under it.
> 
> Do not iterate real dir directly in this case to avoid exposing whiteouts.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/readdir.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> Yi,
> 
> You missed a very big spot and so did I in review.
> Your patch doen't hide whiteouts from readdir and it doesn't
> even pass you own xfstest...
> 
> Anyway, here are the missing parts picked from my impure readdir
> patch.
> 

Hi Amir,

Yes, I missed whiteouts check in readdir because I saw your readdir
patch has already fix it, so my xfstest test this case. Do you
think we should merge this two patch ?

Thanks,
ZhangYi.

Re: [PATCH] ovl: no direct iteration for dir with copy up origin

[Amir Goldstein]

On Mon, Jun 26, 2017 at 4:22 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> On 2017/6/25 22:14, Amir Goldstein Wrote:
>> If a non-merge dir in an overlay mount has an overlay.origin xattr, it
>> means it was once an upper merge dir, which may contain whiteouts and
>> then the lower dir was removed under it.
>>
>> Do not iterate real dir directly in this case to avoid exposing whiteouts.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/readdir.c | 29 +++++++++++++++++++++++++----
>>  1 file changed, 25 insertions(+), 4 deletions(-)
>>
>> Yi,
>>
>> You missed a very big spot and so did I in review.
>> Your patch doen't hide whiteouts from readdir and it doesn't
>> even pass you own xfstest...
>>
>> Anyway, here are the missing parts picked from my impure readdir
>> patch.
>>
>
> Hi Amir,
>
> Yes, I missed whiteouts check in readdir because I saw your readdir
> patch has already fix it, so my xfstest test this case. Do you
> think we should merge this two patch ?
>

Yes, only two problems with my readdir patch:
1. Miklos rejected it (for now)
2. It only complements your V2 patch (impure), not your V3 patch (origin)

So we need those two patches, it's fine if they stay separate because
they fix two related, but distinguished problems (expose whiteouts and rmdir).

I just want to highlight that I did not test the lower layer with origin case,
not sure if you did? There were bugs related to unexpected overlay objects
in lower layer in the past, see xfstest overlay/014 for example test.

So if you can, please post a patch to extend test overlay/031 to mount
the "origin"  with whiteout dir as lower layer and repeat the "ls" and
"rmdir" tests.

Thanks,
Amir.

Re: [PATCH] ovl: no direct iteration for dir with copy up origin

[zhangyi (F)]

On 2017/6/26 13:31, Amir Goldstein Wrote:
> On Mon, Jun 26, 2017 at 4:22 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> On 2017/6/25 22:14, Amir Goldstein Wrote:
>>> If a non-merge dir in an overlay mount has an overlay.origin xattr, it
>>> means it was once an upper merge dir, which may contain whiteouts and
>>> then the lower dir was removed under it.
>>>
>>> Do not iterate real dir directly in this case to avoid exposing whiteouts.
>>>
>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>>> ---
>>>  fs/overlayfs/readdir.c | 29 +++++++++++++++++++++++++----
>>>  1 file changed, 25 insertions(+), 4 deletions(-)
>>>
>>> Yi,
>>>
>>> You missed a very big spot and so did I in review.
>>> Your patch doen't hide whiteouts from readdir and it doesn't
>>> even pass you own xfstest...
>>>
>>> Anyway, here are the missing parts picked from my impure readdir
>>> patch.
>>>
>>
>> Hi Amir,
>>
>> Yes, I missed whiteouts check in readdir because I saw your readdir
>> patch has already fix it, so my xfstest test this case. Do you
>> think we should merge this two patch ?
>>
> 
> Yes, only two problems with my readdir patch:
> 1. Miklos rejected it (for now)
> 2. It only complements your V2 patch (impure), not your V3 patch (origin)
> 

Indeed, thanks for this patch.

> So we need those two patches, it's fine if they stay separate because
> they fix two related, but distinguished problems (expose whiteouts and rmdir).
> 
> I just want to highlight that I did not test the lower layer with origin case,
> not sure if you did? There were bugs related to unexpected overlay objects
> in lower layer in the past, see xfstest overlay/014 for example test.
> 

I tested the lower layer with origin case baseed on these two patches, there
is no problem, whiteouts are not exposed and rmdir/rename execute successfully.

> So if you can, please post a patch to extend test overlay/031 to mount
> the "origin"  with whiteout dir as lower layer and repeat the "ls" and
> "rmdir" tests.
> 

Okay, I am glad to post it, thanks.

Thanks,
ZhangYi.