Re: [PATCH 1/2] repair: support more than 25 ACLs

[Brian Foster <bfoster@redhat.com>]

On Thu, Jun 19, 2014 at 03:33:51PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> v5 superblock supports many more than 25 ACLs on an inode, but
> xfs_repair still thinks that the maximum is 25. Fix it and update
> the ACL definitions to match the kernel definitions. Also fix the
> remote attr maximum size off-by-one that the maximum number of v5
> ACLs tickles.
> 
> Reported-by: Michael L. Semon <mlsemon35@gmail.com>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

This mostly looks good to me, though it seems like it could at least
split into a couple patches. A minor question below...

>  libxfs/xfs_attr_remote.c |  2 +-
>  repair/attr_repair.c     | 74 ++++++++++++++++++++++++++++++++----------------
>  repair/attr_repair.h     | 46 +++++++++++++++++++++---------
>  3 files changed, 84 insertions(+), 38 deletions(-)
> 
> diff --git a/libxfs/xfs_attr_remote.c b/libxfs/xfs_attr_remote.c
> index 5cf5c73..08b983b 100644
> --- a/libxfs/xfs_attr_remote.c
> +++ b/libxfs/xfs_attr_remote.c
> @@ -85,7 +85,7 @@ xfs_attr3_rmt_verify(
>  	if (be32_to_cpu(rmt->rm_bytes) > fsbsize - sizeof(*rmt))
>  		return false;
>  	if (be32_to_cpu(rmt->rm_offset) +
> -				be32_to_cpu(rmt->rm_bytes) >= XATTR_SIZE_MAX)
> +				be32_to_cpu(rmt->rm_bytes) > XATTR_SIZE_MAX)

Corresponds to kernel commit:

bba719b5 xfs: fix off-by-one error in xfs_attr3_rmt_verify

>  		return false;
>  	if (rmt->rm_owner == 0)
>  		return false;
> diff --git a/repair/attr_repair.c b/repair/attr_repair.c
> index 5dd7e5f..87d3b0a 100644
> --- a/repair/attr_repair.c
> +++ b/repair/attr_repair.c
> @@ -25,7 +25,7 @@
>  #include "protos.h"
>  #include "dir2.h"
>  
> -static int xfs_acl_valid(xfs_acl_disk_t *daclp);
> +static int xfs_acl_valid(struct xfs_mount *mp, struct xfs_acl *daclp);
>  static int xfs_mac_valid(xfs_mac_label_t *lp);
>  
>  /*
> @@ -734,11 +734,15 @@ verify_da_path(xfs_mount_t	*mp,
>   * If value is non-zero, then a remote attribute is being passed in
>   */
>  static int
> -valuecheck(char *namevalue, char *value, int namelen, int valuelen)
> +valuecheck(
> +	struct xfs_mount *mp,
> +	char		*namevalue,
> +	char		*value,
> +	int		namelen,
> +	int		valuelen)
>  {
>  	/* for proper alignment issues, get the structs and memmove the values */
>  	xfs_mac_label_t macl;
> -	xfs_acl_t thisacl;
>  	void *valuep;
>  	int clearit = 0;
>  
> @@ -746,18 +750,23 @@ valuecheck(char *namevalue, char *value, int namelen, int valuelen)
>  			(strncmp(namevalue, SGI_ACL_DEFAULT,
>  				SGI_ACL_DEFAULT_SIZE) == 0)) {
>  		if (value == NULL) {
> -			memset(&thisacl, 0, sizeof(xfs_acl_t));
> -			memmove(&thisacl, namevalue+namelen, valuelen);
> -			valuep = &thisacl;
> +			valuep = malloc(valuelen);
> +			if (!valuep)
> +				do_error(_("No memory for ACL check!\n"));
> +			memcpy(valuep, namevalue + namelen, valuelen);
>  		} else
>  			valuep = value;
>  
> -		if (xfs_acl_valid((xfs_acl_disk_t *)valuep) != 0) {
> +		if (xfs_acl_valid(mp, valuep) != 0) {
>  			clearit = 1;
>  			do_warn(
>  	_("entry contains illegal value in attribute named SGI_ACL_FILE "
>  	  "or SGI_ACL_DEFAULT\n"));
>  		}
> +
> +		if (valuep != value)
> +			free(valuep);
> +
>  	} else if (strncmp(namevalue, SGI_MAC_FILE, SGI_MAC_FILE_SIZE) == 0) {
>  		if (value == NULL) {
>  			memset(&macl, 0, sizeof(xfs_mac_label_t));
> @@ -800,6 +809,7 @@ valuecheck(char *namevalue, char *value, int namelen, int valuelen)
>   */
>  static int
>  process_shortform_attr(
> +	struct xfs_mount *mp,
>  	xfs_ino_t	ino,
>  	xfs_dinode_t	*dip,
>  	int		*repair)
> @@ -904,7 +914,7 @@ process_shortform_attr(
>  
>  		/* Only check values for root security attributes */
>  		if (currententry->flags & XFS_ATTR_ROOT)
> -		       junkit = valuecheck((char *)&currententry->nameval[0],
> +		       junkit = valuecheck(mp, (char *)&currententry->nameval[0],
>  					NULL, currententry->namelen, 
>  					currententry->valuelen);
>  
> @@ -1039,6 +1049,7 @@ rmtval_get(xfs_mount_t *mp, xfs_ino_t ino, blkmap_t *blkmap,
>  
>  static int
>  process_leaf_attr_local(
> +	struct xfs_mount	*mp,
>  	xfs_attr_leafblock_t	*leaf,
>  	int			i,
>  	xfs_attr_leaf_entry_t	*entry,
> @@ -1076,7 +1087,7 @@ process_leaf_attr_local(
>  
>  	/* Only check values for root security attributes */
>  	if (entry->flags & XFS_ATTR_ROOT) {
> -		if (valuecheck((char *)&local->nameval[0], NULL, 
> +		if (valuecheck(mp, (char *)&local->nameval[0], NULL, 
>  				local->namelen, be16_to_cpu(local->valuelen))) {
>  			do_warn(
>  	_("bad security value for attribute entry %d in attr block %u, inode %" PRIu64 "\n"),
> @@ -1134,7 +1145,7 @@ process_leaf_attr_remote(
>  			i, ino);
>  		goto bad_free_out;
>  	}
> -	if (valuecheck((char *)&remotep->name[0], value, remotep->namelen,
> +	if (valuecheck(mp, (char *)&remotep->name[0], value, remotep->namelen,
>  				be32_to_cpu(remotep->valuelen))) {
>  		do_warn(
>  	_("remote attribute value check failed for entry %d, inode %" PRIu64 "\n"),
> @@ -1216,15 +1227,15 @@ process_leaf_attr_block(
>  			break;	/* got an overlap */
>  		}
>  
> -		if (entry->flags & XFS_ATTR_LOCAL) 
> -			thissize = process_leaf_attr_local(leaf, i, entry,
> +		if (entry->flags & XFS_ATTR_LOCAL)
> +			thissize = process_leaf_attr_local(mp, leaf, i, entry,
>  						last_hashval, da_bno, ino);
>  		else
>  			thissize = process_leaf_attr_remote(leaf, i, entry,
>  						last_hashval, da_bno, ino,
>  						mp, blkmap);
>  		if (thissize < 0) {
> -			clearit = 1;				
> +			clearit = 1;
>  			break;
>  		}
>  
> @@ -1608,15 +1619,19 @@ process_longform_attr(
>  
>  
>  static int
> -xfs_acl_from_disk(struct xfs_acl **aclp, struct xfs_acl_disk *dacl)
> +xfs_acl_from_disk(
> +	struct xfs_mount	*mp,
> +	struct xfs_icacl	**aclp,
> +	struct xfs_acl		*dacl)
>  {
>  	int			count;
> -	xfs_acl_t		*acl;
> -	xfs_acl_entry_t 	*ace;
> -	xfs_acl_entry_disk_t	*dace, *end;
> +	int			size;
> +	struct xfs_icacl	*acl;
> +	struct xfs_icacl_entry	*ace;
> +	struct xfs_acl_entry	*dace, *end;
>  
>  	count = be32_to_cpu(dacl->acl_cnt);
> -	if (count > XFS_ACL_MAX_ENTRIES) {
> +	if (count > XFS_ACL_MAX_ENTRIES(mp)) {
>  		do_warn(_("Too many ACL entries, count %d\n"), count);
>  		*aclp = NULL;
>  		return EINVAL;
> @@ -1624,7 +1639,16 @@ xfs_acl_from_disk(struct xfs_acl **aclp, struct xfs_acl_disk *dacl)
>  
>  
>  	end = &dacl->acl_entry[0] + count;
> -	acl = malloc((int)((char *)end - (char *)dacl));
> +	size = sizeof(dacl->acl_cnt) + (count * sizeof(struct xfs_acl_entry));
> +	if (size != (int)((char *)end - (char *)dacl)) {
> +		do_warn(_("ACL count (%d) does not match buffer size (%d/%d)\n"),
> +			  count, size, (int)((char *)end - (char *)dacl));
> +		*aclp = NULL;
> +		return EINVAL;
> +	}

This size check seems superfluous. In what scenario could it fail?

> +
> +	acl = malloc(sizeof(struct xfs_icacl) +
> +		     count * sizeof(struct xfs_icacl_entry));
>  	if (!acl) {
>  		do_warn(_("cannot malloc enough for ACL attribute\n"));
>  		do_warn(_("SKIPPING this ACL\n"));
> @@ -1667,7 +1691,7 @@ process_attributes(
>  	if (aformat == XFS_DINODE_FMT_LOCAL) {
>  		ASSERT(be16_to_cpu(asf->hdr.totsize) <=
>  			XFS_DFORK_ASIZE(dip, mp));
> -		err = process_shortform_attr(ino, dip, repair);
> +		err = process_shortform_attr(mp, ino, dip, repair);
>  	} else if (aformat == XFS_DINODE_FMT_EXTENTS ||
>  					aformat == XFS_DINODE_FMT_BTREE)  {
>  			err = process_longform_attr(mp, ino, dip, blkmap,
> @@ -1686,17 +1710,19 @@ process_attributes(
>   * Validate an ACL
>   */
>  static int
> -xfs_acl_valid(xfs_acl_disk_t *daclp)
> +xfs_acl_valid(
> +	struct xfs_mount *mp,
> +	struct xfs_acl	*daclp)
>  {
> -	xfs_acl_t	*aclp = NULL;
> -	xfs_acl_entry_t *entry, *e;
> +	struct xfs_icacl	*aclp = NULL;
> +	struct xfs_icacl_entry	*entry, *e;
>  	int user = 0, group = 0, other = 0, mask = 0, mask_required = 0;
>  	int i, j;
>  
>  	if (daclp == NULL)
>  		goto acl_invalid;
>  
> -	switch (xfs_acl_from_disk(&aclp, daclp)) {
> +	switch (xfs_acl_from_disk(mp, &aclp, daclp)) {
>  	case ENOMEM:
>  		return 0;
>  	case EINVAL:
> diff --git a/repair/attr_repair.h b/repair/attr_repair.h
> index f42536a..0d0c62c 100644
> --- a/repair/attr_repair.h
> +++ b/repair/attr_repair.h
> @@ -37,29 +37,49 @@ typedef __int32_t	xfs_acl_type_t;
>  typedef __int32_t	xfs_acl_tag_t;
>  typedef __int32_t	xfs_acl_id_t;
>  
> -typedef struct xfs_acl_entry {
> +/*
> + * "icacl" = in-core ACL. There is no equivalent in the XFS kernel code,
> + * so they are magic names just for repair. The "acl" types are what the kernel
> + * code uses for the on-disk format names, so use them here too for the on-disk
> + * ACL format definitions.
> + */
> +struct xfs_icacl_entry {
>  	xfs_acl_tag_t	ae_tag;
>  	xfs_acl_id_t	ae_id;
>  	xfs_acl_perm_t	ae_perm;
> -} xfs_acl_entry_t;
> +};
>  
> -#define XFS_ACL_MAX_ENTRIES	25
> -typedef struct xfs_acl {
> -	__int32_t	acl_cnt;
> -	xfs_acl_entry_t	acl_entry[XFS_ACL_MAX_ENTRIES];
> -} xfs_acl_t;
> +struct xfs_icacl {
> +	__int32_t		acl_cnt;
> +	struct xfs_icacl_entry	acl_entry[0];
> +};
>  
> -typedef struct xfs_acl_entry_disk {
> +struct xfs_acl_entry {
>  	__be32		ae_tag;
>  	__be32		ae_id;
>  	__be16		ae_perm;
> -} xfs_acl_entry_disk_t;
> +	__be16		ae_pad;
> +};
>  
> -typedef struct xfs_acl_disk {
> -	__be32		acl_cnt;
> -	xfs_acl_entry_disk_t	acl_entry[XFS_ACL_MAX_ENTRIES];
> -} xfs_acl_disk_t;
> +struct xfs_acl {
> +	__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(struct xfs_acl)) / \
> +						sizeof(struct xfs_acl_entry) \
> +		: 25)
> +
> +#define XFS_ACL_MAX_SIZE(mp) \
> +	(sizeof(struct xfs_acl) + \
> +		sizeof(struct xfs_acl_entry) * XFS_ACL_MAX_ENTRIES((mp)))
>  

Mostly corresponds to kernel commit:

0a8aa193 xfs: increase number of ACL entries for V5 superblocks

Brian

>  #define SGI_ACL_FILE	"SGI_ACL_FILE"
>  #define SGI_ACL_DEFAULT	"SGI_ACL_DEFAULT"
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs