[PATCH V2] ovl: Pass ovl_get_nlink() parameters in right order

[PATCH V2] ovl: Pass ovl_get_nlink() parameters in right order

[Vivek Goyal]

Right now we seem to be passing index as "lowerdentry" and origin.dentry
as "upperdentry". IIUC, we should pass these parameters in reversed order
and this looks like a bug.

Fixes: caf70cb2ba5d ("ovl: cleanup orphan index entries")
Cc: <stable@vger.kernel.org> #4.13
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/namei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 625ed8066570..5ef69bc09e0c 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -435,7 +435,7 @@ int ovl_verify_index(struct dentry *index, struct ovl_path *lower,
 
 	/* Check if index is orphan and don't warn before cleaning it */
 	if (d_inode(index)->i_nlink == 1 &&
-	    ovl_get_nlink(index, origin.dentry, 0) == 0)
+	    ovl_get_nlink(origin.dentry, index, 0) == 0)
 		err = -ENOENT;
 
 	dput(origin.dentry);

Re: [PATCH V2] ovl: Pass ovl_get_nlink() parameters in right order

[Vivek Goyal]

On Mon, Nov 27, 2017 at 01:46:13PM -0500, Vivek Goyal wrote:
> Right now we seem to be passing index as "lowerdentry" and origin.dentry
> as "upperdentry". IIUC, we should pass these parameters in reversed order
> and this looks like a bug.
> 

Hi,

Don't merge this patch yet. I think this breaks overlay/042 and does not
cleanup orphan index. Trying to debug it.

Vivek

> Fixes: caf70cb2ba5d ("ovl: cleanup orphan index entries")
> Cc: <stable@vger.kernel.org> #4.13
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/namei.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 625ed8066570..5ef69bc09e0c 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -435,7 +435,7 @@ int ovl_verify_index(struct dentry *index, struct ovl_path *lower,
>  
>  	/* Check if index is orphan and don't warn before cleaning it */
>  	if (d_inode(index)->i_nlink == 1 &&
> -	    ovl_get_nlink(index, origin.dentry, 0) == 0)
> +	    ovl_get_nlink(origin.dentry, index, 0) == 0)
>  		err = -ENOENT;
>  
>  	dput(origin.dentry);

Re: [PATCH V2] ovl: Pass ovl_get_nlink() parameters in right order

[Amir Goldstein]

On Mon, Nov 27, 2017 at 11:26 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Mon, Nov 27, 2017 at 01:46:13PM -0500, Vivek Goyal wrote:
>> Right now we seem to be passing index as "lowerdentry" and origin.dentry
>> as "upperdentry". IIUC, we should pass these parameters in reversed order
>> and this looks like a bug.
>>
>
> Hi,
>
> Don't merge this patch yet. I think this breaks overlay/042 and does not
> cleanup orphan index. Trying to debug it.
>

Vivek,

The problem is with the test, not with your patch.
I copied sanity check at end of test to verify that orphan index is cleaned
on mount from test overlay/034, but there is a subtle difference between
the 2 tests - with overlay/034 lower is hardlink from the start and therefore
indexed from the first copy up and nlink accounted from the first copy up.
with overlay/042, lower is not a hardlink to begin with, so the first copy up
(at upper path /0) is a non-indexed broken hardlink, which is covering
the lower /0 path that later becomes a hardlink.

At the end of test overlay/042, index is NOT cleaned because the
accounted nlink is not 0, it is 1, because from the index perspective,
it does not know that the upper path at /0 is covered by a non-indexed
copy up.

The whole purpose of the trusted.overlay.nlink accounting is to account
for the not-yet-copied-up lower hardlinks, so the index with the new data
is not unlinked after unlinking all current upper hardlinks.

I wrote a new test to demonstrate this use case and demonstrate the
implications the bug that you found has on end users:
https://github.com/amir73il/xfstests/blob/overlayfs-devel/tests/overlay/047

I will fix test overlay/042.
Thanks for reporting this breakage.

Amir.

Re: [PATCH V2] ovl: Pass ovl_get_nlink() parameters in right order

[Vivek Goyal]

On Tue, Nov 28, 2017 at 02:58:00PM +0200, Amir Goldstein wrote:
> On Mon, Nov 27, 2017 at 11:26 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Mon, Nov 27, 2017 at 01:46:13PM -0500, Vivek Goyal wrote:
> >> Right now we seem to be passing index as "lowerdentry" and origin.dentry
> >> as "upperdentry". IIUC, we should pass these parameters in reversed order
> >> and this looks like a bug.
> >>
> >
> > Hi,
> >
> > Don't merge this patch yet. I think this breaks overlay/042 and does not
> > cleanup orphan index. Trying to debug it.
> >
> 
> Vivek,
> 
> The problem is with the test, not with your patch.
> I copied sanity check at end of test to verify that orphan index is cleaned
> on mount from test overlay/034, but there is a subtle difference between
> the 2 tests - with overlay/034 lower is hardlink from the start and therefore
> indexed from the first copy up and nlink accounted from the first copy up.
> with overlay/042, lower is not a hardlink to begin with, so the first copy up
> (at upper path /0) is a non-indexed broken hardlink, which is covering
> the lower /0 path that later becomes a hardlink.
> 
> At the end of test overlay/042, index is NOT cleaned because the
> accounted nlink is not 0, it is 1, because from the index perspective,
> it does not know that the upper path at /0 is covered by a non-indexed
> copy up.
> 
> The whole purpose of the trusted.overlay.nlink accounting is to account
> for the not-yet-copied-up lower hardlinks, so the index with the new data
> is not unlinked after unlinking all current upper hardlinks.
> 
> I wrote a new test to demonstrate this use case and demonstrate the
> implications the bug that you found has on end users:
> https://github.com/amir73il/xfstests/blob/overlayfs-devel/tests/overlay/047
> 
> I will fix test overlay/042.
> Thanks for reporting this breakage.

Hi Amir,

Thanks for the explanation. Good that it is a test issue and not patch
issue. I will repost the patch as V3. In last version, I forgot to cc
stable list.

Vivek