Re: [PATCH] tmpfs: listxattr should include POSIX ACL xattrs

Re: [PATCH] tmpfs: listxattr should include POSIX ACL xattrs

[Hugh Dickins]

On Tue, 3 Nov 2015, Andreas Gruenbacher wrote:

> When a file on tmpfs has an ACL or a Default ACL, listxattr should include the
> corresponding xattr names.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  fs/kernfs/inode.c     |  2 +-
>  fs/xattr.c            | 53 +++++++++++++++++++++++++++++++++++----------------
>  include/linux/xattr.h |  2 +-
>  mm/shmem.c            |  2 +-
>  4 files changed, 40 insertions(+), 19 deletions(-)

Hmm, can you make a stronger argument for this patch than above?

My ignorance of ACLs and XATTRs is boundless, I'll have to defer to
you and others.  But when I read the listxattr(2) manpage saying
"Filesystems like ext2, ext3 and XFS which implement POSIX ACLs
using extended attributes, might return a list like ...",
I don't see that as mandating that any filesystem which happens
for its own internal convenience to implement ACLs via XATTRs,
has to list the ACLs with the XATTRs - I read it rather as an
apology that some of them (for their own simplicity) do so.

If this patch simplified the code, I'd be all for it;
but it's the reverse, and we seem to have survived for several
years without it: I don't see yet why it's needed.  I've no
fundamental objection, but I'd like to understand why it's
a step forwards rather than a step backwards.

Thanks,
Hugh

> 
> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index 756dd56..3c415bf 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -230,7 +230,7 @@ ssize_t kernfs_iop_listxattr(struct dentry *dentry, char *buf, size_t size)
>  	if (!attrs)
>  		return -ENOMEM;
>  
> -	return simple_xattr_list(&attrs->xattrs, buf, size);
> +	return simple_xattr_list(d_inode(dentry), &attrs->xattrs, buf, size);
>  }
>  
>  static inline void set_default_inode_attr(struct inode *inode, umode_t mode)
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 072fee1..7035d7d 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -926,38 +926,59 @@ static bool xattr_is_trusted(const char *name)
>  	return !strncmp(name, XATTR_TRUSTED_PREFIX, XATTR_TRUSTED_PREFIX_LEN);
>  }
>  
> +static int xattr_list_one(char **buffer, ssize_t *remaining_size,
> +			  const char *name)
> +{
> +	size_t len = strlen(name) + 1;
> +	if (*buffer) {
> +		if (*remaining_size < len)
> +			return -ERANGE;
> +		memcpy(*buffer, name, len);
> +		*buffer += len;
> +	}
> +	*remaining_size -= len;
> +	return 0;
> +}
> +
>  /*
>   * xattr LIST operation for in-memory/pseudo filesystems
>   */
> -ssize_t simple_xattr_list(struct simple_xattrs *xattrs, char *buffer,
> -			  size_t size)
> +ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs,
> +			  char *buffer, size_t size)
>  {
>  	bool trusted = capable(CAP_SYS_ADMIN);
>  	struct simple_xattr *xattr;
> -	size_t used = 0;
> +	ssize_t remaining_size = size;
> +	int err;
> +
> +#ifdef CONFIG_FS_POSIX_ACL
> +	if (inode->i_acl) {
> +		err = xattr_list_one(&buffer, &remaining_size,
> +				     XATTR_NAME_POSIX_ACL_ACCESS);
> +		if (err)
> +			return err;
> +	}
> +	if (inode->i_default_acl) {
> +		err = xattr_list_one(&buffer, &remaining_size,
> +				     XATTR_NAME_POSIX_ACL_DEFAULT);
> +		if (err)
> +			return err;
> +	}
> +#endif
>  
>  	spin_lock(&xattrs->lock);
>  	list_for_each_entry(xattr, &xattrs->head, list) {
> -		size_t len;
> -
>  		/* skip "trusted." attributes for unprivileged callers */
>  		if (!trusted && xattr_is_trusted(xattr->name))
>  			continue;
>  
> -		len = strlen(xattr->name) + 1;
> -		used += len;
> -		if (buffer) {
> -			if (size < used) {
> -				used = -ERANGE;
> -				break;
> -			}
> -			memcpy(buffer, xattr->name, len);
> -			buffer += len;
> -		}
> +		err = xattr_list_one(&buffer, &remaining_size, xattr->name);
> +		if (err)
> +			return err;
>  	}
>  	spin_unlock(&xattrs->lock);
>  
> -	return used;
> +	return size - remaining_size;
>  }
>  
>  /*
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index 91b0a68..b57aed5 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -92,7 +92,7 @@ int simple_xattr_get(struct simple_xattrs *xattrs, const char *name,
>  int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
>  		     const void *value, size_t size, int flags);
>  int simple_xattr_remove(struct simple_xattrs *xattrs, const char *name);
> -ssize_t simple_xattr_list(struct simple_xattrs *xattrs, char *buffer,
> +ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs, char *buffer,
>  			  size_t size);
>  void simple_xattr_list_add(struct simple_xattrs *xattrs,
>  			   struct simple_xattr *new_xattr);
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 48ce829..3d95547 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2645,7 +2645,7 @@ static int shmem_removexattr(struct dentry *dentry, const char *name)
>  static ssize_t shmem_listxattr(struct dentry *dentry, char *buffer, size_t size)
>  {
>  	struct shmem_inode_info *info = SHMEM_I(d_inode(dentry));
> -	return simple_xattr_list(&info->xattrs, buffer, size);
> +	return simple_xattr_list(d_inode(dentry), &info->xattrs, buffer, size);
>  }
>  #endif /* CONFIG_TMPFS_XATTR */
>  
> -- 
> 2.5.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] tmpfs: listxattr should include POSIX ACL xattrs

[Andreas Grünbacher]

2015-11-09 0:37 GMT+01:00 Hugh Dickins <hughd@google.com>:
> On Tue, 3 Nov 2015, Andreas Gruenbacher wrote:
>
>> When a file on tmpfs has an ACL or a Default ACL, listxattr should include the
>> corresponding xattr names.
>>
>> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
>> ---
>>  fs/kernfs/inode.c     |  2 +-
>>  fs/xattr.c            | 53 +++++++++++++++++++++++++++++++++++----------------
>>  include/linux/xattr.h |  2 +-
>>  mm/shmem.c            |  2 +-
>>  4 files changed, 40 insertions(+), 19 deletions(-)
>
> Hmm, can you make a stronger argument for this patch than above?
>
> My ignorance of ACLs and XATTRs is boundless, I'll have to defer to
> you and others.

That shouldn't be a problem.

> But when I read the listxattr(2) manpage saying
> "Filesystems like ext2, ext3 and XFS which implement POSIX ACLs
> using extended attributes, might return a list like ...",
> I don't see that as mandating that any filesystem which happens
> for its own internal convenience to implement ACLs via XATTRs,
> has to list the ACLs with the XATTRs - I read it rather as an
> apology that some of them (for their own simplicity) do so.

The user-space interface for POSIX ACLs is always the same no matter
how they are implemented in the kernel: they are exposed as two
extended attributes called "system.posix_acl_access" (directories and
non-directories) and "system.posix_acl_default" (diectories only). The
extended attribute syscalls are [fl]getxattr, [fl]setxattr,
[fl]listxattr, and [fl]removexattr.

The listxattr syscalls is expected to list all the attributes which
the calling process is allowed to see / access. Tmpfs misbehaves in
this regard; it doesn't list the extended attributes for POSIX ACLs
even when they are set.

This example on the listxattr manpage is rather unfortunate and should
be fixed; how filesystems implement stuff internally should really be
of no relevance here. I'm taking Michael into the CC.

> If this patch simplified the code, I'd be all for it;
> but it's the reverse, and we seem to have survived for several
> years without it: I don't see yet why it's needed.

It's a confusing inconsistency. Many utilities like ls and
getfacl/setfacl check for the presence of POSIX ACLs with getxattr,
they don't use listxattr for that. Other utilities that rely on
listxattr currently won't see when objects on tmpfs have POSIX ACLs
though; this is bad.

> I've no
> fundamental objection, but I'd like to understand why it's
> a step forwards rather than a step backwards.

Yes, I hope I could explain that well enough.

Thanks,
Andreas