Re: [PATCH 3/6] acl: add vfs_set_acl_prepare()

[Christian Brauner <brauner@xxxxxxxxxx>]

On Tue, Sep 06, 2022 at 06:57:46AM +0200, Christoph Hellwig wrote:
> On Mon, Aug 29, 2022 at 02:38:42PM +0200, Christian Brauner wrote:
> > Various filesystems store POSIX ACLs on the backing store in their uapi
> > format. Such filesystems need to translate from the uapi POSIX ACL
> > format into the VFS format during i_op->get_acl(). The VFS provides the
> > posix_acl_from_xattr() helper for this task.
> 
> This has always been rather confusing.  Maybe we should add a separate

Absolutely and it's pretty unsafe given that we're storing k{g,u}id_t in
the uapi struct in the form of {g,u}id_t which we then recover later on.
But I've documented this as best as I could in the helpers.

> structure type for the on-disk vs uapi ACL formats?  They will be the

We do already have separate format for uapi and VFS ACLs. I'm not sure
if you're suggesting another intermediate format.

I'm currently working on a larger series to get rid of the uapi struct
abuse for POSIX ACLs. Building on that work Seth will get rid of similar
abuses for VFS caps. I'm fairly close but the rough idea is:

struct xattr_args {
	const char *name;
	union {
		struct posix_acl *kacl;
		const void *kvalue;
		void *buffer;
	};
	size_t size;
};

struct xattr_handler {
	const char *name;
	const char *prefix;
	int flags;
	bool (*list)(struct dentry *dentry);
	int (*get)(const struct xattr_handler *,
		   struct dentry *dentry,
		   struct inode *inode,
		   struct xattr_args *args);
	int (*set)(const struct xattr_handler *,
		   struct user_namespace *mnt_userns,
		   struct dentry *dentry,
		   struct inode *inode,
		   const struct xattr_args *args,
		   int flags);
};

All __vfs_{g,s}etxattr() helpers stay the same and can't be used to
{g,s}et POSIX ACLs anymore instead we add:

int vfs_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
		const char *acl_name, struct posix_acl *acl, int flags)
{
	struct xattr_args xattr_args = {
		.name = acl_name,
	};

	if (!is_posix_acl_xattr(acl_name))
		return -EINVAL;

	return set_xattr_args(mnt_userns, dentry, &xattr_args, flags);
}

int vfs_get_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
		const char *acl_name, struct posix_acl **acl)
{
	int error;
	struct xattr_args xattr_args = {
		.name = acl_name,
	};

	if (!is_posix_acl_xattr(acl_name))
		return -EINVAL;

	error = get_xattr_args(mnt_userns, dentry, &xattr_args);
	if (error < 0)
		return error;

	*acl = xattr_args.kacl;
	return 0;
}

These two vfs helpers can then used by filesystems like overlayfs to set
POSIX ACLs. This gets rid of passing crucial data that the VFS needs to
interpret around in a void * blob as that's causing a lot of issues
currently bc often filesystems or security hooks don't have any idea how
to interpret them correctly.

So the internal vfs api for getxattr() itself would then be:

ssize_t do_get_acl(struct user_namespace *mnt_userns, struct dentry *d,
		   struct xattr_ctx *ctx)
{
	struct posix_acl *kacl = NULL;

	error = vfs_get_acl(mnt_userns, d, ctx->kname->name, &kacl);
	if (error)
		return error;

	/* convert to uapi format */
	if (ctx->size)
		error = vfs_posix_acl_to_xattr(mnt_userns, d_inode(d),
					       ctx->kacl, ctx->kvalue,
					       ctx->size);
	posix_acl_release(kacl);
	return error;
}

ssize_t
do_getxattr(struct user_namespace *mnt_userns, struct dentry *d,
	struct xattr_ctx *ctx)
{
	ssize_t error;
	char *kname = ctx->kname->name;

	if (ctx->size) {
		if (ctx->size > XATTR_SIZE_MAX)
			ctx->size = XATTR_SIZE_MAX;
		ctx->kvalue = kvzalloc(ctx->size, GFP_KERNEL);
		if (!ctx->kvalue)
			return -ENOMEM;
	}

	if (is_posix_acl_xattr(ctx->kname->name))
		error = do_get_acl(mnt_userns, d, ctx);
	else
		error = vfs_getxattr(mnt_userns, d, kname, ctx->kvalue, ctx->size);
	if (error > 0) {
		if (ctx->size && copy_to_user(ctx->value, ctx->kvalue, error))
			error = -EFAULT;
	} else if (error == -ERANGE && ctx->size >= XATTR_SIZE_MAX) {
		/* The file system tried to returned a value bigger
		   than XATTR_SIZE_MAX bytes. Not possible. */
		error = -E2BIG;
	}

	return error;
}

and all the helpers that hack stuff into uapi POSIX ACLs are then gone.
I'm fairly along but I'm happy to hear alternative ideas.

Christian