[GIT PULL] overlayfs update for 4.9

[GIT PULL] overlayfs update for 4.9

[Miklos Szeredi]

Hi Linus,

Please pull from:

  git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-linus

I tried to submit this though Al because of the VFS changes, but failed
unfortunately.  The VFS changes are small and should only affect overlayfs, so
here it is.

This update contains the following changes:

 - a couple of fixes in the "use mounter's permission to access underlying
   layers" area;

 - mnt_want_write_file() should freeze protect the underlying layer, except in
   the case of i_ops, where the callee is expected to do that

 - use "clone_file_range" to copy up if possible, resulting in a substantial
   speedup;

 - misc fixes and cleanups.

Thanks,
Miklos

---
Amir Goldstein (3):
      vfs: allow vfs_clone_file_range() across mount points
      vfs: call vfs_clone_file_range() under mnt_want_write()
      ovl: use vfs_clone_file_range() for copy up if possible

Miklos Szeredi (7):
      ovl: copy_up_xattr(): use strnlen
      ovl: lookup: do getxattr with mounter's permission
      vfs: mnt_want_write_file() should freeze protect underlying sb
      ovl: explain error values when removing acl from workdir
      ovl: use generic_readlink
      vfs: add vfs_get_link() helper
      ovl: use vfs_get_link()

Richard Weinberger (1):
      ovl: Fix info leak in ovl_lookup_temp()

Vivek Goyal (1):
      ovl: during copy up, switch to mounter's creds early

---
 fs/ioctl.c             |  5 +++-
 fs/namei.c             | 25 +++++++++++++++++
 fs/namespace.c         |  7 +++--
 fs/nfsd/vfs.c          |  3 +-
 fs/open.c              | 15 +++++-----
 fs/overlayfs/copy_up.c | 75 +++++++++++++++++++-------------------------------
 fs/overlayfs/dir.c     |  5 +++-
 fs/overlayfs/inode.c   | 44 +++++++----------------------
 fs/overlayfs/super.c   | 33 ++++++++++++++--------
 fs/read_write.c        | 13 ++++-----
 fs/xattr.c             | 13 ++++++---
 include/linux/fs.h     | 14 ++++++++++
 12 files changed, 135 insertions(+), 117 deletions(-)

Re: [GIT PULL] overlayfs update for 4.9

[Miklos Szeredi]

On Thu, Oct 13, 2016 at 04:37:51PM +0200, Miklos Szeredi wrote:
> Hi Linus,
> 
> Please pull from:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-linus
> 
> I tried to submit this though Al because of the VFS changes, but failed
> unfortunately.  The VFS changes are small and should only affect overlayfs, so
> here it is.

Forgot to mention that it contains a conflict against the xattr update from
Andreas.  The resolution is to take my version as we now can and should do
permission checks on the underlying layers (with mounter's permission).

Thanks,
Miklos

> 
> This update contains the following changes:
> 
>  - a couple of fixes in the "use mounter's permission to access underlying
>    layers" area;
> 
>  - mnt_want_write_file() should freeze protect the underlying layer, except in
>    the case of i_ops, where the callee is expected to do that
> 
>  - use "clone_file_range" to copy up if possible, resulting in a substantial
>    speedup;
> 
>  - misc fixes and cleanups.
> 
> Thanks,
> Miklos
> 
> ---
> Amir Goldstein (3):
>       vfs: allow vfs_clone_file_range() across mount points
>       vfs: call vfs_clone_file_range() under mnt_want_write()
>       ovl: use vfs_clone_file_range() for copy up if possible
> 
> Miklos Szeredi (7):
>       ovl: copy_up_xattr(): use strnlen
>       ovl: lookup: do getxattr with mounter's permission
>       vfs: mnt_want_write_file() should freeze protect underlying sb
>       ovl: explain error values when removing acl from workdir
>       ovl: use generic_readlink
>       vfs: add vfs_get_link() helper
>       ovl: use vfs_get_link()
> 
> Richard Weinberger (1):
>       ovl: Fix info leak in ovl_lookup_temp()
> 
> Vivek Goyal (1):
>       ovl: during copy up, switch to mounter's creds early
> 
> ---
>  fs/ioctl.c             |  5 +++-
>  fs/namei.c             | 25 +++++++++++++++++
>  fs/namespace.c         |  7 +++--
>  fs/nfsd/vfs.c          |  3 +-
>  fs/open.c              | 15 +++++-----
>  fs/overlayfs/copy_up.c | 75 +++++++++++++++++++-------------------------------
>  fs/overlayfs/dir.c     |  5 +++-
>  fs/overlayfs/inode.c   | 44 +++++++----------------------
>  fs/overlayfs/super.c   | 33 ++++++++++++++--------
>  fs/read_write.c        | 13 ++++-----
>  fs/xattr.c             | 13 ++++++---
>  include/linux/fs.h     | 14 ++++++++++
>  12 files changed, 135 insertions(+), 117 deletions(-)

Re: [GIT PULL] overlayfs update for 4.9

[Linus Torvalds]

On Thu, Oct 13, 2016 at 7:37 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> Please pull from:

No.

Or rather, I pulled and then immediately unpulled. When I look at the
diff, I saw an obvious bug in the very first chunk. I'm not going to
pull something that is this obviously buggy and untested.

Your change to fs/ioctl.c to add a -EXDEV error case very clearly
leaks a reference to 'src_file'.

It's too late in the merge window for this to be fixed up any more.
This has been a painful enough merge window for me that I'm not going
to play around with obviously buggy pull requests.

                  Linus

Re: [GIT PULL] overlayfs update for 4.9

[Sedat Dilek]

On Fri, Oct 14, 2016 at 6:03 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Oct 13, 2016 at 7:37 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>
>> Please pull from:
>
> No.
[...]
> It's too late in the merge window for this to be fixed up any more.
> This has been a painful enough merge window for me that I'm not going
> to play around with obviously buggy pull requests.
>

( Cannot say much to the BROKEN stuff. )
Be gracious/clement/merciful/mild...
I expect Miklos will fix that.
AFAICS, Linux v4.9 is getting a LTS version.
And think of the complex correlations in the (v)fs-trees.
Cannot say if there are better strategies to cook all that stuff together.
[ It was a pain in the ass to convince responsibles to get OverlayFS upstream. ]

- Sedat -

P.S.: It's a long time I played with SquashFS/OverlayFS.

Re: [GIT PULL] overlayfs update for 4.9

[Amir Goldstein]

On Fri, Oct 14, 2016 at 7:03 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Oct 13, 2016 at 7:37 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>
>> Please pull from:
>
> No.
>
> Or rather, I pulled and then immediately unpulled. When I look at the
> diff, I saw an obvious bug in the very first chunk. I'm not going to
> pull something that is this obviously buggy and untested.
>
> Your change to fs/ioctl.c to add a -EXDEV error case very clearly
> leaks a reference to 'src_file'.

On the charge of writing obviously buggy code I plead guilty :-/
On the charge of not testing my code I plead not guilty.
I exercised the FICLONERANGE intensively with the xfstests clone test group
and never experienced any problem and any warning running with all
relevant kernel debugging enabled.

So how come this leak went unnoticed?
Because fdget (__fget_light) does not take a reference if the fd is not shared.

So what can we do to catch silly mistakes like this one earlier and
without relying
on Linus's spidy senses?
Writing xfstests to test all fd interfaces with cloned fds? Is this a
scalable solution?
Or maybe it's best to add a trivial CONFIG_DEBUG_FDGET that will always
skip the no refcount optimization?

To me the second option seems preferred from engineering pov
I can post this simple patch if you guys agree to this solution.

>
> It's too late in the merge window for this to be fixed up any more.
> This has been a painful enough merge window for me that I'm not going
> to play around with obviously buggy pull requests.
>

It has been a rocky merge window and I apologize for adding this last straw
and pooping the party for the entire overlay pull request.
In the hope that this may help to sweeten your verdict -
I just got my hands on a brand new testing machine, which is dedicated
to stress testing file systems on the latest rc.

Amir.

>                   Linus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] overlayfs update for 4.9

[Amir Goldstein]

On Fri, Oct 14, 2016 at 9:48 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Oct 14, 2016 at 7:03 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Thu, Oct 13, 2016 at 7:37 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>
>>> Please pull from:
>>
>> No.
>>
>> Or rather, I pulled and then immediately unpulled. When I look at the
>> diff, I saw an obvious bug in the very first chunk. I'm not going to
>> pull something that is this obviously buggy and untested.
>>
>> Your change to fs/ioctl.c to add a -EXDEV error case very clearly
>> leaks a reference to 'src_file'.
>
> On the charge of writing obviously buggy code I plead guilty :-/
> On the charge of not testing my code I plead not guilty.
> I exercised the FICLONERANGE intensively with the xfstests clone test group
> and never experienced any problem and any warning running with all
> relevant kernel debugging enabled.
>
> So how come this leak went unnoticed?
> Because fdget (__fget_light) does not take a reference if the fd is not shared.
>
> So what can we do to catch silly mistakes like this one earlier and
> without relying
> on Linus's spidy senses?
> Writing xfstests to test all fd interfaces with cloned fds? Is this a
> scalable solution?


Well, I added an idle loop thread to xfs_io and sure enough it catches the leak
in test generic/157 (Try cross-device reflink).
I will post patches to fstests.

Sorry...

> Or maybe it's best to add a trivial CONFIG_DEBUG_FDGET that will always
> skip the no refcount optimization?
>
> To me the second option seems preferred from engineering pov
> I can post this simple patch if you guys agree to this solution.
>
>>
>> It's too late in the merge window for this to be fixed up any more.
>> This has been a painful enough merge window for me that I'm not going
>> to play around with obviously buggy pull requests.
>>
>
> It has been a rocky merge window and I apologize for adding this last straw
> and pooping the party for the entire overlay pull request.
> In the hope that this may help to sweeten your verdict -
> I just got my hands on a brand new testing machine, which is dedicated
> to stress testing file systems on the latest rc.
>
> Amir.
>
>>                   Linus
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] overlayfs update for 4.9

[Miklos Szeredi]

On Thu, Oct 6, 2016 at 10:01 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, Oct 6, 2016 at 1:49 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Wed, Oct 05, 2016 at 09:52:10PM +0200, Miklos Szeredi wrote:
>>> Hi Al,
>>>
>>> Please pull from:
>>>
>>>   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-viro
>>>
>>> This has an assortment of fixes and cleanups for overlayfs.
>>>
>>> It also touches the VFS:
>>>
>>>  - add the vfs_get_link() helper for calling i_op->get_link();
>>>  - fix mnt_want_write_file() to freeze protect the underlying sb instead of
>>>    overlay sb;
>>>  - allow vfs_clone_file_range() to be called by overlayfs.
>>
>> Could you explain why e.g. fsetxattr() needs a different treatment in
>> "vfs: mnt_want_write_file() should freeze protect underlying sb"?
>
> Because setxattr is an i_op, not a f_op, so it's overlayfs that's
> going to be called and will do the freeze protect on underlying layer
> (not to mention, that it might be a different underlying sb that will
> get the freeze protection in the end, due to copy up)

Hi Al,

Are you okay with the VFS changes in there?  If so could you please
pull and send to Linus?  Or I can send the pull request to Linus
directly, whichever you prefer.

Thanks,
Miklos

Re: [GIT PULL] overlayfs update for 4.9

[Miklos Szeredi]

On Thu, Oct 6, 2016 at 1:49 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Oct 05, 2016 at 09:52:10PM +0200, Miklos Szeredi wrote:
>> Hi Al,
>>
>> Please pull from:
>>
>>   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-viro
>>
>> This has an assortment of fixes and cleanups for overlayfs.
>>
>> It also touches the VFS:
>>
>>  - add the vfs_get_link() helper for calling i_op->get_link();
>>  - fix mnt_want_write_file() to freeze protect the underlying sb instead of
>>    overlay sb;
>>  - allow vfs_clone_file_range() to be called by overlayfs.
>
> Could you explain why e.g. fsetxattr() needs a different treatment in
> "vfs: mnt_want_write_file() should freeze protect underlying sb"?

Because setxattr is an i_op, not a f_op, so it's overlayfs that's
going to be called and will do the freeze protect on underlying layer
(not to mention, that it might be a different underlying sb that will
get the freeze protection in the end, due to copy up)

Thanks,
Miklos

Re: [GIT PULL] overlayfs update for 4.9

[Al Viro]

On Wed, Oct 05, 2016 at 09:52:10PM +0200, Miklos Szeredi wrote:
> Hi Al,
> 
> Please pull from:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-viro
> 
> This has an assortment of fixes and cleanups for overlayfs.
> 
> It also touches the VFS:
> 
>  - add the vfs_get_link() helper for calling i_op->get_link();
>  - fix mnt_want_write_file() to freeze protect the underlying sb instead of
>    overlay sb;
>  - allow vfs_clone_file_range() to be called by overlayfs.

Could you explain why e.g. fsetxattr() needs a different treatment in
"vfs: mnt_want_write_file() should freeze protect underlying sb"?

Re: [GIT PULL] overlayfs update for 4.9

[Al Viro]

On Wed, Oct 05, 2016 at 09:52:10PM +0200, Miklos Szeredi wrote:
> Hi Al,
> 
> Please pull from:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-viro
> 
> This has an assortment of fixes and cleanups for overlayfs.
> 
> It also touches the VFS:
> 
>  - add the vfs_get_link() helper for calling i_op->get_link();
>  - fix mnt_want_write_file() to freeze protect the underlying sb instead of
>    overlay sb;
>  - allow vfs_clone_file_range() to be called by overlayfs.

Give me about an hour, OK?

[GIT PULL] overlayfs update for 4.9

[Miklos Szeredi]

Hi Al,

Please pull from:

  git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-viro

This has an assortment of fixes and cleanups for overlayfs.

It also touches the VFS:

 - add the vfs_get_link() helper for calling i_op->get_link();
 - fix mnt_want_write_file() to freeze protect the underlying sb instead of
   overlay sb;
 - allow vfs_clone_file_range() to be called by overlayfs.

None of the above should have any effect on filesystems other than overlayfs.

Thanks,
Miklos

---
Amir Goldstein (3):
      vfs: allow vfs_clone_file_range() across mount points
      vfs: call vfs_clone_file_range() under mnt_want_write()
      ovl: use vfs_clone_file_range() for copy up if possible

Miklos Szeredi (8):
      ovl: copy_up_xattr(): use strnlen
      ovl: lookup: do getxattr with mounter's permission
      vfs: mnt_want_write_file() should freeze protect underlying sb
      ovl: make directory ino/dev numbers stable
      ovl: explain error values when removing acl from workdir
      ovl: use generic_readlink
      vfs: add vfs_get_link() helper
      ovl: use vfs_get_link()

Richard Weinberger (1):
      ovl: Fix info leak in ovl_lookup_temp()

Vivek Goyal (1):
      ovl: during copy up, switch to mounter's creds early

---
 fs/ioctl.c             |  5 +++-
 fs/namei.c             | 25 +++++++++++++++++
 fs/namespace.c         |  7 +++--
 fs/nfsd/vfs.c          |  3 +-
 fs/open.c              | 15 +++++-----
 fs/overlayfs/copy_up.c | 75 +++++++++++++++++++-------------------------------
 fs/overlayfs/dir.c     | 32 +++++++++++++++++++--
 fs/overlayfs/inode.c   | 44 +++++++----------------------
 fs/overlayfs/super.c   | 33 ++++++++++++++--------
 fs/read_write.c        | 13 ++++-----
 fs/xattr.c             | 13 ++++++---
 include/linux/fs.h     | 14 ++++++++++
 12 files changed, 160 insertions(+), 119 deletions(-)