[PATCH v4 04/30] fs: add new get acl method

[Christian Brauner]

On Fri, Sep 30, 2022 at 02:24:33PM +0200, Miklos Szeredi wrote:
> On Fri, 30 Sept 2022 at 12:06, Christian Brauner <brauner at kernel.org> wrote:
> >
> > On Fri, Sep 30, 2022 at 11:43:07AM +0200, Miklos Szeredi wrote:
> > > On Fri, 30 Sept 2022 at 11:09, Christian Brauner <brauner at kernel.org> wrote:
> > > >
> > > > On Fri, Sep 30, 2022 at 10:53:05AM +0200, Miklos Szeredi wrote:
> > > > > On Thu, 29 Sept 2022 at 17:31, Christian Brauner <brauner at kernel.org> wrote:
> > > > >
> > > > > > This adds a new ->get_acl() inode operations which takes a dentry
> > > > > > argument which filesystems such as 9p, cifs, and overlayfs can implement
> > > > > > to get posix acls.
> > > > >
> > > > > This is confusing.   For example overlayfs ends up with two functions
> > > > > that are similar, but not quite the same:
> > > > >
> > > > >  ovl_get_acl -> ovl_get_acl_path -> vfs_get_acl -> __get_acl(mnt_userns, ...)
> > > > >
> > > > >  ovl_get_inode_acl -> get_inode_acl -> __get_acl(&init_user_ns, ...)
> > > > >
> > > > > So what's the difference and why do we need both?  If one can retrive
> > > > > the acl without dentry, then why do we need the one with the dentry?
> > > >
> > > > The ->get_inode_acl() method is called during generic_permission() and
> > > > inode_permission() both of which are called from various filesystems in
> > > > their ->permission inode operations. There's no dentry available during
> > > > the permission inode operation and there are filesystems like 9p and
> > > > cifs that need a dentry.
> > >
> > > This doesn't answer the question about why we need two for overlayfs
> > > and what's the difference between them.
> >
> > Oh sorry, I misunderstood your questions then. The reason why I didn't
> > consolidate them was simply the different in permission checking.
> > So currently in current mainline overlayfs does acl = get_acl() in it's
> > get acl method and does vfs_getxattr() in ovl_posix_acl_xattr_get().
> >
> > The difference is that vfs_getxattr() goes through regular lsm hooks
> > checking whereas get_acl() does not. So I thought that using get_acl()
> > was done to not call lsm hooks in there. If that's not the case then I
> > can consolidate both into one implementation.
> 
> So there are two paths to getting an acl: 1) permission checking and
> 2) retrieving the value via getxattr(2).
> 
> This is a similar situation as reading a symlink vs. following it.
> When following a symlink overlayfs always reads the link on the
> underlying fs just as if it was a readlink(2) call, calling
> security_inode_readlink() instead of security_inode_follow_link().
> This is logical: we are reading the link from the underlying storage,
> and following it on overlayfs.
> 
> Applying the same logic to acl: we do need to call the
> security_inode_getxattr() on the underlying fs, even if just want to
> check permissions on overlay.  This is currently not done, which is an
> inconsistency.
> 
> Maybe adding the check to ovl_get_acl() is the right way to go, but
> I'm a little afraid of a performance regression.  Will look into that.

Ok, sounds good. I can probably consolidate the two versions but retain
the difference in permission checking or would you prefer I leave them
distinct for now?

> 
> So this patchset nicely reveals how acl retrieval could be done two
> ways, and how overlayfs employed both for different purposes.  But
> what would be even nicer if there was just one way to retrieve the acl
> and overlayfs and cifs be moved over to that.

I think this is a good long term goal to have. We're certainly not done
with improving things after this work. Sometimes it just takes a little
time to phase out legacy baggage as we all are well aware.