[PATCH 0/3] Overlayfs restoring of origin fh

[PATCH 0/3] Overlayfs restoring of origin fh

[Amir Goldstein]

Miklos,

This series is complementary to the whiteouts expose patches.
Patch 2 restores origin xattr on lookup of merge dir (that was copied up
before v4.12).
Patch 3 sets a 'null' origin fh on dir if we happen to iterate an 'impure'
dir and find a whiteout while looking for copy ups. This is expected to
happen if merge dir was copied up before v4.12, but files inside it were
copied up after v4.12.

This series is currently based on top of non-samefs series and all patches
for v4.15 are piled on my overlayfs-devel branch [1]. Let me know if you
want me to rebase them differently.

I enhanced Zhangyi's xfstest overlay/031, which tests for exposed whiteouts
to cover the 2 cases addressed by this series. The enhanced test is
available on my xfstests branch [2].

Zhangyi,

I would appreciate a review of those patches and the test changes.

Thanks,
Amir.

[1] https://github.com/amir73il/linux/commits/overlayfs-devel
[2] https://github.com/amir73il/xfstests/commits/overlayfs-devel

Amir Goldstein (3):
  ovl: remove unneeded arg from ovl_verify_origin()
  ovl: set origin xattr for merge dir on lookup
  ovl: recover from whiteouts in impure dir iteration

 fs/overlayfs/namei.c     | 63 +++++++++++++++++++++++++++++++++++++++---------
 fs/overlayfs/overlayfs.h |  5 ++--
 fs/overlayfs/readdir.c   | 41 ++++++++++++++++++++++++++++---
 fs/overlayfs/super.c     |  7 +++---
 fs/overlayfs/util.c      | 21 ++++++++++++++++
 5 files changed, 117 insertions(+), 20 deletions(-)

-- 
2.7.4

[PATCH 1/3] ovl: remove unneeded arg from ovl_verify_origin()

[Amir Goldstein]

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c     | 4 ++--
 fs/overlayfs/overlayfs.h | 4 ++--
 fs/overlayfs/super.c     | 5 ++---
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 7b89f5944e04..322c6214114f 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -348,8 +348,8 @@ static int ovl_verify_origin_fh(struct dentry *dentry, const struct ovl_fh *fh)
  *
  * Return 0 on match, -ESTALE on mismatch, < 0 on error.
  */
-int ovl_verify_origin(struct dentry *dentry, struct vfsmount *mnt,
-		      struct dentry *origin, bool is_upper, bool set)
+int ovl_verify_origin(struct dentry *dentry, struct dentry *origin,
+		      bool is_upper, bool set)
 {
 	struct inode *inode;
 	struct ovl_fh *fh;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 29569bf1cc6e..bee51296db38 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -251,8 +251,8 @@ static inline bool ovl_is_impuredir(struct dentry *dentry)
 
 
 /* namei.c */
-int ovl_verify_origin(struct dentry *dentry, struct vfsmount *mnt,
-		      struct dentry *origin, bool is_upper, bool set);
+int ovl_verify_origin(struct dentry *dentry, struct dentry *origin,
+		      bool is_upper, bool set);
 int ovl_verify_index(struct dentry *index, struct ovl_path *lower,
 		     unsigned int numlower);
 int ovl_get_index_name(struct dentry *origin, struct qstr *name);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 3f41ddf6bd0d..af4d3e876398 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1119,7 +1119,6 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	if (!(ovl_force_readonly(ufs)) && ufs->config.index) {
 		/* Verify lower root is upper root origin */
 		err = ovl_verify_origin(upperpath.dentry,
-					oe->lowerstack[0].layer->mnt,
 					oe->lowerstack[0].dentry,
 					false, true);
 		if (err) {
@@ -1131,8 +1130,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 						   OVL_INDEXDIR_NAME, true);
 		if (ufs->indexdir) {
 			/* Verify upper root is index dir origin */
-			err = ovl_verify_origin(ufs->indexdir, ufs->upper_mnt,
-						upperpath.dentry, true, true);
+			err = ovl_verify_origin(ufs->indexdir, upperpath.dentry,
+						true, true);
 			if (err)
 				pr_err("overlayfs: failed to verify index dir origin\n");
 
-- 
2.7.4

[PATCH 2/3] ovl: set origin xattr for merge dir on lookup

[Amir Goldstein]

For old existing merge dirs whose "origin" xattr was not set at copy up
time, we amend the situation on lookup.

If no origin fh is stored in upper of a merge dir, store fh of upper most
lower dir or 'null' fh if lower does not support file handles. We do this
so we can filter out whiteouts in case lower dir is removed offline and
then upper dir becomes a pure upper in a future mount.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c     | 61 ++++++++++++++++++++++++++++++++++++++++--------
 fs/overlayfs/overlayfs.h |  2 +-
 fs/overlayfs/super.c     |  4 ++--
 3 files changed, 54 insertions(+), 13 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 322c6214114f..65606a0a124c 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -87,6 +87,8 @@ static int ovl_acceptable(void *ctx, struct dentry *dentry)
 	return 1;
 }
 
+static struct ovl_fh ovl_null_fh;
+
 static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
 {
 	int res;
@@ -100,7 +102,7 @@ static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
 	}
 	/* Zero size value means "copied up but origin unknown" */
 	if (res == 0)
-		return NULL;
+		goto null_fh;
 
 	fh  = kzalloc(res, GFP_KERNEL);
 	if (!fh)
@@ -118,12 +120,12 @@ static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
 
 	/* Treat larger version and unknown flags as "origin unknown" */
 	if (fh->version > OVL_FH_VERSION || fh->flags & ~OVL_FH_FLAG_ALL)
-		goto out;
+		goto null_fh;
 
 	/* Treat endianness mismatch as "origin unknown" */
 	if (!(fh->flags & OVL_FH_FLAG_ANY_ENDIAN) &&
 	    (fh->flags & OVL_FH_FLAG_BIG_ENDIAN) != OVL_FH_FLAG_CPU_ENDIAN)
-		goto out;
+		goto null_fh;
 
 	return fh;
 
@@ -131,6 +133,10 @@ static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
 	kfree(fh);
 	return NULL;
 
+null_fh:
+	kfree(fh);
+	return &ovl_null_fh;
+
 fail:
 	pr_warn_ratelimited("overlayfs: failed to get origin (%i)\n", res);
 	goto out;
@@ -149,6 +155,9 @@ static struct dentry *ovl_get_origin(struct dentry *dentry,
 	if (IS_ERR_OR_NULL(fh))
 		return (struct dentry *)fh;
 
+	if (fh == &ovl_null_fh)
+		return NULL;
+
 	/*
 	 * Make sure that the stored uuid matches the uuid of the lower
 	 * layer where file handle will be decoded.
@@ -333,6 +342,10 @@ static int ovl_verify_origin_fh(struct dentry *dentry, const struct ovl_fh *fh)
 	if (IS_ERR(ofh))
 		return PTR_ERR(ofh);
 
+	/* NULL fh (no lower fh support) and stored 'null' fh is a match */
+	if (ofh == &ovl_null_fh)
+		return fh ? -ESTALE : 0;
+
 	if (fh->len != ofh->len || memcmp(fh, ofh, fh->len))
 		err = -ESTALE;
 
@@ -345,24 +358,40 @@ static int ovl_verify_origin_fh(struct dentry *dentry, const struct ovl_fh *fh)
  *
  * If @set is true and there is no stored file handle, encode and store origin
  * file handle in OVL_XATTR_ORIGIN.
+ * If @strict is true, then the stored file handle must be non 'null'.
+ * If @strict is false, then a stored 'null' file handle and lower with no file
+ * handle support is considered a match.
  *
  * Return 0 on match, -ESTALE on mismatch, < 0 on error.
  */
 int ovl_verify_origin(struct dentry *dentry, struct dentry *origin,
-		      bool is_upper, bool set)
+		      bool is_upper, bool set, bool strict)
 {
 	struct inode *inode;
-	struct ovl_fh *fh;
+	const struct ovl_fh *fh = NULL;
 	int err;
 
-	fh = ovl_encode_fh(origin, is_upper);
-	err = PTR_ERR(fh);
-	if (IS_ERR(fh))
+	/*
+	 * When lower layer doesn't support export operations store a 'null' fh,
+	 * so we can use the overlay.origin xattr to distignuish between a copy
+	 * up and a pure upper inode.
+	 */
+	err = -EOPNOTSUPP;
+	if (ovl_can_decode_fh(origin->d_sb)) {
+		fh = ovl_encode_fh(origin, is_upper);
+		err = PTR_ERR(fh);
+		if (IS_ERR(fh))
+			goto fail;
+	} else if (strict) {
 		goto fail;
+	}
 
 	err = ovl_verify_origin_fh(dentry, fh);
-	if (set && err == -ENODATA)
-		err = ovl_do_setxattr(dentry, OVL_XATTR_ORIGIN, fh, fh->len, 0);
+	if (set && err == -ENODATA) {
+		err = ovl_do_setxattr(dentry, OVL_XATTR_ORIGIN, fh,
+				      fh ? fh->len : 0, 0);
+	}
+
 	if (err)
 		goto fail;
 
@@ -662,6 +691,18 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		if (!this)
 			continue;
 
+		/*
+		 * If no origin fh is stored in upper of a merge dir, store fh
+		 * of upper most lower dir or 'null' fh if lower does not
+		 * support file handles. We do this so we can filter out
+		 * whiteouts in case lower dir is removed offline and this upper
+		 * becomes a pure upper in a future mount.
+		 */
+		if (this && upperdentry && !ctr) {
+			(void)ovl_verify_origin(upperdentry, this,
+						false, true, false);
+		}
+
 		stack[ctr].dentry = this;
 		stack[ctr].layer = lower.layer;
 		ctr++;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index bee51296db38..df925188394f 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -252,7 +252,7 @@ static inline bool ovl_is_impuredir(struct dentry *dentry)
 
 /* namei.c */
 int ovl_verify_origin(struct dentry *dentry, struct dentry *origin,
-		      bool is_upper, bool set);
+		      bool is_upper, bool set, bool strict);
 int ovl_verify_index(struct dentry *index, struct ovl_path *lower,
 		     unsigned int numlower);
 int ovl_get_index_name(struct dentry *origin, struct qstr *name);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index af4d3e876398..f8bba4f8b940 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1120,7 +1120,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		/* Verify lower root is upper root origin */
 		err = ovl_verify_origin(upperpath.dentry,
 					oe->lowerstack[0].dentry,
-					false, true);
+					false, true, true);
 		if (err) {
 			pr_err("overlayfs: failed to verify upper root origin\n");
 			goto out_free_oe;
@@ -1131,7 +1131,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		if (ufs->indexdir) {
 			/* Verify upper root is index dir origin */
 			err = ovl_verify_origin(ufs->indexdir, upperpath.dentry,
-						true, true);
+						true, true, true);
 			if (err)
 				pr_err("overlayfs: failed to verify index dir origin\n");
 
-- 
2.7.4

Re: [PATCH 2/3] ovl: set origin xattr for merge dir on lookup

[zhangyi (F)]

On 2017/11/8 5:38, Amir Goldstein Wrote:
> For old existing merge dirs whose "origin" xattr was not set at copy up
> time, we amend the situation on lookup.
> 
> If no origin fh is stored in upper of a merge dir, store fh of upper most
> lower dir or 'null' fh if lower does not support file handles. We do this
> so we can filter out whiteouts in case lower dir is removed offline and
> then upper dir becomes a pure upper in a future mount.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/namei.c     | 61 ++++++++++++++++++++++++++++++++++++++++--------
>  fs/overlayfs/overlayfs.h |  2 +-
>  fs/overlayfs/super.c     |  4 ++--
>  3 files changed, 54 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 322c6214114f..65606a0a124c 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -87,6 +87,8 @@ static int ovl_acceptable(void *ctx, struct dentry *dentry)
>  	return 1;
>  }
>  
> +static struct ovl_fh ovl_null_fh;
> +
>  static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
>  {
>  	int res;
> @@ -100,7 +102,7 @@ static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
>  	}
>  	/* Zero size value means "copied up but origin unknown" */
>  	if (res == 0)
> -		return NULL;
> +		goto null_fh;
>  
>  	fh  = kzalloc(res, GFP_KERNEL);
>  	if (!fh)
> @@ -118,12 +120,12 @@ static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
>  
>  	/* Treat larger version and unknown flags as "origin unknown" */
>  	if (fh->version > OVL_FH_VERSION || fh->flags & ~OVL_FH_FLAG_ALL)
> -		goto out;
> +		goto null_fh;
>  
>  	/* Treat endianness mismatch as "origin unknown" */
>  	if (!(fh->flags & OVL_FH_FLAG_ANY_ENDIAN) &&
>  	    (fh->flags & OVL_FH_FLAG_BIG_ENDIAN) != OVL_FH_FLAG_CPU_ENDIAN)
> -		goto out;
> +		goto null_fh;
>  
>  	return fh;
>  
> @@ -131,6 +133,10 @@ static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
>  	kfree(fh);
>  	return NULL;
>  
> +null_fh:
> +	kfree(fh);
> +	return &ovl_null_fh;
> +
>  fail:
>  	pr_warn_ratelimited("overlayfs: failed to get origin (%i)\n", res);
>  	goto out;
> @@ -149,6 +155,9 @@ static struct dentry *ovl_get_origin(struct dentry *dentry,
>  	if (IS_ERR_OR_NULL(fh))
>  		return (struct dentry *)fh;
>  
> +	if (fh == &ovl_null_fh)
> +		return NULL;
> +
>  	/*
>  	 * Make sure that the stored uuid matches the uuid of the lower
>  	 * layer where file handle will be decoded.
> @@ -333,6 +342,10 @@ static int ovl_verify_origin_fh(struct dentry *dentry, const struct ovl_fh *fh)
>  	if (IS_ERR(ofh))
>  		return PTR_ERR(ofh);
>  
> +	/* NULL fh (no lower fh support) and stored 'null' fh is a match */
> +	if (ofh == &ovl_null_fh)
> +		return fh ? -ESTALE : 0;
> +

Is -ESTALE -> -ENODATA better ?

Consider the case: We set 'null' fh in merged upper dir on a 'file handle' not supported
base filesystem, then we copy or tar all underlying directories to another base filesystem which is
'file handle' supported and mount overlayfs again. I think we can update upperdir's fh instead of
return fail.

At the same time, consider the opposite side, all directories move from a base filesystem which 'file
handle' supported to a not supported one. It will trigger NULL pointer crash in ovl_verify_origin_fh()
because fh is NULL but ofh was existed. Following change good ?

-	if (!ofh)
+	if (!ofh || !fh)
		return -ENODATA;

Thanks,
Yi.

>  	if (fh->len != ofh->len || memcmp(fh, ofh, fh->len))
>  		err = -ESTALE;
>  
> @@ -345,24 +358,40 @@ static int ovl_verify_origin_fh(struct dentry *dentry, const struct ovl_fh *fh)
>   *
>   * If @set is true and there is no stored file handle, encode and store origin
>   * file handle in OVL_XATTR_ORIGIN.
> + * If @strict is true, then the stored file handle must be non 'null'.
> + * If @strict is false, then a stored 'null' file handle and lower with no file
> + * handle support is considered a match.
>   *
>   * Return 0 on match, -ESTALE on mismatch, < 0 on error.
>   */
>  int ovl_verify_origin(struct dentry *dentry, struct dentry *origin,
> -		      bool is_upper, bool set)
> +		      bool is_upper, bool set, bool strict)
>  {
>  	struct inode *inode;
> -	struct ovl_fh *fh;
> +	const struct ovl_fh *fh = NULL;
>  	int err;
>  
> -	fh = ovl_encode_fh(origin, is_upper);
> -	err = PTR_ERR(fh);
> -	if (IS_ERR(fh))
> +	/*
> +	 * When lower layer doesn't support export operations store a 'null' fh,
> +	 * so we can use the overlay.origin xattr to distignuish between a copy
> +	 * up and a pure upper inode.
> +	 */
> +	err = -EOPNOTSUPP;
> +	if (ovl_can_decode_fh(origin->d_sb)) {
> +		fh = ovl_encode_fh(origin, is_upper);
> +		err = PTR_ERR(fh);
> +		if (IS_ERR(fh))
> +			goto fail;
> +	} else if (strict) {
>  		goto fail;
> +	}
>  
>  	err = ovl_verify_origin_fh(dentry, fh);
> -	if (set && err == -ENODATA)
> -		err = ovl_do_setxattr(dentry, OVL_XATTR_ORIGIN, fh, fh->len, 0);
> +	if (set && err == -ENODATA) {
> +		err = ovl_do_setxattr(dentry, OVL_XATTR_ORIGIN, fh,
> +				      fh ? fh->len : 0, 0);
> +	}
> +
>  	if (err)
>  		goto fail;
>  
> @@ -662,6 +691,18 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>  		if (!this)
>  			continue;
>  
> +		/*
> +		 * If no origin fh is stored in upper of a merge dir, store fh
> +		 * of upper most lower dir or 'null' fh if lower does not
> +		 * support file handles. We do this so we can filter out
> +		 * whiteouts in case lower dir is removed offline and this upper
> +		 * becomes a pure upper in a future mount.
> +		 */
> +		if (this && upperdentry && !ctr) {
> +			(void)ovl_verify_origin(upperdentry, this,
> +						false, true, false);
> +		}
> +
>  		stack[ctr].dentry = this;
>  		stack[ctr].layer = lower.layer;
>  		ctr++;
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index bee51296db38..df925188394f 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -252,7 +252,7 @@ static inline bool ovl_is_impuredir(struct dentry *dentry)
>  
>  /* namei.c */
>  int ovl_verify_origin(struct dentry *dentry, struct dentry *origin,
> -		      bool is_upper, bool set);
> +		      bool is_upper, bool set, bool strict);
>  int ovl_verify_index(struct dentry *index, struct ovl_path *lower,
>  		     unsigned int numlower);
>  int ovl_get_index_name(struct dentry *origin, struct qstr *name);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index af4d3e876398..f8bba4f8b940 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1120,7 +1120,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>  		/* Verify lower root is upper root origin */
>  		err = ovl_verify_origin(upperpath.dentry,
>  					oe->lowerstack[0].dentry,
> -					false, true);
> +					false, true, true);
>  		if (err) {
>  			pr_err("overlayfs: failed to verify upper root origin\n");
>  			goto out_free_oe;
> @@ -1131,7 +1131,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>  		if (ufs->indexdir) {
>  			/* Verify upper root is index dir origin */
>  			err = ovl_verify_origin(ufs->indexdir, upperpath.dentry,
> -						true, true);
> +						true, true, true);
>  			if (err)
>  				pr_err("overlayfs: failed to verify index dir origin\n");
>  
> 

Re: [PATCH 2/3] ovl: set origin xattr for merge dir on lookup

[Amir Goldstein]

On Wed, Nov 8, 2017 at 8:59 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> On 2017/11/8 5:38, Amir Goldstein Wrote:
>> For old existing merge dirs whose "origin" xattr was not set at copy up
>> time, we amend the situation on lookup.
>>
>> If no origin fh is stored in upper of a merge dir, store fh of upper most
>> lower dir or 'null' fh if lower does not support file handles. We do this
>> so we can filter out whiteouts in case lower dir is removed offline and
>> then upper dir becomes a pure upper in a future mount.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/namei.c     | 61 ++++++++++++++++++++++++++++++++++++++++--------
>>  fs/overlayfs/overlayfs.h |  2 +-
>>  fs/overlayfs/super.c     |  4 ++--
>>  3 files changed, 54 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>> index 322c6214114f..65606a0a124c 100644
>> --- a/fs/overlayfs/namei.c
>> +++ b/fs/overlayfs/namei.c
>> @@ -87,6 +87,8 @@ static int ovl_acceptable(void *ctx, struct dentry *dentry)
>>       return 1;
>>  }
>>
>> +static struct ovl_fh ovl_null_fh;
>> +
>>  static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
>>  {
>>       int res;
>> @@ -100,7 +102,7 @@ static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
>>       }
>>       /* Zero size value means "copied up but origin unknown" */
>>       if (res == 0)
>> -             return NULL;
>> +             goto null_fh;
>>
>>       fh  = kzalloc(res, GFP_KERNEL);
>>       if (!fh)
>> @@ -118,12 +120,12 @@ static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
>>
>>       /* Treat larger version and unknown flags as "origin unknown" */
>>       if (fh->version > OVL_FH_VERSION || fh->flags & ~OVL_FH_FLAG_ALL)
>> -             goto out;
>> +             goto null_fh;
>>
>>       /* Treat endianness mismatch as "origin unknown" */
>>       if (!(fh->flags & OVL_FH_FLAG_ANY_ENDIAN) &&
>>           (fh->flags & OVL_FH_FLAG_BIG_ENDIAN) != OVL_FH_FLAG_CPU_ENDIAN)
>> -             goto out;
>> +             goto null_fh;
>>
>>       return fh;
>>
>> @@ -131,6 +133,10 @@ static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
>>       kfree(fh);
>>       return NULL;
>>
>> +null_fh:
>> +     kfree(fh);
>> +     return &ovl_null_fh;
>> +
>>  fail:
>>       pr_warn_ratelimited("overlayfs: failed to get origin (%i)\n", res);
>>       goto out;
>> @@ -149,6 +155,9 @@ static struct dentry *ovl_get_origin(struct dentry *dentry,
>>       if (IS_ERR_OR_NULL(fh))
>>               return (struct dentry *)fh;
>>
>> +     if (fh == &ovl_null_fh)
>> +             return NULL;
>> +
>>       /*
>>        * Make sure that the stored uuid matches the uuid of the lower
>>        * layer where file handle will be decoded.
>> @@ -333,6 +342,10 @@ static int ovl_verify_origin_fh(struct dentry *dentry, const struct ovl_fh *fh)
>>       if (IS_ERR(ofh))
>>               return PTR_ERR(ofh);
>>
>> +     /* NULL fh (no lower fh support) and stored 'null' fh is a match */
>> +     if (ofh == &ovl_null_fh)
>> +             return fh ? -ESTALE : 0;
>> +
>
> Is -ESTALE -> -ENODATA better ?
>

If -ENODATA is returned of null_fh then ovl_verify_origin() will set
xattr (back to null_fh)
on every lookup, so we need to be able to distinguish between the
cases of no origin
(-ENODATA) and null_fh.
To be honest I have mixed feelings about the ovl_null_fh trick.
We should probably just pick an arbitrary error code to use internally
to describe null_fh,
like -ENOENT, but a part of me thought that the ovl_null_fh trick is
clearer to read???


> Consider the case: We set 'null' fh in merged upper dir on a 'file handle' not supported
> base filesystem, then we copy or tar all underlying directories to another base filesystem which is
> 'file handle' supported and mount overlayfs again. I think we can update upperdir's fh instead of
> return fail.
>

In what way does it help us to update fh in that case?
We are not following to lower directory by fh anyway.
And we cannot really trust that the restored origin is correct
in the context of my 'verify_dir' patches.
The only use case I see for doing that is container migration - if tar
of container
will deliberately replace concrete origin fh on all dirs with null_fh
before migration,
knowing that origin fh are going to become stale on migration.
And then after migration, iterate all dirs in overlay to restore origin.
But we can leave that to a separate change. This one is mostly concerned
about setting origin for the purpose of whiteout exposure.

> At the same time, consider the opposite side, all directories move from a base filesystem which 'file
> handle' supported to a not supported one. It will trigger NULL pointer crash in ovl_verify_origin_fh()
> because fh is NULL but ofh was existed. Following change good ?
>
> -       if (!ofh)
> +       if (!ofh || !fh)
>                 return -ENODATA;
>

Thanks for pointing that out, but change is not good because it looses the
information about finding a stored null_fh.
If we do want to stick with ovl_null_fh trick, I could pass in &ovl_null_fh
instead of NULL, in case of !ovl_can_decode_fh(origin->d_sb).
This fixes the NULL pointer crash and I can remove this special case,
because fh->len + memcmp will do the right thing:

        /* NULL fh (no lower fh support) and stored 'null' fh is a match */
        if (ofh == fh)
                return fh ? -ESTALE : 0;

Amir.

Re: [PATCH 2/3] ovl: set origin xattr for merge dir on lookup

[Amir Goldstein]

On Wed, Nov 8, 2017 at 10:27 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Nov 8, 2017 at 8:59 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> On 2017/11/8 5:38, Amir Goldstein Wrote:
>>> For old existing merge dirs whose "origin" xattr was not set at copy up
>>> time, we amend the situation on lookup.
>>>
>>> If no origin fh is stored in upper of a merge dir, store fh of upper most
>>> lower dir or 'null' fh if lower does not support file handles. We do this
>>> so we can filter out whiteouts in case lower dir is removed offline and
>>> then upper dir becomes a pure upper in a future mount.
>>>
>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>>> ---
[...]
>
>> At the same time, consider the opposite side, all directories move from a base filesystem which 'file
>> handle' supported to a not supported one. It will trigger NULL pointer crash in ovl_verify_origin_fh()
>> because fh is NULL but ofh was existed. Following change good ?
>>
>> -       if (!ofh)
>> +       if (!ofh || !fh)
>>                 return -ENODATA;
>>
>
> Thanks for pointing that out, but change is not good because it looses the
> information about finding a stored null_fh.
> If we do want to stick with ovl_null_fh trick, I could pass in &ovl_null_fh
> instead of NULL, in case of !ovl_can_decode_fh(origin->d_sb).
> This fixes the NULL pointer crash and I can remove this special case,

Zhangyi,

I pushed a fix rebased on overlayfs-next from today to
https://github.com/amir73il/linux/commits/ovl-whiteouts

Tested it with xfs and with xfs without uuid (test patch).
I did not test the migration cases that you listed.

Please review.
Thanks,
Amir.

Re: [PATCH 2/3] ovl: set origin xattr for merge dir on lookup

[zhangyi (F)]

On 2017/11/8 21:30, Amir Goldstein Wrote:
> On Wed, Nov 8, 2017 at 10:27 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Wed, Nov 8, 2017 at 8:59 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>> On 2017/11/8 5:38, Amir Goldstein Wrote:
>>>> For old existing merge dirs whose "origin" xattr was not set at copy up
>>>> time, we amend the situation on lookup.
>>>>
>>>> If no origin fh is stored in upper of a merge dir, store fh of upper most
>>>> lower dir or 'null' fh if lower does not support file handles. We do this
>>>> so we can filter out whiteouts in case lower dir is removed offline and
>>>> then upper dir becomes a pure upper in a future mount.
>>>>
>>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>>>> ---
> [...]
>>
>>> At the same time, consider the opposite side, all directories move from a base filesystem which 'file
>>> handle' supported to a not supported one. It will trigger NULL pointer crash in ovl_verify_origin_fh()
>>> because fh is NULL but ofh was existed. Following change good ?
>>>
>>> -       if (!ofh)
>>> +       if (!ofh || !fh)
>>>                 return -ENODATA;
>>>
>>
>> Thanks for pointing that out, but change is not good because it looses the
>> information about finding a stored null_fh.
>> If we do want to stick with ovl_null_fh trick, I could pass in &ovl_null_fh
>> instead of NULL, in case of !ovl_can_decode_fh(origin->d_sb).
>> This fixes the NULL pointer crash and I can remove this special case,
> 
> Zhangyi,
> 
> I pushed a fix rebased on overlayfs-next from today to
> https://github.com/amir73il/linux/commits/ovl-whiteouts
> 
> Tested it with xfs and with xfs without uuid (test patch).

Hi Amir:

Sorry for the late, I prefer to use ovl_null_fh too, this patch looks good to me.

> I did not test the migration cases that you listed.

As far as know, docker container migration will tar all things in upper to lower layer,
may be I thought to much and we may no need to deal with this case now.

Thanks,
Yi.

> Please review.
> Thanks,
> Amir.
> 

[PATCH 3/3] ovl: recover from whiteouts in impure dir iteration

[Amir Goldstein]

A merge dir iteration builds the list of files and removes whiteous
from the list before exposing the list to user.

An impure dir iteration is just a translation layer, so it cannot remove
entries from real dir list.

When we find a whiteout when starting impure dir iteration, we abort
impure dir iteration, mark that dir has whiteouts by setting the
'whiteouts' inode flag and fall back to merge dir iteration to filter
out the whiteouts.  If dir is upper and xattr supported, we set a
'null' origin xattr, so the 'whiteouts' flag will be set when loading
inode from disk.

If we find a whiteout in the middle of non-merge dir iteration, this
is very strange, because whiteout cannot apear in a non-merge dir, so
return ESTALE to caller. In that case, next iteration will be merge dir
iteration.

For pure upper with xattr support, error will be healed permanently.
For pure lower dir or no xattr support, error will be healed for as long
as dir inode remains in cache.

Note that under certain conditions, we will not have a reason to start
impure dir iteration on a non-merge dir and therefore, whiteouts will
be exposed. Here is an example for such conditions:
An upper dir was copied up with kernel < v4.12, all files in it are
either pure or were copied up with kernel < v4.12, whiteouts in it were
created with kernel <= v4.14.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/readdir.c   | 41 ++++++++++++++++++++++++++++++++++++++---
 fs/overlayfs/util.c      | 21 +++++++++++++++++++++
 3 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index df925188394f..5fb2d434cff3 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -235,6 +235,7 @@ int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
 		       const char *name, const void *value, size_t size,
 		       int xerr);
 int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry);
+int ovl_set_whiteouts(struct dentry *dentry);
 void ovl_set_flag(unsigned long flag, struct inode *inode);
 void ovl_clear_flag(unsigned long flag, struct inode *inode);
 bool ovl_test_flag(unsigned long flag, struct inode *inode);
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 5397ceafc4ac..1e27b9eb389d 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -498,6 +498,9 @@ static int ovl_cache_update_ino(struct path *path, struct ovl_cache_entry *p)
 			this = NULL;
 			goto fail;
 		}
+		/* Nothing there? assume this is a whiteout */
+		if (p->type == DT_CHR)
+			p->is_whiteout = true;
 		goto out;
 	}
 
@@ -577,7 +580,15 @@ static int ovl_dir_read_impure(struct path *path,  struct list_head *list,
 			if (err)
 				return err;
 		}
-		if (p->ino == p->real_ino) {
+		if (p->is_whiteout) {
+			/*
+			 * Found a leftover whiteout in non-merge dir from a
+			 * time it was upper of merge dir? abort impure dir
+			 * iteration and fall back to merge dir iteration to
+			 * filter out whiteouts.
+			 */
+			return -ESTALE;
+		} else if (p->ino == p->real_ino) {
 			list_del(&p->l_node);
 			kfree(p);
 		} else {
@@ -724,9 +735,33 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 		    (ovl_same_sb(dentry->d_sb) &&
 		     (ovl_test_flag(OVL_IMPURE, d_inode(dentry)) ||
 		      OVL_TYPE_MERGE(ovl_path_type(dentry->d_parent))))) {
-			return ovl_iterate_real(file, ctx);
+			err = ovl_iterate_real(file, ctx);
+			/*
+			 * If we found a whiteout leftover when starting
+			 * non-merge dir iteration (pos == 0), we fall back to
+			 * merge dir iteration after setting the OVL_WHITEOUTS
+			 * inode flag.  If we found a whiteout in the middle of
+			 * non-merge dir iteration, this is very strange,
+			 * because whiteout cannot apear in a non-merge dir,
+			 * so return ESTALE to caller in that case. Next
+			 * iteration will be merge dir iteration.
+			 *
+			 * For pure upper with xattr support, the OVL_WHITEOUTS
+			 * flag is stored as a 'null' origin, so error will be
+			 * healed permanently. For pure lower dir or no xattr
+			 * support, error will be healed for as long as dir
+			 * inode stays in cache.
+			 */
+			if (err == -ESTALE) {
+				ovl_set_whiteouts(dentry);
+				if (!ctx->pos)
+					ovl_dir_reset(file);
+			}
+			if (od->is_real)
+				return err;
+		} else {
+			return iterate_dir(od->realfile, ctx);
 		}
-		return iterate_dir(od->realfile, ctx);
 	}
 
 	if (!od->cache) {
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 255ae337fc46..cf46da8bfa08 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -426,6 +426,27 @@ int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry)
 	return err;
 }
 
+int ovl_set_whiteouts(struct dentry *dentry)
+{
+	struct dentry *upper = ovl_dentry_upper(dentry);
+
+	if (ovl_test_flag(OVL_WHITEOUTS, d_inode(dentry)))
+		return 0;
+
+	/*
+	 * Mark that pure dir has whiteouts by setting a the 'whiteouts' flag,
+	 * so we are protected from exposing whiteouts while inode remains in
+	 * cache.  If dir is upper and xattr supported, set a 'null' origin fh
+	 * so the 'whiteouts' flag will be set when loading inode from disk.
+	 */
+	ovl_set_flag(OVL_WHITEOUTS, d_inode(dentry));
+
+	if (!upper)
+		return 0;
+
+	return ovl_check_setxattr(dentry, upper, OVL_XATTR_ORIGIN, "", 0, 0);
+}
+
 void ovl_set_flag(unsigned long flag, struct inode *inode)
 {
 	set_bit(flag, &OVL_I(inode)->flags);
-- 
2.7.4

Re: [PATCH 0/3] Overlayfs restoring of origin fh

[Miklos Szeredi]

On Tue, Nov 7, 2017 at 10:38 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Miklos,
>
> This series is complementary to the whiteouts expose patches.
> Patch 2 restores origin xattr on lookup of merge dir (that was copied up
> before v4.12).
> Patch 3 sets a 'null' origin fh on dir if we happen to iterate an 'impure'
> dir and find a whiteout while looking for copy ups. This is expected to
> happen if merge dir was copied up before v4.12, but files inside it were
> copied up after v4.12.

Applied 1/3.

Does the other two patches have any real usecase?   AFAICS this is not
fixing a backward compatibility issue, so I'm sceptical of the value.

Even if there was a real usecase, we could introduce an "fsck.overlay"
utility that would fix up overlays to be able to take advantage of new
features.

Thanks,
Miklos

Re: [PATCH 0/3] Overlayfs restoring of origin fh

[Amir Goldstein]

On Thu, Nov 9, 2017 at 12:07 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, Nov 7, 2017 at 10:38 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> Miklos,
>>
>> This series is complementary to the whiteouts expose patches.
>> Patch 2 restores origin xattr on lookup of merge dir (that was copied up
>> before v4.12).
>> Patch 3 sets a 'null' origin fh on dir if we happen to iterate an 'impure'
>> dir and find a whiteout while looking for copy ups. This is expected to
>> happen if merge dir was copied up before v4.12, but files inside it were
>> copied up after v4.12.
>
> Applied 1/3.
>
> Does the other two patches have any real usecase?   AFAICS this is not
> fixing a backward compatibility issue, so I'm sceptical of the value.

No real usecase for me. I don't mind if they stay out, but I'll
explain why I posted them.

Patch 2/3 is just pieces from 'verify_dir' patches, so I figured it
may be useful
on its own for this purpose.

Patch 3/3 is just something I saw as 'low hanging' - if we lookup impure files
anyway and detect a whiteouts - there is a proper response, so why not use it?

All copy ups since v4.12 mark the parent dir "impure", but not "origin"
what's the logic in taking care of impurity of parent on copy up and not taking
care of parent whiteouts?

So I see these patches as "consistent behavior" patches, rather than any MUST.

Could be handled by fsck.overlayfs, which should also handle migration of
origin file handles to new location anyway.

Cheers,
Amir.

Re: [PATCH 0/3] Overlayfs restoring of origin fh

[zhangyi (F)]

On 2017/11/9 18:07, Miklos Szeredi Wrote:
> On Tue, Nov 7, 2017 at 10:38 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> Miklos,
>>
>> This series is complementary to the whiteouts expose patches.
>> Patch 2 restores origin xattr on lookup of merge dir (that was copied up
>> before v4.12).
>> Patch 3 sets a 'null' origin fh on dir if we happen to iterate an 'impure'
>> dir and find a whiteout while looking for copy ups. This is expected to
>> happen if merge dir was copied up before v4.12, but files inside it were
>> copied up after v4.12.
> 
> Applied 1/3.
> 
> Does the other two patches have any real usecase?   AFAICS this is not
> fixing a backward compatibility issue, so I'm sceptical of the value.
> 
> Even if there was a real usecase, we could introduce an "fsck.overlay"
> utility that would fix up overlays to be able to take advantage of new
> features.

Hi Miklos,

Now, I am working on the "fsck.overlay" in my spare time, and the following functions
have already been done:
1) Check and (auto)fix redundant whiteouts,
2) Check and (auto)fix invalid redirect/opaque xattr.

But it is still a very crude version now, maybe I can send out for some suggestions
next week. :)

Thanks,
Yi.

> Thanks,
> Miklos
> 
>