Re: [PATCH 06/29] 9p: implement get acl method

[Al Viro <viro@xxxxxxxxxxxxxxxxxx>]

On Thu, Sep 22, 2022 at 05:17:04PM +0200, Christian Brauner wrote:

> -static struct posix_acl *__v9fs_get_acl(struct p9_fid *fid, char *name)
> +static int v9fs_fid_get_acl(struct p9_fid *fid, const char *name,
> +			    struct posix_acl **kacl)
>  {
>  	ssize_t size;
>  	void *value = NULL;
>  	struct posix_acl *acl = NULL;
>  
>  	size = v9fs_fid_xattr_get(fid, name, NULL, 0);
> -	if (size > 0) {
> -		value = kzalloc(size, GFP_NOFS);
> -		if (!value)
> -			return ERR_PTR(-ENOMEM);
> -		size = v9fs_fid_xattr_get(fid, name, value, size);
> -		if (size > 0) {
> -			acl = posix_acl_from_xattr(&init_user_ns, value, size);
> -			if (IS_ERR(acl))
> -				goto err_out;
> -		}
> -	} else if (size == -ENODATA || size == 0 ||
> -		   size == -ENOSYS || size == -EOPNOTSUPP) {
> -		acl = NULL;
> -	} else
> -		acl = ERR_PTR(-EIO);
> +	if (size <= 0)
> +		goto out;
>  
> -err_out:
> +	/* just return the size */
> +	if (!kacl)
> +		goto out;

How can that happen?  Both callers are passing addresses of local variables
as the third argument.  And what's the point of that kacl thing, anyway?
Same callers would be much happier if you returned acl or ERR_PTR()...

> +	value = kzalloc(size, GFP_NOFS);
> +	if (!value) {
> +		size = -ENOMEM;
> +		goto out;
> +	}
> +
> +	size = v9fs_fid_xattr_get(fid, name, value, size);
> +	if (size <= 0)
> +		goto out;
> +
> +	acl = posix_acl_from_xattr(&init_user_ns, value, size);
> +	if (IS_ERR(acl)) {
> +		size = PTR_ERR(acl);
> +		goto out;
> +	}
> +	*kacl = acl;
> +
> +out:
>  	kfree(value);
> +	return size;
> +}

Compare that (and callers of that helper) with

static struct posix_acl *v9fs_fid_get_acl(struct p9_fid *fid, const char *name)
{
	ssize_t size;
	void *value;
	struct posix_acl *acl;

	size = v9fs_fid_xattr_get(fid, name, NULL, 0);
	if (size <= 0)
		return ERR_PTR(size);

	value = kzalloc(size, GFP_NOFS);
	if (!value)
		return ERR_PTR(-ENOMEM);

	size = v9fs_fid_xattr_get(fid, name, value, size);
	if (size > 0)
		acl = posix_acl_from_xattr(&init_user_ns, value, size);
	else
		acl = ERR_PTR(size);
	kfree(value);
	return acl;
}

static struct posix_acl *v9fs_acl_get(struct dentry *dentry, const char *name)
{
	struct p9_fid *fid;
	struct posix_acl *acl;

	fid = v9fs_fid_lookup(dentry);
	if (IS_ERR(fid))
		return ERR_CAST(fid);

	acl = v9fs_fid_get_acl(fid, name);
	p9_fid_put(fid);
  	return acl;
}
  
static struct posix_acl *__v9fs_get_acl(struct p9_fid *fid, const char *name)
{
	struct posix_acl *acl = v9fs_fid_get_acl(fid, name);

	if (IS_ERR(acl)) {
		if (acl == ERR_PTR(-ENODATA) || acl == ERR_PTR(-ENOSYS) ||
		    acl == ERR_PTR(-EOPNOTSUPP))
			acl = NULL;
		else
			acl = ERR_PTR(-EIO);
	}
	return acl;
}