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

[Eric Sandeen <sandeen@xxxxxxxxxxx>]

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;

> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 15105db..dc7bda3 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -970,7 +970,8 @@ xfs_attr3_leaf_create(
>       memset(leaf, 0, args->geo->blksize);
>  
>       memset(&ichdr, 0, sizeof(ichdr));
> -     ichdr.firstused = args->geo->blksize;
> +     /* firstused is 16-bit */
> +     ichdr.firstused = min_t(int, USHRT_MAX, args->geo->blksize);
>  
>       if (xfs_sb_version_hascrc(&mp->m_sb)) {
>               struct xfs_da3_blkinfo *hdr3 = bp->b_addr;
> @@ -986,7 +987,7 @@ xfs_attr3_leaf_create(
>               ichdr.magic = XFS_ATTR_LEAF_MAGIC;
>               ichdr.freemap[0].base = sizeof(struct xfs_attr_leaf_hdr);
>       }
> -     ichdr.freemap[0].size = ichdr.firstused - ichdr.freemap[0].base;
> +     ichdr.freemap[0].size = args->geo->blksize - ichdr.freemap[0].base;

But now freemap.size is out of sync with firstused; is that ok?

-Eric
 
>       xfs_attr3_leaf_hdr_to_disk(leaf, &ichdr);
>       xfs_trans_log_buf(args->trans, bp, 0, args->geo->blksize - 1);
>