Re: [PATCH 3/3] xfs: make struct xfs_buf_log_format have a consistent size

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Jan 08, 2020 at 12:54:02AM -0800, Christoph Hellwig wrote:
> On Tue, Jan 07, 2020 at 08:18:25PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Increase XFS_BLF_DATAMAP_SIZE by 1 to fill in the implied padding at the
> > end of struct xfs_buf_log_format.  This makes the size consistent so
> > that we can check it in xfs_ondisk.h, and will be needed once we start
> > logging attribute values.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_log_format.h |    9 +++++----
> >  fs/xfs/xfs_ondisk.h            |    1 +
> >  2 files changed, 6 insertions(+), 4 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> > index 8ef31d71a9c7..5d8eb8978c33 100644
> > --- a/fs/xfs/libxfs/xfs_log_format.h
> > +++ b/fs/xfs/libxfs/xfs_log_format.h
> > @@ -462,11 +462,12 @@ static inline uint xfs_log_dinode_size(int version)
> >  #define	XFS_BLF_GDQUOT_BUF	(1<<4)
> >  
> >  /*
> > - * This is the structure used to lay out a buf log item in the
> > - * log.  The data map describes which 128 byte chunks of the buffer
> > - * have been logged.
> > + * This is the structure used to lay out a buf log item in the log.  The data
> > + * map describes which 128 byte chunks of the buffer have been logged.  Note
> > + * that XFS_BLF_DATAMAP_SIZE is an odd number so that the structure size will
> > + * be consistent between 32-bit and 64-bit platforms.
> >   */
> > -#define XFS_BLF_DATAMAP_SIZE	((XFS_MAX_BLOCKSIZE / XFS_BLF_CHUNK) / NBWORD)
> > +#define XFS_BLF_DATAMAP_SIZE	(1 + ((XFS_MAX_BLOCKSIZE / XFS_BLF_CHUNK) / NBWORD))
> 
> I don't understand the explanation.  Why would the size differ for
> 32-bit vs 64-bit architectures when it only uses fixed size types?

The structure is 84 bytes in length, which is not an even multiple of 8.
The reason for this is that the end of the structure are 17 unsigned
ints (blf_map_size + blf_map_data).

The blf_blkno field is int64_t, which on amd64 causes the compiler to
round the the structure size up to the nearest 8-byte boundary, or 88
bytes:

/* <1897d> /storage/home/djwong/cdev/work/linux-xfs/fs/xfs/libxfs/xfs_log_format.h:477 */
struct xfs_buf_log_format {
        short unsigned int         blf_type;                                             /*     0     2 */
        short unsigned int         blf_size;                                             /*     2     2 */
        short unsigned int         blf_flags;                                            /*     4     2 */
        short unsigned int         blf_len;                                              /*     6     2 */
        /* typedef int64_t -> s64 -> __s64 */ long long int              blf_blkno;      /*     8     8 */
        unsigned int               blf_map_size;                                         /*    16     4 */
        unsigned int               blf_data_map[17];                                     /*    20    68 */
        /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */

        /* size: 88, cachelines: 2, members: 7 */
        /* last cacheline: 24 bytes */
};

(Same thing with aarch64 and ppc64le gcc.)

i386 gcc doesn't do any of this rounding, so the size is 84 bytes:

/* <182ef> /storage/home/djwong/cdev/work/linux-xfs/fs/xfs/libxfs/xfs_log_format.h:476 */
struct xfs_buf_log_format {
        short unsigned int         blf_type;                                             /*     0     2 */
        short unsigned int         blf_size;                                             /*     2     2 */
        short unsigned int         blf_flags;                                            /*     4     2 */
        short unsigned int         blf_len;                                              /*     6     2 */
        /* typedef int64_t -> s64 -> __s64 */ long long int              blf_blkno;      /*     8     8 */
        unsigned int               blf_map_size;                                         /*    16     4 */
        unsigned int               blf_data_map[16];                                     /*    20    64 */
        /* --- cacheline 1 boundary (64 bytes) was 20 bytes ago --- */

        /* size: 84, cachelines: 2, members: 7 */
        /* last cacheline: 20 bytes */
};

Since we accidentally write to blf_data_map[17] when invalidating a 68k
buffer, that write will corrupt the slab's redzone, or worse, a live
object packed in right after it.

--D