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

[simo]

On Sat, 2012-10-13 at 07:59 +1100, Andrew Bartlett wrote:
> On Fri, 2012-10-12 at 14:05 -0400, simo wrote:
> > On Fri, 2012-10-12 at 17:12 +0200, Volker Lendecke wrote:
> > > On Fri, Oct 12, 2012 at 10:26:15PM +1100, Andrew Bartlett wrote:
> > > > On Fri, 2012-10-12 at 13:11 +0200, Christian Ambach wrote:
> > > > > Hi Andrew,
> > > > > 
> > > > > On 10/09/2012 01:58 PM, Andrew Bartlett wrote:
> > > > > > This patch (still a work in progress, as I know it fails some tests)
> > > > > > is what I've been talking about for months, and which I got VFS
> > > > > > changes in (almost - one small change in this patch) into 4.0.0rc1.
> > > > > >
> > > > > > The idea is simple, have the ACL modules (the posix ACL modules in
> > > > > > particular) provide a blob form of the ACL, and then hash that,
> > > > > > rather than an NT ACL that might change if our ACL mapping code
> > > > > > changes.
> > > > > 
> > > > > I am not sure I am getting the problem you are trying to solve here.
> > > > > What is the benefit of adding another representation of an ACL that is
> > > > > just intermediate?
> > > > > 
> > > > > Can you give some more explanations that go further than the commit
> > > > > message in your attachment?
> > > > 
> > > > This goes back to a discussion that we have had on and off over a few
> > > > months.
> > > > 
> > > > What I'm working on is an improved implementation of the hash in
> > > > vfs_acl_common.c.  The new hash will be of the 'system' ACL, whatever
> > > > that is, rather than the NT ACL it maps to.
> > > 
> > > Why is hashing the NT ACL not sufficient here?
> > 
> > I think Andrew is concerned that if we change the way we map from NT ACL
> > -> Posix ACL that we will fail the hash check and will start considering
> > all ACLs on file as 'modified' and therefore throw away the NT ACL and
> > recalculate back from the Posix ACL, even though no ACL changed on the
> > file but is the mapping that changed.
> > 
> > However I have to ask: *if* we do indeed change the mapping so that the
> > hash check fail is it really bad to fail the check ?
> 
> If what changed is our code, and not administrator or user intent, then
> I don't want to invalidate a whole NAS worth of ACLs.  
> 
> I'm also concerned by things like the nt4_compatible_acls() call that
> changes the returned (and therefore hashed) NT ACL based on the client
> OS.  This kind of variability doesn't belong in an on-disk hash.
> 
> > I mean if we change the mapping it means we did something wrong, so if
> > we keep the NT ACL unchanged and not fail the hash check it means we
> > will not show the user that the ACL really is different (from our new
> > understanding of how it should be represented).
> > 
> > The problem has 2 sides:
> > On the one hand we want to keep ACLs as close as possible to what
> > Windows clients send us, I guess that's why Andrew wants to consider
> > them authoritative and hash them.
> 
> That is the specific design and purpose of this not-on-by-default
> module. 
> 
> The most important part of my changes are to give us options in the
> future.  Specifically, we store both hash values, a timestamp and a
> description of what the blob is, in the hope that when we find we have
> to make some incompatible change, we can still keep the old NT ACLs on
> disk valid, for upgrading users.  
> 
> > On the other hand though, the authoritative ACL from the file access POV
> > is the one on the File System as that's what the kernel enforce.
> > And if we consider the one on the FS the authoritative one, then it can
> > be argued that it is indeed a good idea to break the mapping if the
> > Posix ACL -> NT ACL mapping changes and causes the hash check to fail.
> > 
> > So which PoV should we subscribe to in the Filesystem ?
> > Or did I fall for a false dichotomy ?
> 
> I think you did.

Can you explain where ?

>   I hope what I've said above helps explain it better,

Sorry I am afraid you didn't explain why it is ok to keep mappings on
disk that do not match anymore.

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 ?

Simo.

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