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@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.
> >
> > Reported-by: Michael L. Semon <mlsemon35@xxxxxxxxx>
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > ---
>
> 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@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
