xfs
[Top] [All Lists]

Re: xfs_repair issue with ACLs on v5 XFS when beyond v4 limits

To: "Michael L. Semon" <mlsemon35@xxxxxxxxx>
Subject: Re: xfs_repair issue with ACLs on v5 XFS when beyond v4 limits
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 19 Jun 2014 13:35:52 +1000
Cc: xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <539A61B2.5040202@xxxxxxxxx>
References: <5396799F.3050801@xxxxxxxxx> <20140610055254.GF9508@dastard> <539A61B2.5040202@xxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Jun 12, 2014 at 10:28:02PM -0400, Michael L. Semon wrote:
> On 06/10/2014 01:52 AM, Dave Chinner wrote:
> > On Mon, Jun 09, 2014 at 11:21:03PM -0400, Michael L. Semon wrote:
> >> Hi!  I've been running around in circles trying to work with too many 
> >> ACLs, even losing my ability to count for a while.  Along the way, 
> >> xfs_repair from git xfsprogs (last commit around May 27) is showing 
> >> the following symptoms:
> >>
> >> On v5-superblock XFS...
> >>
> >> 1) When the ACL count is just above the limit from v4-superblock XFS--
> >> 96 is a good test figure--`xfs_repair -n` and `xfs_repair` will both 
> >> end in a segmentation fault.
> > 
> > I couldn't reproduce this - I suspect that this is a problem with
> > the ACL struct having a hardcoded array size or userspace not
> > having the correct padding in the on-disk structure definition and
> > you are on a 32bit system. I think I've fixed that in the patch
> > below.
> 
> Maybe.  Pentium III has a narrower cacheline than the Pentium 4, so 
> I was not surprised to see holes in the XFS kernel code, even in the 
> non-XFS kernel structs.  Do I need to upgrade something (ACL, system 
> kernel headers, etc.) or would a pahole trip through libxfs be more 
> revealing?
> 
> What I'm getting is that if xfs_repair is counting between 200 and 
> 256 ACLs, it will mention that there are too many ACLs, and it will 
> segfault.  With your patch, the areas below and above this range are 
> OK.
> 
> A sample session like the one I overwrote last time looks like this:
> 
> Phase 1 - find and verify superblock...
> Phase 2 - using internal log
>         - zero log...
>         - scan filesystem freespace and inode maps...
>         - found root inode chunk
> Phase 3 - for each AG...
>         - scan and clear agi unlinked lists...
>         - process known inodes and perform inode discovery...
>         - agno = 0
> Too many ACL entries, count 250
> entry contains illegal value in attribute named SGI_ACL_FILE or 
> SGI_ACL_DEFAULT
> (segfault, either Error 4 or Error 5, forgot to bring dmesg)

Ok, your test found a bug in the patch that was causing segv's - at
about 20 ACLs, not 250. It's not the same as what you have reported,
but it was a stack corruption bug and so may just be triggering
differently on your machines.

Can you try the patch below?

> Maybe not...your E-mail patch doesn't have the git version at the 
> bottom, so I wondered whether I installed the entire patch.  What 
> I did get went through `git am` just fine, with one whitespace error.

That's because I didn't use git directly to generate it. As you
found out, it's still a valid patch...

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

repair: support more than 25 ACLs

From: Dave Chinner <dchinner@xxxxxxxxxx>

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.

[V2: fix stack overwrite in valuecheck()]

Reported-by: Michael L. Semon <mlsemon35@xxxxxxxxx>
Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
---
 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"

<Prev in Thread] Current Thread [Next in Thread>