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

[Miklos Szeredi]

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.

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.

Thanks,
Miklos