Re: ACL patch

[Andrew Doran <ad%netbsd.org@localhost>]

On Thu, May 14, 2020 at 08:16:50PM -0000, Christos Zoulas wrote:
> In article <20200512235040.GD9377%homeworld.netbsd.org@localhost>,
> Andrew Doran  <ad%netbsd.org@localhost> wrote:
> >On Tue, May 12, 2020 at 10:37:27PM +0000, Andrew Doran wrote:
> >> On Sun, May 10, 2020 at 09:48:10AM -0400, Christos Zoulas wrote:
> >> > 
> >> > > Maybe I missed it but I didn't see a way for cache_lookup_linked() and
> >> > > cache_have_id() to know a vnode has ACLs.  The presence of ACLs
> >means those
> >> > > routines can't do their imitation of VOP_ACCESS() and need to fail so that
> >> > > the lookup is handled by VOP_LOOKUP().  To handle that on a
> >per-vnode basis
> >> > > you'd want to flag the "has ACLs" fact when an inode is loaded, and do the
> >> > > same when an ACL is added to an in-core inode.  I'll look into it over the
> >> > > next few days unless you want to take care of it.
> >> > 
> >> > So I looked into this a bit and genfs_can_access() is also used by 
> >cache_revlookup.
> >> > I am not sure what to do here. It is simple enough to add a flag to
> >the vnode to indicate
> >> > if it has ACLs, but is it the right thing to do?
> >> 
> >> I tried to pull down your patch again over the weekend to take a look but it
> >> was gone.  The best place for a flag would be cache_enter_id() since it
> >> deals with the necessary synchronisation.  I'll see about adding a flag now.
> >> In addition to the places it's called now it would need to be called
> >> whenever an inode gains an ACL.
> >
> >Ok done should just need to mark the cached ID as invalid when the inode has
> >/ gains an ACL.
> 
> Ok, I made some changes using this in my latest acl patch. Can you please
> check?  https://www.netbsd.org/~christos/acl.diff

One problem with the VV_HASACLS thing is that VOP_ACCESS() is called with an
LK_SHARED lock and you can't modify v_vflag with that.

It looks like you have to explicitly enable ACLs before the file system is
mounted with tunefs, and it's not the default?  In that's true then if either
FS_ACLS or FS_POSIX1EACLS is true during mount you could instead not set
IMNT_NCLOOKUP, which would disable the lookup-in-namecache fastpath and
everything would go through VOP_ACCESS() instead.  It's kind of a big hammer
but at least everything is always correct then.  If not I'll dig into it in
more detail tomorrow morning.

Cheers,
Andrew