[PATCH 0/2] xfsprogs: fixes for CRC support

[PATCH 0/2] xfsprogs: fixes for CRC support

[Dave Chinner]

Hi folks,

These two patches correct a couple of oversights in the new CRC
support. Firstly, repair actually validates ACLs on disk (from a
consistency POV, not correctness) and so it needs to know that
v5 formats handle a lot more than 25 ACLs.

Secondly, we forgot all about documenting the "-m crc=1" option in
the mkfs.xfs(8) man page. So the second patch fixes that and also
adds the missing documentation for the free inode btree option as
well.

With these two fixes and all the other xfsprogs patches I've pulled
in and are currently testing, we're pretty much ready for a 3.2.1-rc
release. Is there anything that we know about that we don't have
patches for yet that needs to be done for 3.2.1?

Cheers,

Dave.

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

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

[Dave Chinner]

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>
---
 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)
 		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;
+	}
+
+	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)))
 
 #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

[PATCH 2/2] mkfs: add "-m" options to the man page

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Because they are missing.

Reported-by: Matthias Schniedermeyer <ms@citd.de>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 man/man8/mkfs.xfs.8 | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
index 8184e10..4ba07bf 100644
--- a/man/man8/mkfs.xfs.8
+++ b/man/man8/mkfs.xfs.8
@@ -7,6 +7,9 @@ mkfs.xfs \- construct an XFS filesystem
 .B \-b
 .I block_size
 ] [
+.B \-m
+.I global_metadata_options
+] [
 .B \-d
 .I data_section_options
 ] [
@@ -125,6 +128,50 @@ The default value is 4096 bytes (4 KiB), the minimum is 512, and the
 maximum is 65536 (64 KiB).
 XFS on Linux currently only supports pagesize or smaller blocks.
 .TP
+.BI \-m " global_metadata_options"
+These options specify metadata format options that either apply to the entire
+filesystem or aren't easily characterised by a specific functionality group. The
+valid
+.I global_metadata_options
+are:
+.RS 1.2i
+.TP
+.BI crc= value
+This is used to create a filesystem which maintains and checks CRC information
+in all metadata objects on disk. The value is either 0 to disable the feature,
+or 1 to enable the use of CRCs.
+.IP
+CRCs enable enhanced error detection due to
+hardware issues, whilst the format changes also improves crash recovery
+algorithms and  the ability of various tools to validate and repair metadata
+corruptions when they are found.
+The CRC algorithm used is CRC32c, so the overhead is dependent on CPU
+architecture as some CPUs have hardware acceleration of this algorithm.
+Typically the overhead of calculating and checking the CRCs is not noticable in
+normal operation.
+.IP
+By default,
+.B mkfs.xfs
+will not enable metadata CRCs.
+.TP
+.BI finobt= value
+This option enables the use of a separate free inode btree index in each
+allocation group. The value is either 0 to disable the feature, or 1 to create
+a free inode btree in each allocation group.
+.IP
+The free inode btree mirrors the existing allocated inode btree index which
+indexes both used and free inodes. The free inode btree does not index used
+inodes, allowing faster, more consistent inode allocation performance as
+filesystems age.
+.IP
+By default,
+.B mkfs.xfs
+will not create free inode btrees. This feature is also currently only available
+for filesystems created with the
+.B \-m crc=1
+option set.
+.RE
+.TP
 .BI \-d " data_section_options"
 These options specify the location, size, and other parameters of the
 data section of the filesystem. The valid
-- 
2.0.0

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

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

[Brian Foster]

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

Re: [PATCH 2/2] mkfs: add "-m" options to the man page

[Brian Foster]

On Thu, Jun 19, 2014 at 03:33:52PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Because they are missing.
> 
> Reported-by: Matthias Schniedermeyer <ms@citd.de>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  man/man8/mkfs.xfs.8 | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
> index 8184e10..4ba07bf 100644
> --- a/man/man8/mkfs.xfs.8
> +++ b/man/man8/mkfs.xfs.8
> @@ -7,6 +7,9 @@ mkfs.xfs \- construct an XFS filesystem
>  .B \-b
>  .I block_size
>  ] [
> +.B \-m
> +.I global_metadata_options
> +] [
>  .B \-d
>  .I data_section_options
>  ] [
> @@ -125,6 +128,50 @@ The default value is 4096 bytes (4 KiB), the minimum is 512, and the
>  maximum is 65536 (64 KiB).
>  XFS on Linux currently only supports pagesize or smaller blocks.
>  .TP
> +.BI \-m " global_metadata_options"
> +These options specify metadata format options that either apply to the entire
> +filesystem or aren't easily characterised by a specific functionality group. The
> +valid
> +.I global_metadata_options
> +are:
> +.RS 1.2i
> +.TP
> +.BI crc= value
> +This is used to create a filesystem which maintains and checks CRC information
> +in all metadata objects on disk. The value is either 0 to disable the feature,
> +or 1 to enable the use of CRCs.
> +.IP
> +CRCs enable enhanced error detection due to
> +hardware issues, whilst the format changes also improves crash recovery
> +algorithms and  the ability of various tools to validate and repair metadata
		 ^ extra space

The rest looks good to me. Thanks for writing up the finobt bits!

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

> +corruptions when they are found.
> +The CRC algorithm used is CRC32c, so the overhead is dependent on CPU
> +architecture as some CPUs have hardware acceleration of this algorithm.
> +Typically the overhead of calculating and checking the CRCs is not noticable in
> +normal operation.
> +.IP
> +By default,
> +.B mkfs.xfs
> +will not enable metadata CRCs.
> +.TP
> +.BI finobt= value
> +This option enables the use of a separate free inode btree index in each
> +allocation group. The value is either 0 to disable the feature, or 1 to create
> +a free inode btree in each allocation group.
> +.IP
> +The free inode btree mirrors the existing allocated inode btree index which
> +indexes both used and free inodes. The free inode btree does not index used
> +inodes, allowing faster, more consistent inode allocation performance as
> +filesystems age.
> +.IP
> +By default,
> +.B mkfs.xfs
> +will not create free inode btrees. This feature is also currently only available
> +for filesystems created with the
> +.B \-m crc=1
> +option set.
> +.RE
> +.TP
>  .BI \-d " data_section_options"
>  These options specify the location, size, and other parameters of the
>  data section of the filesystem. The valid
> -- 
> 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

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

[Dave Chinner]

On Thu, Jun 19, 2014 at 09:01:45AM -0400, Brian Foster wrote:
> 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...

I wrote it as a single patch to make it easy for Michael to test,
and I found several issues along the way...

> >  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

Yup, I'll note that.

> > @@ -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?

Kernel writes a corrupted ACL? Cosmic ray causes a single bit error
in a sector on a non-crc filesystem? We do checks like these on
variable size structures in many other places - not just ACLs - for
verifying internal consistency of the structure we are parsing....

> >  
> > +/*
> > + * 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

Mostly, but it's a completely separate set of definitions to the
kernel and libxfs. Maybe at some point we should revisit that...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Dave Chinner]

repair: support more than 25 ACLs

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. This slipped through
becase the reapir code does not share any of the kernel side ACL
code in libxfs, and instead has all it's own internal ACL
definitions.

Fix the repair code to support more than 25 ACLs and update
the ACL definitions to match the kernel definitions. In doing so,
this tickles a off-by-one bug on remote attribute maximum sizes
that is already fixed in the kernel code. So in addition to fixing
the repair code, this patch pulls in parts of the following kernel
commits:

bba719b5 xfs: fix off-by-one error in xfs_attr3_rmt_verify
0a8aa193 xfs: increase number of ACL entries for V5 superblocks

Reported-by: Michael L. Semon <mlsemon35@gmail.com>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---

V2: update commit message to:
	- better explain the lack of code sharing that lead to this
	  being missed; and
	- indicate the kernel commits that the ACL and attr changes
	  were sourced from.

 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)
 		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;
+	}
+
+	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)))
 
 #define SGI_ACL_FILE	"SGI_ACL_FILE"
 #define SGI_ACL_DEFAULT	"SGI_ACL_DEFAULT"

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

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

[Brian Foster]

On Fri, Jun 20, 2014 at 07:14:14AM +1000, Dave Chinner wrote:
> On Thu, Jun 19, 2014 at 09:01:45AM -0400, Brian Foster wrote:
> > 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>
> > > ---
> > 
...
> 
> > > @@ -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?
> 
> Kernel writes a corrupted ACL? Cosmic ray causes a single bit error
> in a sector on a non-crc filesystem? We do checks like these on
> variable size structures in many other places - not just ACLs - for
> verifying internal consistency of the structure we are parsing....
> 

Hmm, but what exactly are we checking for in this particular instance?
end is calculated as the offset of the first entry in struct xfs_acl
(constant) plus count * the entry structure size (variable * constant).
size is calculated as the size of the non-entry bit of xfs_acl
(constant) plus count * the entry structure size. The only variable
between the two calculations is count, and it's used in both. It seems
like these would always end up equivalent, regardless of what's on disk.

The only way I can see this fail is if we were to add a field to
xfs_acl. The end calculation will inherit the size of the field by
virtue of using the entry offset at the end of the structure. The size
calculation would end up wrong as it checks the non-entry field size
explicitly. I'm not sure what that would tell us beyond the need to fix
this particular bit of code, so this really just seems like a potential
bug to me. Am I missing something else? (If so, I'd suggest a more
informative error message or a comment).

Brian

> > >  
> > > +/*
> > > + * 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
> 
> Mostly, but it's a completely separate set of definitions to the
> kernel and libxfs. Maybe at some point we should revisit that...
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

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

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

[Dave Chinner]

On Fri, Jun 20, 2014 at 08:14:26AM -0400, Brian Foster wrote:
> On Fri, Jun 20, 2014 at 07:14:14AM +1000, Dave Chinner wrote:
> > On Thu, Jun 19, 2014 at 09:01:45AM -0400, Brian Foster wrote:
> > > 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>
> > > > ---
> > > 
> ...
> > 
> > > > @@ -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?
> > 
> > Kernel writes a corrupted ACL? Cosmic ray causes a single bit error
> > in a sector on a non-crc filesystem? We do checks like these on
> > variable size structures in many other places - not just ACLs - for
> > verifying internal consistency of the structure we are parsing....
> > 
> 
> Hmm, but what exactly are we checking for in this particular instance?
> end is calculated as the offset of the first entry in struct xfs_acl
> (constant) plus count * the entry structure size (variable * constant).
> size is calculated as the size of the non-entry bit of xfs_acl
> (constant) plus count * the entry structure size. The only variable
> between the two calculations is count, and it's used in both. It seems
> like these would always end up equivalent, regardless of what's on disk.

Ah, right, I see your point now. The old code used a fixed size
structure (i.e. with an array of 25 ACLs in it). Hence the check was
valid for that case, where a corrupted count could result in a
structure overrun.

> The only way I can see this fail is if we were to add a field to
> xfs_acl.

Actually, the old code did have a bug like this in it because the
structure repair defined had different sizes on 32 and 64 bit
machines. i.e. it didn't have the 4 bytes of padding the kernel
structure had...

> The end calculation will inherit the size of the field by
> virtue of using the entry offset at the end of the structure. The size
> calculation would end up wrong as it checks the non-entry field size
> explicitly. I'm not sure what that would tell us beyond the need to fix
> this particular bit of code, so this really just seems like a potential
> bug to me. Am I missing something else? (If so, I'd suggest a more
> informative error message or a comment).

No, I just misunderstood your comment. You are right, the code
doesn't provide any value now, so I'll remove it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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