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

Groups > linux.kernel > #1480284 > unrolled thread

[GIT PULL] overlayfs fix for 4.8-rc5

Back to article view | Back to linux.kernel

Contents

  [GIT PULL] overlayfs fix for 4.8-rc5 Miklos Szeredi <miklos@szeredi.hu> - 2016-09-09 21:40 +0200
    Re: [GIT PULL] overlayfs fix for 4.8-rc5 Linus Torvalds <torvalds@linux-foundation.org> - 2016-09-09 22:00 +0200
      Re: [GIT PULL] overlayfs fix for 4.8-rc5 Miklos Szeredi <miklos@szeredi.hu> - 2016-09-09 22:10 +0200
        Re: [GIT PULL] overlayfs fix for 4.8-rc5 Linus Torvalds <torvalds@linux-foundation.org> - 2016-09-09 22:40 +0200
          Re: [GIT PULL] overlayfs fix for 4.8-rc5 Miklos Szeredi <miklos@szeredi.hu> - 2016-09-09 23:20 +0200
            Re: [GIT PULL] overlayfs fix for 4.8-rc5 Linus Torvalds <torvalds@linux-foundation.org> - 2016-09-09 23:50 +0200

#1480284 — [GIT PULL] overlayfs fix for 4.8-rc5

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(-)

[toc] | [next] | [standalone]

#1480295

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

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

#1480305

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

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

#1480339

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

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

#1480361

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

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

#1480370

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web