[GIT PULL] overlayfs fix for 4.8-rc5

[GIT PULL] overlayfs fix for 4.8-rc5

[Miklos Szeredi]

Hi Linus,

Please pull from:

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

This fixes a regression caused by the last pull request.

Thanks,
Miklos

---
Miklos Szeredi (1):
      ovl: fix workdir creation

---
 fs/overlayfs/super.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Re: [GIT PULL] overlayfs fix for 4.8-rc5

[Linus Torvalds]

On Fri, Sep 9, 2016 at 12:37 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> This fixes a regression caused by the last pull request.

So why are you checking error numbers one by one?

Why is an error not just always an error?

              Linus

Re: [GIT PULL] overlayfs fix for 4.8-rc5

[Miklos Szeredi]

On Fri, Sep 9, 2016 at 9:58 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Sep 9, 2016 at 12:37 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>
>> This fixes a regression caused by the last pull request.
>
> So why are you checking error numbers one by one?
>
> Why is an error not just always an error?

The issue is that not all errors should result in failure.

That code tries to remove ACL from a directory, and there are several cases:

1) success: that's good obviously
2) error: no ACL was found: that's also good
3) error: ACL's are not supported by the filesystem: this is also good
4) error: ACL was there but we failed to remove it for some other
reason: this is not good

The patch adds handling of case 3.

Thanks,
Miklos

Re: [GIT PULL] overlayfs fix for 4.8-rc5

[Linus Torvalds]

On Fri, Sep 9, 2016 at 1:08 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> That code tries to remove ACL from a directory, and there are several cases:
>
> 1) success: that's good obviously
> 2) error: no ACL was found: that's also good
> 3) error: ACL's are not supported by the filesystem: this is also good
> 4) error: ACL was there but we failed to remove it for some other
> reason: this is not good
>
> The patch adds handling of case 3.

I'm not convinced your explanation is correct.

The thing is, you added a test for -EOPNOTSUPP, and that is in fact at
least partly case (2) (eg xattr_resolve_name())

And EOPNOTSUPP actually seems to be the _clear_ case. The ENODATA case
is the one that is hard to actually verify. I tried to see that "yes,
all filesystems return ENODATA", but it wasn't obvious at all (p9fs?)
If I read the cifs code right, it returns EOPNOTSUPP for the "not
found" case too.

And ext2/ext4 returns ERANGE for some "we don't support that" cases,
while gfs2 seems to return EINVAL for those cases. Those are obviously
also cases of (2), but the fuse code doesn't test for it.

So the error list seems to be rather random, and no, ENODATA and
EOPNOTSUPP do not seem to be the only errors that would match the
above at all.

I dunno. I guess this is a corner case that really doesn't matter in
practice, but the whole "let's test a few special cases" approach
fails the smell test to me, and doesn't actually seem to match your
cases above very well.

              Linus

Re: [GIT PULL] overlayfs fix for 4.8-rc5

[Miklos Szeredi]

On Fri, Sep 9, 2016 at 10:36 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Sep 9, 2016 at 1:08 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>
>> That code tries to remove ACL from a directory, and there are several cases:
>>
>> 1) success: that's good obviously
>> 2) error: no ACL was found: that's also good
>> 3) error: ACL's are not supported by the filesystem: this is also good
>> 4) error: ACL was there but we failed to remove it for some other
>> reason: this is not good
>>
>> The patch adds handling of case 3.
>
> I'm not convinced your explanation is correct.
>
> The thing is, you added a test for -EOPNOTSUPP, and that is in fact at
> least partly case (2) (eg xattr_resolve_name())
>
> And EOPNOTSUPP actually seems to be the _clear_ case. The ENODATA case
> is the one that is hard to actually verify. I tried to see that "yes,
> all filesystems return ENODATA", but it wasn't obvious at all (p9fs?)
> If I read the cifs code right, it returns EOPNOTSUPP for the "not
> found" case too.
>
> And ext2/ext4 returns ERANGE for some "we don't support that" cases,
> while gfs2 seems to return EINVAL for those cases. Those are obviously
> also cases of (2), but the fuse code doesn't test for it.
>
> So the error list seems to be rather random, and no, ENODATA and
> EOPNOTSUPP do not seem to be the only errors that would match the
> above at all.
>
> I dunno. I guess this is a corner case that really doesn't matter in
> practice, but the whole "let's test a few special cases" approach
> fails the smell test to me, and doesn't actually seem to match your
> cases above very well.

Okay, so how do we do this correctly?  The desired end result is
clear:  FOOBAR xattr doesn't exist on the directory.  This "doesn't
exist" is a negative result, so the only way to check it is to check
for errors.  Either with removexattr() or getxattr() this will get us
into the same trap: that there are multiple errors that have this
meaning.

Same thing with ENOENT: checking for non-existence can be difficult,
because ENAMETOOLONG is actually a special form of ENOENT.

I just don't see the "right" soltion here, and while I agree that the
current one is ugly, it should actually work. We won't get ERANGE,
because the posix acl xattrs have short names that'll fit in all
filesystems and 9p isn't even a valid fs for the upper layer of an
overlay.

Thanks,
Miklos

Re: [GIT PULL] overlayfs fix for 4.8-rc5

[Linus Torvalds]

On Fri, Sep 9, 2016 at 2:18 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> Okay, so how do we do this correctly?  The desired end result is
> clear:  FOOBAR xattr doesn't exist on the directory.  This "doesn't
> exist" is a negative result, so the only way to check it is to check
> for errors.  Either with removexattr() or getxattr() this will get us
> into the same trap: that there are multiple errors that have this
> meaning.
>
> Same thing with ENOENT: checking for non-existence can be difficult,
> because ENAMETOOLONG is actually a special form of ENOENT.

Yeah, ENOENT, ENAMETOOLONG, ERANGE, EINVAL can all be different
versions of "this doesn't exist" - perhaps because such a name
*cannot* exist on the filesystem.

> I just don't see the "right" soltion here, and while I agree that the
> current one is ugly, it should actually work. We won't get ERANGE,
> because the posix acl xattrs have short names that'll fit in all
> filesystems and 9p isn't even a valid fs for the upper layer of an
> overlay.

So maybe the right thing to do is to just document it, and have a
comment that shows that the code at least knows about these things and
that it doesn't matter in practice.

The "just check for two errors without any comments about why those
particular two errors are different from all the *other* errors" is
what I really objected to.

            Linus