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

[simo]

On Tue, 2012-10-16 at 13:49 +1100, Andrew Bartlett wrote:
> On Sat, 2012-10-13 at 16:27 -0400, simo wrote:
> > On Sat, 2012-10-13 at 15:09 +1100, Andrew Bartlett wrote:
> 
> 
> > 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.
> 
> The reason that this area concerns me is that it is impractical to keep
> a copy of the previous POSIX -> NT mapping code around, because it isn't
> a simple algorithm embodied in one function, but a quite large body of
> code with interactions with other subsystems including idmap. 

It looks like this is a problem are then, however it was just to show
there are other options, I am not saying we need to go there.

> > > 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).
> 
> It would simply be extra data on the input to the sha256 hash, not extra
> data in the xattr. 

Ok this was my main concern, increase data size.
However it is not clear to me what's the point of hashing stuff beyond
the actual ACL, how do you know what changed then ?
Assume you do a 'compatible' change in one of the other things you hash
together with the ACL blob, you then couldn't tell if what changes it he
actual ACL or the additional info.

Can you please send a brief document with the exact algorithm and data
you envision to use ?

> > 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
> 
> To which mapping do you refer?
> 
> NT -> POSIX or POSIX -> NT?

NT -> Posix of course. We all know Posix -> NT is lossy and we have
acl_xattr to do that.

> If the NT -> POSIX mapping changes, we now have the data to reset the
> posix ACL, which the kernel will use.

Right, so is part of your plan to refresh the ACL when this situation is
detected ?

> If the POSIX -> NT mapping changes, we now have the data to continue to
> ignore the result of this mapping, as the administrator requested (by
> choosing to invoke vfs_acl_xattr). 

As long as we change the Posix ACL if the NT -> Posix mapping changes
this would be fine of course.

> > 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'm not using them interchangeably.  Indeed, I'm proposing to use only
> the system (posix) ACL hash.  The hash of the NT ACL is kept only for
> redundancy. 

Ok can you explain what it would be used for ? If it is not used I would
rather go all the way and eliminate it.

> > I hope this explains my concern.
> 
> I'm still horribly lost as to the specific concerns you have.  I'm not
> sure we can resolve them, as some of them go to the fundamental design
> and purpose of the module (that the posix -> NT ACL mapping can be
> disregarded, as long as we know the posix ACL and associated metadata
> hasn't been changed).  
> 
> At this stage, I'm not really sure how to proceed.  Could you prepare a
> patch with the changes you would like to see?
> 
> Otherwise, I can only propose not to proceed, as I don't seem to have
> any way to resolve your concerns, and pending any actual change to the
> POSIX -> NT ACL mapping I can live with what we have for now.  

Main concerns (for me) were:
- size of xattr
- changing posix acl if mapping changes

I also have the concern that this is too posix ACL specific, I would
like a scheme that is compatible with any arbitrary ACLs, but it seem
that this is taken care of by using a blob from the VFS module and
treating it as an opaque structure to hash, so assuming nothing else in
your proposal makes assumption on what is the ACL on disk or what
mapping is used (I assume a vfs module that store a different type of
acl will be able to replace the whole mapping).

Simo.

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