* xfs_repair issue with ACLs on v5 XFS when beyond v4 limits
@ 2014-06-10 3:21 Michael L. Semon
2014-06-10 5:52 ` Dave Chinner
0 siblings, 1 reply; 5+ messages in thread
From: Michael L. Semon @ 2014-06-10 3:21 UTC (permalink / raw)
To: xfs-oss
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.
2) When the ACL count is in a higher range--as low as 250, IIRC--
xfs_repair will complain about "Too many ACL entries" and proceed to
remove them. Below is a full session:
root@oldsvrhw:~# mkfs.xfs -f -m crc=1 $SCRATCH_DEV
meta-data=/dev/sdb5 isize=512 agcount=4, agsize=786432 blks
= sectsz=512 attr=2, projid32bit=1
= crc=1 finobt=0
data = bsize=4096 blocks=3145728, imaxpct=25
= sunit=0 swidth=0 blks
naming =version 2 bsize=4096 ascii-ci=0 ftype=1
log =internal log bsize=4096 blocks=2560, version=2
= sectsz=512 sunit=0 blks, lazy-count=1
realtime =none extsz=4096 blocks=0, rtextents=0
root@oldsvrhw:~# mount $SCRATCH_DEV $SCRATCH_MNT
root@oldsvrhw:~# mkdir $SCRATCH_MNT/acl-dir
root@oldsvrhw:~# for a in `seq 1000 1325`; do setfacl -d -m u:$a:r-- $SCRATCH_MNT/acl-dir; done
root@oldsvrhw:~# sync
root@oldsvrhw:~# umount $SCRATCH_MNT
root@oldsvrhw:~# xfs_repair -n $SCRATCH_DEV
Phase 1 - find and verify superblock...
Phase 2 - using internal log
- scan filesystem freespace and inode maps...
- found root inode chunk
Phase 3 - for each AG...
- scan (but don't clear) agi unlinked lists...
- process known inodes and perform inode discovery...
- agno = 0
Too many ACL entries, count 330
entry contains illegal value in attribute named SGI_ACL_FILE or SGI_ACL_DEFAULT
remote attribute value check failed for entry 0, inode 67
problem with attribute contents in inode 67
would clear attr fork
bad nblocks 2 for inode 67, would reset to 0
bad anextents 1 for inode 67, would reset to 0
- agno = 1
- agno = 2
- agno = 3
- process newly discovered inodes...
Phase 4 - check for duplicate blocks...
- setting up duplicate extent list...
- check for inodes claiming duplicate blocks...
- agno = 0
- agno = 1
- agno = 2
- agno = 3
No modify flag set, skipping phase 5
Phase 6 - check inode connectivity...
- traversing filesystem ...
- traversal finished ...
- moving disconnected inodes to lost+found ...
Phase 7 - verify link counts...
No modify flag set, skipping filesystem flush and exiting.
root@oldsvrhw:~# xfs_repair $SCRATCH_DEV
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 330
entry contains illegal value in attribute named SGI_ACL_FILE or SGI_ACL_DEFAULT
remote attribute value check failed for entry 0, inode 67
problem with attribute contents in inode 67
clearing inode 67 attributes
correcting nblocks for inode 67, was 2 - counted 0
- agno = 1
- agno = 2
- agno = 3
- process newly discovered inodes...
Phase 4 - check for duplicate blocks...
- setting up duplicate extent list...
- check for inodes claiming duplicate blocks...
- agno = 0
bad attribute format 1 in inode 67, resetting value
- agno = 1
- agno = 2
- agno = 3
Phase 5 - rebuild AG headers and trees...
- reset superblock...
Phase 6 - check inode connectivity...
- resetting contents of realtime bitmap and summary inodes
- traversing filesystem ...
- traversal finished ...
- moving disconnected inodes to lost+found ...
Phase 7 - verify and correct link counts...
done
root@oldsvrhw:~# mount $SCRATCH_DEV $SCRATCH_MNT
root@oldsvrhw:~# getfacl $SCRATCH_MNT/acl-dir
getfacl: Removing leading '/' from absolute path names
# file: mnt/xfstests-scratch/acl-dir
# owner: root
# group: root
user::rwx
group::r-x
other::r-x
3) When the ACL count is at the max for v5-superblock XFS--at least
with both regular and default ACL slots filled--xfs_repair will
complain of corrupt remote attributes. AFAIK, xfs_repair doesn't
bother with the "Too many ACL entries" line. Those ACLs will be
cleansed, too.
I can't tell if this is due to one missed check or three different
tiny issues.
ACLs seem to be OK when doing normal filesystem operations and using
setfacl and getfacl. No kernel stack traces have been seen on this
path of testing.
Sorry I missed this one in all of my limits testing. This was
discovered when I saw a bug in my ACL population script and hit
Ctrl-c so I could stop and edit the script. Donations of brown paper
bags are welcome...the plastic bags I'm using make it hard to breathe
and don't hide my face very well...
Test PC was a Pentium III, 733 MHz, 512 MB of RAM, running kernel
3.15.0-rc8 + xfs-oss/for-next, on Slackware 14.1.
Thanks!
Michael
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: xfs_repair issue with ACLs on v5 XFS when beyond v4 limits 2014-06-10 3:21 xfs_repair issue with ACLs on v5 XFS when beyond v4 limits Michael L. Semon @ 2014-06-10 5:52 ` Dave Chinner 2014-06-13 2:28 ` Michael L. Semon 0 siblings, 1 reply; 5+ messages in thread From: Dave Chinner @ 2014-06-10 5:52 UTC (permalink / raw) To: Michael L. Semon; +Cc: xfs-oss 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. > > 2) When the ACL count is in a higher range--as low as 250, IIRC-- > xfs_repair will complain about "Too many ACL entries" and proceed to > remove them. Below is a full session: Yup, never been taught about the expanded ACL count. I didn't even realised that repair validated acls directly... > > root@oldsvrhw:~# mkfs.xfs -f -m crc=1 $SCRATCH_DEV > meta-data=/dev/sdb5 isize=512 agcount=4, agsize=786432 blks > = sectsz=512 attr=2, projid32bit=1 > = crc=1 finobt=0 > data = bsize=4096 blocks=3145728, imaxpct=25 > = sunit=0 swidth=0 blks > naming =version 2 bsize=4096 ascii-ci=0 ftype=1 > log =internal log bsize=4096 blocks=2560, version=2 > = sectsz=512 sunit=0 blks, lazy-count=1 > realtime =none extsz=4096 blocks=0, rtextents=0 > > root@oldsvrhw:~# mount $SCRATCH_DEV $SCRATCH_MNT > > root@oldsvrhw:~# mkdir $SCRATCH_MNT/acl-dir > > root@oldsvrhw:~# for a in `seq 1000 1325`; do setfacl -d -m u:$a:r-- $SCRATCH_MNT/acl-dir; done ..... Ok, I can reproduce that, and I've fixed it in the patch below. > 3) When the ACL count is at the max for v5-superblock XFS--at least > with both regular and default ACL slots filled--xfs_repair will > complain of corrupt remote attributes. AFAIK, xfs_repair doesn't > bother with the "Too many ACL entries" line. Those ACLs will be > cleansed, too. Ok, I see that, too: - agno = 0 Metadata corruption detected at block 0x190/0x1000 Corrupt remote block for attributes of inode 67 Ah, of course - there was an off-by-one in the remote attr max size validation that we fixed in the kernel. The kernel code hasn't been siynced to userspace yet. Ok, the patch below fixes that as well. Can you turn this into a new fstest so we don't break this accidentally again? Can you try the patch below? It should fix the problem you are seeing. > Sorry I missed this one in all of my limits testing. This was > discovered when I saw a bug in my ACL population script and hit > Ctrl-c so I could stop and edit the script. Donations of brown paper > bags are welcome...the plastic bags I'm using make it hard to breathe > and don't hide my face very well... We've all missed it, so pass the paper bags all around. To prevent this from happening again in future, can you wrap this all up in a new generic fstest that creates several different numbers of ACLs on a file and runs repair on the filesystem after each incremental increase in the number of ACLs? Thanks for the testing and the bug report, Michael! Cheers, Dave. -- Dave Chinner david@fromorbit.com 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. 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 | 68 ++++++++++++++++++++++++++++++++---------------- repair/attr_repair.h | 46 +++++++++++++++++++++++--------- 3 files changed, 79 insertions(+), 37 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..5ff1647 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,16 @@ 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; + struct xfs_acl thisacl; void *valuep; int clearit = 0; @@ -746,13 +751,13 @@ 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); + memset(&thisacl, 0, sizeof(struct xfs_acl)); + memmove(&thisacl, namevalue + namelen, valuelen); valuep = &thisacl; } 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 " @@ -800,6 +805,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 +910,7 @@ process_shortform_attr( /* Only check values for root security attributes */ if (currententry->flags & XFS_ATTR_ROOT) - junkit = valuecheck((char *)¤tentry->nameval[0], + junkit = valuecheck(mp, (char *)¤tentry->nameval[0], NULL, currententry->namelen, currententry->valuelen); @@ -1039,6 +1045,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 +1083,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 +1141,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 +1223,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 +1615,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 +1635,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 +1687,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 +1706,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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: xfs_repair issue with ACLs on v5 XFS when beyond v4 limits 2014-06-10 5:52 ` Dave Chinner @ 2014-06-13 2:28 ` Michael L. Semon 2014-06-19 3:35 ` Dave Chinner 0 siblings, 1 reply; 5+ messages in thread From: Michael L. Semon @ 2014-06-13 2:28 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs-oss 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) >> 2) When the ACL count is in a higher range--as low as 250, IIRC-- >> xfs_repair will complain about "Too many ACL entries" and proceed to >> remove them. Below is a full session: > > Yup, never been taught about the expanded ACL count. I didn't even > realised that repair validated acls directly... > >> >> root@oldsvrhw:~# mkfs.xfs -f -m crc=1 $SCRATCH_DEV >> meta-data=/dev/sdb5 isize=512 agcount=4, agsize=786432 blks >> = sectsz=512 attr=2, projid32bit=1 >> = crc=1 finobt=0 >> data = bsize=4096 blocks=3145728, imaxpct=25 >> = sunit=0 swidth=0 blks >> naming =version 2 bsize=4096 ascii-ci=0 ftype=1 >> log =internal log bsize=4096 blocks=2560, version=2 >> = sectsz=512 sunit=0 blks, lazy-count=1 >> realtime =none extsz=4096 blocks=0, rtextents=0 >> >> root@oldsvrhw:~# mount $SCRATCH_DEV $SCRATCH_MNT >> >> root@oldsvrhw:~# mkdir $SCRATCH_MNT/acl-dir >> >> root@oldsvrhw:~# for a in `seq 1000 1325`; do setfacl -d -m u:$a:r-- $SCRATCH_MNT/acl-dir; done > ..... > > Ok, I can reproduce that, and I've fixed it in 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. >> 3) When the ACL count is at the max for v5-superblock XFS--at least >> with both regular and default ACL slots filled--xfs_repair will >> complain of corrupt remote attributes. AFAIK, xfs_repair doesn't >> bother with the "Too many ACL entries" line. Those ACLs will be >> cleansed, too. > > Ok, I see that, too: > > - agno = 0 > Metadata corruption detected at block 0x190/0x1000 > Corrupt remote block for attributes of inode 67 > > Ah, of course - there was an off-by-one in the remote attr max size > validation that we fixed in the kernel. The kernel code hasn't been > siynced to userspace yet. Ok, the patch below fixes that as well. This is most definitely fixed. Thanks! > Can you turn this into a new fstest so we > don't break this accidentally again? I'm struggling, and it will take a long time, but I'll try. If you have a compiled ACL generator somewhere in xfstests, it will save the time of reading ACL-related man pages. In the meantime, I developed a fairly coherent script that goes through a filesystem 4 ACL entries, checking and making filesystems in between. I started it by making filesystems on a ramdisk and storing the results on a tmpfs mount, but it uses a normal $SCRATCH_DEV now. It won't show an error immediately, but all xfs_repair output will be stored on a tmpfs amount for your perusal. The script is enclosed, with a debug "step=1" line uncommented so you can go after the 200-250 ACL-count issue. Read it first because you'll want to alter something for your setup. It's not worthy of the xfstests harness, but it should be comprehensive enough to let you run fs_mark in the middle of it if need be. The smaller the partition you use, the faster the script will go. > Can you try the patch below? It should fix the problem you are > seeing. > >> Sorry I missed this one in all of my limits testing. This was >> discovered when I saw a bug in my ACL population script and hit >> Ctrl-c so I could stop and edit the script. Donations of brown paper >> bags are welcome...the plastic bags I'm using make it hard to breathe >> and don't hide my face very well... > > We've all missed it, so pass the paper bags all around. To prevent > this from happening again in future, can you wrap this all up in a > new generic fstest that creates several different numbers of ACLs > on a file and runs repair on the filesystem after each incremental > increase in the number of ACLs? > > Thanks for the testing and the bug report, Michael! No problem. I'll keep trying! Michael #!/bin/bash # Suggested filename: pop_acl.sh # ACL coverage script # Michael L. Semon # Thu Jun 12 20:38:22 UTC 2014 # This script builds ACL entries, four at a time, and applies them to a new # directory on a fresh filesystem. The filesystem is umounted and repaired # after each creation to look for issues. A new filesystem is made with # each pass of the main loop. # Later, a new filesystem is made, and the new directory is populated to # its limit (hopefully). The filesystem is umounted and repaired again. # There is a WORK_DIR that holds temporary data, repair results, and the # cumulative ACL count. It is set to mount a tmpfs area for this purpose. # This behavior can be overridden early in the script. # There are two sections marked OPTIONAL ACTIVITIES. In these sections, you # can add your own commands. Should they be executed in a directory with # default ACLs, file creation stress in particular should exercise ACL # inheritance at the same time. # This script can be set to use v5-superblock XFS (xfs or v5xfs), regular # v4-superblock XFS (x4xfs), JFS (jfs), or F2FS (f2fs). btrfs # can also be used in single mode. An FSTYP variable is coded with a # default of "xfs". The script can also be executed like this: # # env FSTYP=jfs ./pop_acl.sh # -------- basic globals -------- # This line should be commented out so that xfstests SCRATCH_DEV can be used. # SCRATCH_DEV=/dev/ram0 if [ -z $SCRATCH_DEV ]; then echo 'SCRATCH_DEV is empty. Exiting.' exit fi if [ -z $SCRATCH_MNT ]; then echo 'SCRATCH_MNT is empty. Exiting.' exit fi # Choices are xfs (which means v5), v5xfs, v4xfs, jfs, f2fs, and btrfs. FSTYP=${FSTYP:=xfs} # How many entries (times 4) does this script make for each filesystem? # Default is (measured max - 8 - 4) / 8, integer division in bc. case $FSTYP in xfs|v5xfs) # Calculated from 10922 (5461+5461) step=1363 ;; v4xfs) # Calculated from 50 (25+25) step=4 ;; jfs) # Calculated from 16382 (8191+8191) step=2046 ;; f2fs) # Calculated from 507 (250+257) step=123 ;; btrfs) # Calculated from 978 (489+489) for mixed data/metadata mode on a # small partition. # A value of 4050 (2025+2025) was calculated after btrfs auto-set # the big metadata feature on a larger partition. step=241 ;; *) step=1 ;; esac # Uncomment this to go after corner cases: step=1 # Base number for calculating uids and gids. id=1000 # -------- Work Area (section that pertains to tmpfs) --------- # If you want to store results on a hard disk, override WORK_DIR # and comment out the tmpfs mount below. # work area (tmpfs by default) WORK_DIR=/tmp/tmpfs # WORK_DIR=/usr/src # results dir RES_DIR=$WORK_DIR/results_dir # reference acl list acl_list=$WORK_DIR/acl_list if [ -e $WORK_DIR ]; then umount $WORK_DIR else mkdir $WORK_DIR fi mount -t tmpfs tmpfs $WORK_DIR [ -e $RES_DIR ] || mkdir $RES_DIR # -------- end of section that pertains to tmpfs --------- # -------- basic setup -------- # Set up the base entries: cat << here > $acl_list user::rwx group::rwx other::r-x mask::rwx default:user::r-x default:group::r-x default:other::--x default:mask::r-x here # Done here in case SCRATCH_DEV is already open. umount $SCRATCH_DEV # ======== MAIN LOOP (does most of the work) ======== while true; do uid=$id gid=$((id+6144)) duid=$((gid+6144)) dgid=$((duid+6144)) # Make and mount a new filesystem, and do so early so it's # obviously mounted while the ACL build loop does its work. case $FSTYP in xfs|v5xfs) mkfs.xfs -f -m crc=1 $SCRATCH_DEV > /dev/null 2>&1 mount -t xfs $SCRATCH_DEV $SCRATCH_MNT ;; v4xfs) mkfs.xfs -f $SCRATCH_DEV > /dev/null 2>&1 mount -t xfs $SCRATCH_DEV $SCRATCH_MNT ;; jfs) jfs_mkfs -q $SCRATCH_DEV > /dev/null 2>&1 mount -t jfs $SCRATCH_DEV $SCRATCH_MNT ;; f2fs) mkfs.f2fs $SCRATCH_DEV > /dev/null 2>&1 mount -t f2fs $SCRATCH_DEV $SCRATCH_MNT ;; btrfs) # Only on single for now: mkfs.btrfs -f $SCRATCH_DEV > /dev/null 2>&1 mount -t btrfs $SCRATCH_DEV $SCRATCH_MNT ;; *) echo "Filesystem $FSTYP is not supported. Exiting." exit ;; esac mkdir $SCRATCH_MNT/acl-dir # echo uid, gid, uid for default entries, gid for default entries echo "ids: $uid, $gid, $duid, $dgid; iterating by $step" cat /dev/null > $RES_DIR/added.acl # Make our entries onto the reference directory. for inn in `seq 1 $step`; do cat << here >> $RES_DIR/added.acl user:$((uid+inn)):r-- group:$((gid+inn)):r-- default:user:$((duid+inn)):r-- default:group:$((dgid+inn)):r-- here done cp -a $acl_list $WORK_DIR/last_acl_list_accepted cat $RES_DIR/added.acl >> $acl_list # Copy the ACL from the reference dir into the target filesystem. setfacl --set-file=$acl_list $SCRATCH_MNT/acl-dir || break getfacl -c $SCRATCH_MNT/acl-dir 2>/dev/null | grep -v "^$" | \ wc -l | sed -e "s/^/$id: /" \ >> $RES_DIR/acl.count 2>/dev/null # ==================== OPTIONAL ACTIVTIES #1 =================== # There is a separate list for later, once a maximum ACL count # has been found. It might be a good idea to keep this part # light. Commented out for being optional. # # fs_mark -L 3 -D 4 -n 100 -s 4096 -d $SCRATCH_MNT/acl-dir # touch $SCRATCH_MNT/acl-dir/child # mkdir $SCRATCH_MNT/acl-dir/child-dir # touch $SCRATCH_MNT/acl-dir/child-dir/grandchild # mkdir $SCRATCH_MNT/acl-dir/child-dir/grandchild-dir # touch $SCRATCH_MNT/acl-dir/child-dir/great-grandchild # ============================================================== # umount. Uncomment the sync line if needed. # sync umount $SCRATCH_DEV # Repair the filesystem. Make a no-modify pass first, in case # the results are different, beneficial to JFS in particular. case $FSTYP in xfs|v5xfs|v4xfs) xfs_repair -n $SCRATCH_DEV >$RES_DIR/$id.nrepair 2>&1 xfs_repair $SCRATCH_DEV >$RES_DIR/$id.repair 2>&1 ;; jfs) jfs_fsck -fnv $SCRATCH_DEV >$RES_DIR/$id.nrepair 2>&1 jfs_fsck -fyv $SCRATCH_DEV >$RES_DIR/$id.repair 2>&1 ;; f2fs) # Don't know if/when fsck.f2fs will fix issues, so... fsck.f2fs $SCRATCH_DEV >$RES_DIR/$id.repair 2>&1 ln $RES_DIR/$id.repair $RES_DIR/$id.nrepair 2>&1 ;; btrfs) btrfsck $SCRATCH_DEV >$RES_DIR/$id.nrepair 2>&1 btrfsck --repair $SCRATCH_DEV >$RES_DIR/$id.repair 2>&1 ;; *) echo "Filesystem $FSTYP is not supported. Exiting." exit ;; esac ln -sf $RES_DIR/$id.nrepair $RES_DIR/current.nrepair ln -sf $RES_DIR/$id.repair $RES_DIR/current.repair id=$((id+$step)) done umount $SCRATCH_DEV # ======== SECONDARY SECTION (hopefully fills ACLs out to maximum) ======== # echo uid, gid, uid for default entries, gid for default entries # echo "ids: $uid, $gid, $duid, $dgid, iterating $step (x4) ids" # Make and mount a new filesystem. case $FSTYP in xfs|v5xfs) mkfs.xfs -f -m crc=1 $SCRATCH_DEV > /dev/null 2>&1 mount -t xfs $SCRATCH_DEV $SCRATCH_MNT ;; v4xfs) mkfs.xfs -f $SCRATCH_DEV > /dev/null 2>&1 mount -t xfs $SCRATCH_DEV $SCRATCH_MNT ;; jfs) jfs_mkfs -q $SCRATCH_DEV > /dev/null 2>&1 mount -t jfs $SCRATCH_DEV $SCRATCH_MNT ;; f2fs) mkfs.f2fs $SCRATCH_DEV > /dev/null 2>&1 mount -t f2fs $SCRATCH_DEV $SCRATCH_MNT ;; btrfs) # Only on single for now: mkfs.btrfs -f $SCRATCH_DEV > /dev/null 2>&1 mount -t btrfs $SCRATCH_DEV $SCRATCH_MNT ;; *) echo "Filesystem $FSTYP is not supported. Exiting." exit ;; esac mkdir $SCRATCH_MNT/acl-dir # Copy the ACL from the reference dir into the target filesystem. setfacl --set-file=$WORK_DIR/last_acl_list_accepted \ $SCRATCH_MNT/acl-dir || break # ---------------------- FILL UP THE ACLs ---------------------- # This section tries to fill the ACL space in a balanced way. echo "Attempt to find the maximum ACL count..." echo "...filling default user and group together..." while true; do # Increment first here, let the later loops retry it. duid=$((duid + 1)) setfacl -d -m u:$duid:rwx $SCRATCH_MNT/acl-dir || break dgid=$((dgid + 1)) setfacl -d -m g:$dgid:rwx $SCRATCH_MNT/acl-dir || break done echo "...filling default user..." while true; do setfacl -d -m u:$duid:rwx $SCRATCH_MNT/acl-dir || break duid=$((duid + 1)) done echo "...filling default group..." while true; do setfacl -d -m g:$dgid:rwx $SCRATCH_MNT/acl-dir || break dgid=$((dgid + 1)) done echo "...filling user and group together..." while true; do # Increment first here, let the later loops retry it. uid=$((uid + 1)) setfacl -m u:$uid:rwx $SCRATCH_MNT/acl-dir || break gid=$((gid + 1)) setfacl -m g:$gid:rwx $SCRATCH_MNT/acl-dir || break done echo "...filling user..." while true; do setfacl -m u:$uid:rwx $SCRATCH_MNT/acl-dir || break uid=$((uid + 1)) done echo "...filling group..." while true; do setfacl -m g:$gid:rwx $SCRATCH_MNT/acl-dir || break gid=$((gid + 1)) done # It seems that the mask has to be set to eke out that last entry, # but try the others, anyway: echo "...setting the original base entries again..." setfacl -d -m u::r-x $SCRATCH_MNT/acl-dir setfacl -m u::rwx $SCRATCH_MNT/acl-dir setfacl -d -m g::r-x $SCRATCH_MNT/acl-dir setfacl -m g::rwx $SCRATCH_MNT/acl-dir setfacl -d -m o::--x $SCRATCH_MNT/acl-dir setfacl -m o::r-x $SCRATCH_MNT/acl-dir setfacl -d -m m::rwx $SCRATCH_MNT/acl-dir setfacl -m m::rwx $SCRATCH_MNT/acl-dir echo "...done filling the inode with entries." # -------------------------------------------------------------- getfacl -c $SCRATCH_MNT/acl-dir 2>/dev/null | grep -v "^$" | \ wc -l | sed -e "s/^/final: /" \ >> $RES_DIR/acl.count 2>/dev/null echo "Making max ACL reference file at $WORK_DIR/max_acl_file." getfacl $SCRATCH_MNT/acl-dir 2>/dev/null > $WORK_DIR/max_acl_file # ==================== OPTIONAL ACTIVTIES #2 =================== # This might be were all of the heavy fsstress, fsx, fio, and # fs_mark activities could go. Commented out for being # optional. # # This section is placed here so the ACL breakdown will be the # last output from the script. # fs_mark -F -D 4 -n 100 -s 4096 -d $SCRATCH_MNT/acl-dir # touch $SCRATCH_MNT/acl-dir/child # mkdir $SCRATCH_MNT/acl-dir/child-dir # touch $SCRATCH_MNT/acl-dir/child-dir/grandchild # mkdir $SCRATCH_MNT/acl-dir/child-dir/grandchild-dir # touch $SCRATCH_MNT/acl-dir/child-dir/great-grandchild # ============================================================== echo "ACL count (last 3 loop iterations and final pass):" tail -n 4 $RES_DIR/acl.count | sed -e 's/^/ /' echo "ACL breakdown (default entries only, should total to maximum default):" grep "^default" $WORK_DIR/max_acl_file | cut -d ':' -f 1-2 | \ sort | uniq -c | sort -rg echo "ACL breakdown (all, should total to maximum):" grep -v "^#" $WORK_DIR/max_acl_file | grep -v "^$" | cut -d ':' -f 1 | \ sort | uniq -c | sort -rg # umount. Uncomment the sync line if needed. # sync umount $SCRATCH_DEV # Repair the filesystem. Make a no-modify pass first, in case # the results are different, beneficial to JFS in particular. case $FSTYP in xfs|v5xfs|v4xfs) xfs_repair -n $SCRATCH_DEV >$RES_DIR/$id.nrepair 2>&1 xfs_repair $SCRATCH_DEV >$RES_DIR/$id.repair 2>&1 ;; jfs) jfs_fsck -fnv $SCRATCH_DEV >$RES_DIR/final.nrepair 2>&1 jfs_fsck -fyv $SCRATCH_DEV >$RES_DIR/final.repair 2>&1 ;; f2fs) # Don't know if/when fsck.f2fs will fix issues, so... fsck.f2fs $SCRATCH_DEV >$RES_DIR/final.repair 2>&1 ln $RES_DIR/final.repair $RES_DIR/final.nrepair 2>&1 ;; btrfs) btrfsck $SCRATCH_DEV >$RES_DIR/final.nrepair 2>&1 btrfsck --repair $SCRATCH_DEV >$RES_DIR/final.repair 2>&1 ;; *) echo "Filesystem $FSTYP is not supported. Exiting." exit ;; esac ln -sf $RES_DIR/final.nrepair $RES_DIR/current.nrepair ln -sf $RES_DIR/final.repair $RES_DIR/current.repair # end of script _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: xfs_repair issue with ACLs on v5 XFS when beyond v4 limits 2014-06-13 2:28 ` Michael L. Semon @ 2014-06-19 3:35 ` Dave Chinner 2014-06-20 23:24 ` Michael L. Semon 0 siblings, 1 reply; 5+ messages in thread From: Dave Chinner @ 2014-06-19 3:35 UTC (permalink / raw) To: Michael L. Semon; +Cc: xfs-oss 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@fromorbit.com 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. 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@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 *)¤tentry->nameval[0], + junkit = valuecheck(mp, (char *)¤tentry->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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: xfs_repair issue with ACLs on v5 XFS when beyond v4 limits 2014-06-19 3:35 ` Dave Chinner @ 2014-06-20 23:24 ` Michael L. Semon 0 siblings, 0 replies; 5+ messages in thread From: Michael L. Semon @ 2014-06-20 23:24 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs-oss On 06/18/2014 11:35 PM, Dave Chinner wrote: > 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? This patch works! The range from 4 ACL entries to the ACL limit seems to be fine to xfs_repair. No segfaults, and the ACL limit is OK for this case. >> 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... Indeed. All is well here, and hopefully, xfs_repair is a patch or two more ready for the masses. Good work! Michael _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-06-20 23:24 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-06-10 3:21 xfs_repair issue with ACLs on v5 XFS when beyond v4 limits Michael L. Semon 2014-06-10 5:52 ` Dave Chinner 2014-06-13 2:28 ` Michael L. Semon 2014-06-19 3:35 ` Dave Chinner 2014-06-20 23:24 ` Michael L. Semon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox