Re: [PATCH 2/3] xfs: fix implicit padding in directory and attr CRC formats

Ben Myers
2013-06-13T00:58:20Z

On Wed, Jun 12, 2013 at 12:19:07PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Michael L. Semon has been testing CRC patches ona 32 bit system and
on a

> been seeing assert failures in the directory code from xfs/080.
> Thanks to Michael's heroic efforts with printk debugging, we found
> that the problem was that the last free space being left in the
> directory structure was too small to fit a unused tag structure and
> it was being corrupted and attempting to log a region out of bounds.
> Hence the assert failure looked something like:
>
> .....
> #5 calling xfs_dir2_data_log_unused() 36 32
> #1 4092 4095 4096
> #2 8182 8183 4096
first?
last?
bp->b_length?

> XFS: Assertion failed: first <= last && last < BBTOB(bp->b_length), file: fs/xfs/xfs_trans_buf.c, line: 568
>
> Where #1 showed the first region of the dup being logged (i.e. the
> last 4 bytes of a directory buffer) and #2 shows the corrupt values
> being calculated from the length of the dup entry which overflowed
> the size of the buffer.
>
> It turns out that the problem was not in the logging code, nor in
> the freespace handling code. It is an initial condition bug that
> only shows up on 32 bit systems. When a new buffer is initialised,
> where's the freespace that is set up:
>
> [ 172.316249] calling xfs_dir2_leaf_addname() from xfs_dir_createname()
> [ 172.316346] #9 calling xfs_dir2_data_log_unused()
> [ 172.316351] #1 calling xfs_trans_log_buf() 60 63 4096
> [ 172.316353] #2 calling xfs_trans_log_buf() 4094 4095 4096
>
> Note the offset of the first region being logged? It's 60 bytes into
> the buffer. Once I saw that, I pretty much knew what the bug was
> going to be caused by this.
>
> Essentially, all direct entries are rounded to 8 bytes in length,
> and all entries start with an 8 byte alignment. This means that we
> can decode inplace as variables are naturally aligned. With the
> directory data supposedly starting on a 8 byte boundary, and all
> entries padded to 8 bytes, the minimum freespace in a directory
> block is supposed to be 8 bytes, which is large enough to fit a
> unused data entry structure (6 bytes in size). The fact we only have
> 4 bytes of free space indicates a directory data block alignment
> problem.
>
> And what do you know - there's an implicit hole in the directory
> data block header for the CRC format, which means the header is 60
> byte on 32 bit intel systems and 64 bytes on 64 bit systems. Needs
> padding. And while looking at the structures, I found the same
> problem in the attr leaf header. Fix them both.
>
> Note that this only affects 32 bit systems with CRCs enabled.
> Everything else is just fine. Note that filesystems created before
CRC enabled filesystems

I suggest this be added to head off any confusion.

> this fix on such systems will not be readable with this fix applied.
>
> Reported-by: Michael L. Semon <mlsemon35@gmail.com>
> Debugged-by: Michael L. Semon <mlsemon35@gmail.com>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> fs/xfs/xfs_attr_leaf.h | 1 +
> fs/xfs/xfs_dir2_format.h | 5 +++--
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_attr_leaf.h b/fs/xfs/xfs_attr_leaf.h
> index f9d7846..444a770 100644
> --- a/fs/xfs/xfs_attr_leaf.h
> +++ b/fs/xfs/xfs_attr_leaf.h
> @@ -128,6 +128,7 @@ struct xfs_attr3_leaf_hdr {
> __u8 holes;
> __u8 pad1;
> struct xfs_attr_leaf_map freemap[XFS_ATTR_LEAF_MAPSIZE];
> + __be32 pad2; /* 64 bit alignment */
> };
>
> #define XFS_ATTR3_LEAF_CRC_OFF (offsetof(struct xfs_attr3_leaf_hdr, info.crc))
> diff --git a/fs/xfs/xfs_dir2_format.h b/fs/xfs/xfs_dir2_format.h
> index 995f1f5..7826782 100644
> --- a/fs/xfs/xfs_dir2_format.h
> +++ b/fs/xfs/xfs_dir2_format.h
> @@ -266,6 +266,7 @@ struct xfs_dir3_blk_hdr {
> struct xfs_dir3_data_hdr {
> struct xfs_dir3_blk_hdr hdr;
> xfs_dir2_data_free_t best_free[XFS_DIR2_DATA_FD_COUNT];
> + __be32 pad; /* 64 bit alignment */

I counted these up and it looks fine. Nice work gents.

Reviewed-by: Ben Myers <bpm@sgi.com>