xfs
[Top] [All Lists]

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

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH] xfs: avoid firstused overflow in attr3 leaf header with 64k blocks
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 24 Feb 2015 08:58:02 +1100
Cc: Brian Foster <bfoster@xxxxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <54EB8E5D.8080905@xxxxxxxxxxx>
References: <1424722050-24149-1-git-send-email-bfoster@xxxxxxxxxx> <54EB8E5D.8080905@xxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxxxxxxxx

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