All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ovl: fix EIO from lookup of non-indexed upper
@ 2017-10-12 16:03 Amir Goldstein
  2017-10-13 13:34 ` Vivek Goyal
  2017-10-24  9:59 ` Miklos Szeredi
  0 siblings, 2 replies; 6+ messages in thread
From: Amir Goldstein @ 2017-10-12 16:03 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Vivek Goyal, linux-unionfs, # v4 . 13

Commit fbaf94ee3cd5 ("ovl: don't set origin on broken lower hardlink")
attempt to avoid the condition of non-indexed upper inode with lower
hardlink as origin. If this condition is found, lookup returns EIO.

The protection of commit mentioned above does not cover the case of lower
that is not a hardlink when it is copied up (with either index=off/on)
and then lower is hardlinked while overlay is offline.

Changes to lower layer while overlayfs is offline should not result in
unexpected behavior, so a permanent EIO error after creating a link in
lower layer should not be considered as correct behavior.

This fix replaces EIO error with a warning in cases where upper has
origin but no index is found, or index is found that does not match upper
inode. In those cases, lookup will not fail and the returned overlay
inode will be hashed by upper inode instead of by lower origin inode.

Fixes: 359f392ca53e ("ovl: lookup index entry for copy up origin")
Cc: <stable@vger.kernel.org> # v4.13
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Miklos,

Following a discussion with Vivek about metacopy feature and the option
of setting ORIGIN for non-indexed lower hardlinks on copy up, I came to
a conclusion that the current EIO behavior is not quite tollerant to lower
changes as one would hope and that it should be fixed in stable kernels.

I've implemented an xfstest to test the EIO behavior has been fixed [1].

Amir.

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

 fs/overlayfs/inode.c     | 20 ++++++++++++++++----
 fs/overlayfs/namei.c     | 22 ++++++++++++----------
 fs/overlayfs/overlayfs.h |  3 ++-
 3 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index a619addecafc..321511ed8c42 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -598,18 +598,30 @@ static bool ovl_verify_inode(struct inode *inode, struct dentry *lowerdentry,
 	return true;
 }
 
-struct inode *ovl_get_inode(struct dentry *dentry, struct dentry *upperdentry)
+struct inode *ovl_get_inode(struct dentry *dentry, struct dentry *upperdentry,
+			    struct dentry *index)
 {
 	struct dentry *lowerdentry = ovl_dentry_lower(dentry);
 	struct inode *realinode = upperdentry ? d_inode(upperdentry) : NULL;
 	struct inode *inode;
+	/* Already indexed or could be indexed on copy up? */
+	bool indexed = (index || (ovl_indexdir(dentry->d_sb) && !upperdentry));
+
+	if (WARN_ON(upperdentry && indexed && !lowerdentry))
+		return ERR_PTR(-EIO);
 
 	if (!realinode)
 		realinode = d_inode(lowerdentry);
 
-	if (!S_ISDIR(realinode->i_mode) &&
-	    (upperdentry || (lowerdentry && ovl_indexdir(dentry->d_sb)))) {
-		struct inode *key = d_inode(lowerdentry ?: upperdentry);
+	/*
+	 * Copy up origin (lower) may exist for non-indexed upper, but we must
+	 * not use lower as hash key in that case.
+	 * Hash inodes that are or could be indexed by origin inode and
+	 * non-indexed upper inodes that could be hard linked by upper inode.
+	 */
+	if (!S_ISDIR(realinode->i_mode) && (upperdentry || indexed)) {
+		struct inode *key = d_inode(indexed ? lowerdentry :
+						      upperdentry);
 		unsigned int nlink;
 
 		inode = iget5_locked(dentry->d_sb, (unsigned long) key,
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 654bea1a5ac9..88ff1daeb3d7 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -517,17 +517,14 @@ static struct dentry *ovl_lookup_index(struct dentry *dentry,
 	inode = d_inode(index);
 	if (d_is_negative(index)) {
 		if (upper && d_inode(origin)->i_nlink > 1) {
-			pr_warn_ratelimited("overlayfs: hard link with origin but no index (ino=%lu).\n",
-					    d_inode(origin)->i_ino);
-			goto fail;
+			pr_warn_ratelimited("overlayfs: hard link with origin but no index (%pd2).\n",
+					    upper);
 		}
-
-		dput(index);
-		index = NULL;
+		goto out_dput;
 	} else if (upper && d_inode(upper) != inode) {
-		pr_warn_ratelimited("overlayfs: wrong index found (index=%pd2, ino=%lu, upper ino=%lu).\n",
-				    index, inode->i_ino, d_inode(upper)->i_ino);
-		goto fail;
+		pr_warn_ratelimited("overlayfs: broken hard link - ignoring index (%pd2).\n",
+				    upper);
+		goto out_dput;
 	} else if (ovl_dentry_weird(index) || ovl_is_whiteout(index) ||
 		   ((inode->i_mode ^ d_inode(origin)->i_mode) & S_IFMT)) {
 		/*
@@ -547,6 +544,11 @@ static struct dentry *ovl_lookup_index(struct dentry *dentry,
 	kfree(name.name);
 	return index;
 
+out_dput:
+	dput(index);
+	index = NULL;
+	goto out;
+
 fail:
 	dput(index);
 	index = ERR_PTR(-EIO);
@@ -709,7 +711,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		upperdentry = dget(index);
 
 	if (upperdentry || ctr) {
-		inode = ovl_get_inode(dentry, upperdentry);
+		inode = ovl_get_inode(dentry, upperdentry, index);
 		err = PTR_ERR(inode);
 		if (IS_ERR(inode))
 			goto out_free_oe;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index c706a6f99928..d9a0edd4e57e 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -286,7 +286,8 @@ int ovl_update_time(struct inode *inode, struct timespec *ts, int flags);
 bool ovl_is_private_xattr(const char *name);
 
 struct inode *ovl_new_inode(struct super_block *sb, umode_t mode, dev_t rdev);
-struct inode *ovl_get_inode(struct dentry *dentry, struct dentry *upperdentry);
+struct inode *ovl_get_inode(struct dentry *dentry, struct dentry *upperdentry,
+			    struct dentry *index);
 static inline void ovl_copyattr(struct inode *from, struct inode *to)
 {
 	to->i_uid = from->i_uid;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] ovl: fix EIO from lookup of non-indexed upper
  2017-10-12 16:03 [PATCH] ovl: fix EIO from lookup of non-indexed upper Amir Goldstein
@ 2017-10-13 13:34 ` Vivek Goyal
  2017-10-13 15:20   ` Amir Goldstein
  2017-10-24  9:59 ` Miklos Szeredi
  1 sibling, 1 reply; 6+ messages in thread
From: Vivek Goyal @ 2017-10-13 13:34 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Miklos Szeredi, linux-unionfs, # v4 . 13

On Thu, Oct 12, 2017 at 07:03:04PM +0300, Amir Goldstein wrote:
> Commit fbaf94ee3cd5 ("ovl: don't set origin on broken lower hardlink")
> attempt to avoid the condition of non-indexed upper inode with lower
> hardlink as origin. If this condition is found, lookup returns EIO.
> 
> The protection of commit mentioned above does not cover the case of lower
> that is not a hardlink when it is copied up (with either index=off/on)
> and then lower is hardlinked while overlay is offline.
> 
> Changes to lower layer while overlayfs is offline should not result in
> unexpected behavior, so a permanent EIO error after creating a link in
> lower layer should not be considered as correct behavior.
> 
> This fix replaces EIO error with a warning in cases where upper has
> origin but no index is found, or index is found that does not match upper
> inode. In those cases, lookup will not fail and the returned overlay
> inode will be hashed by upper inode instead of by lower origin inode.
> 
> Fixes: 359f392ca53e ("ovl: lookup index entry for copy up origin")
> Cc: <stable@vger.kernel.org> # v4.13
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> Miklos,
> 
> Following a discussion with Vivek about metacopy feature and the option
> of setting ORIGIN for non-indexed lower hardlinks on copy up, I came to
> a conclusion that the current EIO behavior is not quite tollerant to lower
> changes as one would hope and that it should be fixed in stable kernels.
> 
> I've implemented an xfstest to test the EIO behavior has been fixed [1].
> 
> Amir.
> 
> [1] https://github.com/amir73il/xfstests/commits/overlayfs-devel
> 
>  fs/overlayfs/inode.c     | 20 ++++++++++++++++----
>  fs/overlayfs/namei.c     | 22 ++++++++++++----------
>  fs/overlayfs/overlayfs.h |  3 ++-
>  3 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index a619addecafc..321511ed8c42 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -598,18 +598,30 @@ static bool ovl_verify_inode(struct inode *inode, struct dentry *lowerdentry,
>  	return true;
>  }
>  
> -struct inode *ovl_get_inode(struct dentry *dentry, struct dentry *upperdentry)
> +struct inode *ovl_get_inode(struct dentry *dentry, struct dentry *upperdentry,
> +			    struct dentry *index)
>  {
>  	struct dentry *lowerdentry = ovl_dentry_lower(dentry);
>  	struct inode *realinode = upperdentry ? d_inode(upperdentry) : NULL;
>  	struct inode *inode;
> +	/* Already indexed or could be indexed on copy up? */
> +	bool indexed = (index || (ovl_indexdir(dentry->d_sb) && !upperdentry));

Hi Amir,

Looks like current code hashes inodes of lowerdentry even if nlink=1
(index=on, upperdentry=NULL). IIUC this will never be indexed. 

So I am wondering why do we hash inodes of lower when nlink=1. When is it
used.

Vivek

> +
> +	if (WARN_ON(upperdentry && indexed && !lowerdentry))
> +		return ERR_PTR(-EIO);
>  
>  	if (!realinode)
>  		realinode = d_inode(lowerdentry);
>  
> -	if (!S_ISDIR(realinode->i_mode) &&
> -	    (upperdentry || (lowerdentry && ovl_indexdir(dentry->d_sb)))) {
> -		struct inode *key = d_inode(lowerdentry ?: upperdentry);
> +	/*
> +	 * Copy up origin (lower) may exist for non-indexed upper, but we must
> +	 * not use lower as hash key in that case.
> +	 * Hash inodes that are or could be indexed by origin inode and
> +	 * non-indexed upper inodes that could be hard linked by upper inode.
> +	 */
> +	if (!S_ISDIR(realinode->i_mode) && (upperdentry || indexed)) {
> +		struct inode *key = d_inode(indexed ? lowerdentry :
> +						      upperdentry);
>  		unsigned int nlink;
>  
>  		inode = iget5_locked(dentry->d_sb, (unsigned long) key,
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 654bea1a5ac9..88ff1daeb3d7 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -517,17 +517,14 @@ static struct dentry *ovl_lookup_index(struct dentry *dentry,
>  	inode = d_inode(index);
>  	if (d_is_negative(index)) {
>  		if (upper && d_inode(origin)->i_nlink > 1) {
> -			pr_warn_ratelimited("overlayfs: hard link with origin but no index (ino=%lu).\n",
> -					    d_inode(origin)->i_ino);
> -			goto fail;
> +			pr_warn_ratelimited("overlayfs: hard link with origin but no index (%pd2).\n",
> +					    upper);
>  		}
> -
> -		dput(index);
> -		index = NULL;
> +		goto out_dput;
>  	} else if (upper && d_inode(upper) != inode) {
> -		pr_warn_ratelimited("overlayfs: wrong index found (index=%pd2, ino=%lu, upper ino=%lu).\n",
> -				    index, inode->i_ino, d_inode(upper)->i_ino);
> -		goto fail;
> +		pr_warn_ratelimited("overlayfs: broken hard link - ignoring index (%pd2).\n",
> +				    upper);
> +		goto out_dput;
>  	} else if (ovl_dentry_weird(index) || ovl_is_whiteout(index) ||
>  		   ((inode->i_mode ^ d_inode(origin)->i_mode) & S_IFMT)) {
>  		/*
> @@ -547,6 +544,11 @@ static struct dentry *ovl_lookup_index(struct dentry *dentry,
>  	kfree(name.name);
>  	return index;
>  
> +out_dput:
> +	dput(index);
> +	index = NULL;
> +	goto out;
> +
>  fail:
>  	dput(index);
>  	index = ERR_PTR(-EIO);
> @@ -709,7 +711,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>  		upperdentry = dget(index);
>  
>  	if (upperdentry || ctr) {
> -		inode = ovl_get_inode(dentry, upperdentry);
> +		inode = ovl_get_inode(dentry, upperdentry, index);
>  		err = PTR_ERR(inode);
>  		if (IS_ERR(inode))
>  			goto out_free_oe;
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index c706a6f99928..d9a0edd4e57e 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -286,7 +286,8 @@ int ovl_update_time(struct inode *inode, struct timespec *ts, int flags);
>  bool ovl_is_private_xattr(const char *name);
>  
>  struct inode *ovl_new_inode(struct super_block *sb, umode_t mode, dev_t rdev);
> -struct inode *ovl_get_inode(struct dentry *dentry, struct dentry *upperdentry);
> +struct inode *ovl_get_inode(struct dentry *dentry, struct dentry *upperdentry,
> +			    struct dentry *index);
>  static inline void ovl_copyattr(struct inode *from, struct inode *to)
>  {
>  	to->i_uid = from->i_uid;
> -- 
> 2.7.4

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ovl: fix EIO from lookup of non-indexed upper
  2017-10-13 13:34 ` Vivek Goyal
@ 2017-10-13 15:20   ` Amir Goldstein
  0 siblings, 0 replies; 6+ messages in thread
From: Amir Goldstein @ 2017-10-13 15:20 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: Miklos Szeredi, overlayfs, # v4 . 13

On Fri, Oct 13, 2017 at 4:34 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Oct 12, 2017 at 07:03:04PM +0300, Amir Goldstein wrote:
>> Commit fbaf94ee3cd5 ("ovl: don't set origin on broken lower hardlink")
>> attempt to avoid the condition of non-indexed upper inode with lower
>> hardlink as origin. If this condition is found, lookup returns EIO.
>>
>> The protection of commit mentioned above does not cover the case of lower
>> that is not a hardlink when it is copied up (with either index=off/on)
>> and then lower is hardlinked while overlay is offline.
>>
>> Changes to lower layer while overlayfs is offline should not result in
>> unexpected behavior, so a permanent EIO error after creating a link in
>> lower layer should not be considered as correct behavior.
>>
>> This fix replaces EIO error with a warning in cases where upper has
>> origin but no index is found, or index is found that does not match upper
>> inode. In those cases, lookup will not fail and the returned overlay
>> inode will be hashed by upper inode instead of by lower origin inode.
>>
>> Fixes: 359f392ca53e ("ovl: lookup index entry for copy up origin")
>> Cc: <stable@vger.kernel.org> # v4.13
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>
>> Miklos,
>>
>> Following a discussion with Vivek about metacopy feature and the option
>> of setting ORIGIN for non-indexed lower hardlinks on copy up, I came to
>> a conclusion that the current EIO behavior is not quite tollerant to lower
>> changes as one would hope and that it should be fixed in stable kernels.
>>
>> I've implemented an xfstest to test the EIO behavior has been fixed [1].
>>
>> Amir.
>>
>> [1] https://github.com/amir73il/xfstests/commits/overlayfs-devel
>>
>>  fs/overlayfs/inode.c     | 20 ++++++++++++++++----
>>  fs/overlayfs/namei.c     | 22 ++++++++++++----------
>>  fs/overlayfs/overlayfs.h |  3 ++-
>>  3 files changed, 30 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
>> index a619addecafc..321511ed8c42 100644
>> --- a/fs/overlayfs/inode.c
>> +++ b/fs/overlayfs/inode.c
>> @@ -598,18 +598,30 @@ static bool ovl_verify_inode(struct inode *inode, struct dentry *lowerdentry,
>>       return true;
>>  }
>>
>> -struct inode *ovl_get_inode(struct dentry *dentry, struct dentry *upperdentry)
>> +struct inode *ovl_get_inode(struct dentry *dentry, struct dentry *upperdentry,
>> +                         struct dentry *index)
>>  {
>>       struct dentry *lowerdentry = ovl_dentry_lower(dentry);
>>       struct inode *realinode = upperdentry ? d_inode(upperdentry) : NULL;
>>       struct inode *inode;
>> +     /* Already indexed or could be indexed on copy up? */
>> +     bool indexed = (index || (ovl_indexdir(dentry->d_sb) && !upperdentry));
>
> Hi Amir,
>
> Looks like current code hashes inodes of lowerdentry even if nlink=1
> (index=on, upperdentry=NULL). IIUC this will never be indexed.
>
> So I am wondering why do we hash inodes of lower when nlink=1. When is it
> used.
>

To understand this need a bit of past and future perspective on index feature.
I am developing index feature for:
1. Not breaking hardlinks
2. NFS export

My first version of index patches indexed all non-dir on copy up as well
as hash all non-dir inodes, with the intention of using index for both goals.

Miklos reduced the scope of index=on to index only lower hardlinks,
because at this point in time, the sole purpose of index in not
breaking hardlinks.
(I have now implemented index=all to cater NFS export).

IIRC, in one version of the patches, Miklos added hashing conditional
to lower nlink > 1 and I voted against it, so he relaxed the condition.

My argument was that we need to minimize the damage in case lower nlink
is changed while overlay is mounted. My feeling was that allowing 2
overlay inode
to co-exist, one from when lower nlink == 1 and another from when
lower nlink > 1
can lead us to very bad places, because concurrent copy up of the 2
lower aliases
is not protected by the same overlay inode lock.

Anyway, AFAICT, we are gaining something (robustness) and not loosing anything
from hashing lower inodes.

Amir.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ovl: fix EIO from lookup of non-indexed upper
  2017-10-12 16:03 [PATCH] ovl: fix EIO from lookup of non-indexed upper Amir Goldstein
  2017-10-13 13:34 ` Vivek Goyal
@ 2017-10-24  9:59 ` Miklos Szeredi
  2017-10-24 10:15   ` Amir Goldstein
  1 sibling, 1 reply; 6+ messages in thread
From: Miklos Szeredi @ 2017-10-24  9:59 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Vivek Goyal, linux-unionfs, # v4 . 13

On Thu, Oct 12, 2017 at 6:03 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Commit fbaf94ee3cd5 ("ovl: don't set origin on broken lower hardlink")
> attempt to avoid the condition of non-indexed upper inode with lower
> hardlink as origin. If this condition is found, lookup returns EIO.
>
> The protection of commit mentioned above does not cover the case of lower
> that is not a hardlink when it is copied up (with either index=off/on)
> and then lower is hardlinked while overlay is offline.
>
> Changes to lower layer while overlayfs is offline should not result in
> unexpected behavior, so a permanent EIO error after creating a link in
> lower layer should not be considered as correct behavior.
>
> This fix replaces EIO error with a warning in cases where upper has
> origin but no index is found, or index is found that does not match upper
> inode. In those cases, lookup will not fail and the returned overlay
> inode will be hashed by upper inode instead of by lower origin inode.
>
> Fixes: 359f392ca53e ("ovl: lookup index entry for copy up origin")
> Cc: <stable@vger.kernel.org> # v4.13
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> Miklos,
>
> Following a discussion with Vivek about metacopy feature and the option
> of setting ORIGIN for non-indexed lower hardlinks on copy up, I came to
> a conclusion that the current EIO behavior is not quite tollerant to lower
> changes as one would hope and that it should be fixed in stable kernels.

Okay, but I started wondering if we really should be writing warnings
to the kernel log if this situation is considered normal.

Is it worth warning about these?

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ovl: fix EIO from lookup of non-indexed upper
  2017-10-24  9:59 ` Miklos Szeredi
@ 2017-10-24 10:15   ` Amir Goldstein
  2017-10-24 10:18     ` Miklos Szeredi
  0 siblings, 1 reply; 6+ messages in thread
From: Amir Goldstein @ 2017-10-24 10:15 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Vivek Goyal, linux-unionfs, # v4 . 13

On Tue, Oct 24, 2017 at 12:59 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, Oct 12, 2017 at 6:03 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> Commit fbaf94ee3cd5 ("ovl: don't set origin on broken lower hardlink")
>> attempt to avoid the condition of non-indexed upper inode with lower
>> hardlink as origin. If this condition is found, lookup returns EIO.
>>
>> The protection of commit mentioned above does not cover the case of lower
>> that is not a hardlink when it is copied up (with either index=off/on)
>> and then lower is hardlinked while overlay is offline.
>>
>> Changes to lower layer while overlayfs is offline should not result in
>> unexpected behavior, so a permanent EIO error after creating a link in
>> lower layer should not be considered as correct behavior.
>>
>> This fix replaces EIO error with a warning in cases where upper has
>> origin but no index is found, or index is found that does not match upper
>> inode. In those cases, lookup will not fail and the returned overlay
>> inode will be hashed by upper inode instead of by lower origin inode.
>>
>> Fixes: 359f392ca53e ("ovl: lookup index entry for copy up origin")
>> Cc: <stable@vger.kernel.org> # v4.13
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>
>> Miklos,
>>
>> Following a discussion with Vivek about metacopy feature and the option
>> of setting ORIGIN for non-indexed lower hardlinks on copy up, I came to
>> a conclusion that the current EIO behavior is not quite tollerant to lower
>> changes as one would hope and that it should be fixed in stable kernels.
>
> Okay, but I started wondering if we really should be writing warnings
> to the kernel log if this situation is considered normal.
>
> Is it worth warning about these?
>

No I guess it is not worth it for plain index=on case, but it may be worth
it for upcoming index=all, which implies a complete index.
Although we can defer the warning to file handle encode time when that
matters.

BTW, I just sent a patch to remove the backward compatibility disclaimer
about OVERLAY_FS_INDEX  from Kconfig following the EIO change.
Maybe I was being too brave?...

Let me know if you want me to send a new patch for EIO behavior
or you will sort it out yourself (remember there is also the ENOENT case).

Amir.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ovl: fix EIO from lookup of non-indexed upper
  2017-10-24 10:15   ` Amir Goldstein
@ 2017-10-24 10:18     ` Miklos Szeredi
  0 siblings, 0 replies; 6+ messages in thread
From: Miklos Szeredi @ 2017-10-24 10:18 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Vivek Goyal, linux-unionfs, # v4 . 13

On Tue, Oct 24, 2017 at 12:15 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Oct 24, 2017 at 12:59 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Thu, Oct 12, 2017 at 6:03 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> Commit fbaf94ee3cd5 ("ovl: don't set origin on broken lower hardlink")
>>> attempt to avoid the condition of non-indexed upper inode with lower
>>> hardlink as origin. If this condition is found, lookup returns EIO.
>>>
>>> The protection of commit mentioned above does not cover the case of lower
>>> that is not a hardlink when it is copied up (with either index=off/on)
>>> and then lower is hardlinked while overlay is offline.
>>>
>>> Changes to lower layer while overlayfs is offline should not result in
>>> unexpected behavior, so a permanent EIO error after creating a link in
>>> lower layer should not be considered as correct behavior.
>>>
>>> This fix replaces EIO error with a warning in cases where upper has
>>> origin but no index is found, or index is found that does not match upper
>>> inode. In those cases, lookup will not fail and the returned overlay
>>> inode will be hashed by upper inode instead of by lower origin inode.
>>>
>>> Fixes: 359f392ca53e ("ovl: lookup index entry for copy up origin")
>>> Cc: <stable@vger.kernel.org> # v4.13
>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>>> ---
>>>
>>> Miklos,
>>>
>>> Following a discussion with Vivek about metacopy feature and the option
>>> of setting ORIGIN for non-indexed lower hardlinks on copy up, I came to
>>> a conclusion that the current EIO behavior is not quite tollerant to lower
>>> changes as one would hope and that it should be fixed in stable kernels.
>>
>> Okay, but I started wondering if we really should be writing warnings
>> to the kernel log if this situation is considered normal.
>>
>> Is it worth warning about these?
>>
>
> No I guess it is not worth it for plain index=on case, but it may be worth
> it for upcoming index=all, which implies a complete index.
> Although we can defer the warning to file handle encode time when that
> matters.
>
> BTW, I just sent a patch to remove the backward compatibility disclaimer
> about OVERLAY_FS_INDEX  from Kconfig following the EIO change.
> Maybe I was being too brave?...

Will think about it.

>
> Let me know if you want me to send a new patch for EIO behavior
> or you will sort it out yourself (remember there is also the ENOENT case).

I'll sort it out.

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-10-24 10:18 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-12 16:03 [PATCH] ovl: fix EIO from lookup of non-indexed upper Amir Goldstein
2017-10-13 13:34 ` Vivek Goyal
2017-10-13 15:20   ` Amir Goldstein
2017-10-24  9:59 ` Miklos Szeredi
2017-10-24 10:15   ` Amir Goldstein
2017-10-24 10:18     ` Miklos Szeredi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.