ovl: Pass ovl_get_nlink() parameters in right order

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 <[email protected]> #4.13

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/namei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/overlayfs/namei.c +1 -1

index 625ed8066570..5ef69bc09e0c 100644
@@ -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);

Quoted 4 lines · 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.

Hi,

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

Quoted 11 lines · Vivek Goyal

Quoted 4 lines · 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.

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.