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 |
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?
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
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
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.
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 --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);