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

From: Christian Brauner <brauner@kernel.org>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: linux-fsdevel@vger.kernel.org, Seth Forshee <sforshee@kernel.org>,
	Christoph Hellwig <hch@lst.de>, Al Viro <viro@zeniv.linux.org.uk>,
	linux-security-module@vger.kernel.org,
	Steve French <sfrench@samba.org>
Subject: Re: [PATCH v4 04/30] fs: add new get acl method
Date: Fri, 30 Sep 2022 12:05:57 +0200	[thread overview]
Message-ID: <20220930100557.7hqjrz77s3wcbrxx@wittgenstein> (raw)
In-Reply-To: <CAJfpegsu9r84J-3wN=z8OOzHd+7YRBn9CNFMDWSbftCEm0e27A@mail.gmail.com>

On Fri, Sep 30, 2022 at 11:43:07AM +0200, Miklos Szeredi wrote:
> On Fri, 30 Sept 2022 at 11:09, Christian Brauner <brauner@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@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.