[PATCH] ovl: fix some bug exist in ovl_get_inode

[PATCH] ovl: fix some bug exist in ovl_get_inode

[yangerkun]

Run generic/461 with ext4 upper/lower layer sometimes may trigger the
bug as below(linux 4.19):

[  551.001349] overlayfs: failed to get metacopy (-5)
[  551.003464] overlayfs: failed to get inode (-5)
[  551.004243] overlayfs: cleanup of 'd44/fd51' failed (-5)
[  551.004941] overlayfs: failed to get origin (-5)
[  551.005199] ------------[ cut here ]------------
[  551.006697] WARNING: CPU: 3 PID: 24674 at fs/inode.c:1528 iput+0x33b/0x400
...
[  551.027219] Call Trace:
[  551.027623]  ovl_create_object+0x13f/0x170
[  551.028268]  ovl_create+0x27/0x30
[  551.028799]  path_openat+0x1a35/0x1ea0
[  551.029377]  do_filp_open+0xad/0x160
[  551.029944]  ? vfs_writev+0xe9/0x170
[  551.030499]  ? page_counter_try_charge+0x77/0x120
[  551.031245]  ? __alloc_fd+0x160/0x2a0
[  551.031832]  ? do_sys_open+0x189/0x340
[  551.032417]  ? get_unused_fd_flags+0x34/0x40
[  551.033081]  do_sys_open+0x189/0x340
[  551.033632]  __x64_sys_creat+0x24/0x30
[  551.034219]  do_syscall_64+0xd5/0x430
[  551.034800]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
...
[  556.107515] BUG: Dentry 000000006bc1d73f{i=4129c,n=fd51}  still in use (-1) [unmount of ext4 sdb]
[  556.108946] ------------[ cut here ]------------
[  556.109686] WARNING: CPU: 1 PID: 24682 at fs/dcache.c:1557 umount_check+0x95/0xc0
[  556.130343]  d_walk+0x10d/0x430
[  556.130832]  do_one_tree+0x30/0x60
[  556.131365]  shrink_dcache_for_umount+0x38/0xe0
[  556.132063]  generic_shutdown_super+0x2e/0x1c0
[  556.132747]  kill_block_super+0x29/0x80
[  556.133338]  deactivate_locked_super+0x7a/0x100
[  556.134034]  deactivate_super+0x9d/0xb0
[  556.134627]  cleanup_mnt+0x67/0x100
[  556.135173]  __cleanup_mnt+0x16/0x20
[  556.135731]  task_work_run+0xdb/0x110
[  556.136306]  exit_to_usermode_loop+0x197/0x1b0
[  556.136991]  do_syscall_64+0x3ce/0x430
[  556.137571]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
...
[  556.378140] VFS: Busy inodes after unmount of sdb. Self-destruct in 5 seconds.  Have a nice day...

After check the code, there may some bug need to fix:
1. We need to call iput once ovl_check_metacopy_xattr fail.
2. We need to call unlock_new_inode or the above iput(also with iput in
   ovl_create_object) will trigger the a WARN_ON since  the I_NEW still
   exists.
3. We should move the init for upperdentry to the place below
   ovl_check_metacopy_xattr. Or the dentry reference will decrease to
   -1(error path in ovl_create_upper will inc, ovl_destroy_inode too).

Fixes: 9d3dfea3d35a ("ovl: Modify ovl_lookup() and friends to lookup metacopy dentry")
Signed-off-by: yangerkun <yangerkun@huawei.com>
---
 fs/overlayfs/inode.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 981f11ec51bc..8f59e89e14e8 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -959,7 +959,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
 	int fsid = bylower ? lowerpath->layer->fsid : 0;
 	bool is_dir, metacopy = false;
 	unsigned long ino = 0;
-	int err = oip->newinode ? -EEXIST : -ENOMEM;
+	int err = 0;
 
 	if (!realinode)
 		realinode = d_inode(lowerdentry);
@@ -975,8 +975,11 @@ struct inode *ovl_get_inode(struct super_block *sb,
 		unsigned int nlink = is_dir ? 1 : realinode->i_nlink;
 
 		inode = ovl_iget5(sb, oip->newinode, key);
-		if (!inode)
+		if (!inode) {
+			err = oip->newinode ? -EEXIST : -ENOMEM;
 			goto out_err;
+		}
+
 		if (!(inode->i_state & I_NEW)) {
 			/*
 			 * Verify that the underlying files stored in the inode
@@ -984,7 +987,6 @@ struct inode *ovl_get_inode(struct super_block *sb,
 			 */
 			if (!ovl_verify_inode(inode, lowerdentry, upperdentry,
 					      true)) {
-				iput(inode);
 				err = -ESTALE;
 				goto out_err;
 			}
@@ -1009,8 +1011,6 @@ struct inode *ovl_get_inode(struct super_block *sb,
 		ino = realinode->i_ino;
 		fsid = lowerpath->layer->fsid;
 	}
-	ovl_fill_inode(inode, realinode->i_mode, realinode->i_rdev);
-	ovl_inode_init(inode, oip, ino, fsid);
 
 	if (upperdentry && ovl_is_impuredir(upperdentry))
 		ovl_set_flag(OVL_IMPURE, inode);
@@ -1027,6 +1027,8 @@ struct inode *ovl_get_inode(struct super_block *sb,
 			ovl_set_flag(OVL_UPPERDATA, inode);
 	}
 
+	ovl_fill_inode(inode, realinode->i_mode, realinode->i_rdev);
+	ovl_inode_init(inode, oip, ino, fsid);
 	OVL_I(inode)->redirect = oip->redirect;
 
 	if (bylower)
@@ -1040,13 +1042,20 @@ struct inode *ovl_get_inode(struct super_block *sb,
 		}
 	}
 
-	if (inode->i_state & I_NEW)
+clear_new:
+	if (inode && (inode->i_state & I_NEW))
 		unlock_new_inode(inode);
+	if (err < 0) {
+		/* Or the iput show be called by ovl_create_object. */
+		if (inode && (inode != oip->newinode))
+			iput(inode);
+
+		inode = ERR_PTR(err);
+	}
 out:
 	return inode;
 
 out_err:
 	pr_warn_ratelimited("failed to get inode (%i)\n", err);
-	inode = ERR_PTR(err);
-	goto out;
+	goto clear_new;
 }
-- 
2.21.3

Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Amir Goldstein]

On Wed, May 27, 2020 at 6:45 AM yangerkun <yangerkun@huawei.com> wrote:
>
> Run generic/461 with ext4 upper/lower layer sometimes may trigger the
> bug as below(linux 4.19):
>
> [  551.001349] overlayfs: failed to get metacopy (-5)
> [  551.003464] overlayfs: failed to get inode (-5)
> [  551.004243] overlayfs: cleanup of 'd44/fd51' failed (-5)
> [  551.004941] overlayfs: failed to get origin (-5)
> [  551.005199] ------------[ cut here ]------------
> [  551.006697] WARNING: CPU: 3 PID: 24674 at fs/inode.c:1528 iput+0x33b/0x400
> ...
> [  551.027219] Call Trace:
> [  551.027623]  ovl_create_object+0x13f/0x170
> [  551.028268]  ovl_create+0x27/0x30
> [  551.028799]  path_openat+0x1a35/0x1ea0
> [  551.029377]  do_filp_open+0xad/0x160
> [  551.029944]  ? vfs_writev+0xe9/0x170
> [  551.030499]  ? page_counter_try_charge+0x77/0x120
> [  551.031245]  ? __alloc_fd+0x160/0x2a0
> [  551.031832]  ? do_sys_open+0x189/0x340
> [  551.032417]  ? get_unused_fd_flags+0x34/0x40
> [  551.033081]  do_sys_open+0x189/0x340
> [  551.033632]  __x64_sys_creat+0x24/0x30
> [  551.034219]  do_syscall_64+0xd5/0x430
> [  551.034800]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> ...
> [  556.107515] BUG: Dentry 000000006bc1d73f{i=4129c,n=fd51}  still in use (-1) [unmount of ext4 sdb]
> [  556.108946] ------------[ cut here ]------------
> [  556.109686] WARNING: CPU: 1 PID: 24682 at fs/dcache.c:1557 umount_check+0x95/0xc0
> [  556.130343]  d_walk+0x10d/0x430
> [  556.130832]  do_one_tree+0x30/0x60
> [  556.131365]  shrink_dcache_for_umount+0x38/0xe0
> [  556.132063]  generic_shutdown_super+0x2e/0x1c0
> [  556.132747]  kill_block_super+0x29/0x80
> [  556.133338]  deactivate_locked_super+0x7a/0x100
> [  556.134034]  deactivate_super+0x9d/0xb0
> [  556.134627]  cleanup_mnt+0x67/0x100
> [  556.135173]  __cleanup_mnt+0x16/0x20
> [  556.135731]  task_work_run+0xdb/0x110
> [  556.136306]  exit_to_usermode_loop+0x197/0x1b0
> [  556.136991]  do_syscall_64+0x3ce/0x430
> [  556.137571]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> ...
> [  556.378140] VFS: Busy inodes after unmount of sdb. Self-destruct in 5 seconds.  Have a nice day...
>
> After check the code, there may some bug need to fix:
> 1. We need to call iput once ovl_check_metacopy_xattr fail.
> 2. We need to call unlock_new_inode or the above iput(also with iput in
>    ovl_create_object) will trigger the a WARN_ON since  the I_NEW still
>    exists.
> 3. We should move the init for upperdentry to the place below
>    ovl_check_metacopy_xattr. Or the dentry reference will decrease to
>    -1(error path in ovl_create_upper will inc, ovl_destroy_inode too).
>

OR we don't check metacopy xattr in ovl_get_inode().

In ovl_lookup() we already checked metacopy xattr.
No reason to check it again in this subtle context.

In ovl_lookup() can store value of upper metacopy and after we get
the inode, set the OVL_UPPERDATA inode flag according to
upperdentry && !uppermetacopy.

That would be consistent with ovl_obtain_alias() which sets the
OVL_UPPERDATA inode flag after getting the inode.

Thanks,
Amir.

Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Miklos Szeredi]

On Wed, May 27, 2020 at 1:16 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, May 27, 2020 at 6:45 AM yangerkun <yangerkun@huawei.com> wrote:
> >
> > Run generic/461 with ext4 upper/lower layer sometimes may trigger the
> > bug as below(linux 4.19):
> >
> > [  551.001349] overlayfs: failed to get metacopy (-5)
> > [  551.003464] overlayfs: failed to get inode (-5)
> > [  551.004243] overlayfs: cleanup of 'd44/fd51' failed (-5)
> > [  551.004941] overlayfs: failed to get origin (-5)
> > [  551.005199] ------------[ cut here ]------------
> > [  551.006697] WARNING: CPU: 3 PID: 24674 at fs/inode.c:1528 iput+0x33b/0x400
> > ...
> > [  551.027219] Call Trace:
> > [  551.027623]  ovl_create_object+0x13f/0x170
> > [  551.028268]  ovl_create+0x27/0x30
> > [  551.028799]  path_openat+0x1a35/0x1ea0
> > [  551.029377]  do_filp_open+0xad/0x160
> > [  551.029944]  ? vfs_writev+0xe9/0x170
> > [  551.030499]  ? page_counter_try_charge+0x77/0x120
> > [  551.031245]  ? __alloc_fd+0x160/0x2a0
> > [  551.031832]  ? do_sys_open+0x189/0x340
> > [  551.032417]  ? get_unused_fd_flags+0x34/0x40
> > [  551.033081]  do_sys_open+0x189/0x340
> > [  551.033632]  __x64_sys_creat+0x24/0x30
> > [  551.034219]  do_syscall_64+0xd5/0x430
> > [  551.034800]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > ...
> > [  556.107515] BUG: Dentry 000000006bc1d73f{i=4129c,n=fd51}  still in use (-1) [unmount of ext4 sdb]
> > [  556.108946] ------------[ cut here ]------------
> > [  556.109686] WARNING: CPU: 1 PID: 24682 at fs/dcache.c:1557 umount_check+0x95/0xc0
> > [  556.130343]  d_walk+0x10d/0x430
> > [  556.130832]  do_one_tree+0x30/0x60
> > [  556.131365]  shrink_dcache_for_umount+0x38/0xe0
> > [  556.132063]  generic_shutdown_super+0x2e/0x1c0
> > [  556.132747]  kill_block_super+0x29/0x80
> > [  556.133338]  deactivate_locked_super+0x7a/0x100
> > [  556.134034]  deactivate_super+0x9d/0xb0
> > [  556.134627]  cleanup_mnt+0x67/0x100
> > [  556.135173]  __cleanup_mnt+0x16/0x20
> > [  556.135731]  task_work_run+0xdb/0x110
> > [  556.136306]  exit_to_usermode_loop+0x197/0x1b0
> > [  556.136991]  do_syscall_64+0x3ce/0x430
> > [  556.137571]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > ...
> > [  556.378140] VFS: Busy inodes after unmount of sdb. Self-destruct in 5 seconds.  Have a nice day...
> >
> > After check the code, there may some bug need to fix:
> > 1. We need to call iput once ovl_check_metacopy_xattr fail.
> > 2. We need to call unlock_new_inode or the above iput(also with iput in
> >    ovl_create_object) will trigger the a WARN_ON since  the I_NEW still
> >    exists.
> > 3. We should move the init for upperdentry to the place below
> >    ovl_check_metacopy_xattr. Or the dentry reference will decrease to
> >    -1(error path in ovl_create_upper will inc, ovl_destroy_inode too).
> >
>
> OR we don't check metacopy xattr in ovl_get_inode().
>
> In ovl_lookup() we already checked metacopy xattr.
> No reason to check it again in this subtle context.
>
> In ovl_lookup() can store value of upper metacopy and after we get
> the inode, set the OVL_UPPERDATA inode flag according to
> upperdentry && !uppermetacopy.
>
> That would be consistent with ovl_obtain_alias() which sets the
> OVL_UPPERDATA inode flag after getting the inode.

I agree that that is a good direction, however for the actual fix I
think the following is sufficient (whitespace damaged, only for
review).

The reason we can skip the metacopy check for the ->newinode != NULL
case is that that only happens on object creation, which very
obviously won't have metacopy set.

Thanks,
Miklos

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 3b7ed5d2279c..fd7f1d4adf04 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -889,7 +889,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
     if (oip->index)
         ovl_set_flag(OVL_INDEX, inode);

-    if (upperdentry) {
+    if (upperdentry && !oip->newinode) {
         err = ovl_check_metacopy_xattr(upperdentry);
         if (err < 0)
             goto out_err;

Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Amir Goldstein]

On Wed, May 27, 2020 at 5:46 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, May 27, 2020 at 1:16 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Wed, May 27, 2020 at 6:45 AM yangerkun <yangerkun@huawei.com> wrote:
> > >
> > > Run generic/461 with ext4 upper/lower layer sometimes may trigger the
> > > bug as below(linux 4.19):
> > >
> > > [  551.001349] overlayfs: failed to get metacopy (-5)
> > > [  551.003464] overlayfs: failed to get inode (-5)
> > > [  551.004243] overlayfs: cleanup of 'd44/fd51' failed (-5)
> > > [  551.004941] overlayfs: failed to get origin (-5)
> > > [  551.005199] ------------[ cut here ]------------
> > > [  551.006697] WARNING: CPU: 3 PID: 24674 at fs/inode.c:1528 iput+0x33b/0x400
> > > ...
> > > [  551.027219] Call Trace:
> > > [  551.027623]  ovl_create_object+0x13f/0x170
> > > [  551.028268]  ovl_create+0x27/0x30
> > > [  551.028799]  path_openat+0x1a35/0x1ea0
> > > [  551.029377]  do_filp_open+0xad/0x160
> > > [  551.029944]  ? vfs_writev+0xe9/0x170
> > > [  551.030499]  ? page_counter_try_charge+0x77/0x120
> > > [  551.031245]  ? __alloc_fd+0x160/0x2a0
> > > [  551.031832]  ? do_sys_open+0x189/0x340
> > > [  551.032417]  ? get_unused_fd_flags+0x34/0x40
> > > [  551.033081]  do_sys_open+0x189/0x340
> > > [  551.033632]  __x64_sys_creat+0x24/0x30
> > > [  551.034219]  do_syscall_64+0xd5/0x430
> > > [  551.034800]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > ...
> > > [  556.107515] BUG: Dentry 000000006bc1d73f{i=4129c,n=fd51}  still in use (-1) [unmount of ext4 sdb]
> > > [  556.108946] ------------[ cut here ]------------
> > > [  556.109686] WARNING: CPU: 1 PID: 24682 at fs/dcache.c:1557 umount_check+0x95/0xc0
> > > [  556.130343]  d_walk+0x10d/0x430
> > > [  556.130832]  do_one_tree+0x30/0x60
> > > [  556.131365]  shrink_dcache_for_umount+0x38/0xe0
> > > [  556.132063]  generic_shutdown_super+0x2e/0x1c0
> > > [  556.132747]  kill_block_super+0x29/0x80
> > > [  556.133338]  deactivate_locked_super+0x7a/0x100
> > > [  556.134034]  deactivate_super+0x9d/0xb0
> > > [  556.134627]  cleanup_mnt+0x67/0x100
> > > [  556.135173]  __cleanup_mnt+0x16/0x20
> > > [  556.135731]  task_work_run+0xdb/0x110
> > > [  556.136306]  exit_to_usermode_loop+0x197/0x1b0
> > > [  556.136991]  do_syscall_64+0x3ce/0x430
> > > [  556.137571]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > ...
> > > [  556.378140] VFS: Busy inodes after unmount of sdb. Self-destruct in 5 seconds.  Have a nice day...
> > >
> > > After check the code, there may some bug need to fix:
> > > 1. We need to call iput once ovl_check_metacopy_xattr fail.
> > > 2. We need to call unlock_new_inode or the above iput(also with iput in
> > >    ovl_create_object) will trigger the a WARN_ON since  the I_NEW still
> > >    exists.
> > > 3. We should move the init for upperdentry to the place below
> > >    ovl_check_metacopy_xattr. Or the dentry reference will decrease to
> > >    -1(error path in ovl_create_upper will inc, ovl_destroy_inode too).
> > >
> >
> > OR we don't check metacopy xattr in ovl_get_inode().
> >
> > In ovl_lookup() we already checked metacopy xattr.
> > No reason to check it again in this subtle context.
> >
> > In ovl_lookup() can store value of upper metacopy and after we get
> > the inode, set the OVL_UPPERDATA inode flag according to
> > upperdentry && !uppermetacopy.
> >
> > That would be consistent with ovl_obtain_alias() which sets the
> > OVL_UPPERDATA inode flag after getting the inode.
>
> I agree that that is a good direction, however for the actual fix I
> think the following is sufficient (whitespace damaged, only for
> review).
>
> The reason we can skip the metacopy check for the ->newinode != NULL
> case is that that only happens on object creation, which very
> obviously won't have metacopy set.
>

Yes, its subtle but should be sufficient, because lookup shouldn't
get this far anyway.

However, if we are making that distinction, might as well skip the
entire block from ovl_inode_init() to unlock_new_inode().
Doesn't look like any of it is relevant for create.

Thanks,
Amir.

Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Vivek Goyal]

On Wed, May 27, 2020 at 04:46:37PM +0200, Miklos Szeredi wrote:
> On Wed, May 27, 2020 at 1:16 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Wed, May 27, 2020 at 6:45 AM yangerkun <yangerkun@huawei.com> wrote:
> > >
> > > Run generic/461 with ext4 upper/lower layer sometimes may trigger the
> > > bug as below(linux 4.19):
> > >
> > > [  551.001349] overlayfs: failed to get metacopy (-5)
> > > [  551.003464] overlayfs: failed to get inode (-5)
> > > [  551.004243] overlayfs: cleanup of 'd44/fd51' failed (-5)
> > > [  551.004941] overlayfs: failed to get origin (-5)
> > > [  551.005199] ------------[ cut here ]------------
> > > [  551.006697] WARNING: CPU: 3 PID: 24674 at fs/inode.c:1528 iput+0x33b/0x400
> > > ...
> > > [  551.027219] Call Trace:
> > > [  551.027623]  ovl_create_object+0x13f/0x170
> > > [  551.028268]  ovl_create+0x27/0x30
> > > [  551.028799]  path_openat+0x1a35/0x1ea0
> > > [  551.029377]  do_filp_open+0xad/0x160
> > > [  551.029944]  ? vfs_writev+0xe9/0x170
> > > [  551.030499]  ? page_counter_try_charge+0x77/0x120
> > > [  551.031245]  ? __alloc_fd+0x160/0x2a0
> > > [  551.031832]  ? do_sys_open+0x189/0x340
> > > [  551.032417]  ? get_unused_fd_flags+0x34/0x40
> > > [  551.033081]  do_sys_open+0x189/0x340
> > > [  551.033632]  __x64_sys_creat+0x24/0x30
> > > [  551.034219]  do_syscall_64+0xd5/0x430
> > > [  551.034800]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > ...
> > > [  556.107515] BUG: Dentry 000000006bc1d73f{i=4129c,n=fd51}  still in use (-1) [unmount of ext4 sdb]
> > > [  556.108946] ------------[ cut here ]------------
> > > [  556.109686] WARNING: CPU: 1 PID: 24682 at fs/dcache.c:1557 umount_check+0x95/0xc0
> > > [  556.130343]  d_walk+0x10d/0x430
> > > [  556.130832]  do_one_tree+0x30/0x60
> > > [  556.131365]  shrink_dcache_for_umount+0x38/0xe0
> > > [  556.132063]  generic_shutdown_super+0x2e/0x1c0
> > > [  556.132747]  kill_block_super+0x29/0x80
> > > [  556.133338]  deactivate_locked_super+0x7a/0x100
> > > [  556.134034]  deactivate_super+0x9d/0xb0
> > > [  556.134627]  cleanup_mnt+0x67/0x100
> > > [  556.135173]  __cleanup_mnt+0x16/0x20
> > > [  556.135731]  task_work_run+0xdb/0x110
> > > [  556.136306]  exit_to_usermode_loop+0x197/0x1b0
> > > [  556.136991]  do_syscall_64+0x3ce/0x430
> > > [  556.137571]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > ...
> > > [  556.378140] VFS: Busy inodes after unmount of sdb. Self-destruct in 5 seconds.  Have a nice day...
> > >
> > > After check the code, there may some bug need to fix:
> > > 1. We need to call iput once ovl_check_metacopy_xattr fail.
> > > 2. We need to call unlock_new_inode or the above iput(also with iput in
> > >    ovl_create_object) will trigger the a WARN_ON since  the I_NEW still
> > >    exists.
> > > 3. We should move the init for upperdentry to the place below
> > >    ovl_check_metacopy_xattr. Or the dentry reference will decrease to
> > >    -1(error path in ovl_create_upper will inc, ovl_destroy_inode too).
> > >
> >
> > OR we don't check metacopy xattr in ovl_get_inode().
> >
> > In ovl_lookup() we already checked metacopy xattr.
> > No reason to check it again in this subtle context.
> >
> > In ovl_lookup() can store value of upper metacopy and after we get
> > the inode, set the OVL_UPPERDATA inode flag according to
> > upperdentry && !uppermetacopy.
> >
> > That would be consistent with ovl_obtain_alias() which sets the
> > OVL_UPPERDATA inode flag after getting the inode.
> 
> I agree that that is a good direction, however for the actual fix I
> think the following is sufficient (whitespace damaged, only for
> review).
> 
> The reason we can skip the metacopy check for the ->newinode != NULL
> case is that that only happens on object creation, which very
> obviously won't have metacopy set.
> 
> Thanks,
> Miklos
> 
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 3b7ed5d2279c..fd7f1d4adf04 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -889,7 +889,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
>      if (oip->index)
>          ovl_set_flag(OVL_INDEX, inode);
> 
> -    if (upperdentry) {
> +    if (upperdentry && !oip->newinode) {
>          err = ovl_check_metacopy_xattr(upperdentry);
>          if (err < 0)
>              goto out_err;

Hi Miklos and Amir,

How about enahncing above a bit to deal with error. Will this work. Just
compile tested.

Thanks
Vivek

---
 fs/overlayfs/inode.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Index: redhat-linux/fs/overlayfs/inode.c
===================================================================
--- redhat-linux.orig/fs/overlayfs/inode.c	2020-05-26 15:24:57.209940278 -0400
+++ redhat-linux/fs/overlayfs/inode.c	2020-05-27 11:58:17.015732493 -0400
@@ -1018,10 +1018,13 @@ struct inode *ovl_get_inode(struct super
 	if (oip->index)
 		ovl_set_flag(OVL_INDEX, inode);
 
-	if (upperdentry) {
+	if (upperdentry && !oip->newinode) {
 		err = ovl_check_metacopy_xattr(upperdentry);
-		if (err < 0)
+		if (err < 0) {
+			if (inode->i_state & I_NEW)
+				iget_failed(inode);
 			goto out_err;
+		}
 		metacopy = err;
 		if (!metacopy)
 			ovl_set_flag(OVL_UPPERDATA, inode);

Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Amir Goldstein]

On Wed, May 27, 2020 at 7:02 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Wed, May 27, 2020 at 04:46:37PM +0200, Miklos Szeredi wrote:
> > On Wed, May 27, 2020 at 1:16 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Wed, May 27, 2020 at 6:45 AM yangerkun <yangerkun@huawei.com> wrote:
> > > >
> > > > Run generic/461 with ext4 upper/lower layer sometimes may trigger the
> > > > bug as below(linux 4.19):
> > > >
> > > > [  551.001349] overlayfs: failed to get metacopy (-5)
> > > > [  551.003464] overlayfs: failed to get inode (-5)
> > > > [  551.004243] overlayfs: cleanup of 'd44/fd51' failed (-5)
> > > > [  551.004941] overlayfs: failed to get origin (-5)
> > > > [  551.005199] ------------[ cut here ]------------
> > > > [  551.006697] WARNING: CPU: 3 PID: 24674 at fs/inode.c:1528 iput+0x33b/0x400
> > > > ...
> > > > [  551.027219] Call Trace:
> > > > [  551.027623]  ovl_create_object+0x13f/0x170
> > > > [  551.028268]  ovl_create+0x27/0x30
> > > > [  551.028799]  path_openat+0x1a35/0x1ea0
> > > > [  551.029377]  do_filp_open+0xad/0x160
> > > > [  551.029944]  ? vfs_writev+0xe9/0x170
> > > > [  551.030499]  ? page_counter_try_charge+0x77/0x120
> > > > [  551.031245]  ? __alloc_fd+0x160/0x2a0
> > > > [  551.031832]  ? do_sys_open+0x189/0x340
> > > > [  551.032417]  ? get_unused_fd_flags+0x34/0x40
> > > > [  551.033081]  do_sys_open+0x189/0x340
> > > > [  551.033632]  __x64_sys_creat+0x24/0x30
> > > > [  551.034219]  do_syscall_64+0xd5/0x430
> > > > [  551.034800]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > > ...
> > > > [  556.107515] BUG: Dentry 000000006bc1d73f{i=4129c,n=fd51}  still in use (-1) [unmount of ext4 sdb]
> > > > [  556.108946] ------------[ cut here ]------------
> > > > [  556.109686] WARNING: CPU: 1 PID: 24682 at fs/dcache.c:1557 umount_check+0x95/0xc0
> > > > [  556.130343]  d_walk+0x10d/0x430
> > > > [  556.130832]  do_one_tree+0x30/0x60
> > > > [  556.131365]  shrink_dcache_for_umount+0x38/0xe0
> > > > [  556.132063]  generic_shutdown_super+0x2e/0x1c0
> > > > [  556.132747]  kill_block_super+0x29/0x80
> > > > [  556.133338]  deactivate_locked_super+0x7a/0x100
> > > > [  556.134034]  deactivate_super+0x9d/0xb0
> > > > [  556.134627]  cleanup_mnt+0x67/0x100
> > > > [  556.135173]  __cleanup_mnt+0x16/0x20
> > > > [  556.135731]  task_work_run+0xdb/0x110
> > > > [  556.136306]  exit_to_usermode_loop+0x197/0x1b0
> > > > [  556.136991]  do_syscall_64+0x3ce/0x430
> > > > [  556.137571]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > > ...
> > > > [  556.378140] VFS: Busy inodes after unmount of sdb. Self-destruct in 5 seconds.  Have a nice day...
> > > >
> > > > After check the code, there may some bug need to fix:
> > > > 1. We need to call iput once ovl_check_metacopy_xattr fail.
> > > > 2. We need to call unlock_new_inode or the above iput(also with iput in
> > > >    ovl_create_object) will trigger the a WARN_ON since  the I_NEW still
> > > >    exists.
> > > > 3. We should move the init for upperdentry to the place below
> > > >    ovl_check_metacopy_xattr. Or the dentry reference will decrease to
> > > >    -1(error path in ovl_create_upper will inc, ovl_destroy_inode too).
> > > >
> > >
> > > OR we don't check metacopy xattr in ovl_get_inode().
> > >
> > > In ovl_lookup() we already checked metacopy xattr.
> > > No reason to check it again in this subtle context.
> > >
> > > In ovl_lookup() can store value of upper metacopy and after we get
> > > the inode, set the OVL_UPPERDATA inode flag according to
> > > upperdentry && !uppermetacopy.
> > >
> > > That would be consistent with ovl_obtain_alias() which sets the
> > > OVL_UPPERDATA inode flag after getting the inode.
> >
> > I agree that that is a good direction, however for the actual fix I
> > think the following is sufficient (whitespace damaged, only for
> > review).
> >
> > The reason we can skip the metacopy check for the ->newinode != NULL
> > case is that that only happens on object creation, which very
> > obviously won't have metacopy set.
> >
> > Thanks,
> > Miklos
> >
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index 3b7ed5d2279c..fd7f1d4adf04 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -889,7 +889,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
> >      if (oip->index)
> >          ovl_set_flag(OVL_INDEX, inode);
> >
> > -    if (upperdentry) {
> > +    if (upperdentry && !oip->newinode) {
> >          err = ovl_check_metacopy_xattr(upperdentry);
> >          if (err < 0)
> >              goto out_err;
>
> Hi Miklos and Amir,
>
> How about enahncing above a bit to deal with error. Will this work. Just
> compile tested.
>

Please no. it makes no sense.
Proper fix IMO is to use value checked in ovl_lookup()
instead of rechecking here.
Miklos' fix should works by avoiding hitting this check
in the most likely scenario (that getxattr either fails or succeeds,
but not randomly).

Thanks,
Amir.

Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Vivek Goyal]

On Wed, May 27, 2020 at 02:16:00PM +0300, Amir Goldstein wrote:

[..]
> > After check the code, there may some bug need to fix:
> > 1. We need to call iput once ovl_check_metacopy_xattr fail.
> > 2. We need to call unlock_new_inode or the above iput(also with iput in
> >    ovl_create_object) will trigger the a WARN_ON since  the I_NEW still
> >    exists.
> > 3. We should move the init for upperdentry to the place below
> >    ovl_check_metacopy_xattr. Or the dentry reference will decrease to
> >    -1(error path in ovl_create_upper will inc, ovl_destroy_inode too).
> >
> 
> OR we don't check metacopy xattr in ovl_get_inode().
> 
> In ovl_lookup() we already checked metacopy xattr.
> No reason to check it again in this subtle context.
> 
> In ovl_lookup() can store value of upper metacopy and after we get
> the inode, set the OVL_UPPERDATA inode flag according to
> upperdentry && !uppermetacopy.

I think reason behind initializing this attr in ovl_get_inode() was
that this is OVL_UPPERDATA is an inode property. So conceptually
it makes sense to initialize it when inode is being instantiated. And
that too under lock so that there are no races.

I was trying to think if we can trigger a race if we move OVL_UPPERDATA
initialization in ovl_lookup(). But given this is only one way
transition, I could not think of any. So for this speicific flag,
it probably is ok to initialize outside of ovl_get_inode().

Vivek

Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Vivek Goyal]

On Wed, May 27, 2020 at 02:16:00PM +0300, Amir Goldstein wrote:

[..]
> That would be consistent with ovl_obtain_alias() which sets the
> OVL_UPPERDATA inode flag after getting the inode.

BTW, Is setting of OVL_UPPERDATA in ovl_obtain_alias() redundant as of
now, given ovl_get_inode() is already setting it.

Vivek

Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Vivek Goyal]

On Wed, May 27, 2020 at 02:16:00PM +0300, Amir Goldstein wrote:
> On Wed, May 27, 2020 at 6:45 AM yangerkun <yangerkun@huawei.com> wrote:
> >
> > Run generic/461 with ext4 upper/lower layer sometimes may trigger the
> > bug as below(linux 4.19):
> >
> > [  551.001349] overlayfs: failed to get metacopy (-5)
> > [  551.003464] overlayfs: failed to get inode (-5)
> > [  551.004243] overlayfs: cleanup of 'd44/fd51' failed (-5)
> > [  551.004941] overlayfs: failed to get origin (-5)
> > [  551.005199] ------------[ cut here ]------------
> > [  551.006697] WARNING: CPU: 3 PID: 24674 at fs/inode.c:1528 iput+0x33b/0x400
> > ...
> > [  551.027219] Call Trace:
> > [  551.027623]  ovl_create_object+0x13f/0x170
> > [  551.028268]  ovl_create+0x27/0x30
> > [  551.028799]  path_openat+0x1a35/0x1ea0
> > [  551.029377]  do_filp_open+0xad/0x160
> > [  551.029944]  ? vfs_writev+0xe9/0x170
> > [  551.030499]  ? page_counter_try_charge+0x77/0x120
> > [  551.031245]  ? __alloc_fd+0x160/0x2a0
> > [  551.031832]  ? do_sys_open+0x189/0x340
> > [  551.032417]  ? get_unused_fd_flags+0x34/0x40
> > [  551.033081]  do_sys_open+0x189/0x340
> > [  551.033632]  __x64_sys_creat+0x24/0x30
> > [  551.034219]  do_syscall_64+0xd5/0x430
> > [  551.034800]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > ...
> > [  556.107515] BUG: Dentry 000000006bc1d73f{i=4129c,n=fd51}  still in use (-1) [unmount of ext4 sdb]
> > [  556.108946] ------------[ cut here ]------------
> > [  556.109686] WARNING: CPU: 1 PID: 24682 at fs/dcache.c:1557 umount_check+0x95/0xc0
> > [  556.130343]  d_walk+0x10d/0x430
> > [  556.130832]  do_one_tree+0x30/0x60
> > [  556.131365]  shrink_dcache_for_umount+0x38/0xe0
> > [  556.132063]  generic_shutdown_super+0x2e/0x1c0
> > [  556.132747]  kill_block_super+0x29/0x80
> > [  556.133338]  deactivate_locked_super+0x7a/0x100
> > [  556.134034]  deactivate_super+0x9d/0xb0
> > [  556.134627]  cleanup_mnt+0x67/0x100
> > [  556.135173]  __cleanup_mnt+0x16/0x20
> > [  556.135731]  task_work_run+0xdb/0x110
> > [  556.136306]  exit_to_usermode_loop+0x197/0x1b0
> > [  556.136991]  do_syscall_64+0x3ce/0x430
> > [  556.137571]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > ...
> > [  556.378140] VFS: Busy inodes after unmount of sdb. Self-destruct in 5 seconds.  Have a nice day...
> >
> > After check the code, there may some bug need to fix:
> > 1. We need to call iput once ovl_check_metacopy_xattr fail.
> > 2. We need to call unlock_new_inode or the above iput(also with iput in
> >    ovl_create_object) will trigger the a WARN_ON since  the I_NEW still
> >    exists.
> > 3. We should move the init for upperdentry to the place below
> >    ovl_check_metacopy_xattr. Or the dentry reference will decrease to
> >    -1(error path in ovl_create_upper will inc, ovl_destroy_inode too).
> >
> 
> OR we don't check metacopy xattr in ovl_get_inode().
> 
> In ovl_lookup() we already checked metacopy xattr.
> No reason to check it again in this subtle context.
> 
> In ovl_lookup() can store value of upper metacopy and after we get
> the inode, set the OVL_UPPERDATA inode flag according to
> upperdentry && !uppermetacopy.
> 
> That would be consistent with ovl_obtain_alias() which sets the
> OVL_UPPERDATA inode flag after getting the inode.

Hi Amir,

This patch implements what you are suggesting. Compile tested only.
Does this look ok?

May be I don't need to split it up in lmetacopy and umetacopy. Ideally,
lookup in lower layers should stop if an upper regular file is not
metacopy. IOW, (upperdentry && !metacopy) might be sufficient check.
Will look closely if this patch looks fine.

Thanks
Vivek


---
 fs/overlayfs/inode.c |   11 +----------
 fs/overlayfs/namei.c |   12 +++++++-----
 2 files changed, 8 insertions(+), 15 deletions(-)

Index: redhat-linux/fs/overlayfs/namei.c
===================================================================
--- redhat-linux.orig/fs/overlayfs/namei.c	2020-04-13 13:52:53.438294244 -0400
+++ redhat-linux/fs/overlayfs/namei.c	2020-05-27 15:28:38.242809626 -0400
@@ -823,7 +823,7 @@ struct dentry *ovl_lookup(struct inode *
 	struct dentry *this;
 	unsigned int i;
 	int err;
-	bool metacopy = false;
+	bool lmetacopy = false, umetacopy=false;
 	struct ovl_lookup_data d = {
 		.sb = dentry->d_sb,
 		.name = dentry->d_name,
@@ -869,7 +869,7 @@ struct dentry *ovl_lookup(struct inode *
 				goto out_put_upper;
 
 			if (d.metacopy)
-				metacopy = true;
+				umetacopy = true;
 		}
 
 		if (d.redirect) {
@@ -941,7 +941,7 @@ struct dentry *ovl_lookup(struct inode *
 		}
 
 		if (d.metacopy)
-			metacopy = true;
+			lmetacopy = true;
 		/*
 		 * Do not store intermediate metacopy dentries in chain,
 		 * except top most lower metacopy dentry
@@ -982,7 +982,7 @@ struct dentry *ovl_lookup(struct inode *
 		}
 	}
 
-	if (metacopy) {
+	if (lmetacopy || umetacopy) {
 		/*
 		 * Found a metacopy dentry but did not find corresponding
 		 * data dentry
@@ -1023,7 +1023,7 @@ struct dentry *ovl_lookup(struct inode *
 	 *
 	 * Always lookup index of non-dir non-metacopy and non-upper.
 	 */
-	if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
+	if (ctr && (!upperdentry || (!d.is_dir && !(lmetacopy || umetacopy))))
 		origin = stack[0].dentry;
 
 	if (origin && ovl_indexdir(dentry->d_sb) &&
@@ -1074,6 +1074,8 @@ struct dentry *ovl_lookup(struct inode *
 		err = PTR_ERR(inode);
 		if (IS_ERR(inode))
 			goto out_free_oe;
+		if (upperdentry && !umetacopy)
+			ovl_set_flag(OVL_UPPERDATA, inode);
 	}
 
 	ovl_dentry_update_reval(dentry, upperdentry,
Index: redhat-linux/fs/overlayfs/inode.c
===================================================================
--- redhat-linux.orig/fs/overlayfs/inode.c	2020-04-22 11:32:28.237700957 -0400
+++ redhat-linux/fs/overlayfs/inode.c	2020-05-27 15:30:21.059809626 -0400
@@ -939,7 +939,7 @@ struct inode *ovl_get_inode(struct super
 	bool bylower = ovl_hash_bylower(sb, upperdentry, lowerdentry,
 					oip->index);
 	int fsid = bylower ? lowerpath->layer->fsid : 0;
-	bool is_dir, metacopy = false;
+	bool is_dir;
 	unsigned long ino = 0;
 	int err = oip->newinode ? -EEXIST : -ENOMEM;
 
@@ -1000,15 +1000,6 @@ struct inode *ovl_get_inode(struct super
 	if (oip->index)
 		ovl_set_flag(OVL_INDEX, inode);
 
-	if (upperdentry) {
-		err = ovl_check_metacopy_xattr(upperdentry);
-		if (err < 0)
-			goto out_err;
-		metacopy = err;
-		if (!metacopy)
-			ovl_set_flag(OVL_UPPERDATA, inode);
-	}
-
 	OVL_I(inode)->redirect = oip->redirect;
 
 	if (bylower)

Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Amir Goldstein]

On Wed, May 27, 2020 at 10:49 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Wed, May 27, 2020 at 02:16:00PM +0300, Amir Goldstein wrote:
> > On Wed, May 27, 2020 at 6:45 AM yangerkun <yangerkun@huawei.com> wrote:
> > >
> > > Run generic/461 with ext4 upper/lower layer sometimes may trigger the
> > > bug as below(linux 4.19):
> > >
> > > [  551.001349] overlayfs: failed to get metacopy (-5)
> > > [  551.003464] overlayfs: failed to get inode (-5)
> > > [  551.004243] overlayfs: cleanup of 'd44/fd51' failed (-5)
> > > [  551.004941] overlayfs: failed to get origin (-5)
> > > [  551.005199] ------------[ cut here ]------------
> > > [  551.006697] WARNING: CPU: 3 PID: 24674 at fs/inode.c:1528 iput+0x33b/0x400
> > > ...
> > > [  551.027219] Call Trace:
> > > [  551.027623]  ovl_create_object+0x13f/0x170
> > > [  551.028268]  ovl_create+0x27/0x30
> > > [  551.028799]  path_openat+0x1a35/0x1ea0
> > > [  551.029377]  do_filp_open+0xad/0x160
> > > [  551.029944]  ? vfs_writev+0xe9/0x170
> > > [  551.030499]  ? page_counter_try_charge+0x77/0x120
> > > [  551.031245]  ? __alloc_fd+0x160/0x2a0
> > > [  551.031832]  ? do_sys_open+0x189/0x340
> > > [  551.032417]  ? get_unused_fd_flags+0x34/0x40
> > > [  551.033081]  do_sys_open+0x189/0x340
> > > [  551.033632]  __x64_sys_creat+0x24/0x30
> > > [  551.034219]  do_syscall_64+0xd5/0x430
> > > [  551.034800]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > ...
> > > [  556.107515] BUG: Dentry 000000006bc1d73f{i=4129c,n=fd51}  still in use (-1) [unmount of ext4 sdb]
> > > [  556.108946] ------------[ cut here ]------------
> > > [  556.109686] WARNING: CPU: 1 PID: 24682 at fs/dcache.c:1557 umount_check+0x95/0xc0
> > > [  556.130343]  d_walk+0x10d/0x430
> > > [  556.130832]  do_one_tree+0x30/0x60
> > > [  556.131365]  shrink_dcache_for_umount+0x38/0xe0
> > > [  556.132063]  generic_shutdown_super+0x2e/0x1c0
> > > [  556.132747]  kill_block_super+0x29/0x80
> > > [  556.133338]  deactivate_locked_super+0x7a/0x100
> > > [  556.134034]  deactivate_super+0x9d/0xb0
> > > [  556.134627]  cleanup_mnt+0x67/0x100
> > > [  556.135173]  __cleanup_mnt+0x16/0x20
> > > [  556.135731]  task_work_run+0xdb/0x110
> > > [  556.136306]  exit_to_usermode_loop+0x197/0x1b0
> > > [  556.136991]  do_syscall_64+0x3ce/0x430
> > > [  556.137571]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > ...
> > > [  556.378140] VFS: Busy inodes after unmount of sdb. Self-destruct in 5 seconds.  Have a nice day...
> > >
> > > After check the code, there may some bug need to fix:
> > > 1. We need to call iput once ovl_check_metacopy_xattr fail.
> > > 2. We need to call unlock_new_inode or the above iput(also with iput in
> > >    ovl_create_object) will trigger the a WARN_ON since  the I_NEW still
> > >    exists.
> > > 3. We should move the init for upperdentry to the place below
> > >    ovl_check_metacopy_xattr. Or the dentry reference will decrease to
> > >    -1(error path in ovl_create_upper will inc, ovl_destroy_inode too).
> > >
> >
> > OR we don't check metacopy xattr in ovl_get_inode().
> >
> > In ovl_lookup() we already checked metacopy xattr.
> > No reason to check it again in this subtle context.
> >
> > In ovl_lookup() can store value of upper metacopy and after we get
> > the inode, set the OVL_UPPERDATA inode flag according to
> > upperdentry && !uppermetacopy.
> >
> > That would be consistent with ovl_obtain_alias() which sets the
> > OVL_UPPERDATA inode flag after getting the inode.
>
> Hi Amir,
>
> This patch implements what you are suggesting. Compile tested only.
> Does this look ok?
>

It looks correct.

> May be I don't need to split it up in lmetacopy and umetacopy. Ideally,
> lookup in lower layers should stop if an upper regular file is not
> metacopy. IOW, (upperdentry && !metacopy) might be sufficient check.
> Will look closely if this patch looks fine.
>

I would stick uppermetacopy much like upperredirect and upperopaque.

This test:

        if (metacopy) {
                /*
                 * Found a metacopy dentry but did not find corresponding
                 * data dentry
                 */
                if (d.metacopy) {

Is equivalent to if (d.metacopy) {

I am not sure about:
        if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
                origin = stack[0].dentry;

I will let you figure it out, but it feels like it is actually testing
!uppermetacopy

Thanks,
Amir.

Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Vivek Goyal]

On Wed, May 27, 2020 at 11:11:38PM +0300, Amir Goldstein wrote:

[..]
> > > OR we don't check metacopy xattr in ovl_get_inode().
> > >
> > > In ovl_lookup() we already checked metacopy xattr.
> > > No reason to check it again in this subtle context.
> > >
> > > In ovl_lookup() can store value of upper metacopy and after we get
> > > the inode, set the OVL_UPPERDATA inode flag according to
> > > upperdentry && !uppermetacopy.
> > >
> > > That would be consistent with ovl_obtain_alias() which sets the
> > > OVL_UPPERDATA inode flag after getting the inode.
> >
> > Hi Amir,
> >
> > This patch implements what you are suggesting. Compile tested only.
> > Does this look ok?
> >
> 
> It looks correct.
> 
> > May be I don't need to split it up in lmetacopy and umetacopy. Ideally,
> > lookup in lower layers should stop if an upper regular file is not
> > metacopy. IOW, (upperdentry && !metacopy) might be sufficient check.
> > Will look closely if this patch looks fine.
> >
> 
> I would stick uppermetacopy much like upperredirect and upperopaque.

Ok, I introduced uppermetacopy and lowermetacopy. I need to make
sure that I don't following metacopy file to lower layer if
metacopy feature is off. This check should be done both for upper
and lower metcopy files.

> 
> This test:
> 
>         if (metacopy) {
>                 /*
>                  * Found a metacopy dentry but did not find corresponding
>                  * data dentry
>                  */
>                 if (d.metacopy) {
> 
> Is equivalent to if (d.metacopy) {

Agreed. Updated the patch.

> 
> I am not sure about:
>         if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
>                 origin = stack[0].dentry;
> 
> I will let you figure it out, but it feels like it is actually testing
> !uppermetacopy

Yes this is testing !uppermetacopy. I really want to simplify it a bit
or atleast document it a bit that why metacopy case is different. Upper,
regular files done't go through lower layer loop but upper metacopy
files do. That's one difference which introduces some interesting
code changes.

- lower layer lookup loop already sets "origin" for metacopy files if
  indexing is on. This does not happen for regular non-metacopy files
  so they need to set origin here explicitly.

  if index feature is off, then we will not set "origin" for metacopy
  files in lower layer loop. But do we really need to set it given
  index is off and we don't want to lookup index.

- We don't want to set origin if upper never had xattr ORIGIN. For
  regular files, ctr will be 0 or 1 if ORIGIN xattr was found on
  upper. But for metacopy upper files, ctr can be non-zero even
  if ORGIN xattr was not found. So that's another reason that
  we check for upper metacopy here.

Difference between the case of regular and metacopy is subtle and
I think this should be simplified otherwise its very easy to break
it. 

I will spend some time on this after fixing the issue at hand. /me
always gets lost in the mage of index and origin. There seem to
be so many permutation and combination and its not clear to me
when metacopy file is different than regular file w.r.t origin
and index. It will be nice if we can minimize this difference and
document it well so that future modifications are easy.

Here is V2 of the patch. I added changelog. Also updated it to
set OVL_UPPERDATA in ovl_instantiate(). This is creating a new
file, so it can't be metacopy and should set OVL_UPPERDATA.

Miklos and Amir, please let me know what do you think about this
patch. I ran xfstetests overlay tests and these pass (except two
which fail even without the patch and are meant to fail.).

Thanks
Vivek


Subject: overlayfs: Initialize OVL_UPPERDATA in ovl_lookup()

Currently ovl_get_inode() initializes OVL_UPPERDATA flag and for that it
has to call ovl_check_metacopy_xattr() and check if metacopy xattr is
present or not.

yangerkun reported sometimes underlying filesystem might return -EIO
and in that case error handling path does not cleanup properly leading
to various warnings.

Run generic/461 with ext4 upper/lower layer sometimes may trigger the
bug as below(linux 4.19):

[  551.001349] overlayfs: failed to get metacopy (-5)
[  551.003464] overlayfs: failed to get inode (-5)
[  551.004243] overlayfs: cleanup of 'd44/fd51' failed (-5)
[  551.004941] overlayfs: failed to get origin (-5)
[  551.005199] ------------[ cut here ]------------
[  551.006697] WARNING: CPU: 3 PID: 24674 at fs/inode.c:1528 iput+0x33b/0x400
...
[  551.027219] Call Trace:
[  551.027623]  ovl_create_object+0x13f/0x170
[  551.028268]  ovl_create+0x27/0x30
[  551.028799]  path_openat+0x1a35/0x1ea0
[  551.029377]  do_filp_open+0xad/0x160
[  551.029944]  ? vfs_writev+0xe9/0x170
[  551.030499]  ? page_counter_try_charge+0x77/0x120
[  551.031245]  ? __alloc_fd+0x160/0x2a0
[  551.031832]  ? do_sys_open+0x189/0x340
[  551.032417]  ? get_unused_fd_flags+0x34/0x40
[  551.033081]  do_sys_open+0x189/0x340
[  551.033632]  __x64_sys_creat+0x24/0x30
[  551.034219]  do_syscall_64+0xd5/0x430
[  551.034800]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

One solution is to improve error handling and call iget_failed() if error
is encountered. Amir thinks that this path is little intricate and there
is not real need to check and initialize OVL_UPPERDATA in ovl_get_inode().
Instead caller of ovl_get_inode() can initialize this state. And this
will avoid double checking of metacopy xattr lookup in ovl_lookup()
and ovl_get_inode().

OVL_UPPERDATA is inode flag. So I was little concerned that initializing
it outside ovl_get_inode() might have some races. But this is one way
transition. That is once a file has been fully copied up, it can't go
back to metacopy file again. And that seems to help avoid races. So
as of now I can't see any races w.r.t OVL_UPPERDATA being set wrongly. So
move settingof OVL_UPPERDATA inside the callers of ovl_get_inode().
ovl_obtain_alias() already does it. So only two callers now left
are ovl_lookup() and ovl_instantiate(). 

metacopy variable has been split into two variables, lowermetacopy
and uppermetacopy. It just makes it easier to understand whether
metacopy if set on lower or upper. We need to set OVL_UPPERDATA
only in case of uppermetacopy.

Reported-by: yangerkun <yangerkun@huawei.com>
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/dir.c   |    2 ++
 fs/overlayfs/inode.c |   11 +----------
 fs/overlayfs/namei.c |   25 ++++++++++++-------------
 3 files changed, 15 insertions(+), 23 deletions(-)

Index: redhat-linux/fs/overlayfs/namei.c
===================================================================
--- redhat-linux.orig/fs/overlayfs/namei.c	2020-05-28 10:51:57.838556592 -0400
+++ redhat-linux/fs/overlayfs/namei.c	2020-05-28 12:11:36.876964037 -0400
@@ -823,7 +823,7 @@ struct dentry *ovl_lookup(struct inode *
 	struct dentry *this;
 	unsigned int i;
 	int err;
-	bool metacopy = false;
+	bool uppermetacopy=false, lowermetacopy=false;
 	struct ovl_lookup_data d = {
 		.sb = dentry->d_sb,
 		.name = dentry->d_name,
@@ -869,7 +869,7 @@ struct dentry *ovl_lookup(struct inode *
 				goto out_put_upper;
 
 			if (d.metacopy)
-				metacopy = true;
+				uppermetacopy = true;
 		}
 
 		if (d.redirect) {
@@ -941,7 +941,7 @@ struct dentry *ovl_lookup(struct inode *
 		}
 
 		if (d.metacopy)
-			metacopy = true;
+			lowermetacopy = true;
 		/*
 		 * Do not store intermediate metacopy dentries in chain,
 		 * except top most lower metacopy dentry
@@ -982,16 +982,13 @@ struct dentry *ovl_lookup(struct inode *
 		}
 	}
 
-	if (metacopy) {
-		/*
-		 * Found a metacopy dentry but did not find corresponding
-		 * data dentry
-		 */
-		if (d.metacopy) {
-			err = -EIO;
-			goto out_put;
-		}
+	/* Found a metacopy dentry but did not find corresponding data dentry */
+	if (d.metacopy) {
+		err = -EIO;
+		goto out_put;
+	}
 
+	if (lowermetacopy || uppermetacopy) {
 		err = -EPERM;
 		if (!ofs->config.metacopy) {
 			pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n",
@@ -1023,7 +1020,7 @@ struct dentry *ovl_lookup(struct inode *
 	 *
 	 * Always lookup index of non-dir non-metacopy and non-upper.
 	 */
-	if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
+	if (ctr && (!upperdentry || (!d.is_dir && !uppermetacopy)))
 		origin = stack[0].dentry;
 
 	if (origin && ovl_indexdir(dentry->d_sb) &&
@@ -1074,6 +1071,8 @@ struct dentry *ovl_lookup(struct inode *
 		err = PTR_ERR(inode);
 		if (IS_ERR(inode))
 			goto out_free_oe;
+		if (upperdentry && !uppermetacopy)
+			ovl_set_flag(OVL_UPPERDATA, inode);
 	}
 
 	ovl_dentry_update_reval(dentry, upperdentry,
Index: redhat-linux/fs/overlayfs/inode.c
===================================================================
--- redhat-linux.orig/fs/overlayfs/inode.c	2020-05-27 17:06:04.224809626 -0400
+++ redhat-linux/fs/overlayfs/inode.c	2020-05-28 10:52:12.890556592 -0400
@@ -939,7 +939,7 @@ struct inode *ovl_get_inode(struct super
 	bool bylower = ovl_hash_bylower(sb, upperdentry, lowerdentry,
 					oip->index);
 	int fsid = bylower ? lowerpath->layer->fsid : 0;
-	bool is_dir, metacopy = false;
+	bool is_dir;
 	unsigned long ino = 0;
 	int err = oip->newinode ? -EEXIST : -ENOMEM;
 
@@ -1000,15 +1000,6 @@ struct inode *ovl_get_inode(struct super
 	if (oip->index)
 		ovl_set_flag(OVL_INDEX, inode);
 
-	if (upperdentry) {
-		err = ovl_check_metacopy_xattr(upperdentry);
-		if (err < 0)
-			goto out_err;
-		metacopy = err;
-		if (!metacopy)
-			ovl_set_flag(OVL_UPPERDATA, inode);
-	}
-
 	OVL_I(inode)->redirect = oip->redirect;
 
 	if (bylower)
Index: redhat-linux/fs/overlayfs/dir.c
===================================================================
--- redhat-linux.orig/fs/overlayfs/dir.c	2020-04-22 08:47:37.419523036 -0400
+++ redhat-linux/fs/overlayfs/dir.c	2020-05-28 11:36:15.338556592 -0400
@@ -262,6 +262,8 @@ static int ovl_instantiate(struct dentry
 		inode = ovl_get_inode(dentry->d_sb, &oip);
 		if (IS_ERR(inode))
 			return PTR_ERR(inode);
+		if (inode == oip.newinode)
+			ovl_set_flag(OVL_UPPERDATA, inode);
 	} else {
 		WARN_ON(ovl_inode_real(inode) != d_inode(newdentry));
 		dput(newdentry);

Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Amir Goldstein]

On Thu, May 28, 2020 at 8:35 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Wed, May 27, 2020 at 11:11:38PM +0300, Amir Goldstein wrote:
>
> [..]
> > > > OR we don't check metacopy xattr in ovl_get_inode().
> > > >
> > > > In ovl_lookup() we already checked metacopy xattr.
> > > > No reason to check it again in this subtle context.
> > > >
> > > > In ovl_lookup() can store value of upper metacopy and after we get
> > > > the inode, set the OVL_UPPERDATA inode flag according to
> > > > upperdentry && !uppermetacopy.
> > > >
> > > > That would be consistent with ovl_obtain_alias() which sets the
> > > > OVL_UPPERDATA inode flag after getting the inode.
> > >
> > > Hi Amir,
> > >
> > > This patch implements what you are suggesting. Compile tested only.
> > > Does this look ok?
> > >
> >
> > It looks correct.
> >
> > > May be I don't need to split it up in lmetacopy and umetacopy. Ideally,
> > > lookup in lower layers should stop if an upper regular file is not
> > > metacopy. IOW, (upperdentry && !metacopy) might be sufficient check.
> > > Will look closely if this patch looks fine.
> > >
> >
> > I would stick uppermetacopy much like upperredirect and upperopaque.
>
> Ok, I introduced uppermetacopy and lowermetacopy. I need to make
> sure that I don't following metacopy file to lower layer if
> metacopy feature is off. This check should be done both for upper
> and lower metcopy files.

You don't need lowermetacopy for that. you can check d.metacopy
directly.

>
> >
> > This test:
> >
> >         if (metacopy) {
> >                 /*
> >                  * Found a metacopy dentry but did not find corresponding
> >                  * data dentry
> >                  */
> >                 if (d.metacopy) {
> >
> > Is equivalent to if (d.metacopy) {
>
> Agreed. Updated the patch.
>
> >
> > I am not sure about:
> >         if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> >                 origin = stack[0].dentry;
> >
> > I will let you figure it out, but it feels like it is actually testing
> > !uppermetacopy
>
> Yes this is testing !uppermetacopy. I really want to simplify it a bit
> or atleast document it a bit that why metacopy case is different. Upper,
> regular files done't go through lower layer loop but upper metacopy
> files do. That's one difference which introduces some interesting
> code changes.
>
> - lower layer lookup loop already sets "origin" for metacopy files if
>   indexing is on. This does not happen for regular non-metacopy files
>   so they need to set origin here explicitly.
>
>   if index feature is off, then we will not set "origin" for metacopy
>   files in lower layer loop. But do we really need to set it given
>   index is off and we don't want to lookup index.
>
> - We don't want to set origin if upper never had xattr ORIGIN. For
>   regular files, ctr will be 0 or 1 if ORIGIN xattr was found on
>   upper. But for metacopy upper files, ctr can be non-zero even
>   if ORGIN xattr was not found. So that's another reason that
>   we check for upper metacopy here.
>
> Difference between the case of regular and metacopy is subtle and
> I think this should be simplified otherwise its very easy to break
> it.
>
> I will spend some time on this after fixing the issue at hand. /me
> always gets lost in the mage of index and origin. There seem to
> be so many permutation and combination and its not clear to me
> when metacopy file is different than regular file w.r.t origin
> and index. It will be nice if we can minimize this difference and
> document it well so that future modifications are easy.

I agree it should be simplified.
If you cannot figure out how, let me know and I will try.


>
> Here is V2 of the patch. I added changelog. Also updated it to
> set OVL_UPPERDATA in ovl_instantiate(). This is creating a new
> file, so it can't be metacopy and should set OVL_UPPERDATA.
>
> Miklos and Amir, please let me know what do you think about this
> patch. I ran xfstetests overlay tests and these pass (except two
> which fail even without the patch and are meant to fail.).
>
> Thanks
> Vivek
>
>
> Subject: overlayfs: Initialize OVL_UPPERDATA in ovl_lookup()
>
> Currently ovl_get_inode() initializes OVL_UPPERDATA flag and for that it
> has to call ovl_check_metacopy_xattr() and check if metacopy xattr is
> present or not.
>
> yangerkun reported sometimes underlying filesystem might return -EIO
> and in that case error handling path does not cleanup properly leading
> to various warnings.
>
> Run generic/461 with ext4 upper/lower layer sometimes may trigger the
> bug as below(linux 4.19):
>
> [  551.001349] overlayfs: failed to get metacopy (-5)
> [  551.003464] overlayfs: failed to get inode (-5)
> [  551.004243] overlayfs: cleanup of 'd44/fd51' failed (-5)
> [  551.004941] overlayfs: failed to get origin (-5)
> [  551.005199] ------------[ cut here ]------------
> [  551.006697] WARNING: CPU: 3 PID: 24674 at fs/inode.c:1528 iput+0x33b/0x400
> ...
> [  551.027219] Call Trace:
> [  551.027623]  ovl_create_object+0x13f/0x170
> [  551.028268]  ovl_create+0x27/0x30
> [  551.028799]  path_openat+0x1a35/0x1ea0
> [  551.029377]  do_filp_open+0xad/0x160
> [  551.029944]  ? vfs_writev+0xe9/0x170
> [  551.030499]  ? page_counter_try_charge+0x77/0x120
> [  551.031245]  ? __alloc_fd+0x160/0x2a0
> [  551.031832]  ? do_sys_open+0x189/0x340
> [  551.032417]  ? get_unused_fd_flags+0x34/0x40
> [  551.033081]  do_sys_open+0x189/0x340
> [  551.033632]  __x64_sys_creat+0x24/0x30
> [  551.034219]  do_syscall_64+0xd5/0x430
> [  551.034800]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> One solution is to improve error handling and call iget_failed() if error
> is encountered. Amir thinks that this path is little intricate and there
> is not real need to check and initialize OVL_UPPERDATA in ovl_get_inode().
> Instead caller of ovl_get_inode() can initialize this state. And this
> will avoid double checking of metacopy xattr lookup in ovl_lookup()
> and ovl_get_inode().
>
> OVL_UPPERDATA is inode flag. So I was little concerned that initializing
> it outside ovl_get_inode() might have some races. But this is one way
> transition. That is once a file has been fully copied up, it can't go
> back to metacopy file again. And that seems to help avoid races. So
> as of now I can't see any races w.r.t OVL_UPPERDATA being set wrongly. So
> move settingof OVL_UPPERDATA inside the callers of ovl_get_inode().
> ovl_obtain_alias() already does it. So only two callers now left
> are ovl_lookup() and ovl_instantiate().
>
> metacopy variable has been split into two variables, lowermetacopy
> and uppermetacopy. It just makes it easier to understand whether
> metacopy if set on lower or upper. We need to set OVL_UPPERDATA
> only in case of uppermetacopy.
>
> Reported-by: yangerkun <yangerkun@huawei.com>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/dir.c   |    2 ++
>  fs/overlayfs/inode.c |   11 +----------
>  fs/overlayfs/namei.c |   25 ++++++++++++-------------
>  3 files changed, 15 insertions(+), 23 deletions(-)
>
> Index: redhat-linux/fs/overlayfs/namei.c
> ===================================================================
> --- redhat-linux.orig/fs/overlayfs/namei.c      2020-05-28 10:51:57.838556592 -0400
> +++ redhat-linux/fs/overlayfs/namei.c   2020-05-28 12:11:36.876964037 -0400
> @@ -823,7 +823,7 @@ struct dentry *ovl_lookup(struct inode *
>         struct dentry *this;
>         unsigned int i;
>         int err;
> -       bool metacopy = false;
> +       bool uppermetacopy=false, lowermetacopy=false;

spaces around = and no need for lowermetacopy

>         struct ovl_lookup_data d = {
>                 .sb = dentry->d_sb,
>                 .name = dentry->d_name,
> @@ -869,7 +869,7 @@ struct dentry *ovl_lookup(struct inode *
>                                 goto out_put_upper;
>
>                         if (d.metacopy)
> -                               metacopy = true;
> +                               uppermetacopy = true;
>                 }
>
>                 if (d.redirect) {
> @@ -941,7 +941,7 @@ struct dentry *ovl_lookup(struct inode *
>                 }
>
>                 if (d.metacopy)
> -                       metacopy = true;
> +                       lowermetacopy = true;
>                 /*
>                  * Do not store intermediate metacopy dentries in chain,
>                  * except top most lower metacopy dentry
> @@ -982,16 +982,13 @@ struct dentry *ovl_lookup(struct inode *
>                 }
>         }
>
> -       if (metacopy) {
> -               /*
> -                * Found a metacopy dentry but did not find corresponding
> -                * data dentry
> -                */
> -               if (d.metacopy) {
> -                       err = -EIO;
> -                       goto out_put;
> -               }
> +       /* Found a metacopy dentry but did not find corresponding data dentry */
> +       if (d.metacopy) {
> +               err = -EIO;
> +               goto out_put;
> +       }
>
> +       if (lowermetacopy || uppermetacopy) {
>                 err = -EPERM;
>                 if (!ofs->config.metacopy) {
>                         pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n",

Move that test up to where setting metacopy = true for lower layers
similar to "refusing to follow redirect" and make it:
       if (uppermetacopy || d.metacopy) {

Then you got rid of lowermetacopy.

> @@ -1023,7 +1020,7 @@ struct dentry *ovl_lookup(struct inode *
>          *
>          * Always lookup index of non-dir non-metacopy and non-upper.
>          */
> -       if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> +       if (ctr && (!upperdentry || (!d.is_dir && !uppermetacopy)))
>                 origin = stack[0].dentry;
>

I think this should be:

          * Always lookup index of non-dir and non-upper.
          */
          if (!origin && ctr && (!upperdentry || !d.is_dir))
                 origin = stack[0].dentry;

uppermetacopy is guaranteed to either have origin already set or
exit with an an error for ovl_verify_origin().

HOWEVER, if we set origin to lower, which turns out to be a lower
metacopy, we then skip this layer to the next one, but origin remains
set on the skipped layer dentry, which we had already dput().
Ay ay ay!

I think it would be best to move the check
                 * Do not store intermediate metacopy dentries in chain,
to right after ovl_lookup_layer(), before the ovl_fix_origin() and
ovl_verify_origin() checks.

Thanks,
Amir.

Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Vivek Goyal]

On Fri, May 29, 2020 at 12:07:45AM +0300, Amir Goldstein wrote:

[..]
> > +       /* Found a metacopy dentry but did not find corresponding data dentry */
> > +       if (d.metacopy) {
> > +               err = -EIO;
> > +               goto out_put;
> > +       }
> >
> > +       if (lowermetacopy || uppermetacopy) {
> >                 err = -EPERM;
> >                 if (!ofs->config.metacopy) {
> >                         pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n",
> 
> Move that test up to where setting metacopy = true for lower layers
> similar to "refusing to follow redirect" and make it:
>        if (uppermetacopy || d.metacopy) {
> 
> Then you got rid of lowermetacopy.

Agreed. Will change. 

> 
> > @@ -1023,7 +1020,7 @@ struct dentry *ovl_lookup(struct inode *
> >          *
> >          * Always lookup index of non-dir non-metacopy and non-upper.
> >          */
> > -       if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> > +       if (ctr && (!upperdentry || (!d.is_dir && !uppermetacopy)))
> >                 origin = stack[0].dentry;
> >
> 
> I think this should be:
> 
>           * Always lookup index of non-dir and non-upper.
>           */
>           if (!origin && ctr && (!upperdentry || !d.is_dir))
>                  origin = stack[0].dentry;
> 
> uppermetacopy is guaranteed to either have origin already set or
> exit with an an error for ovl_verify_origin().

Only if index is enabled and upper had origin xattr.

(!d.is_dir && ofs->config.index && origin_path)

So if index is disabled or uppermetacopy did not have "origin" xattr,
we will not have origin set by the time we come out of the loop.

I see for non-metacopy regular files, if upper did not have origin
xattr, that means origin_path will by NULL. That means ctr will be
0 and that means we will not set "origin" for non-metacopy regular
files in such case. So question is, should we set "origin" for
metacopy upper files in such a case.

We did not have origin xattr, but we looked up lower layers for
upper metacopy. In theory, stack[0].dentry is origin for upper
metacopy files. Should we use it? Current logic does not and that's
why this additiona check (!d.is_dir && !uppermetacopy).

> 
> HOWEVER, if we set origin to lower, which turns out to be a lower
> metacopy, we then skip this layer to the next one, but origin remains
> set on the skipped layer dentry, which we had already dput().
> Ay ay ay!

We only skip the intermediate metacopy entries in lower. So top most
lower metacopy will still be retained. For example, if there are 3
lower layers where top two are metacopy and one data, then we will
only skip middle one. And middle one should not be origin for upper.

                /*
                 * Do not store intermediate metacopy dentries in chain,
                 * except top most lower metacopy dentry
                 */
                if (d.metacopy && ctr) {
                        dput(this);
                        continue;
                }

For the first lower, ctr will be 0 and we will always store it in 
stack. So if it is metacopy dentry, it will still be stored at
stack[0]. 

Do you still see the problem?

> 
> I think it would be best to move the check
>                  * Do not store intermediate metacopy dentries in chain,
> to right after ovl_lookup_layer(), before the ovl_fix_origin() and
> ovl_verify_origin() checks.

Thanks
Vivek

Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Amir Goldstein]

> > > @@ -1023,7 +1020,7 @@ struct dentry *ovl_lookup(struct inode *
> > >          *
> > >          * Always lookup index of non-dir non-metacopy and non-upper.
> > >          */
> > > -       if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> > > +       if (ctr && (!upperdentry || (!d.is_dir && !uppermetacopy)))
> > >                 origin = stack[0].dentry;
> > >
> >
> > I think this should be:
> >
> >           * Always lookup index of non-dir and non-upper.
> >           */
> >           if (!origin && ctr && (!upperdentry || !d.is_dir))
> >                  origin = stack[0].dentry;
> >
> > uppermetacopy is guaranteed to either have origin already set or
> > exit with an an error for ovl_verify_origin().
>
> Only if index is enabled and upper had origin xattr.
>
> (!d.is_dir && ofs->config.index && origin_path)
>
> So if index is disabled or uppermetacopy did not have "origin" xattr,
> we will not have origin set by the time we come out of the loop.
>

True. But if index is disabled, setting origin is moot. origin is only
ever used here to lookup the index.

About "origin" xattr. If it is not set in upper that lower fs probably does
not have file handle support. In that case, index cannot be enabled
anyway.

> I see for non-metacopy regular files, if upper did not have origin
> xattr, that means origin_path will by NULL. That means ctr will be
> 0 and that means we will not set "origin" for non-metacopy regular
> files in such case. So question is, should we set "origin" for
> metacopy upper files in such a case.
>
> We did not have origin xattr, but we looked up lower layers for
> upper metacopy. In theory, stack[0].dentry is origin for upper
> metacopy files. Should we use it? Current logic does not and that's
> why this additiona check (!d.is_dir && !uppermetacopy).
>

I agree with your analysis, but this is a very theoretical discussion.
Unless I am missing something, I think we have written a very complex
condition for a corner case that doesn't seem to be valid or interesting.

Basically, for non-dir, if there is no "origin" xattr, then there should be no
index, because the metacopy feature was added way long after we
started storing "origin" on copy up. That's not the case for directories.

There is one corner case where it may be relevant -
overlay layers with metacopy that were created on fs with no file handle
support (or no uuid) that are migrated to a filesystem with file handle
support (and metacopy xattr are preserved in migration).
In that case, index may be enabled while upper metacopy exists
without "origin".

What happens if we do not set origin and do not lookup index in that case?
We can get two overlay inodes, both from different metacopy upper inodes
redirected to the same lower inode, that have the same st_ino, but differnt
metadata.

I suppose we should have a protection in place for not making broken
upper hardlinks that are metacopy, but I don't see it...

> >
> > HOWEVER, if we set origin to lower, which turns out to be a lower
> > metacopy, we then skip this layer to the next one, but origin remains
> > set on the skipped layer dentry, which we had already dput().
> > Ay ay ay!
>
> We only skip the intermediate metacopy entries in lower. So top most
> lower metacopy will still be retained. For example, if there are 3
> lower layers where top two are metacopy and one data, then we will
> only skip middle one. And middle one should not be origin for upper.
>
>                 /*
>                  * Do not store intermediate metacopy dentries in chain,
>                  * except top most lower metacopy dentry
>                  */
>                 if (d.metacopy && ctr) {
>                         dput(this);
>                         continue;
>                 }
>
> For the first lower, ctr will be 0 and we will always store it in
> stack. So if it is metacopy dentry, it will still be stored at
> stack[0].
>
> Do you still see the problem?

No. it's fine. My eyes missed the ctr condition.
I still think since you are changing this code.
It will be much easier to follow if both simple continue statement
are at the top of the loop.

Thanks,
Amir.

Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Vivek Goyal]

On Fri, May 29, 2020 at 06:46:43PM +0300, Amir Goldstein wrote:
> > > > @@ -1023,7 +1020,7 @@ struct dentry *ovl_lookup(struct inode *
> > > >          *
> > > >          * Always lookup index of non-dir non-metacopy and non-upper.
> > > >          */
> > > > -       if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> > > > +       if (ctr && (!upperdentry || (!d.is_dir && !uppermetacopy)))
> > > >                 origin = stack[0].dentry;
> > > >
> > >
> > > I think this should be:
> > >
> > >           * Always lookup index of non-dir and non-upper.
> > >           */
> > >           if (!origin && ctr && (!upperdentry || !d.is_dir))
> > >                  origin = stack[0].dentry;
> > >
> > > uppermetacopy is guaranteed to either have origin already set or
> > > exit with an an error for ovl_verify_origin().
> >
> > Only if index is enabled and upper had origin xattr.
> >
> > (!d.is_dir && ofs->config.index && origin_path)
> >
> > So if index is disabled or uppermetacopy did not have "origin" xattr,
> > we will not have origin set by the time we come out of the loop.
> >
> 
> True. But if index is disabled, setting origin is moot. origin is only
> ever used here to lookup the index.

Well, while looking up for index, we are checking for presence of
index dir (and not checking whether index is currently enabled or
not). So if somebody mounts overlayfs with index=on and later remounts
with index=off, we can still start looking up the index even if it
is not enabled. Is it intentional? If not, to simplify it, should
we lookup index only if it is enabled.

        if (origin && ofs->config.index &&
            (!d.is_dir || ovl_index_all(dentry->d_sb))) {
                index = ovl_lookup_index(ofs, upperdentry, origin, true);


> 
> About "origin" xattr. If it is not set in upper that lower fs probably does
> not have file handle support. In that case, index cannot be enabled
> anyway.

What about the case of multiple lower layers. IIUC, we will only 
ensure that top most lower layer has file handle support and not
worry about rest of the layers. This will break the case of setting
origin for !upperdentry. This will lookup index and fail if lower
layer does not support file handle.

So may be while enabling index, we should make sure all lower
layers support file handles otherwise fail?

> 
> > I see for non-metacopy regular files, if upper did not have origin
> > xattr, that means origin_path will by NULL. That means ctr will be
> > 0 and that means we will not set "origin" for non-metacopy regular
> > files in such case. So question is, should we set "origin" for
> > metacopy upper files in such a case.
> >
> > We did not have origin xattr, but we looked up lower layers for
> > upper metacopy. In theory, stack[0].dentry is origin for upper
> > metacopy files. Should we use it? Current logic does not and that's
> > why this additiona check (!d.is_dir && !uppermetacopy).
> >
> 
> I agree with your analysis, but this is a very theoretical discussion.
> Unless I am missing something, I think we have written a very complex
> condition for a corner case that doesn't seem to be valid or interesting.

I agree. I want to simplify it too. Just trying to make sure that
I don't end up breaking some valid configuration.

> 
> Basically, for non-dir, if there is no "origin" xattr, then there should be no
> index, because the metacopy feature was added way long after we
> started storing "origin" on copy up. That's not the case for directories.
> 
> There is one corner case where it may be relevant -
> overlay layers with metacopy that were created on fs with no file handle
> support (or no uuid) that are migrated to a filesystem with file handle
> support (and metacopy xattr are preserved in migration).
> In that case, index may be enabled while upper metacopy exists
> without "origin".
> 
> What happens if we do not set origin and do not lookup index in that case?
> We can get two overlay inodes, both from different metacopy upper inodes
> redirected to the same lower inode, that have the same st_ino, but differnt
> metadata.

We do not set origin on upper for broken hardlinks. So we will report
inode number from upper. I tried. it.

I tried following.

- touch foo.txt
- ln foo.txt foo-link.txt
- mount with metacopy=on
- chwon test:test foo.txt
- umount
- Goto upper/ and remove origin xattr from foo.txt. But there should not
  be one because we do not create ORIGIN for broken hardlinks if index is
  not enabled.
- mount overlay with index=on
- Do stat on foo.txt and foo-link.txt. foo.txt reports inode number from
  upper and foo-link.txt reports inode number from lower.
- chown test:test foo-link.txt
- stat foo-link.txt still reports inode number from lower.

Anyway, at this point of time, how about following.

- For non-upper dentry, always set origin.
- For upperdentry, there are 3 cases.
	- directories
	- regular files
	- regular metacopy files

  For directories and regular metacopy files only use verified origin.
  That means upper has origin xattr and it matches patch based looked
  up dentry. If we did not verify because either ORIGIN xattr is not
  there, or because index is not enabled or because ovl_verify_lower()
  is not set, then don't use path based looked up dentry as origin.

  For the case of regular file upper dentry, use unverified origin.
  It implies that ORGIN xattr is there. As there is no path based
  lookup origin for upper regular files. 

I am attaching a simple patch. Please let me know what do you think
of it.

> 
> > >
> > > HOWEVER, if we set origin to lower, which turns out to be a lower
> > > metacopy, we then skip this layer to the next one, but origin remains
> > > set on the skipped layer dentry, which we had already dput().
> > > Ay ay ay!
> >
> > We only skip the intermediate metacopy entries in lower. So top most
> > lower metacopy will still be retained. For example, if there are 3
> > lower layers where top two are metacopy and one data, then we will
> > only skip middle one. And middle one should not be origin for upper.
> >
> >                 /*
> >                  * Do not store intermediate metacopy dentries in chain,
> >                  * except top most lower metacopy dentry
> >                  */
> >                 if (d.metacopy && ctr) {
> >                         dput(this);
> >                         continue;
> >                 }
> >
> > For the first lower, ctr will be 0 and we will always store it in
> > stack. So if it is metacopy dentry, it will still be stored at
> > stack[0].
> >
> > Do you still see the problem?
> 
> No. it's fine. My eyes missed the ctr condition.
> I still think since you are changing this code.
> It will be much easier to follow if both simple continue statement
> are at the top of the loop.

Ok, will do.

Here is the patch to simplify the condition or origin. I will add some
changelog and comments in code in v2 of patch if you like the patch.

Thanks
Vivek


---
 fs/overlayfs/namei.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: redhat-linux/fs/overlayfs/namei.c
===================================================================
--- redhat-linux.orig/fs/overlayfs/namei.c	2020-05-29 14:24:45.997113946 -0400
+++ redhat-linux/fs/overlayfs/namei.c	2020-05-29 14:46:46.692113946 -0400
@@ -1005,6 +1005,7 @@ struct dentry *ovl_lookup(struct inode *
 		}
 		stack = origin_path;
 		ctr = 1;
+		origin = origin_path->dentry;
 		origin_path = NULL;
 	}
 
@@ -1021,9 +1022,9 @@ struct dentry *ovl_lookup(struct inode *
 	 * index. This case should be handled in same way as a non-dir upper
 	 * without ORIGIN is handled.
 	 *
-	 * Always lookup index of non-dir non-metacopy and non-upper.
+	 * Always lookup index of non-upper.
 	 */
-	if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
+	if (!origin && ctr && !upperdentry)
 		origin = stack[0].dentry;
 
 	if (origin && ovl_indexdir(dentry->d_sb) &&

Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Amir Goldstein]

On Fri, May 29, 2020 at 10:01 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Fri, May 29, 2020 at 06:46:43PM +0300, Amir Goldstein wrote:
> > > > > @@ -1023,7 +1020,7 @@ struct dentry *ovl_lookup(struct inode *
> > > > >          *
> > > > >          * Always lookup index of non-dir non-metacopy and non-upper.
> > > > >          */
> > > > > -       if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> > > > > +       if (ctr && (!upperdentry || (!d.is_dir && !uppermetacopy)))
> > > > >                 origin = stack[0].dentry;
> > > > >
> > > >
> > > > I think this should be:
> > > >
> > > >           * Always lookup index of non-dir and non-upper.
> > > >           */
> > > >           if (!origin && ctr && (!upperdentry || !d.is_dir))
> > > >                  origin = stack[0].dentry;
> > > >
> > > > uppermetacopy is guaranteed to either have origin already set or
> > > > exit with an an error for ovl_verify_origin().
> > >
> > > Only if index is enabled and upper had origin xattr.
> > >
> > > (!d.is_dir && ofs->config.index && origin_path)
> > >
> > > So if index is disabled or uppermetacopy did not have "origin" xattr,
> > > we will not have origin set by the time we come out of the loop.
> > >
> >
> > True. But if index is disabled, setting origin is moot. origin is only
> > ever used here to lookup the index.
>
> Well, while looking up for index, we are checking for presence of
> index dir (and not checking whether index is currently enabled or
> not). So if somebody mounts overlayfs with index=on and later remounts
> with index=off, we can still start looking up the index even if it
> is not enabled. Is it intentional? If not, to simplify it, should
> we lookup index only if it is enabled.
>
>         if (origin && ofs->config.index &&
>             (!d.is_dir || ovl_index_all(dentry->d_sb))) {
>                 index = ovl_lookup_index(ofs, upperdentry, origin, true);
>

What do you mean by remount? actual -o remount cannot
change any overlay config variables.
For umount/mount,  ofs->indexdir doesn't mean that there is an
index dir, it means that index dir is in use because index is enabled.
See:

        if (!(ovl_force_readonly(ofs)) && ofs->config.index) {
...
                err = ovl_get_indexdir(sb, ofs, oe, &upperpath);
...
        if (!ofs->indexdir) {
                ofs->config.index = false;

Most of the code checks for ofs->indexdir.
The test for ofs->config.index is also correct, but inconsistent, although
I see that we also use ofs->config.index in other places in ovl_lookup().

>
> >
> > About "origin" xattr. If it is not set in upper that lower fs probably does
> > not have file handle support. In that case, index cannot be enabled
> > anyway.
>
> What about the case of multiple lower layers. IIUC, we will only
> ensure that top most lower layer has file handle support and not
> worry about rest of the layers. This will break the case of setting
> origin for !upperdentry. This will lookup index and fail if lower
> layer does not support file handle.
>
> So may be while enabling index, we should make sure all lower
> layers support file handles otherwise fail?
>

Enabling index requires that all layers support file handles:

                pr_warn("fs on '%s' does not support file handles,
falling back to index=off,nfs_export=off.\n",
                        name);
        }

> >
> > > I see for non-metacopy regular files, if upper did not have origin
> > > xattr, that means origin_path will by NULL. That means ctr will be
> > > 0 and that means we will not set "origin" for non-metacopy regular
> > > files in such case. So question is, should we set "origin" for
> > > metacopy upper files in such a case.
> > >
> > > We did not have origin xattr, but we looked up lower layers for
> > > upper metacopy. In theory, stack[0].dentry is origin for upper
> > > metacopy files. Should we use it? Current logic does not and that's
> > > why this additiona check (!d.is_dir && !uppermetacopy).
> > >
> >
> > I agree with your analysis, but this is a very theoretical discussion.
> > Unless I am missing something, I think we have written a very complex
> > condition for a corner case that doesn't seem to be valid or interesting.
>
> I agree. I want to simplify it too. Just trying to make sure that
> I don't end up breaking some valid configuration.
>
> >
> > Basically, for non-dir, if there is no "origin" xattr, then there should be no
> > index, because the metacopy feature was added way long after we
> > started storing "origin" on copy up. That's not the case for directories.
> >
> > There is one corner case where it may be relevant -
> > overlay layers with metacopy that were created on fs with no file handle
> > support (or no uuid) that are migrated to a filesystem with file handle
> > support (and metacopy xattr are preserved in migration).
> > In that case, index may be enabled while upper metacopy exists
> > without "origin".
> >
> > What happens if we do not set origin and do not lookup index in that case?
> > We can get two overlay inodes, both from different metacopy upper inodes
> > redirected to the same lower inode, that have the same st_ino, but differnt
> > metadata.
>
> We do not set origin on upper for broken hardlinks. So we will report
> inode number from upper. I tried. it.

Good point. I figured we had to have some protection in place.

>
> I tried following.
>
> - touch foo.txt
> - ln foo.txt foo-link.txt
> - mount with metacopy=on
> - chwon test:test foo.txt
> - umount
> - Goto upper/ and remove origin xattr from foo.txt. But there should not
>   be one because we do not create ORIGIN for broken hardlinks if index is
>   not enabled.
> - mount overlay with index=on
> - Do stat on foo.txt and foo-link.txt. foo.txt reports inode number from
>   upper and foo-link.txt reports inode number from lower.
> - chown test:test foo-link.txt
> - stat foo-link.txt still reports inode number from lower.
>
> Anyway, at this point of time, how about following.
>
> - For non-upper dentry, always set origin.
> - For upperdentry, there are 3 cases.
>         - directories
>         - regular files
>         - regular metacopy files
>
>   For directories and regular metacopy files only use verified origin.
>   That means upper has origin xattr and it matches patch based looked
>   up dentry. If we did not verify because either ORIGIN xattr is not
>   there, or because index is not enabled or because ovl_verify_lower()
>   is not set, then don't use path based looked up dentry as origin.
>
>   For the case of regular file upper dentry, use unverified origin.
>   It implies that ORGIN xattr is there. As there is no path based
>   lookup origin for upper regular files.
>
> I am attaching a simple patch. Please let me know what do you think
> of it.
>
> >
> > > >
> > > > HOWEVER, if we set origin to lower, which turns out to be a lower
> > > > metacopy, we then skip this layer to the next one, but origin remains
> > > > set on the skipped layer dentry, which we had already dput().
> > > > Ay ay ay!
> > >
> > > We only skip the intermediate metacopy entries in lower. So top most
> > > lower metacopy will still be retained. For example, if there are 3
> > > lower layers where top two are metacopy and one data, then we will
> > > only skip middle one. And middle one should not be origin for upper.
> > >
> > >                 /*
> > >                  * Do not store intermediate metacopy dentries in chain,
> > >                  * except top most lower metacopy dentry
> > >                  */
> > >                 if (d.metacopy && ctr) {
> > >                         dput(this);
> > >                         continue;
> > >                 }
> > >
> > > For the first lower, ctr will be 0 and we will always store it in
> > > stack. So if it is metacopy dentry, it will still be stored at
> > > stack[0].
> > >
> > > Do you still see the problem?
> >
> > No. it's fine. My eyes missed the ctr condition.
> > I still think since you are changing this code.
> > It will be much easier to follow if both simple continue statement
> > are at the top of the loop.
>
> Ok, will do.
>
> Here is the patch to simplify the condition or origin. I will add some
> changelog and comments in code in v2 of patch if you like the patch.
>
> Thanks
> Vivek
>
>
> ---
>  fs/overlayfs/namei.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> Index: redhat-linux/fs/overlayfs/namei.c
> ===================================================================
> --- redhat-linux.orig/fs/overlayfs/namei.c      2020-05-29 14:24:45.997113946 -0400
> +++ redhat-linux/fs/overlayfs/namei.c   2020-05-29 14:46:46.692113946 -0400
> @@ -1005,6 +1005,7 @@ struct dentry *ovl_lookup(struct inode *
>                 }
>                 stack = origin_path;
>                 ctr = 1;
> +               origin = origin_path->dentry;
>                 origin_path = NULL;
>         }
>
> @@ -1021,9 +1022,9 @@ struct dentry *ovl_lookup(struct inode *
>          * index. This case should be handled in same way as a non-dir upper
>          * without ORIGIN is handled.
>          *
> -        * Always lookup index of non-dir non-metacopy and non-upper.
> +        * Always lookup index of non-upper.
>          */
> -       if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> +       if (!origin && ctr && !upperdentry)
>                 origin = stack[0].dentry;
>

As I wrote in review, the condition could be further simplified to
(!upperdentry).

Thanks for digging up the truth,
Amir.

Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Vivek Goyal]

On Sat, May 30, 2020 at 02:35:14PM +0300, Amir Goldstein wrote:
> On Fri, May 29, 2020 at 10:01 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Fri, May 29, 2020 at 06:46:43PM +0300, Amir Goldstein wrote:
> > > > > > @@ -1023,7 +1020,7 @@ struct dentry *ovl_lookup(struct inode *
> > > > > >          *
> > > > > >          * Always lookup index of non-dir non-metacopy and non-upper.
> > > > > >          */
> > > > > > -       if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> > > > > > +       if (ctr && (!upperdentry || (!d.is_dir && !uppermetacopy)))
> > > > > >                 origin = stack[0].dentry;
> > > > > >
> > > > >
> > > > > I think this should be:
> > > > >
> > > > >           * Always lookup index of non-dir and non-upper.
> > > > >           */
> > > > >           if (!origin && ctr && (!upperdentry || !d.is_dir))
> > > > >                  origin = stack[0].dentry;
> > > > >
> > > > > uppermetacopy is guaranteed to either have origin already set or
> > > > > exit with an an error for ovl_verify_origin().
> > > >
> > > > Only if index is enabled and upper had origin xattr.
> > > >
> > > > (!d.is_dir && ofs->config.index && origin_path)
> > > >
> > > > So if index is disabled or uppermetacopy did not have "origin" xattr,
> > > > we will not have origin set by the time we come out of the loop.
> > > >
> > >
> > > True. But if index is disabled, setting origin is moot. origin is only
> > > ever used here to lookup the index.
> >
> > Well, while looking up for index, we are checking for presence of
> > index dir (and not checking whether index is currently enabled or
> > not). So if somebody mounts overlayfs with index=on and later remounts
> > with index=off, we can still start looking up the index even if it
> > is not enabled. Is it intentional? If not, to simplify it, should
> > we lookup index only if it is enabled.
> >
> >         if (origin && ofs->config.index &&
> >             (!d.is_dir || ovl_index_all(dentry->d_sb))) {
> >                 index = ovl_lookup_index(ofs, upperdentry, origin, true);
> >
> 
> What do you mean by remount? actual -o remount cannot
> change any overlay config variables.
> For umount/mount,  ofs->indexdir doesn't mean that there is an
> index dir, it means that index dir is in use because index is enabled.
> See:
> 
>         if (!(ovl_force_readonly(ofs)) && ofs->config.index) {
> ...
>                 err = ovl_get_indexdir(sb, ofs, oe, &upperpath);
> ...
>         if (!ofs->indexdir) {
>                 ofs->config.index = false;

I mean umount/mount. Got it so ovl_indexdir() will be true only if
index is enabled (and not just because index dir is present).

> 
> Most of the code checks for ofs->indexdir.
> The test for ofs->config.index is also correct, but inconsistent, although
> I see that we also use ofs->config.index in other places in ovl_lookup().

It will be nice if we had a single check to determine if index is
enabled or not. (instead of having instances of both ofs->config.index
as well as ovl_indexdir()).

> 
> >
> > >
> > > About "origin" xattr. If it is not set in upper that lower fs probably does
> > > not have file handle support. In that case, index cannot be enabled
> > > anyway.
> >
> > What about the case of multiple lower layers. IIUC, we will only
> > ensure that top most lower layer has file handle support and not
> > worry about rest of the layers. This will break the case of setting
> > origin for !upperdentry. This will lookup index and fail if lower
> > layer does not support file handle.
> >
> > So may be while enabling index, we should make sure all lower
> > layers support file handles otherwise fail?
> >
> 
> Enabling index requires that all layers support file handles:
> 
>                 pr_warn("fs on '%s' does not support file handles,
> falling back to index=off,nfs_export=off.\n",
>                         name);
>         }

Got it. So we will make sure all lower layers support file handles.

Thanks
Vivek