[GIT PULL] overlayfs fixes for 4.9-rc3

[GIT PULL] overlayfs fixes for 4.9-rc3

[Miklos Szeredi]

Hi Linus,

Please pull from:

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

Fix two more POSIX ACL bugs introduced in 4.8 and add a missing fsync during
copy up to prevent possible data loss.

Also introduce the concept of feature flags to allow backward incompatible
changes to the overlay format.  This should have been there from day one; the
best we can do now is backport to stable kernels.  Add the check for features
without adding any actual features yet.

Thanks,
Miklos

---
Miklos Szeredi (4):
      ovl: update S_ISGID when setting posix ACLs
      ovl: fix get_acl() on tmpfs
      ovl: fsync after copy-up
      ovl: check fs features

---
 Documentation/filesystems/overlayfs.txt | 12 +++++++
 fs/overlayfs/copy_up.c                  |  2 ++
 fs/overlayfs/inode.c                    |  3 --
 fs/overlayfs/overlayfs.h                |  1 +
 fs/overlayfs/super.c                    | 56 +++++++++++++++++++++++++++++++++
 5 files changed, 71 insertions(+), 3 deletions(-)

Re: [GIT PULL] overlayfs fixes for 4.9-rc3

[Linus Torvalds]

On Fri, Nov 4, 2016 at 2:30 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> Also introduce the concept of feature flags to allow backward incompatible
> changes to the overlay format.  This should have been there from day one; the
> best we can do now is backport to stable kernels.  Add the check for features
> without adding any actual features yet.

No. I pulled the three other commits, but not that last one.

That feature just seems to actively *encourage* backwards incompatible
features. It's a bad idea. Don't do it. If we've been able to do
without it so far, then why should we suddenly start doing things like
this?

So I don't agree that it should have been there since day one, it just
shouldn't exist at all.

                  Linus

Re: [GIT PULL] overlayfs fixes for 4.9-rc3

[Amir Goldstein]

On Sat, Nov 5, 2016 at 5:06 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Nov 4, 2016 at 2:30 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>
>> Also introduce the concept of feature flags to allow backward incompatible
>> changes to the overlay format.  This should have been there from day one; the
>> best we can do now is backport to stable kernels.  Add the check for features
>> without adding any actual features yet.
>
> No. I pulled the three other commits, but not that last one.
>
> That feature just seems to actively *encourage* backwards incompatible
> features. It's a bad idea. Don't do it. If we've been able to do
> without it so far, then why should we suddenly start doing things like
> this?
>
> So I don't agree that it should have been there since day one, it just
> shouldn't exist at all.
>

Linus,

Can you please clarify your objection?

I suppose you do not object to the concept of on-disk format version nor on-disk
format compatible/incompatible features sets.
Just to fact that overlayfs didn't have those form day one, so it
should find a way
to cope with that situation without patching stable kernels?

Thanks,
Amir.

Re: [GIT PULL] overlayfs fixes for 4.9-rc3

[Linus Torvalds]

On Fri, Nov 4, 2016 at 11:44 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>
> Can you please clarify your objection?

There are several:

 - timing. No way in hell will I take a new feature like this during an rc

 - lack of explanation. Why is this bad feature needed in the first
place? Why would overlayfs versioning _ever_ be a good idea?

 - is the implementation even sane? Right now I don't think overlayfs
even requires xattr support in the upper filesystem, so the whole
concept seems frankly totally misdesigned.

> I suppose you do not object to the concept of on-disk format version nor on-disk
> format compatible/incompatible features sets.

I object both to the concept and to the implementation and to the
timing. The thing seems broken. Doing it during the rc cycle makes it
doubly so.

> Just to fact that overlayfs didn't have those form day one, so it
> should find a way to cope with that situation without patching
> stable kernels?

What "situation"? There's no f*cking explanation of why we'd even want
this crap. Not in the commit message, not in the pull request, not
*anywhere*.

And then the commit marks that shit for stable? When it clearly
doesn't fix anything, and it has never ever been needed before?

NO.

               Linus

Re: [GIT PULL] overlayfs fixes for 4.9-rc3

[Miklos Szeredi]

On Sat, Nov 5, 2016 at 6:41 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Nov 4, 2016 at 11:44 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>
>> Can you please clarify your objection?
>
> There are several:
>
>  - timing. No way in hell will I take a new feature like this during an rc

I can do it next merge window; the reason I wanted this patch in as
early as possible because, as I said, it's already too late.  But it's
no big deal.

>  - lack of explanation. Why is this bad feature needed in the first
> place? Why would overlayfs versioning _ever_ be a good idea?

The feature that would be introduced is this: allow directory renames
to work without having to recursively copy-up the subtree.  Whatever
mechanism is devised to do this will be backward incompatible.  Maybe
it's a misguided idea, but it's been through several rounds of reviews
on the relevant mailing lists and there weren't any objections (yet).

And the thing is, backward incompatibility is less of an issue for
overlayfs than for normal filesystems, because it's usually not
something people store their root filesystems on, and if so they can
simply not turn off this feature.

>
>  - is the implementation even sane? Right now I don't think overlayfs
> even requires xattr support in the upper filesystem, so the whole
> concept seems frankly totally misdesigned.

overlayfs relies on xattr to create opaque directories (i.e. you
remove a directory (residing on the lower layer) and create one with
the same name).  So it is needed for normal r/w operation.   And
definitely for the above feature which also uses xattr.

Thanks,
Miklos

Re: [GIT PULL] overlayfs fixes for 4.9-rc3

[Linus Torvalds]

On Sat, Nov 5, 2016 at 12:45 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> The feature that would be introduced is this: allow directory renames
> to work without having to recursively copy-up the subtree.  Whatever
> mechanism is devised to do this will be backward incompatible.  Maybe
> it's a misguided idea, but it's been through several rounds of reviews
> on the relevant mailing lists and there weren't any objections (yet).
>
> And the thing is, backward incompatibility is less of an issue for
> overlayfs than for normal filesystems, because it's usually not
> something people store their root filesystems on, and if so they can
> simply not turn off this feature.

(a) that should be explained

(b) that has nothing to do with being marked for stable

(c) that new doesn't actually explain in any way why you'd want
"feature flags" thing, for exactly the same "backwards incompatibility
is less of an issue" reason that you state.

Why not "just do it", in other words. For exactly the reasons you say.
Make it a mount option that people can choose to use or not.

> overlayfs relies on xattr to create opaque directories

Yes and no. It relies on it for THAT ONE FEATURE, which you don't have
to use. As far as I can tell, overlayfs does *not* rely on xattrs in
general.

               Linus

Re: [GIT PULL] overlayfs fixes for 4.9-rc3

[Peter Rosin]

> And the thing is, backward incompatibility is less of an issue for
> overlayfs than for normal filesystems, because it's usually not
> something people store their root filesystems on, and if so they can
> simply not turn off this feature.

That got my attention. What backwards incompatible thing is it that
I simply cannot turn off for the overlayfs that I use as root fs?
Now, we don't boot straight into the overlay as root fs, buy we do
pivot it in early enough. If you are somehow suggesting that
overlayfs as root fs is not something that needs considering you
need to think again. We depend on it, and I will flag any regression
that we are walking into.

But hopefully you just messed up the negations, and you meant that we
*can* simply turn <whatever> off?

Cheers,
Peter

[I hope threading works, got the message-id from an archive]