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@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>
> > > > ---
> > >
> ...
> >
> > > > @@ -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@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
