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@xxxxxxxxxx>
> ---
> 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 from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
