Re: [PATCH 3/9] xfs: increase number of ACL entries for V5 superblocks

Brian Foster
2013-05-29T16:43:43Z

On 05/27/2013 02:38 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> The limit of 25 ACL entries is arbitrary, but baked into the on-disk
> format. For version 5 superblocks, increase it to the maximum nuber
> of ACLs that can fit into a single xattr.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> fs/xfs/xfs_acl.c | 31 ++++++++++++++++++-------------
> fs/xfs/xfs_acl.h | 30 +++++++++++++++++++++++-------
> 2 files changed, 41 insertions(+), 20 deletions(-)
>
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index 1d32f1d..b6bed9b 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -21,6 +21,8 @@
> #include "xfs_bmap_btree.h"
> #include "xfs_inode.h"
> #include "xfs_vnodeops.h"
> +#include "xfs_sb.h"
> +#include "xfs_mount.h"
> #include "xfs_trace.h"
> #include <linux/slab.h>
> #include <linux/xattr.h>
> @@ -34,7 +36,9 @@
> */
>
> STATIC struct posix_acl *
> -xfs_acl_from_disk(struct xfs_acl *aclp)
> +xfs_acl_from_disk(
> + struct xfs_acl *aclp,
> + int max_entries)
> {
> struct posix_acl_entry *acl_e;
> struct posix_acl *acl;
> @@ -42,7 +46,7 @@ xfs_acl_from_disk(struct xfs_acl *aclp)
> unsigned int count, i;
>
> count = be32_to_cpu(aclp->acl_cnt);
> - if (count > XFS_ACL_MAX_ENTRIES)
> + if (count > max_entries)
> return ERR_PTR(-EFSCORRUPTED);
>
> acl = posix_acl_alloc(count, GFP_KERNEL);
> @@ -108,7 +112,7 @@ xfs_get_acl(struct inode *inode, int type)
> struct xfs_inode *ip = XFS_I(inode);
> struct posix_acl *acl;
> struct xfs_acl *xfs_acl;
> - int len = sizeof(struct xfs_acl);
> + int len = XFS_ACL_SIZE(ip->i_mount);
> unsigned char *ea_name;
> int error;
>
> @@ -133,8 +137,8 @@ xfs_get_acl(struct inode *inode, int type)
> * If we have a cached ACLs value just return it, not need to
> * go out to the disk.
> */
> -
> - xfs_acl = kzalloc(sizeof(struct xfs_acl), GFP_KERNEL);
> + len = XFS_ACL_SIZE(ip->i_mount);

Duplicate initialization of len. Harmless (perhaps Ben can drop it) and
the parens in XFS_ACL_MAX_ENTRIES() are fixed, so:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> + xfs_acl = kzalloc(len, GFP_KERNEL);
> if (!xfs_acl)
> return ERR_PTR(-ENOMEM);
>
> @@ -153,7 +157,7 @@ xfs_get_acl(struct inode *inode, int type)
> goto out;
> }
>
> - acl = xfs_acl_from_disk(xfs_acl);
> + acl = xfs_acl_from_disk(xfs_acl, XFS_ACL_MAX_ENTRIES(ip->i_mount));
> if (IS_ERR(acl))
> goto out;
>
> @@ -189,16 +193,17 @@ xfs_set_acl(struct inode *inode, int type, struct posix_acl *acl)
>
> if (acl) {
> struct xfs_acl *xfs_acl;
> - int len;
> + int len = XFS_ACL_SIZE(ip->i_mount);
>
> - xfs_acl = kzalloc(sizeof(struct xfs_acl), GFP_KERNEL);
> + xfs_acl = kzalloc(len, GFP_KERNEL);
> if (!xfs_acl)
> return -ENOMEM;
>
> xfs_acl_to_disk(xfs_acl, acl);
> - len = sizeof(struct xfs_acl) -
> - (sizeof(struct xfs_acl_entry) *
> - (XFS_ACL_MAX_ENTRIES - acl->a_count));
> +
> + /* subtract away the unused acl entries */
> + len -= sizeof(struct xfs_acl_entry) *
> + (XFS_ACL_MAX_ENTRIES(ip->i_mount) - acl->a_count);
>
> error = -xfs_attr_set(ip, ea_name, (unsigned char *)xfs_acl,
> len, ATTR_ROOT);
> @@ -243,7 +248,7 @@ xfs_set_mode(struct inode *inode, umode_t mode)
> static int
> xfs_acl_exists(struct inode *inode, unsigned char *name)
> {
> - int len = sizeof(struct xfs_acl);
> + int len = XFS_ACL_SIZE(XFS_M(inode->i_sb));
>
> return (xfs_attr_get(XFS_I(inode), name, NULL, &len,
> ATTR_ROOT|ATTR_KERNOVAL) == 0);
> @@ -379,7 +384,7 @@ xfs_xattr_acl_set(struct dentry *dentry, const char *name,
> goto out_release;
>
> error = -EINVAL;
> - if (acl->a_count > XFS_ACL_MAX_ENTRIES)
> + if (acl->a_count > XFS_ACL_MAX_ENTRIES(XFS_M(inode->i_sb)))
> goto out_release;
>
> if (type == ACL_TYPE_ACCESS) {
> diff --git a/fs/xfs/xfs_acl.h b/fs/xfs/xfs_acl.h
> index 39632d9..0da8725 100644
> --- a/fs/xfs/xfs_acl.h
> +++ b/fs/xfs/xfs_acl.h
> @@ -22,19 +22,35 @@ struct inode;
> struct posix_acl;
> struct xfs_inode;
>
> -#define XFS_ACL_MAX_ENTRIES 25
> #define XFS_ACL_NOT_PRESENT (-1)
>
> /* On-disk XFS access control list structure */
> +struct xfs_acl_entry {
> + __be32 ae_tag;
> + __be32 ae_id;
> + __be16 ae_perm;
> + __be16 ae_pad; /* fill the implicit hole in the structure */
> +};
> +
> struct xfs_acl {
> - __be32 acl_cnt;
> - struct xfs_acl_entry {
> - __be32 ae_tag;
> - __be32 ae_id;
> - __be16 ae_perm;
> - } acl_entry[XFS_ACL_MAX_ENTRIES];
> + __be32 acl_cnt;
> + struct xfs_acl_entry acl_entry[0];
> };
>
> +/*
> + * The number of ACL entries allowed is defined by the on-disk format.
> + * For v4 superblocks, that is limited to 25 entries. For v5 superblocks, it is
> + * limited only by the maximum size of the xattr that stores the information.
> + */
> +#define XFS_ACL_MAX_ENTRIES(mp) \
> + (xfs_sb_version_hascrc(&mp->m_sb) \
> + ? (XATTR_SIZE_MAX - sizeof(__be32)) / sizeof(struct xfs_acl_entry) \
> + : 25)
> +
> +#define XFS_ACL_SIZE(mp) \
> + (sizeof(struct xfs_acl) + \
> + sizeof(struct xfs_acl_entry) * XFS_ACL_MAX_ENTRIES((mp)))
> +
> /* On-disk XFS extended attribute names */
> #define SGI_ACL_FILE (unsigned char *)"SGI_ACL_FILE"
> #define SGI_ACL_DEFAULT (unsigned char *)"SGI_ACL_DEFAULT"