[PATCH 0/3] xfs: fix buf log item memory corruption on non-amd64

[PATCH 0/3] xfs: fix buf log item memory corruption on non-amd64

[Darrick J. Wong]

Hi all,

This second series corrects a memory corruption problem that I noticed
when running fstests on i386 and on a 64k-page aarch64 machine.  The
root cause is the fact that on v5 filesystems, a remote xattribute value
can be allocated 128K of disk space (64k for the value, 64 bytes for the
header).

xattr invalidation will try to xfs_trans_binval the attribute value
buffer, which creates a (zeroed) buffer log item.  The dirty buffer in
the buffer log item isn't large enough to handle > 64k of dirty data and
we write past the end of the array, corrupting memory.  On amd64 the
compiler inserts an invisible padding area just past the end of the
dirty bitmap, which is why we don't see the problem on our laptops. :P

Since we don't ever log remote xattr values, we can fix this problem by
making sure that no part of the code that handles remote attr values
ever supplies a transaction context to a xfs_buf function.  Finish the
series by adding a few asserts so that we'll shut down the log if this
kind of overrun ever happens again.

This has been lightly tested with fstests.  Enjoy!
Comments and questions are, as always, welcome.

--D

[PATCH 1/3] xfs: refactor remote attr value buffer invalidation

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

While running generic/103, I observed what looks like memory corruption
and (with slub debugging turned on) a slub redzone warning on i386 when
inactivating an inode with a 64k remote attr value.

On a v5 filesystem, maximally sized remote attr values require one block
more than 64k worth of space to hold both the remote attribute value
header (64 bytes).  On a 4k block filesystem this results in a 68k
buffer; on a 64k block filesystem, this would be a 128k buffer.  Note
that even though we'll never use more than 65,600 bytes of this buffer,
XFS_MAX_BLOCKSIZE is 64k.

This is a problem because the definition of struct xfs_buf_log_format
allows for XFS_MAX_BLOCKSIZE worth of dirty bitmap (64k).  On i386 when we
invalidate a remote attribute, xfs_trans_binval zeroes all 68k worth of
the dirty map, writing right off the end of the log item and corrupting
memory.  We've gotten away with this on x86_64 for years because the
compiler inserts a u32 padding on the end of struct xfs_buf_log_format.

Fortunately for us, remote attribute values are written to disk with
xfs_bwrite(), which is to say that they are not logged.  Fix the problem
by removing all places where we could end up creating a buffer log item
for a remote attribute value and leave a note explaining why.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr_remote.c |   66 ++++++++++++++++++++++++++++-----------
 fs/xfs/libxfs/xfs_attr_remote.h |    1 +
 fs/xfs/xfs_attr_inactive.c      |   29 +----------------
 3 files changed, 51 insertions(+), 45 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index a6ef5df42669..6ffe0eb8b422 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -25,6 +25,23 @@
 
 #define ATTR_RMTVALUE_MAPSIZE	1	/* # of map entries at once */
 
+/*
+ * Remote Attribute Values
+ * =======================
+ *
+ * Remote extended attribute values are conceptually simple -- they're written
+ * to data blocks mapped by an inode's attribute fork, and they have an upper
+ * size limit of 64k.  Setting a value does not involve the XFS log.
+ *
+ * However, on a v5 filesystem, maximally sized remote attr values require one
+ * block more than 64k worth of space to hold both the remote attribute value
+ * header (64 bytes).  On a 4k block filesystem this results in a 68k buffer;
+ * on a 64k block filesystem, this would be a 128k buffer.  Note that the log
+ * format can only handle a dirty buffer of XFS_MAX_BLOCKSIZE length (64k).
+ * Therefore, we /must/ ensure that remote attribute value buffers never touch
+ * the logging system and therefore never have a log item.
+ */
+
 /*
  * Each contiguous block has a header, so it is not just a simple attribute
  * length to FSB conversion.
@@ -401,7 +418,7 @@ xfs_attr_rmtval_get(
 			       (map[i].br_startblock != HOLESTARTBLOCK));
 			dblkno = XFS_FSB_TO_DADDR(mp, map[i].br_startblock);
 			dblkcnt = XFS_FSB_TO_BB(mp, map[i].br_blockcount);
-			error = xfs_trans_read_buf(mp, args->trans,
+			error = xfs_trans_read_buf(mp, NULL,
 						   mp->m_ddev_targp,
 						   dblkno, dblkcnt, 0, &bp,
 						   &xfs_attr3_rmt_buf_ops);
@@ -411,7 +428,7 @@ xfs_attr_rmtval_get(
 			error = xfs_attr_rmtval_copyout(mp, bp, args->dp->i_ino,
 							&offset, &valuelen,
 							&dst);
-			xfs_trans_brelse(args->trans, bp);
+			xfs_buf_relse(bp);
 			if (error)
 				return error;
 
@@ -552,6 +569,34 @@ xfs_attr_rmtval_set(
 	return 0;
 }
 
+/* Mark stale any buffers for the remote value. */
+void
+xfs_attr_rmtval_stale(
+	struct xfs_inode	*ip,
+	struct xfs_bmbt_irec	*map)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_buf		*bp;
+	xfs_daddr_t		dblkno;
+	int			dblkcnt;
+
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	if (map->br_startblock == HOLESTARTBLOCK)
+		return;
+
+	dblkno = XFS_FSB_TO_DADDR(mp, map->br_startblock),
+	dblkcnt = XFS_FSB_TO_BB(mp, map->br_blockcount);
+
+	/*
+	 * If the "remote" value is in the cache, remove it.
+	 */
+	bp = xfs_buf_incore(mp->m_ddev_targp, dblkno, dblkcnt, XBF_TRYLOCK);
+	if (bp) {
+		xfs_buf_stale(bp);
+		xfs_buf_relse(bp);
+	}
+}
+
 /*
  * Remove the value associated with an attribute by deleting the
  * out-of-line buffer that it is stored on.
@@ -560,7 +605,6 @@ int
 xfs_attr_rmtval_remove(
 	struct xfs_da_args	*args)
 {
-	struct xfs_mount	*mp = args->dp->i_mount;
 	xfs_dablk_t		lblkno;
 	int			blkcnt;
 	int			error;
@@ -575,9 +619,6 @@ xfs_attr_rmtval_remove(
 	blkcnt = args->rmtblkcnt;
 	while (blkcnt > 0) {
 		struct xfs_bmbt_irec	map;
-		struct xfs_buf		*bp;
-		xfs_daddr_t		dblkno;
-		int			dblkcnt;
 		int			nmap;
 
 		/*
@@ -592,18 +633,7 @@ xfs_attr_rmtval_remove(
 		ASSERT((map.br_startblock != DELAYSTARTBLOCK) &&
 		       (map.br_startblock != HOLESTARTBLOCK));
 
-		dblkno = XFS_FSB_TO_DADDR(mp, map.br_startblock),
-		dblkcnt = XFS_FSB_TO_BB(mp, map.br_blockcount);
-
-		/*
-		 * If the "remote" value is in the cache, remove it.
-		 */
-		bp = xfs_buf_incore(mp->m_ddev_targp, dblkno, dblkcnt, XBF_TRYLOCK);
-		if (bp) {
-			xfs_buf_stale(bp);
-			xfs_buf_relse(bp);
-			bp = NULL;
-		}
+		xfs_attr_rmtval_stale(args->dp, &map);
 
 		lblkno += map.br_blockcount;
 		blkcnt -= map.br_blockcount;
diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
index 9d20b66ad379..ce7287ac5b1f 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.h
+++ b/fs/xfs/libxfs/xfs_attr_remote.h
@@ -11,5 +11,6 @@ int xfs_attr3_rmt_blocks(struct xfs_mount *mp, int attrlen);
 int xfs_attr_rmtval_get(struct xfs_da_args *args);
 int xfs_attr_rmtval_set(struct xfs_da_args *args);
 int xfs_attr_rmtval_remove(struct xfs_da_args *args);
+void xfs_attr_rmtval_stale(struct xfs_inode *ip, struct xfs_bmbt_irec *map);
 
 #endif /* __XFS_ATTR_REMOTE_H__ */
diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
index 5ff49523d8ea..20453ce69a4b 100644
--- a/fs/xfs/xfs_attr_inactive.c
+++ b/fs/xfs/xfs_attr_inactive.c
@@ -36,11 +36,8 @@ xfs_attr3_leaf_freextent(
 	int			blkcnt)
 {
 	struct xfs_bmbt_irec	map;
-	struct xfs_buf		*bp;
 	xfs_dablk_t		tblkno;
-	xfs_daddr_t		dblkno;
 	int			tblkcnt;
-	int			dblkcnt;
 	int			nmap;
 	int			error;
 
@@ -63,35 +60,13 @@ xfs_attr3_leaf_freextent(
 		ASSERT(nmap == 1);
 		ASSERT(map.br_startblock != DELAYSTARTBLOCK);
 
-		/*
-		 * If it's a hole, these are already unmapped
-		 * so there's nothing to invalidate.
-		 */
-		if (map.br_startblock != HOLESTARTBLOCK) {
-
-			dblkno = XFS_FSB_TO_DADDR(dp->i_mount,
-						  map.br_startblock);
-			dblkcnt = XFS_FSB_TO_BB(dp->i_mount,
-						map.br_blockcount);
-			bp = xfs_trans_get_buf(*trans,
-					dp->i_mount->m_ddev_targp,
-					dblkno, dblkcnt, 0);
-			if (!bp)
-				return -ENOMEM;
-			xfs_trans_binval(*trans, bp);
-			/*
-			 * Roll to next transaction.
-			 */
-			error = xfs_trans_roll_inode(trans, dp);
-			if (error)
-				return error;
-		}
+		xfs_attr_rmtval_stale(dp, &map);
 
 		tblkno += map.br_blockcount;
 		tblkcnt -= map.br_blockcount;
 	}
 
-	return 0;
+	return xfs_trans_roll_inode(trans, dp);
 }
 
 /*

[PATCH 2/3] xfs: complain if anyone tries to create a too-large buffer log item

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Complain if someone calls xfs_buf_item_init on a buffer that is larger
than the dirty bitmap can handle, or tries to log a region that's past
the end of the dirty bitmap.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_buf_item.c |   23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)


diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 3984779e5911..bfbe8a5b8959 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -761,18 +761,25 @@ xfs_buf_item_init(
 	 * buffer. This makes the implementation as simple as possible.
 	 */
 	error = xfs_buf_item_get_format(bip, bp->b_map_count);
-	ASSERT(error == 0);
-	if (error) {	/* to stop gcc throwing set-but-unused warnings */
-		kmem_cache_free(xfs_buf_item_zone, bip);
-		return error;
+	if (error) {
+		xfs_err(mp, "could not allocate buffer item, err=%d", error);
+		goto err;
 	}
 
-
 	for (i = 0; i < bip->bli_format_count; i++) {
 		chunks = DIV_ROUND_UP(BBTOB(bp->b_maps[i].bm_len),
 				      XFS_BLF_CHUNK);
 		map_size = DIV_ROUND_UP(chunks, NBWORD);
 
+		if (map_size > XFS_BLF_DATAMAP_SIZE) {
+			xfs_err(mp,
+	"buffer item dirty bitmap (%u uints) too small to reflect %u bytes!",
+					map_size,
+					BBTOB(bp->b_maps[i].bm_len));
+			error = -EFSCORRUPTED;
+			goto err;
+		}
+
 		bip->bli_formats[i].blf_type = XFS_LI_BUF;
 		bip->bli_formats[i].blf_blkno = bp->b_maps[i].bm_bn;
 		bip->bli_formats[i].blf_len = bp->b_maps[i].bm_len;
@@ -782,6 +789,9 @@ xfs_buf_item_init(
 	bp->b_log_item = bip;
 	xfs_buf_hold(bp);
 	return 0;
+err:
+	kmem_cache_free(xfs_buf_item_zone, bip);
+	return error;
 }
 
 
@@ -805,6 +815,9 @@ xfs_buf_item_log_segment(
 	uint		end_bit;
 	uint		mask;
 
+	ASSERT(first < XFS_BLF_DATAMAP_SIZE * XFS_BLF_CHUNK * NBWORD);
+	ASSERT(last < XFS_BLF_DATAMAP_SIZE * XFS_BLF_CHUNK * NBWORD);
+
 	/*
 	 * Convert byte offsets to bit numbers.
 	 */

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

[Darrick J. Wong]

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

Re: [PATCH 1/3] xfs: refactor remote attr value buffer invalidation

[Christoph Hellwig]

The refactor in the subject is very misleading.  You are not refactoring
code, but fixing a bug.

> -			error = xfs_trans_read_buf(mp, args->trans,
> +			error = xfs_trans_read_buf(mp, NULL,
>  						   mp->m_ddev_targp,
>  						   dblkno, dblkcnt, 0, &bp,
>  						   &xfs_attr3_rmt_buf_ops);

xfs_trans_read_buf with an always NULL tp is a strange interface.  Any
reason not to use xfs_buf_read directly?

> +/* Mark stale any buffers for the remote value. */
> +void
> +xfs_attr_rmtval_stale(
> +	struct xfs_inode	*ip,
> +	struct xfs_bmbt_irec	*map)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_buf		*bp;
> +	xfs_daddr_t		dblkno;
> +	int			dblkcnt;
> +
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> +	if (map->br_startblock == HOLESTARTBLOCK)
> +		return;
> +
> +	dblkno = XFS_FSB_TO_DADDR(mp, map->br_startblock),
> +	dblkcnt = XFS_FSB_TO_BB(mp, map->br_blockcount);

Now this helper seems like a real refactoring in that it splits out a
common helper.  It matches one o the call sites exactly, while the
other has a major change, so I think it shouldn't just be one extra
patch, but instead of two extra patche to clearly document the changes.

> -		/*
> -		 * If it's a hole, these are already unmapped
> -		 * so there's nothing to invalidate.
> -		 */
> -		if (map.br_startblock != HOLESTARTBLOCK) {

Isn't this something we should keep in the caller?  That way the actual
invalide helper can assert that the map contains neither a hole or
a delaystartblock.

> -			bp = xfs_trans_get_buf(*trans,
> -					dp->i_mount->m_ddev_targp,
> -					dblkno, dblkcnt, 0);
> -			if (!bp)
> -				return -ENOMEM;
> -			xfs_trans_binval(*trans, bp);

And this is a pretty big change in that we now trylock and never read
a buffer from disk if it isn't in core.  That change looks fine to me
from trying to understand what is going on, but it clearly needs to
be split out and documented.

> -			/*
> -			 * Roll to next transaction.
> -			 */
> -			error = xfs_trans_roll_inode(trans, dp);
> -			if (error)
> -				return error;
> -		}
> +		xfs_attr_rmtval_stale(dp, &map);
>  
>  		tblkno += map.br_blockcount;
>  		tblkcnt -= map.br_blockcount;
>  	}
>  
> -	return 0;
> +	return xfs_trans_roll_inode(trans, dp);

xfs_attr3_leaf_freextent not doesn't do anything with the trans but
rolling it.  I think you can drop both the roll and the trans argument.

Re: [PATCH 2/3] xfs: complain if anyone tries to create a too-large buffer log item

[Christoph Hellwig]

On Tue, Jan 07, 2020 at 08:18:19PM -0800, Darrick J. Wong wrote:
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 3984779e5911..bfbe8a5b8959 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -761,18 +761,25 @@ xfs_buf_item_init(
>  	 * buffer. This makes the implementation as simple as possible.
>  	 */
>  	error = xfs_buf_item_get_format(bip, bp->b_map_count);
> -	ASSERT(error == 0);
> -	if (error) {	/* to stop gcc throwing set-but-unused warnings */
> -		kmem_cache_free(xfs_buf_item_zone, bip);
> -		return error;
> +	if (error) {
> +		xfs_err(mp, "could not allocate buffer item, err=%d", error);
> +		goto err;
>  	}

The error handling here is weird, as xfs_buf_item_get_format can't fail
to start with.  I'd rather see a prep patch removing the bogus check
for the kmem_zalloc and change the return value from
xfs_buf_item_get_format to void.

Otherwise the patch looks good.

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

[Christoph Hellwig]

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?

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

[Darrick J. Wong]

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

Re: [PATCH 1/3] xfs: refactor remote attr value buffer invalidation

[Darrick J. Wong]

On Wed, Jan 08, 2020 at 12:49:22AM -0800, Christoph Hellwig wrote:
> The refactor in the subject is very misleading.  You are not refactoring
> code, but fixing a bug.

Ok, I'll make that clearer.

> > -			error = xfs_trans_read_buf(mp, args->trans,
> > +			error = xfs_trans_read_buf(mp, NULL,
> >  						   mp->m_ddev_targp,
> >  						   dblkno, dblkcnt, 0, &bp,
> >  						   &xfs_attr3_rmt_buf_ops);
> 
> xfs_trans_read_buf with an always NULL tp is a strange interface.  Any
> reason not to use xfs_buf_read directly?

If the remote value checksum fails validation, xfs_trans_read_buf will
collapse EFSBADCRC to EFSCORRUPTED.  It'll also take care of releasing
the buffer.

I agree that xfs_buf_read is a more logical choice here, but it doesn't
do those things and I think we'd be better off changing xfs_buf_read
(and _buf_get) to return EFSBADCRC/EFSCORRUPTED/ENOMEM.

> > +/* Mark stale any buffers for the remote value. */
> > +void
> > +xfs_attr_rmtval_stale(
> > +	struct xfs_inode	*ip,
> > +	struct xfs_bmbt_irec	*map)
> > +{
> > +	struct xfs_mount	*mp = ip->i_mount;
> > +	struct xfs_buf		*bp;
> > +	xfs_daddr_t		dblkno;
> > +	int			dblkcnt;
> > +
> > +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> > +	if (map->br_startblock == HOLESTARTBLOCK)
> > +		return;
> > +
> > +	dblkno = XFS_FSB_TO_DADDR(mp, map->br_startblock),
> > +	dblkcnt = XFS_FSB_TO_BB(mp, map->br_blockcount);
> 
> Now this helper seems like a real refactoring in that it splits out a
> common helper.  It matches one o the call sites exactly, while the
> other has a major change, so I think it shouldn't just be one extra
> patch, but instead of two extra patche to clearly document the changes.

Ok.

> > -		/*
> > -		 * If it's a hole, these are already unmapped
> > -		 * so there's nothing to invalidate.
> > -		 */
> > -		if (map.br_startblock != HOLESTARTBLOCK) {
> 
> Isn't this something we should keep in the caller?  That way the actual
> invalide helper can assert that the map contains neither a hole or
> a delaystartblock.

Yeah, we could keep that in the caller.

> > -			bp = xfs_trans_get_buf(*trans,
> > -					dp->i_mount->m_ddev_targp,
> > -					dblkno, dblkcnt, 0);
> > -			if (!bp)
> > -				return -ENOMEM;
> > -			xfs_trans_binval(*trans, bp);
> 
> And this is a pretty big change in that we now trylock and never read
> a buffer from disk if it isn't in core.  That change looks fine to me
> from trying to understand what is going on, but it clearly needs to
> be split out and documented.

<nod>

"Find any incore buffers associated with the remote attr value and mark
them stale so they go away."

> > -			/*
> > -			 * Roll to next transaction.
> > -			 */
> > -			error = xfs_trans_roll_inode(trans, dp);
> > -			if (error)
> > -				return error;
> > -		}
> > +		xfs_attr_rmtval_stale(dp, &map);
> >  
> >  		tblkno += map.br_blockcount;
> >  		tblkcnt -= map.br_blockcount;
> >  	}
> >  
> > -	return 0;
> > +	return xfs_trans_roll_inode(trans, dp);
> 
> xfs_attr3_leaf_freextent not doesn't do anything with the trans but
> rolling it.  I think you can drop both the roll and the trans argument.

Yeah, I was 90% convinced of that too.  That'll be another prep patch.

--D

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

[Darrick J. Wong]

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

Re: [PATCH 2/3] xfs: complain if anyone tries to create a too-large buffer log item

[Darrick J. Wong]

On Wed, Jan 08, 2020 at 12:51:31AM -0800, Christoph Hellwig wrote:
> On Tue, Jan 07, 2020 at 08:18:19PM -0800, Darrick J. Wong wrote:
> > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> > index 3984779e5911..bfbe8a5b8959 100644
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -761,18 +761,25 @@ xfs_buf_item_init(
> >  	 * buffer. This makes the implementation as simple as possible.
> >  	 */
> >  	error = xfs_buf_item_get_format(bip, bp->b_map_count);
> > -	ASSERT(error == 0);
> > -	if (error) {	/* to stop gcc throwing set-but-unused warnings */
> > -		kmem_cache_free(xfs_buf_item_zone, bip);
> > -		return error;
> > +	if (error) {
> > +		xfs_err(mp, "could not allocate buffer item, err=%d", error);
> > +		goto err;
> >  	}
> 
> The error handling here is weird, as xfs_buf_item_get_format can't fail
> to start with.  I'd rather see a prep patch removing the bogus check
> for the kmem_zalloc and change the return value from
> xfs_buf_item_get_format to void.
> 
> Otherwise the patch looks good.

Ok.

--D

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

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

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

[Darrick J. Wong]

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