Re: [PATCH 3/3] ovl: redirect on rename-dir

[Amir Goldstein]

On Thu, Nov 3, 2016 at 5:50 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> On Fri, Oct 28, 2016 at 6:15 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>> On Tue, Oct 25, 2016 at 09:34:47AM +0200, Miklos Szeredi wrote:
...
>>
>> I'm not sure if vfs_path_lookup() is the right tool here.  It might be
>> usable for making such a tool, but as it is you are setting one hell of
>> a trap for yourself...
>
> Agreed, it's not the right tool.   A custom loop of lookup_one_len's
> should work much better and doesn't add all that much complexity.
> Updated patch pushed to:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git #redirect
>
> This version also passes the recycling tests by Amir and enables the
> redirect feature by default on an empty upperdir.
>

Miklos,

You did not address my comment about the 'stack' allocation overflow
in ovl_lookup
I believe the (possible) overflow is demonstrated by the following debug patch:

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index c7cacbb..7171bfb 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -231,5 +231,7 @@ struct dentry *ovl_lookup(struct inode *dir,
struct dentry *dentry,
                                goto out_put;

                        if (redirect && poe != dentry->d_sb->s_root->d_fsdata) {
+                               int stackroom = poe->numlower - ctr;
+
                                poe = dentry->d_sb->s_root->d_fsdata;

@@ -238,6 +240,8 @@ struct dentry *ovl_lookup(struct inode *dir,
struct dentry *dentry,
                                                break;
                                if (WARN_ON(i == poe->numlower))
                                        break;
+                               if (WARN_ON(poe->numlower - i - 1 > stackroom))
+                                       break;
                        }
                }
        }
-- 
2.7.4

In cases where a directory is moved into another directory with merge history
shorter then total number of layers,
lookup will need to grow the 'stack' while redirecting.
Bug will be hit only after remount or dcache drop, which was the
reason I wrote the
recycle test in the first place...

I instrumented unionmount-tests with test name prints to kmsg (a la xfstests)
Pushed to https://github.com/amir73il/unionmount-testsuite.git #ovl_rename_dir

And as you can see, 5 subtests hit the overflow warning.

[ 1759.692281] TEST rename-new-dir.py:161: Rename empty dir over
removed empty lower dir
[ 1759.747217] WARNING: CPU: 0 PID: 9065 at
/home/amir/src/linux/fs/overlayfs/namei.c:271 ovl_lookup+0x81d/0x870
[overlay]

[ 1759.748887] TEST rename-new-dir.py:172: Rename empty dir over
removed populated lower dir
[ 1759.836195] WARNING: CPU: 2 PID: 9065 at
/home/amir/src/linux/fs/overlayfs/namei.c:271 ovl_lookup+0x81d/0x870
[overlay]

[ 1763.519285] TEST rename-new-pop-dir.py:170: Rename new dir over
removed unioned empty dir
[ 1763.592055] WARNING: CPU: 3 PID: 9065 at
/home/amir/src/linux/fs/overlayfs/namei.c:271 ovl_lookup+0x81d/0x870
[overlay]

[ 1763.592989] TEST rename-new-pop-dir.py:183: Rename new dir over
removed unioned dir, different files
[ 1763.658290] WARNING: CPU: 3 PID: 9065 at
/home/amir/src/linux/fs/overlayfs/namei.c:271 ovl_lookup+0x81d/0x870
[overlay]

[ 1763.660379] TEST rename-new-pop-dir.py:197: Rename new dir over
removed unioned dir, same files
[ 1763.731482] WARNING: CPU: 0 PID: 9065 at
/home/amir/src/linux/fs/overlayfs/namei.c:271 ovl_lookup+0x81d/0x870
[overlay]

I hope I am not missing anything.

Cheers,
Amir.