diff mbox series

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

Message ID 157845710512.84011.14528616369807048509.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: fix buf log item memory corruption on non-amd64 | expand

Commit Message

Darrick J. Wong Jan. 8, 2020, 4:18 a.m. UTC 
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(-)

Comments

Christoph Hellwig Jan. 8, 2020, 8:54 a.m. UTC | #1 
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?
Darrick J. Wong Jan. 8, 2020, 4:32 p.m. UTC | #2 
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
Darrick J. Wong Jan. 8, 2020, 5:17 p.m. UTC | #3 
On Wed, Jan 08, 2020 at 08:32:29AM -0800, Darrick J. Wong wrote:
> 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 */
> };

And of course I forgot to pop the patch before building and pahole'ing,
so here's the correct version from x86_64:

/* <1897d> /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: 88, cachelines: 2, members: 7 */
        /* padding: 4 */
        /* last cacheline: 24 bytes */
};

--D

> (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
Dave Chinner Jan. 8, 2020, 9:51 p.m. UTC | #4 
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))

Shouldn't this do something like:

#define __XFS_BLF_DATAMAP_SIZE	((XFS_MAX_BLOCKSIZE / XFS_BLF_CHUNK) / NBWORD)
#define XFS_BLF_DATAMAP_SIZE	(__XFS_BLF_DATAMAP_SIZE + (__XFS_BLF_DATAMAP_SIZE & 1))

or use an appropriate round up macro so that if we change the log
chunk size or the max block size, it still evaluates correctly for
all platforms?

/me has prototype patches around somewhere that change
XFS_BLF_CHUNK...

Cheers,

Dave.
Darrick J. Wong Jan. 8, 2020, 10:33 p.m. UTC | #5 
On Thu, Jan 09, 2020 at 08:51:20AM +1100, Dave Chinner 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))
> 
> Shouldn't this do something like:
> 
> #define __XFS_BLF_DATAMAP_SIZE	((XFS_MAX_BLOCKSIZE / XFS_BLF_CHUNK) / NBWORD)
> #define XFS_BLF_DATAMAP_SIZE	(__XFS_BLF_DATAMAP_SIZE + (__XFS_BLF_DATAMAP_SIZE & 1))
> 
> or use an appropriate round up macro so that if we change the log
> chunk size or the max block size, it still evaluates correctly for
> all platforms?

Ok, works for me.

> /me has prototype patches around somewhere that change
> XFS_BLF_CHUNK...

I remember those...

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
diff mbox series

Patch

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))
 
 typedef struct xfs_buf_log_format {
 	unsigned short	blf_type;	/* buf log item type indicator */
diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index b6701b4f59a9..5f04d8a5ab2a 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -111,6 +111,7 @@  xfs_check_ondisk_structs(void)
 	XFS_CHECK_STRUCT_SIZE(xfs_dir2_sf_hdr_t,		10);
 
 	/* log structures */
+	XFS_CHECK_STRUCT_SIZE(struct xfs_buf_log_format,	88);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_dq_logformat,		24);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_32,	28);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_64,	32);