Re: [PATCH] xfs: avoid firstused overflow in attr3 leaf header with 64k blocks

[Dave Chinner <david@fromorbit.com>]

On Mon, Feb 23, 2015 at 02:32:29PM -0600, Eric Sandeen wrote:
> On 2/23/15 2:07 PM, Brian Foster wrote:
> > The attr3 leaf header has a 16-bit firstused field that tracks the first
> > used entry offset. This field is initialized to the block size in
> > xfs_attr3_leaf_create() and updated accordingly in
> > xfs_attr3_leaf_add_work() when new attributes are added.
> > 
> > The initialization of firstused overflows if the block size exceeds
> > 16-bits. E.g., xfstests test generic/117 causes assert failures on a
> > -bsize=64k fs on ppc64 because ichdr.firstused evaluates to 0.
> 
> cool :)
> 
> > Update the firstused initialization to not exceed the maximum value of
> > an unsigned short. This avoids the overflow to 0 and allows firstused to
> > be updated appropriately on subsequent xattr addition. Also update the
> > freemap size calculation to use the actual block size rather than the
> > potentially minimized version stored in firstused.
> 
> I'm a little scared by this; does this truncated value risk going to disk?
> (Yes, I think so.)  Is that ok?	 Does that ... mean we lose a byte of space
> we'd otherwise have?  Maybe that's ok ...
> 
> FWIW, I think the same problem exists in xfs_attr3_leaf_compact():
> 
>         /* Initialise the incore headers */
>         ichdr_src = *ichdr_dst; /* struct copy */
>         ichdr_dst->firstused = args->geo->blksize;
> 
> and xfs_attr3_leaf_unbalance():
> 
>                 tmphdr.firstused = state->args->geo->blksize;

And a loop in xfs_attr3_leaf_remove() that does:

		tmp = args->geo->blksize;
		.....
		ichdr.firstused = tmp;

so if the the loop in between does not modify tmp...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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