Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1511223 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] ovl: redirect on rename-dir Al Viro <viro@ZenIV.linux.org.uk> - 2016-10-28 18:20 +0200
    Re: [PATCH 3/3] ovl: redirect on rename-dir Miklos Szeredi <miklos@szeredi.hu> - 2016-11-03 17:00 +0100
      Re: [PATCH 3/3] ovl: redirect on rename-dir Amir Goldstein <amir73il@gmail.com> - 2016-11-04 10:40 +0100
        Re: [PATCH 3/3] ovl: redirect on rename-dir Miklos Szeredi <miklos@szeredi.hu> - 2016-11-04 14:50 +0100

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

On Tue, Oct 25, 2016 at 09:34:47AM +0200, Miklos Szeredi wrote:
> Current code returns EXDEV when a directory would need to be copied up to
> move.  We could copy up the directory tree in this case, but there's
> another solution: point to old lower directory from moved upper directory.
> 
> This is achieved with a "trusted.overlay.redirect" xattr storing the path
> relative to the root of the overlay.  After such attribute has been set,
> the directory can be moved without further actions required.
> 
> This is a backward incompatible feature, old kernels won't be able to
> correctly mount an overlay containing redirected directories.

> +			err = vfs_path_lookup(lowerpath.dentry, lowerpath.mnt,
> +					      redirect, 0, &thispath);
> +
> +			if (err) {
> +				if (err == -ENOENT || err == -ENAMETOOLONG)
> +					this = NULL;
> +			} else {
> +				this = thispath.dentry;
> +				mntput(thispath.mnt);
> +				if (!this->d_inode) {
> +					dput(this);
> +					this = NULL;
> +				} else if (ovl_dentry_weird(this)) {
> +					dput(this);
> +					err = -EREMOTE;
> +				}
> +			}

I'm not happy with that one - you are relying upon the fairly subtle
assertions here.
	1)  Had lowerpath.mnt *not* been a privately cloned one with nothing
mounted on it, you would've been screwed.
	2) Had that thing contained a "jumper" symlink (a-la procfs ones),
you would've been screwed.  Currently only procfs has those, and it would've
been rejected before getting there, but this is brittle and non-obvious.
	3) Any automount point in there (nfs4 referrals, etc.) can
break the assumption that nothing could've been mounted on it.  And _that_
might have not been stepped onto; back when the path had been stored, there'd
been no automount point at all, so we have avoided ovl_dentry_weird() rejects,
and by now nothing on the path had been visited yet, so ovl_dentry_weird()
didn't have a chance to trigger.  Note that calling it on the last dentry
is no good - we might have crossed the automount point in the middle of that
path, so this last dentry might be nice and shiny - and on another filesystem.
So unlike (1) and (2) it's not just a fishy-looking thing that happens to
work for non-local reasons; AFAICS, it's actually a bug.

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...

It might be made to work, if we figure out the right semantics for disabling
symlinks on per-vfsmount basis (and no, the posted nolinks patches are not
it) and mark these private clones with that and with similar "disable
automount traversals" flag (again, needs the right semantics; the area is
convoluted as it is).  But in that case I would strongly recommend adding
an exported wrapper around vfs_path_lookup() that would verify that these
flags *are* set.

[toc] | [next] | [standalone]

#1514644

On Fri, Oct 28, 2016 at 6:15 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Oct 25, 2016 at 09:34:47AM +0200, Miklos Szeredi wrote:
>> Current code returns EXDEV when a directory would need to be copied up to
>> move.  We could copy up the directory tree in this case, but there's
>> another solution: point to old lower directory from moved upper directory.
>>
>> This is achieved with a "trusted.overlay.redirect" xattr storing the path
>> relative to the root of the overlay.  After such attribute has been set,
>> the directory can be moved without further actions required.
>>
>> This is a backward incompatible feature, old kernels won't be able to
>> correctly mount an overlay containing redirected directories.
>
>> +                     err = vfs_path_lookup(lowerpath.dentry, lowerpath.mnt,
>> +                                           redirect, 0, &thispath);
>> +
>> +                     if (err) {
>> +                             if (err == -ENOENT || err == -ENAMETOOLONG)
>> +                                     this = NULL;
>> +                     } else {
>> +                             this = thispath.dentry;
>> +                             mntput(thispath.mnt);
>> +                             if (!this->d_inode) {
>> +                                     dput(this);
>> +                                     this = NULL;
>> +                             } else if (ovl_dentry_weird(this)) {
>> +                                     dput(this);
>> +                                     err = -EREMOTE;
>> +                             }
>> +                     }
>
> I'm not happy with that one - you are relying upon the fairly subtle
> assertions here.
>         1)  Had lowerpath.mnt *not* been a privately cloned one with nothing
> mounted on it, you would've been screwed.
>         2) Had that thing contained a "jumper" symlink (a-la procfs ones),
> you would've been screwed.  Currently only procfs has those, and it would've
> been rejected before getting there, but this is brittle and non-obvious.
>         3) Any automount point in there (nfs4 referrals, etc.) can
> break the assumption that nothing could've been mounted on it.  And _that_
> might have not been stepped onto; back when the path had been stored, there'd
> been no automount point at all, so we have avoided ovl_dentry_weird() rejects,
> and by now nothing on the path had been visited yet, so ovl_dentry_weird()
> didn't have a chance to trigger.  Note that calling it on the last dentry
> is no good - we might have crossed the automount point in the middle of that
> path, so this last dentry might be nice and shiny - and on another filesystem.
> So unlike (1) and (2) it's not just a fishy-looking thing that happens to
> work for non-local reasons; AFAICS, it's actually a bug.
>
> 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.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1515095

On Thu, Nov 3, 2016 at 5:50 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, Oct 28, 2016 at 6:15 PM, Al Viro <viro@zeniv.linux.org.uk> 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.

[toc] | [prev] | [next] | [standalone]

#1515199

On Fri, Nov 4, 2016 at 10:29 AM, Amir Goldstein <amir73il@gmail.com> wrote:

> 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:

Oops, missed that.  Good spotting!

And there's more shit that unionfs-testsuite didn't discover (not even
involving multiple layers):

rm -rf /lower /upper /work
mkdir -p /lower/a/b/c /upper /work
mount -t overlay overlay -oupperdir=/upper,lowerdir=/lower,workdir=/work /mnt
mv /mnt/a /mnt/z
mv /mnt/z/b /mnt/q
ls /mnt/q
umount /mnt
mount -t overlay overlay -oupperdir=/upper,lowerdir=/lower,workdir=/work /mnt
ls /mnt/q
umount /mnt

Next update coming up...

Thanks,
Miklos

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web