On Thu, May 14, 2020 at 08:16:50PM -0000, Christos Zoulas wrote: > In article <20200512235040.gd9...@homeworld.netbsd.org>, > Andrew Doran <a...@netbsd.org> 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
