[PATCH][WIP] Make vfs_acl_xattr use hash of the posix ACL

[simo]

On Sat, 2012-10-13 at 15:09 +1100, Andrew Bartlett wrote:
> On Fri, 2012-10-12 at 17:53 -0400, simo wrote:
> 
> > Sorry I am afraid you didn't explain why it is ok to keep mappings on
> > disk that do not match anymore.
> 
> Thinking about this some more, we can now do that.  We can confirm if
> the mapping NT -> posix has changed (because we have stored the result
> of that), as well as tell if the posix ACL itself has changed, or
> (because I store both hashes) the posix ACL is unchanged but the posix
> -> NT mapping has changed.
> 
> We couldn't do that before, all we would know is that *either* the posix
> -> NT mapping (which is the mapping this module is specifically trying
> to avoid returning to the client) has changed, or the the posix ACL has
> changed. 

Well, the one way to do it without 2 hashes is to keep around the old
mapping method in case a change is needed and use that as a fallback to
check the validity of the hash.

But I am not interested in the problem of how to detect the difference.

The problem is that if you change the mapping you'll need to do some
more than just match one or the other hash.
Mapping is done in order to make the kernel do the right access checks
for us. So if we change the mapping it means the kernel wasn't, and
arguably will keep not doing it unless the Posix ACL is changed on the
disc to reflect the new mapping.

> > I am guessing that if we ever change the mapping it will be because the
> > previous one was incorrect ? Isn't this a good reason to invalidate
> > mappings ?
> 
> As this module was specifically to avoid (in the normal case) returning
> the POSIX -> NT mapping to the client, my argument is that we should be
> able to, if need by improve that mapping without invaliding the hash. 

This is what I have a problem with. I think that if you change the
mapping you ought to change the ACL on the file, as that is the
authoritative one. You can detect and fix the ACL on the fs instead of
doing a lossy Posix -> NT ACL, that is ok.

> If your concern is that changes in idmap values should also invalidate
> the hash, then it would not be difficult to include a table of idmap ID
> -> SID mappings in the hash. 

No we can't. My concern is that we add more data here and on a lot of
file systems EAs are limited in size (we already have limitations on the
size of the NT ACL we can store).
To be clear I do not have a concern about storing the Posix ACL hash,
that's fine, but I have a concern in what you seem to imply, that if the
Posix ACL hash matches we are not going to change the ACL even if the
mapping changed

At least, this was my impression.

And this is why I am concerned about having 2 hashes, it will be easy to
use them interchangeably and 'forget' to check if the mapping still
matches, and if there are changes to be made on disk.

I hope this explains my concern.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>