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

[Andrew Bartlett]

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. 

> > 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. 

> 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?

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

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

> 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. 

> 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.  

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org