Re: [PATCH v3 14/29] acl: add vfs_set_acl()

From: Christian Brauner <brauner@kernel.org>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-fsdevel@vger.kernel.org, Seth Forshee <sforshee@kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Mimi Zohar <zohar@linux.ibm.com>,
	linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org
Subject: Re: [PATCH v3 14/29] acl: add vfs_set_acl()
Date: Thu, 29 Sep 2022 10:25:54 +0200	[thread overview]
Message-ID: <20220929082554.5rclj4ioo37qg254@wittgenstein> (raw)
In-Reply-To: <20220929081727.GB3699@lst.de>

On Thu, Sep 29, 2022 at 10:17:27AM +0200, Christoph Hellwig wrote:
> > +EXPORT_SYMBOL(vfs_set_acl);
> 
> I think all this stackable file system infrastucture should be
> EXPORT_SYMBOL_GPL, like a lot of the other internal stuff.

Ok, sounds good.

> 
> > +int xattr_permission(struct user_namespace *mnt_userns, struct inode *inode,
> > +		     const char *name, int mask)
> 
> Hmm.  The only think ACLs actually need from xattr_permission are
> the immutable / append check and the HAS_UNMAPPED_ID one.  I'd rather
> open code that, or if you cane come up with a sane name do a smaller
> helper rather than doing all the strcmp on the prefixes for now
> good reason.

I'll see if a little helper makes more sense than open-coding.

> 
> > +static inline int vfs_set_acl(struct user_namespace *mnt_userns,
> > +			      struct dentry *dentry, const char *name,
> > +			      struct posix_acl *acl)
> > +{
> > +	return 0;
> 
> Should this really return 0 if ACLs are not supported?

Yeah, we should probably -EOPNOTSUPP for all of:
vfs_{get,set,remove}_acl() in this case. Good point, thanks!