[PATCHSET 0/6] xfs: fix various problems with log intent item recovery

[PATCHSET 0/6] xfs: fix various problems with log intent item recovery

[Darrick J. Wong]

Hi all,

Starting with 6.1-rc1, CONFIG_FORTIFY_SOURCE checks became smart enough
to detect memcpy() callers that copy beyond what seems to be the end of
a struct.  Unfortunately, gcc has a bug wherein it cannot reliably
compute the size of a struct containing another struct containing a flex
array at the end.  This is the case with the xfs log item format
structures, which means that -rc1 starts complaining all over the place.

Fix these problems by memcpying the struct head and the flex arrays
separately.  Although it's tempting to use the FLEX_ARRAY macros, the
structs involved are part of the ondisk log format.  Some day we're
going to want to make the ondisk log contents endian-safe, which means
that we will have to stop using memcpy entirely.

While we're at it, fix some deficiencies in the validation of recovered
log intent items -- if the size of the recovery buffer is not even large
enough to cover the flex array record count in the head, we should abort
the recovery of that item immediately.

The last patch of this series changes the EFI/EFD sizeof functions names
and behaviors to be consistent with the similarly named sizeof helpers
for other log intent items.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=fix-log-recovery-misuse-6.1
---
 fs/xfs/libxfs/xfs_log_format.h |   60 ++++++++++++++++++++++++++++---
 fs/xfs/xfs_attr_item.c         |   54 ++++++++++++----------------
 fs/xfs/xfs_bmap_item.c         |   46 +++++++++++-------------
 fs/xfs/xfs_extfree_item.c      |   78 +++++++++++++++-------------------------
 fs/xfs/xfs_extfree_item.h      |   16 ++++++++
 fs/xfs/xfs_ondisk.h            |   23 ++++++++++--
 fs/xfs/xfs_refcount_item.c     |   45 +++++++++++------------
 fs/xfs/xfs_rmap_item.c         |   58 ++++++++++++++----------------
 fs/xfs/xfs_super.c             |   12 ++----
 9 files changed, 216 insertions(+), 176 deletions(-)

[PATCH 1/6] xfs: fix validation in attr log item recovery

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Before we start fixing all the complaints about memcpy'ing log items
around, let's fix some inadequate validation in the xattr log item
recovery code and get rid of the (now trivial) copy_format function.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_attr_item.c |   54 ++++++++++++++++++++----------------------------
 1 file changed, 23 insertions(+), 31 deletions(-)


diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index cf5ce607dc05..ee8f678a10a1 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -245,28 +245,6 @@ xfs_attri_init(
 	return attrip;
 }
 
-/*
- * Copy an attr format buffer from the given buf, and into the destination attr
- * format structure.
- */
-STATIC int
-xfs_attri_copy_format(
-	struct xfs_log_iovec		*buf,
-	struct xfs_attri_log_format	*dst_attr_fmt)
-{
-	struct xfs_attri_log_format	*src_attr_fmt = buf->i_addr;
-	size_t				len;
-
-	len = sizeof(struct xfs_attri_log_format);
-	if (buf->i_len != len) {
-		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
-		return -EFSCORRUPTED;
-	}
-
-	memcpy((char *)dst_attr_fmt, (char *)src_attr_fmt, len);
-	return 0;
-}
-
 static inline struct xfs_attrd_log_item *ATTRD_ITEM(struct xfs_log_item *lip)
 {
 	return container_of(lip, struct xfs_attrd_log_item, attrd_item);
@@ -731,24 +709,44 @@ xlog_recover_attri_commit_pass2(
 	struct xfs_attri_log_nameval	*nv;
 	const void			*attr_value = NULL;
 	const void			*attr_name;
-	int                             error;
+	size_t				len;
 
 	attri_formatp = item->ri_buf[0].i_addr;
 	attr_name = item->ri_buf[1].i_addr;
 
 	/* Validate xfs_attri_log_format before the large memory allocation */
+	len = sizeof(struct xfs_attri_log_format);
+	if (item->ri_buf[0].i_len != len) {
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
+		return -EFSCORRUPTED;
+	}
+
 	if (!xfs_attri_validate(mp, attri_formatp)) {
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
 		return -EFSCORRUPTED;
 	}
 
+	/* Validate the attr name */
+	if (item->ri_buf[1].i_len !=
+			xlog_calc_iovec_len(attri_formatp->alfi_name_len)) {
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
+		return -EFSCORRUPTED;
+	}
+
 	if (!xfs_attr_namecheck(attr_name, attri_formatp->alfi_name_len)) {
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
 		return -EFSCORRUPTED;
 	}
 
-	if (attri_formatp->alfi_value_len)
+	/* Validate the attr value, if present */
+	if (attri_formatp->alfi_value_len != 0) {
+		if (item->ri_buf[2].i_len != xlog_calc_iovec_len(attri_formatp->alfi_value_len)) {
+			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
+			return -EFSCORRUPTED;
+		}
+
 		attr_value = item->ri_buf[2].i_addr;
+	}
 
 	/*
 	 * Memory alloc failure will cause replay to abort.  We attach the
@@ -760,9 +758,7 @@ xlog_recover_attri_commit_pass2(
 			attri_formatp->alfi_value_len);
 
 	attrip = xfs_attri_init(mp, nv);
-	error = xfs_attri_copy_format(&item->ri_buf[0], &attrip->attri_format);
-	if (error)
-		goto out;
+	memcpy(&attrip->attri_format, attri_formatp, len);
 
 	/*
 	 * The ATTRI has two references. One for the ATTRD and one for ATTRI to
@@ -774,10 +770,6 @@ xlog_recover_attri_commit_pass2(
 	xfs_attri_release(attrip);
 	xfs_attri_log_nameval_put(nv);
 	return 0;
-out:
-	xfs_attri_item_free(attrip);
-	xfs_attri_log_nameval_put(nv);
-	return error;
 }
 
 /*

[PATCH 2/6] xfs: fix memcpy fortify errors in BUI log format copying

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of
memcpy.  Unfortunately, it doesn't handle flex arrays correctly:

------------[ cut here ]------------
memcpy: detected field-spanning write (size 48) of single field "dst_bui_fmt" at fs/xfs/xfs_bmap_item.c:628 (size 16)

Fix this by refactoring the xfs_bui_copy_format function to handle the
copying of the head and the flex array members separately.  While we're
at it, fix a minor validation deficiency in the recovery function.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_bmap_item.c |   46 ++++++++++++++++++++++------------------------
 fs/xfs/xfs_ondisk.h    |    5 +++++
 2 files changed, 27 insertions(+), 24 deletions(-)


diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 51f66e982484..a1da6205252b 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -608,28 +608,18 @@ static const struct xfs_item_ops xfs_bui_item_ops = {
 	.iop_relog	= xfs_bui_item_relog,
 };
 
-/*
- * Copy an BUI format buffer from the given buf, and into the destination
- * BUI format structure.  The BUI/BUD items were designed not to need any
- * special alignment handling.
- */
-static int
+static inline void
 xfs_bui_copy_format(
-	struct xfs_log_iovec		*buf,
-	struct xfs_bui_log_format	*dst_bui_fmt)
+	struct xfs_bui_log_format	*dst,
+	const struct xfs_bui_log_format	*src)
 {
-	struct xfs_bui_log_format	*src_bui_fmt;
-	uint				len;
+	unsigned int			i;
 
-	src_bui_fmt = buf->i_addr;
-	len = xfs_bui_log_format_sizeof(src_bui_fmt->bui_nextents);
+	memcpy(dst, src, offsetof(struct xfs_bui_log_format, bui_extents));
 
-	if (buf->i_len == len) {
-		memcpy(dst_bui_fmt, src_bui_fmt, len);
-		return 0;
-	}
-	XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
-	return -EFSCORRUPTED;
+	for (i = 0; i < src->bui_nextents; i++)
+		memcpy(&dst->bui_extents[i], &src->bui_extents[i],
+				sizeof(struct xfs_map_extent));
 }
 
 /*
@@ -646,23 +636,31 @@ xlog_recover_bui_commit_pass2(
 	struct xlog_recover_item	*item,
 	xfs_lsn_t			lsn)
 {
-	int				error;
 	struct xfs_mount		*mp = log->l_mp;
 	struct xfs_bui_log_item		*buip;
 	struct xfs_bui_log_format	*bui_formatp;
+	size_t				len;
 
 	bui_formatp = item->ri_buf[0].i_addr;
 
+	if (item->ri_buf[0].i_len < xfs_bui_log_format_sizeof(0)) {
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
+		return -EFSCORRUPTED;
+	}
+
 	if (bui_formatp->bui_nextents != XFS_BUI_MAX_FAST_EXTENTS) {
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
 		return -EFSCORRUPTED;
 	}
+
+	len = xfs_bui_log_format_sizeof(bui_formatp->bui_nextents);
+	if (item->ri_buf[0].i_len != len) {
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
+		return -EFSCORRUPTED;
+	}
+
 	buip = xfs_bui_init(mp);
-	error = xfs_bui_copy_format(&item->ri_buf[0], &buip->bui_format);
-	if (error) {
-		xfs_bui_item_free(buip);
-		return error;
-	}
+	xfs_bui_copy_format(&buip->bui_format, bui_formatp);
 	atomic_set(&buip->bui_next_extent, bui_formatp->bui_nextents);
 	/*
 	 * Insert the intent into the AIL directly and drop one reference so
diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index 758702b9495f..56917e236370 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -134,6 +134,11 @@ xfs_check_ondisk_structs(void)
 	XFS_CHECK_STRUCT_SIZE(struct xfs_trans_header,		16);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_attri_log_format,	40);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_attrd_log_format,	16);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_bui_log_format,	16);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_bud_log_format,	16);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_map_extent,		32);
+
+	XFS_CHECK_OFFSET(struct xfs_bui_log_format, bui_extents,	16);
 
 	/*
 	 * The v5 superblock format extended several v4 header structures with

[PATCH 3/6] xfs: fix memcpy fortify errors in CUI log format copying

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of
memcpy.  Since we're already fixing problems with BUI item copying, we
should fix it everything else.

Refactor the xfs_cui_copy_format function to handle the copying of the
head and the flex array members separately.  While we're at it, fix a
minor validation deficiency in the recovery function.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_ondisk.h        |    4 ++++
 fs/xfs/xfs_refcount_item.c |   45 +++++++++++++++++++++-----------------------
 2 files changed, 25 insertions(+), 24 deletions(-)


diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index 56917e236370..e20d2844b0c5 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -136,9 +136,13 @@ xfs_check_ondisk_structs(void)
 	XFS_CHECK_STRUCT_SIZE(struct xfs_attrd_log_format,	16);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_bui_log_format,	16);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_bud_log_format,	16);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_cui_log_format,	16);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_cud_log_format,	16);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_map_extent,		32);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_phys_extent,		16);
 
 	XFS_CHECK_OFFSET(struct xfs_bui_log_format, bui_extents,	16);
+	XFS_CHECK_OFFSET(struct xfs_cui_log_format, cui_extents,	16);
 
 	/*
 	 * The v5 superblock format extended several v4 header structures with
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 7e97bf19793d..24cf4c64ebaa 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -622,28 +622,18 @@ static const struct xfs_item_ops xfs_cui_item_ops = {
 	.iop_relog	= xfs_cui_item_relog,
 };
 
-/*
- * Copy an CUI format buffer from the given buf, and into the destination
- * CUI format structure.  The CUI/CUD items were designed not to need any
- * special alignment handling.
- */
-static int
+static inline void
 xfs_cui_copy_format(
-	struct xfs_log_iovec		*buf,
-	struct xfs_cui_log_format	*dst_cui_fmt)
+	struct xfs_cui_log_format	*dst,
+	const struct xfs_cui_log_format	*src)
 {
-	struct xfs_cui_log_format	*src_cui_fmt;
-	uint				len;
+	unsigned int			i;
 
-	src_cui_fmt = buf->i_addr;
-	len = xfs_cui_log_format_sizeof(src_cui_fmt->cui_nextents);
+	memcpy(dst, src, offsetof(struct xfs_cui_log_format, cui_extents));
 
-	if (buf->i_len == len) {
-		memcpy(dst_cui_fmt, src_cui_fmt, len);
-		return 0;
-	}
-	XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
-	return -EFSCORRUPTED;
+	for (i = 0; i < src->cui_nextents; i++)
+		memcpy(&dst->cui_extents[i], &src->cui_extents[i],
+				sizeof(struct xfs_phys_extent));
 }
 
 /*
@@ -660,19 +650,26 @@ xlog_recover_cui_commit_pass2(
 	struct xlog_recover_item	*item,
 	xfs_lsn_t			lsn)
 {
-	int				error;
 	struct xfs_mount		*mp = log->l_mp;
 	struct xfs_cui_log_item		*cuip;
 	struct xfs_cui_log_format	*cui_formatp;
+	size_t				len;
 
 	cui_formatp = item->ri_buf[0].i_addr;
 
+	if (item->ri_buf[0].i_len < xfs_cui_log_format_sizeof(0)) {
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
+		return -EFSCORRUPTED;
+	}
+
+	len = xfs_cui_log_format_sizeof(cui_formatp->cui_nextents);
+	if (item->ri_buf[0].i_len != len) {
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
+		return -EFSCORRUPTED;
+	}
+
 	cuip = xfs_cui_init(mp, cui_formatp->cui_nextents);
-	error = xfs_cui_copy_format(&item->ri_buf[0], &cuip->cui_format);
-	if (error) {
-		xfs_cui_item_free(cuip);
-		return error;
-	}
+	xfs_cui_copy_format(&cuip->cui_format, cui_formatp);
 	atomic_set(&cuip->cui_next_extent, cui_formatp->cui_nextents);
 	/*
 	 * Insert the intent into the AIL directly and drop one reference so

[PATCH 4/6] xfs: fix memcpy fortify errors in RUI log format copying

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of
memcpy.  Since we're already fixing problems with BUI item copying, we
should fix it everything else.

Refactor the xfs_rui_copy_format function to handle the copying of the
head and the flex array members separately.  While we're at it, fix a
minor validation deficiency in the recovery function.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_ondisk.h    |    3 ++
 fs/xfs/xfs_rmap_item.c |   58 ++++++++++++++++++++++--------------------------
 2 files changed, 30 insertions(+), 31 deletions(-)


diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index e20d2844b0c5..19c1df00b48e 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -138,11 +138,14 @@ xfs_check_ondisk_structs(void)
 	XFS_CHECK_STRUCT_SIZE(struct xfs_bud_log_format,	16);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_cui_log_format,	16);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_cud_log_format,	16);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_rui_log_format,	16);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_rud_log_format,	16);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_map_extent,		32);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_phys_extent,		16);
 
 	XFS_CHECK_OFFSET(struct xfs_bui_log_format, bui_extents,	16);
 	XFS_CHECK_OFFSET(struct xfs_cui_log_format, cui_extents,	16);
+	XFS_CHECK_OFFSET(struct xfs_rui_log_format, rui_extents,	16);
 
 	/*
 	 * The v5 superblock format extended several v4 header structures with
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index fef92e02f3bb..27047e73f582 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -155,31 +155,6 @@ xfs_rui_init(
 	return ruip;
 }
 
-/*
- * Copy an RUI format buffer from the given buf, and into the destination
- * RUI format structure.  The RUI/RUD items were designed not to need any
- * special alignment handling.
- */
-STATIC int
-xfs_rui_copy_format(
-	struct xfs_log_iovec		*buf,
-	struct xfs_rui_log_format	*dst_rui_fmt)
-{
-	struct xfs_rui_log_format	*src_rui_fmt;
-	uint				len;
-
-	src_rui_fmt = buf->i_addr;
-	len = xfs_rui_log_format_sizeof(src_rui_fmt->rui_nextents);
-
-	if (buf->i_len != len) {
-		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
-		return -EFSCORRUPTED;
-	}
-
-	memcpy(dst_rui_fmt, src_rui_fmt, len);
-	return 0;
-}
-
 static inline struct xfs_rud_log_item *RUD_ITEM(struct xfs_log_item *lip)
 {
 	return container_of(lip, struct xfs_rud_log_item, rud_item);
@@ -652,6 +627,20 @@ static const struct xfs_item_ops xfs_rui_item_ops = {
 	.iop_relog	= xfs_rui_item_relog,
 };
 
+static inline void
+xfs_rui_copy_format(
+	struct xfs_rui_log_format	*dst,
+	const struct xfs_rui_log_format	*src)
+{
+	unsigned int			i;
+
+	memcpy(dst, src, offsetof(struct xfs_rui_log_format, rui_extents));
+
+	for (i = 0; i < src->rui_nextents; i++)
+		memcpy(&dst->rui_extents[i], &src->rui_extents[i],
+				sizeof(struct xfs_map_extent));
+}
+
 /*
  * This routine is called to create an in-core extent rmap update
  * item from the rui format structure which was logged on disk.
@@ -666,19 +655,26 @@ xlog_recover_rui_commit_pass2(
 	struct xlog_recover_item	*item,
 	xfs_lsn_t			lsn)
 {
-	int				error;
 	struct xfs_mount		*mp = log->l_mp;
 	struct xfs_rui_log_item		*ruip;
 	struct xfs_rui_log_format	*rui_formatp;
+	size_t				len;
 
 	rui_formatp = item->ri_buf[0].i_addr;
 
+	if (item->ri_buf[0].i_len < xfs_rui_log_format_sizeof(0)) {
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
+		return -EFSCORRUPTED;
+	}
+
+	len = xfs_rui_log_format_sizeof(rui_formatp->rui_nextents);
+	if (item->ri_buf[0].i_len != len) {
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
+		return -EFSCORRUPTED;
+	}
+
 	ruip = xfs_rui_init(mp, rui_formatp->rui_nextents);
-	error = xfs_rui_copy_format(&item->ri_buf[0], &ruip->rui_format);
-	if (error) {
-		xfs_rui_item_free(ruip);
-		return error;
-	}
+	xfs_rui_copy_format(&ruip->rui_format, rui_formatp);
 	atomic_set(&ruip->rui_next_extent, rui_formatp->rui_nextents);
 	/*
 	 * Insert the intent into the AIL directly and drop one reference so

[PATCH 5/6] xfs: fix memcpy fortify errors in EFI log format copying

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of
memcpy.  Since we're already fixing problems with BUI item copying, we
should fix it everything else.

An extra difficulty here is that the ef[id]_extents arrays are declared
as single-element arrays.  This is not the convention for flex arrays in
the modern kernel, and it causes all manner of problems with static
checking tools, since they often cannot tell the difference between a
single element array and a flex array.

So for starters, change those array[1] declarations to array[]
declarations to signal that they are proper flex arrays and adjust all
the "size-1" expressions to fit the new declaration style.

Next, refactor the xfs_efi_copy_format function to handle the copying of
the head and the flex array members separately.  While we're at it, fix
a minor validation deficiency in the recovery function.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_log_format.h |   12 ++++++------
 fs/xfs/xfs_extfree_item.c      |   31 +++++++++++++++++++++----------
 fs/xfs/xfs_ondisk.h            |   11 +++++++----
 fs/xfs/xfs_super.c             |    4 ++--
 4 files changed, 36 insertions(+), 22 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
index b351b9dc6561..2f41fa8477c9 100644
--- a/fs/xfs/libxfs/xfs_log_format.h
+++ b/fs/xfs/libxfs/xfs_log_format.h
@@ -613,7 +613,7 @@ typedef struct xfs_efi_log_format {
 	uint16_t		efi_size;	/* size of this item */
 	uint32_t		efi_nextents;	/* # extents to free */
 	uint64_t		efi_id;		/* efi identifier */
-	xfs_extent_t		efi_extents[1];	/* array of extents to free */
+	xfs_extent_t		efi_extents[];	/* array of extents to free */
 } xfs_efi_log_format_t;
 
 typedef struct xfs_efi_log_format_32 {
@@ -621,7 +621,7 @@ typedef struct xfs_efi_log_format_32 {
 	uint16_t		efi_size;	/* size of this item */
 	uint32_t		efi_nextents;	/* # extents to free */
 	uint64_t		efi_id;		/* efi identifier */
-	xfs_extent_32_t		efi_extents[1];	/* array of extents to free */
+	xfs_extent_32_t		efi_extents[];	/* array of extents to free */
 } __attribute__((packed)) xfs_efi_log_format_32_t;
 
 typedef struct xfs_efi_log_format_64 {
@@ -629,7 +629,7 @@ typedef struct xfs_efi_log_format_64 {
 	uint16_t		efi_size;	/* size of this item */
 	uint32_t		efi_nextents;	/* # extents to free */
 	uint64_t		efi_id;		/* efi identifier */
-	xfs_extent_64_t		efi_extents[1];	/* array of extents to free */
+	xfs_extent_64_t		efi_extents[];	/* array of extents to free */
 } xfs_efi_log_format_64_t;
 
 /*
@@ -642,7 +642,7 @@ typedef struct xfs_efd_log_format {
 	uint16_t		efd_size;	/* size of this item */
 	uint32_t		efd_nextents;	/* # of extents freed */
 	uint64_t		efd_efi_id;	/* id of corresponding efi */
-	xfs_extent_t		efd_extents[1];	/* array of extents freed */
+	xfs_extent_t		efd_extents[];	/* array of extents freed */
 } xfs_efd_log_format_t;
 
 typedef struct xfs_efd_log_format_32 {
@@ -650,7 +650,7 @@ typedef struct xfs_efd_log_format_32 {
 	uint16_t		efd_size;	/* size of this item */
 	uint32_t		efd_nextents;	/* # of extents freed */
 	uint64_t		efd_efi_id;	/* id of corresponding efi */
-	xfs_extent_32_t		efd_extents[1];	/* array of extents freed */
+	xfs_extent_32_t		efd_extents[];	/* array of extents freed */
 } __attribute__((packed)) xfs_efd_log_format_32_t;
 
 typedef struct xfs_efd_log_format_64 {
@@ -658,7 +658,7 @@ typedef struct xfs_efd_log_format_64 {
 	uint16_t		efd_size;	/* size of this item */
 	uint32_t		efd_nextents;	/* # of extents freed */
 	uint64_t		efd_efi_id;	/* id of corresponding efi */
-	xfs_extent_64_t		efd_extents[1];	/* array of extents freed */
+	xfs_extent_64_t		efd_extents[];	/* array of extents freed */
 } xfs_efd_log_format_64_t;
 
 /*
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 27ccfcd82f04..466cc5c5cd33 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -76,7 +76,7 @@ xfs_efi_item_sizeof(
 	struct xfs_efi_log_item *efip)
 {
 	return sizeof(struct xfs_efi_log_format) +
-	       (efip->efi_format.efi_nextents - 1) * sizeof(xfs_extent_t);
+	       efip->efi_format.efi_nextents * sizeof(xfs_extent_t);
 }
 
 STATIC void
@@ -160,7 +160,7 @@ xfs_efi_init(
 	ASSERT(nextents > 0);
 	if (nextents > XFS_EFI_MAX_FAST_EXTENTS) {
 		size = (uint)(sizeof(struct xfs_efi_log_item) +
-			((nextents - 1) * sizeof(xfs_extent_t)));
+			(nextents * sizeof(xfs_extent_t)));
 		efip = kmem_zalloc(size, 0);
 	} else {
 		efip = kmem_cache_zalloc(xfs_efi_cache,
@@ -189,14 +189,19 @@ xfs_efi_copy_format(xfs_log_iovec_t *buf, xfs_efi_log_format_t *dst_efi_fmt)
 	xfs_efi_log_format_t *src_efi_fmt = buf->i_addr;
 	uint i;
 	uint len = sizeof(xfs_efi_log_format_t) +
-		(src_efi_fmt->efi_nextents - 1) * sizeof(xfs_extent_t);
+		src_efi_fmt->efi_nextents * sizeof(xfs_extent_t);
 	uint len32 = sizeof(xfs_efi_log_format_32_t) +
-		(src_efi_fmt->efi_nextents - 1) * sizeof(xfs_extent_32_t);
+		src_efi_fmt->efi_nextents * sizeof(xfs_extent_32_t);
 	uint len64 = sizeof(xfs_efi_log_format_64_t) +
-		(src_efi_fmt->efi_nextents - 1) * sizeof(xfs_extent_64_t);
+		src_efi_fmt->efi_nextents * sizeof(xfs_extent_64_t);
 
 	if (buf->i_len == len) {
-		memcpy((char *)dst_efi_fmt, (char*)src_efi_fmt, len);
+		memcpy(dst_efi_fmt, src_efi_fmt,
+		       offsetof(struct xfs_efi_log_format, efi_extents));
+		for (i = 0; i < src_efi_fmt->efi_nextents; i++)
+			memcpy(&dst_efi_fmt->efi_extents[i],
+			       &src_efi_fmt->efi_extents[i],
+			       sizeof(struct xfs_extent));
 		return 0;
 	} else if (buf->i_len == len32) {
 		xfs_efi_log_format_32_t *src_efi_fmt_32 = buf->i_addr;
@@ -256,7 +261,7 @@ xfs_efd_item_sizeof(
 	struct xfs_efd_log_item *efdp)
 {
 	return sizeof(xfs_efd_log_format_t) +
-	       (efdp->efd_format.efd_nextents - 1) * sizeof(xfs_extent_t);
+	       efdp->efd_format.efd_nextents * sizeof(xfs_extent_t);
 }
 
 STATIC void
@@ -341,7 +346,7 @@ xfs_trans_get_efd(
 
 	if (nextents > XFS_EFD_MAX_FAST_EXTENTS) {
 		efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) +
-				(nextents - 1) * sizeof(struct xfs_extent),
+				nextents * sizeof(struct xfs_extent),
 				0);
 	} else {
 		efdp = kmem_cache_zalloc(xfs_efd_cache,
@@ -733,6 +738,12 @@ xlog_recover_efi_commit_pass2(
 
 	efi_formatp = item->ri_buf[0].i_addr;
 
+	if (item->ri_buf[0].i_len <
+			offsetof(struct xfs_efi_log_format, efi_extents)) {
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
+		return -EFSCORRUPTED;
+	}
+
 	efip = xfs_efi_init(mp, efi_formatp->efi_nextents);
 	error = xfs_efi_copy_format(&item->ri_buf[0], &efip->efi_format);
 	if (error) {
@@ -772,9 +783,9 @@ xlog_recover_efd_commit_pass2(
 
 	efd_formatp = item->ri_buf[0].i_addr;
 	ASSERT((item->ri_buf[0].i_len == (sizeof(xfs_efd_log_format_32_t) +
-		((efd_formatp->efd_nextents - 1) * sizeof(xfs_extent_32_t)))) ||
+		(efd_formatp->efd_nextents * sizeof(xfs_extent_32_t)))) ||
 	       (item->ri_buf[0].i_len == (sizeof(xfs_efd_log_format_64_t) +
-		((efd_formatp->efd_nextents - 1) * sizeof(xfs_extent_64_t)))));
+		(efd_formatp->efd_nextents * sizeof(xfs_extent_64_t)))));
 
 	xlog_recover_release_intent(log, XFS_LI_EFI, efd_formatp->efd_efi_id);
 	return 0;
diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index 19c1df00b48e..9737b5a9f405 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -118,10 +118,10 @@ xfs_check_ondisk_structs(void)
 	/* 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);
-	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32,	28);
-	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64,	32);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_32,	16);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_64,	16);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32,	16);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64,	16);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_extent_32,		12);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_extent_64,		16);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_log_dinode,		176);
@@ -146,6 +146,9 @@ xfs_check_ondisk_structs(void)
 	XFS_CHECK_OFFSET(struct xfs_bui_log_format, bui_extents,	16);
 	XFS_CHECK_OFFSET(struct xfs_cui_log_format, cui_extents,	16);
 	XFS_CHECK_OFFSET(struct xfs_rui_log_format, rui_extents,	16);
+	XFS_CHECK_OFFSET(struct xfs_efi_log_format, efi_extents,	16);
+	XFS_CHECK_OFFSET(struct xfs_efi_log_format_32, efi_extents,	16);
+	XFS_CHECK_OFFSET(struct xfs_efi_log_format_64, efi_extents,	16);
 
 	/*
 	 * The v5 superblock format extended several v4 header structures with
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index f029c6702dda..8485e3b37ca0 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -2029,7 +2029,7 @@ xfs_init_caches(void)
 
 	xfs_efd_cache = kmem_cache_create("xfs_efd_item",
 					(sizeof(struct xfs_efd_log_item) +
-					(XFS_EFD_MAX_FAST_EXTENTS - 1) *
+					XFS_EFD_MAX_FAST_EXTENTS *
 					sizeof(struct xfs_extent)),
 					0, 0, NULL);
 	if (!xfs_efd_cache)
@@ -2037,7 +2037,7 @@ xfs_init_caches(void)
 
 	xfs_efi_cache = kmem_cache_create("xfs_efi_item",
 					 (sizeof(struct xfs_efi_log_item) +
-					 (XFS_EFI_MAX_FAST_EXTENTS - 1) *
+					 XFS_EFI_MAX_FAST_EXTENTS *
 					 sizeof(struct xfs_extent)),
 					 0, 0, NULL);
 	if (!xfs_efi_cache)

[PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Refactor all the open-coded sizeof logic for EFI/EFD log item and log
format structures into common helper functions whose names reflect the
struct names.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_log_format.h |   48 ++++++++++++++++++++++++++++
 fs/xfs/xfs_extfree_item.c      |   69 ++++++++++++----------------------------
 fs/xfs/xfs_extfree_item.h      |   16 +++++++++
 fs/xfs/xfs_super.c             |   12 ++-----
 4 files changed, 88 insertions(+), 57 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
index 2f41fa8477c9..f13e0809dc63 100644
--- a/fs/xfs/libxfs/xfs_log_format.h
+++ b/fs/xfs/libxfs/xfs_log_format.h
@@ -616,6 +616,14 @@ typedef struct xfs_efi_log_format {
 	xfs_extent_t		efi_extents[];	/* array of extents to free */
 } xfs_efi_log_format_t;
 
+static inline size_t
+xfs_efi_log_format_sizeof(
+	unsigned int		nr)
+{
+	return sizeof(struct xfs_efi_log_format) +
+			nr * sizeof(struct xfs_extent);
+}
+
 typedef struct xfs_efi_log_format_32 {
 	uint16_t		efi_type;	/* efi log item type */
 	uint16_t		efi_size;	/* size of this item */
@@ -624,6 +632,14 @@ typedef struct xfs_efi_log_format_32 {
 	xfs_extent_32_t		efi_extents[];	/* array of extents to free */
 } __attribute__((packed)) xfs_efi_log_format_32_t;
 
+static inline size_t
+xfs_efi_log_format32_sizeof(
+	unsigned int		nr)
+{
+	return sizeof(struct xfs_efi_log_format_32) +
+			nr * sizeof(struct xfs_extent_32);
+}
+
 typedef struct xfs_efi_log_format_64 {
 	uint16_t		efi_type;	/* efi log item type */
 	uint16_t		efi_size;	/* size of this item */
@@ -632,6 +648,14 @@ typedef struct xfs_efi_log_format_64 {
 	xfs_extent_64_t		efi_extents[];	/* array of extents to free */
 } xfs_efi_log_format_64_t;
 
+static inline size_t
+xfs_efi_log_format64_sizeof(
+	unsigned int		nr)
+{
+	return sizeof(struct xfs_efi_log_format_64) +
+			nr * sizeof(struct xfs_extent_64);
+}
+
 /*
  * This is the structure used to lay out an efd log item in the
  * log.  The efd_extents array is a variable size array whose
@@ -645,6 +669,14 @@ typedef struct xfs_efd_log_format {
 	xfs_extent_t		efd_extents[];	/* array of extents freed */
 } xfs_efd_log_format_t;
 
+static inline size_t
+xfs_efd_log_format_sizeof(
+	unsigned int		nr)
+{
+	return sizeof(struct xfs_efd_log_format) +
+			nr * sizeof(struct xfs_extent);
+}
+
 typedef struct xfs_efd_log_format_32 {
 	uint16_t		efd_type;	/* efd log item type */
 	uint16_t		efd_size;	/* size of this item */
@@ -653,6 +685,14 @@ typedef struct xfs_efd_log_format_32 {
 	xfs_extent_32_t		efd_extents[];	/* array of extents freed */
 } __attribute__((packed)) xfs_efd_log_format_32_t;
 
+static inline size_t
+xfs_efd_log_format32_sizeof(
+	unsigned int		nr)
+{
+	return sizeof(struct xfs_efd_log_format_32) +
+			nr * sizeof(struct xfs_extent_32);
+}
+
 typedef struct xfs_efd_log_format_64 {
 	uint16_t		efd_type;	/* efd log item type */
 	uint16_t		efd_size;	/* size of this item */
@@ -661,6 +701,14 @@ typedef struct xfs_efd_log_format_64 {
 	xfs_extent_64_t		efd_extents[];	/* array of extents freed */
 } xfs_efd_log_format_64_t;
 
+static inline size_t
+xfs_efd_log_format64_sizeof(
+	unsigned int		nr)
+{
+	return sizeof(struct xfs_efd_log_format_64) +
+			nr * sizeof(struct xfs_extent_64);
+}
+
 /*
  * RUI/RUD (reverse mapping) log format definitions
  */
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 466cc5c5cd33..bffb2b91e39a 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -66,27 +66,16 @@ xfs_efi_release(
 	xfs_efi_item_free(efip);
 }
 
-/*
- * This returns the number of iovecs needed to log the given efi item.
- * We only need 1 iovec for an efi item.  It just logs the efi_log_format
- * structure.
- */
-static inline int
-xfs_efi_item_sizeof(
-	struct xfs_efi_log_item *efip)
-{
-	return sizeof(struct xfs_efi_log_format) +
-	       efip->efi_format.efi_nextents * sizeof(xfs_extent_t);
-}
-
 STATIC void
 xfs_efi_item_size(
 	struct xfs_log_item	*lip,
 	int			*nvecs,
 	int			*nbytes)
 {
+	struct xfs_efi_log_item	*efip = EFI_ITEM(lip);
+
 	*nvecs += 1;
-	*nbytes += xfs_efi_item_sizeof(EFI_ITEM(lip));
+	*nbytes += xfs_efi_log_format_sizeof(efip->efi_format.efi_nextents);
 }
 
 /*
@@ -112,7 +101,7 @@ xfs_efi_item_format(
 
 	xlog_copy_iovec(lv, &vecp, XLOG_REG_TYPE_EFI_FORMAT,
 			&efip->efi_format,
-			xfs_efi_item_sizeof(efip));
+			xfs_efi_log_format_sizeof(efip->efi_format.efi_nextents));
 }
 
 
@@ -155,13 +144,11 @@ xfs_efi_init(
 
 {
 	struct xfs_efi_log_item	*efip;
-	uint			size;
 
 	ASSERT(nextents > 0);
 	if (nextents > XFS_EFI_MAX_FAST_EXTENTS) {
-		size = (uint)(sizeof(struct xfs_efi_log_item) +
-			(nextents * sizeof(xfs_extent_t)));
-		efip = kmem_zalloc(size, 0);
+		efip = kmem_zalloc(xfs_efi_log_item_sizeof(nextents),
+				GFP_KERNEL | __GFP_NOFAIL);
 	} else {
 		efip = kmem_cache_zalloc(xfs_efi_cache,
 					 GFP_KERNEL | __GFP_NOFAIL);
@@ -188,12 +175,9 @@ xfs_efi_copy_format(xfs_log_iovec_t *buf, xfs_efi_log_format_t *dst_efi_fmt)
 {
 	xfs_efi_log_format_t *src_efi_fmt = buf->i_addr;
 	uint i;
-	uint len = sizeof(xfs_efi_log_format_t) +
-		src_efi_fmt->efi_nextents * sizeof(xfs_extent_t);
-	uint len32 = sizeof(xfs_efi_log_format_32_t) +
-		src_efi_fmt->efi_nextents * sizeof(xfs_extent_32_t);
-	uint len64 = sizeof(xfs_efi_log_format_64_t) +
-		src_efi_fmt->efi_nextents * sizeof(xfs_extent_64_t);
+	uint len = xfs_efi_log_format_sizeof(src_efi_fmt->efi_nextents);
+	uint len32 = xfs_efi_log_format32_sizeof(src_efi_fmt->efi_nextents);
+	uint len64 = xfs_efi_log_format64_sizeof(src_efi_fmt->efi_nextents);
 
 	if (buf->i_len == len) {
 		memcpy(dst_efi_fmt, src_efi_fmt,
@@ -251,27 +235,16 @@ xfs_efd_item_free(struct xfs_efd_log_item *efdp)
 		kmem_cache_free(xfs_efd_cache, efdp);
 }
 
-/*
- * This returns the number of iovecs needed to log the given efd item.
- * We only need 1 iovec for an efd item.  It just logs the efd_log_format
- * structure.
- */
-static inline int
-xfs_efd_item_sizeof(
-	struct xfs_efd_log_item *efdp)
-{
-	return sizeof(xfs_efd_log_format_t) +
-	       efdp->efd_format.efd_nextents * sizeof(xfs_extent_t);
-}
-
 STATIC void
 xfs_efd_item_size(
 	struct xfs_log_item	*lip,
 	int			*nvecs,
 	int			*nbytes)
 {
+	struct xfs_efd_log_item	*efdp = EFD_ITEM(lip);
+
 	*nvecs += 1;
-	*nbytes += xfs_efd_item_sizeof(EFD_ITEM(lip));
+	*nbytes += xfs_efd_log_format_sizeof(efdp->efd_format.efd_nextents);
 }
 
 /*
@@ -296,7 +269,7 @@ xfs_efd_item_format(
 
 	xlog_copy_iovec(lv, &vecp, XLOG_REG_TYPE_EFD_FORMAT,
 			&efdp->efd_format,
-			xfs_efd_item_sizeof(efdp));
+			xfs_efd_log_format_sizeof(efdp->efd_format.efd_nextents));
 }
 
 /*
@@ -345,9 +318,8 @@ xfs_trans_get_efd(
 	ASSERT(nextents > 0);
 
 	if (nextents > XFS_EFD_MAX_FAST_EXTENTS) {
-		efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) +
-				nextents * sizeof(struct xfs_extent),
-				0);
+		efdp = kmem_zalloc(xfs_efd_log_item_sizeof(nextents),
+				GFP_KERNEL | __GFP_NOFAIL);
 	} else {
 		efdp = kmem_cache_zalloc(xfs_efd_cache,
 					GFP_KERNEL | __GFP_NOFAIL);
@@ -738,8 +710,7 @@ xlog_recover_efi_commit_pass2(
 
 	efi_formatp = item->ri_buf[0].i_addr;
 
-	if (item->ri_buf[0].i_len <
-			offsetof(struct xfs_efi_log_format, efi_extents)) {
+	if (item->ri_buf[0].i_len < xfs_efi_log_format_sizeof(0)) {
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
 		return -EFSCORRUPTED;
 	}
@@ -782,10 +753,10 @@ xlog_recover_efd_commit_pass2(
 	struct xfs_efd_log_format	*efd_formatp;
 
 	efd_formatp = item->ri_buf[0].i_addr;
-	ASSERT((item->ri_buf[0].i_len == (sizeof(xfs_efd_log_format_32_t) +
-		(efd_formatp->efd_nextents * sizeof(xfs_extent_32_t)))) ||
-	       (item->ri_buf[0].i_len == (sizeof(xfs_efd_log_format_64_t) +
-		(efd_formatp->efd_nextents * sizeof(xfs_extent_64_t)))));
+	ASSERT(item->ri_buf[0].i_len == xfs_efd_log_format32_sizeof(
+						efd_formatp->efd_nextents) ||
+	       item->ri_buf[0].i_len == xfs_efd_log_format64_sizeof(
+						efd_formatp->efd_nextents));
 
 	xlog_recover_release_intent(log, XFS_LI_EFI, efd_formatp->efd_efi_id);
 	return 0;
diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
index 186d0f2137f1..da6a5afa607c 100644
--- a/fs/xfs/xfs_extfree_item.h
+++ b/fs/xfs/xfs_extfree_item.h
@@ -52,6 +52,14 @@ struct xfs_efi_log_item {
 	xfs_efi_log_format_t	efi_format;
 };
 
+static inline size_t
+xfs_efi_log_item_sizeof(
+	unsigned int		nr)
+{
+	return offsetof(struct xfs_efi_log_item, efi_format) +
+			xfs_efi_log_format_sizeof(nr);
+}
+
 /*
  * This is the "extent free done" log item.  It is used to log
  * the fact that some extents earlier mentioned in an efi item
@@ -64,6 +72,14 @@ struct xfs_efd_log_item {
 	xfs_efd_log_format_t	efd_format;
 };
 
+static inline size_t
+xfs_efd_log_item_sizeof(
+	unsigned int		nr)
+{
+	return offsetof(struct xfs_efd_log_item, efd_format) +
+			xfs_efd_log_format_sizeof(nr);
+}
+
 /*
  * Max number of extents in fast allocation path.
  */
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 8485e3b37ca0..ee4b429a2f2c 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -2028,18 +2028,14 @@ xfs_init_caches(void)
 		goto out_destroy_trans_cache;
 
 	xfs_efd_cache = kmem_cache_create("xfs_efd_item",
-					(sizeof(struct xfs_efd_log_item) +
-					XFS_EFD_MAX_FAST_EXTENTS *
-					sizeof(struct xfs_extent)),
-					0, 0, NULL);
+			xfs_efd_log_item_sizeof(XFS_EFD_MAX_FAST_EXTENTS),
+			0, 0, NULL);
 	if (!xfs_efd_cache)
 		goto out_destroy_buf_item_cache;
 
 	xfs_efi_cache = kmem_cache_create("xfs_efi_item",
-					 (sizeof(struct xfs_efi_log_item) +
-					 XFS_EFI_MAX_FAST_EXTENTS *
-					 sizeof(struct xfs_extent)),
-					 0, 0, NULL);
+			xfs_efi_log_item_sizeof(XFS_EFI_MAX_FAST_EXTENTS),
+			0, 0, NULL);
 	if (!xfs_efi_cache)
 		goto out_destroy_efd_cache;
 

Re: [PATCH 1/6] xfs: fix validation in attr log item recovery

[Kees Cook]

On Mon, Oct 24, 2022 at 02:32:37PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Before we start fixing all the complaints about memcpy'ing log items
> around, let's fix some inadequate validation in the xattr log item
> recovery code and get rid of the (now trivial) copy_format function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

I can't speak to the new checks, but yeah, the decomposition to a direct
check and memcpy looks correct.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 2/6] xfs: fix memcpy fortify errors in BUI log format copying

[Kees Cook]

On Mon, Oct 24, 2022 at 02:32:42PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of
> memcpy.  Unfortunately, it doesn't handle flex arrays correctly:
> 
> ------------[ cut here ]------------
> memcpy: detected field-spanning write (size 48) of single field "dst_bui_fmt" at fs/xfs/xfs_bmap_item.c:628 (size 16)
> 
> Fix this by refactoring the xfs_bui_copy_format function to handle the
> copying of the head and the flex array members separately.  While we're
> at it, fix a minor validation deficiency in the recovery function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_bmap_item.c |   46 ++++++++++++++++++++++------------------------
>  fs/xfs/xfs_ondisk.h    |    5 +++++
>  2 files changed, 27 insertions(+), 24 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index 51f66e982484..a1da6205252b 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -608,28 +608,18 @@ static const struct xfs_item_ops xfs_bui_item_ops = {
>  	.iop_relog	= xfs_bui_item_relog,
>  };
>  
> -/*
> - * Copy an BUI format buffer from the given buf, and into the destination
> - * BUI format structure.  The BUI/BUD items were designed not to need any
> - * special alignment handling.
> - */
> -static int
> +static inline void
>  xfs_bui_copy_format(
> -	struct xfs_log_iovec		*buf,
> -	struct xfs_bui_log_format	*dst_bui_fmt)
> +	struct xfs_bui_log_format	*dst,
> +	const struct xfs_bui_log_format	*src)
>  {
> -	struct xfs_bui_log_format	*src_bui_fmt;
> -	uint				len;
> +	unsigned int			i;
>  
> -	src_bui_fmt = buf->i_addr;
> -	len = xfs_bui_log_format_sizeof(src_bui_fmt->bui_nextents);
> +	memcpy(dst, src, offsetof(struct xfs_bui_log_format, bui_extents));

I think this would work:

	*dst = *src;

>  
> -	if (buf->i_len == len) {
> -		memcpy(dst_bui_fmt, src_bui_fmt, len);
> -		return 0;
> -	}
> -	XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> -	return -EFSCORRUPTED;
> +	for (i = 0; i < src->bui_nextents; i++)
> +		memcpy(&dst->bui_extents[i], &src->bui_extents[i],
> +				sizeof(struct xfs_map_extent));

Same here:

		dst->bui_extents[i] = src->bui_extents[i];

No reason to bring memcpy into this at all. :)


-- 
Kees Cook

Re: [PATCH 5/6] xfs: fix memcpy fortify errors in EFI log format copying

[Kees Cook]

On Mon, Oct 24, 2022 at 02:32:59PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of
> memcpy.  Since we're already fixing problems with BUI item copying, we
> should fix it everything else.
> 
> An extra difficulty here is that the ef[id]_extents arrays are declared
> as single-element arrays.  This is not the convention for flex arrays in
> the modern kernel, and it causes all manner of problems with static
> checking tools, since they often cannot tell the difference between a
> single element array and a flex array.
> 
> So for starters, change those array[1] declarations to array[]
> declarations to signal that they are proper flex arrays and adjust all
> the "size-1" expressions to fit the new declaration style.

This looks very familiar! :)

https://lore.kernel.org/linux-xfs/20210419082804.2076124-1-hch@lst.de/

It seems like it might make more sense to start with hch's series, and
see what's missing?

> 
> Next, refactor the xfs_efi_copy_format function to handle the copying of
> the head and the flex array members separately.  While we're at it, fix
> a minor validation deficiency in the recovery function.

This feels like 3 separate logical changes in a single patch, but,
regardless:

Reviewed-by: Kees Cook <keescook@chromium.org>

This will proactively fix XFS under CONFIG_UBSAN_BOUNDS once
-fstrict-flex-arrays is added. Thank you!

-- 
Kees Cook

Re: [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic

[Kees Cook]

On Mon, Oct 24, 2022 at 02:33:05PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Refactor all the open-coded sizeof logic for EFI/EFD log item and log
> format structures into common helper functions whose names reflect the
> struct names.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_log_format.h |   48 ++++++++++++++++++++++++++++
>  fs/xfs/xfs_extfree_item.c      |   69 ++++++++++++----------------------------
>  fs/xfs/xfs_extfree_item.h      |   16 +++++++++
>  fs/xfs/xfs_super.c             |   12 ++-----
>  4 files changed, 88 insertions(+), 57 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index 2f41fa8477c9..f13e0809dc63 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -616,6 +616,14 @@ typedef struct xfs_efi_log_format {
>  	xfs_extent_t		efi_extents[];	/* array of extents to free */
>  } xfs_efi_log_format_t;
>  
> +static inline size_t
> +xfs_efi_log_format_sizeof(
> +	unsigned int		nr)
> +{
> +	return sizeof(struct xfs_efi_log_format) +
> +			nr * sizeof(struct xfs_extent);
> +}

These are all just open-coded versions of struct_size(). It's better
to use those, instead, as they will never overflow. (They saturate at
SIZE_MAX.) i.e. what you proposed here:
https://lore.kernel.org/linux-xfs/20210311031745.GT3419940@magnolia/

Otherwise, yeah, looks good.

-Kees

-- 
Kees Cook

Re: [PATCH 1/6] xfs: fix validation in attr log item recovery

[Allison Henderson]

On Mon, 2022-10-24 at 14:32 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Before we start fixing all the complaints about memcpy'ing log items
> around, let's fix some inadequate validation in the xattr log item
> recovery code and get rid of the (now trivial) copy_format function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>\
Ok, looks good to me
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>  fs/xfs/xfs_attr_item.c |   54 ++++++++++++++++++++------------------
> ----------
>  1 file changed, 23 insertions(+), 31 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index cf5ce607dc05..ee8f678a10a1 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -245,28 +245,6 @@ xfs_attri_init(
>         return attrip;
>  }
>  
> -/*
> - * Copy an attr format buffer from the given buf, and into the
> destination attr
> - * format structure.
> - */
> -STATIC int
> -xfs_attri_copy_format(
> -       struct xfs_log_iovec            *buf,
> -       struct xfs_attri_log_format     *dst_attr_fmt)
> -{
> -       struct xfs_attri_log_format     *src_attr_fmt = buf->i_addr;
> -       size_t                          len;
> -
> -       len = sizeof(struct xfs_attri_log_format);
> -       if (buf->i_len != len) {
> -               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> -               return -EFSCORRUPTED;
> -       }
> -
> -       memcpy((char *)dst_attr_fmt, (char *)src_attr_fmt, len);
> -       return 0;
> -}
> -
>  static inline struct xfs_attrd_log_item *ATTRD_ITEM(struct
> xfs_log_item *lip)
>  {
>         return container_of(lip, struct xfs_attrd_log_item,
> attrd_item);
> @@ -731,24 +709,44 @@ xlog_recover_attri_commit_pass2(
>         struct xfs_attri_log_nameval    *nv;
>         const void                      *attr_value = NULL;
>         const void                      *attr_name;
> -       int                             error;
> +       size_t                          len;
>  
>         attri_formatp = item->ri_buf[0].i_addr;
>         attr_name = item->ri_buf[1].i_addr;
>  
>         /* Validate xfs_attri_log_format before the large memory
> allocation */
> +       len = sizeof(struct xfs_attri_log_format);
> +       if (item->ri_buf[0].i_len != len) {
> +               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> +               return -EFSCORRUPTED;
> +       }
> +
>         if (!xfs_attri_validate(mp, attri_formatp)) {
>                 XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
>                 return -EFSCORRUPTED;
>         }
>  
> +       /* Validate the attr name */
> +       if (item->ri_buf[1].i_len !=
> +                       xlog_calc_iovec_len(attri_formatp-
> >alfi_name_len)) {
> +               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> +               return -EFSCORRUPTED;
> +       }
> +
>         if (!xfs_attr_namecheck(attr_name, attri_formatp-
> >alfi_name_len)) {
>                 XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
>                 return -EFSCORRUPTED;
>         }
>  
> -       if (attri_formatp->alfi_value_len)
> +       /* Validate the attr value, if present */
> +       if (attri_formatp->alfi_value_len != 0) {
> +               if (item->ri_buf[2].i_len !=
> xlog_calc_iovec_len(attri_formatp->alfi_value_len)) {
> +                       XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
> mp);
> +                       return -EFSCORRUPTED;
> +               }
> +
>                 attr_value = item->ri_buf[2].i_addr;
> +       }
>  
>         /*
>          * Memory alloc failure will cause replay to abort.  We
> attach the
> @@ -760,9 +758,7 @@ xlog_recover_attri_commit_pass2(
>                         attri_formatp->alfi_value_len);
>  
>         attrip = xfs_attri_init(mp, nv);
> -       error = xfs_attri_copy_format(&item->ri_buf[0], &attrip-
> >attri_format);
> -       if (error)
> -               goto out;
> +       memcpy(&attrip->attri_format, attri_formatp, len);
>  
>         /*
>          * The ATTRI has two references. One for the ATTRD and one
> for ATTRI to
> @@ -774,10 +770,6 @@ xlog_recover_attri_commit_pass2(
>         xfs_attri_release(attrip);
>         xfs_attri_log_nameval_put(nv);
>         return 0;
> -out:
> -       xfs_attri_item_free(attrip);
> -       xfs_attri_log_nameval_put(nv);
> -       return error;
>  }
>  
>  /*
> 

Re: [PATCH 2/6] xfs: fix memcpy fortify errors in BUI log format copying

[Allison Henderson]

On Mon, 2022-10-24 at 14:32 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of
> memcpy.  Unfortunately, it doesn't handle flex arrays correctly:
> 
> ------------[ cut here ]------------
> memcpy: detected field-spanning write (size 48) of single field
> "dst_bui_fmt" at fs/xfs/xfs_bmap_item.c:628 (size 16)
> 
> Fix this by refactoring the xfs_bui_copy_format function to handle
> the
> copying of the head and the flex array members separately.  While
> we're
> at it, fix a minor validation deficiency in the recovery function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Ok, makes sense:
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/xfs_bmap_item.c |   46 ++++++++++++++++++++++----------------
> --------
>  fs/xfs/xfs_ondisk.h    |    5 +++++
>  2 files changed, 27 insertions(+), 24 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index 51f66e982484..a1da6205252b 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -608,28 +608,18 @@ static const struct xfs_item_ops
> xfs_bui_item_ops = {
>         .iop_relog      = xfs_bui_item_relog,
>  };
>  
> -/*
> - * Copy an BUI format buffer from the given buf, and into the
> destination
> - * BUI format structure.  The BUI/BUD items were designed not to
> need any
> - * special alignment handling.
> - */
> -static int
> +static inline void
>  xfs_bui_copy_format(
> -       struct xfs_log_iovec            *buf,
> -       struct xfs_bui_log_format       *dst_bui_fmt)
> +       struct xfs_bui_log_format       *dst,
> +       const struct xfs_bui_log_format *src)
>  {
> -       struct xfs_bui_log_format       *src_bui_fmt;
> -       uint                            len;
> +       unsigned int                    i;
>  
> -       src_bui_fmt = buf->i_addr;
> -       len = xfs_bui_log_format_sizeof(src_bui_fmt->bui_nextents);
> +       memcpy(dst, src, offsetof(struct xfs_bui_log_format,
> bui_extents));
>  
> -       if (buf->i_len == len) {
> -               memcpy(dst_bui_fmt, src_bui_fmt, len);
> -               return 0;
> -       }
> -       XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> -       return -EFSCORRUPTED;
> +       for (i = 0; i < src->bui_nextents; i++)
> +               memcpy(&dst->bui_extents[i], &src->bui_extents[i],
> +                               sizeof(struct xfs_map_extent));
>  }
>  
>  /*
> @@ -646,23 +636,31 @@ xlog_recover_bui_commit_pass2(
>         struct xlog_recover_item        *item,
>         xfs_lsn_t                       lsn)
>  {
> -       int                             error;
>         struct xfs_mount                *mp = log->l_mp;
>         struct xfs_bui_log_item         *buip;
>         struct xfs_bui_log_format       *bui_formatp;
> +       size_t                          len;
>  
>         bui_formatp = item->ri_buf[0].i_addr;
>  
> +       if (item->ri_buf[0].i_len < xfs_bui_log_format_sizeof(0)) {
> +               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log-
> >l_mp);
> +               return -EFSCORRUPTED;
> +       }
> +
>         if (bui_formatp->bui_nextents != XFS_BUI_MAX_FAST_EXTENTS) {
>                 XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log-
> >l_mp);
>                 return -EFSCORRUPTED;
>         }
> +
> +       len = xfs_bui_log_format_sizeof(bui_formatp->bui_nextents);
> +       if (item->ri_buf[0].i_len != len) {
> +               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log-
> >l_mp);
> +               return -EFSCORRUPTED;
> +       }
> +
>         buip = xfs_bui_init(mp);
> -       error = xfs_bui_copy_format(&item->ri_buf[0], &buip-
> >bui_format);
> -       if (error) {
> -               xfs_bui_item_free(buip);
> -               return error;
> -       }
> +       xfs_bui_copy_format(&buip->bui_format, bui_formatp);
>         atomic_set(&buip->bui_next_extent, bui_formatp-
> >bui_nextents);
>         /*
>          * Insert the intent into the AIL directly and drop one
> reference so
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index 758702b9495f..56917e236370 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -134,6 +134,11 @@ xfs_check_ondisk_structs(void)
>         XFS_CHECK_STRUCT_SIZE(struct xfs_trans_header,          16);
>         XFS_CHECK_STRUCT_SIZE(struct xfs_attri_log_format,      40);
>         XFS_CHECK_STRUCT_SIZE(struct xfs_attrd_log_format,      16);
> +       XFS_CHECK_STRUCT_SIZE(struct xfs_bui_log_format,        16);
> +       XFS_CHECK_STRUCT_SIZE(struct xfs_bud_log_format,        16);
> +       XFS_CHECK_STRUCT_SIZE(struct xfs_map_extent,            32);
> +
> +       XFS_CHECK_OFFSET(struct xfs_bui_log_format,
> bui_extents,        16);
>  
>         /*
>          * The v5 superblock format extended several v4 header
> structures with
> 

Re: [PATCH 3/6] xfs: fix memcpy fortify errors in CUI log format copying

[Allison Henderson]

On Mon, 2022-10-24 at 14:32 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of
> memcpy.  Since we're already fixing problems with BUI item copying,
> we
> should fix it everything else.
> 
> Refactor the xfs_cui_copy_format function to handle the copying of
> the
> head and the flex array members separately.  While we're at it, fix a
> minor validation deficiency in the recovery function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Alrighty, looks good
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/xfs_ondisk.h        |    4 ++++
>  fs/xfs/xfs_refcount_item.c |   45 +++++++++++++++++++++-------------
> ----------
>  2 files changed, 25 insertions(+), 24 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index 56917e236370..e20d2844b0c5 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -136,9 +136,13 @@ xfs_check_ondisk_structs(void)
>         XFS_CHECK_STRUCT_SIZE(struct xfs_attrd_log_format,      16);
>         XFS_CHECK_STRUCT_SIZE(struct xfs_bui_log_format,        16);
>         XFS_CHECK_STRUCT_SIZE(struct xfs_bud_log_format,        16);
> +       XFS_CHECK_STRUCT_SIZE(struct xfs_cui_log_format,        16);
> +       XFS_CHECK_STRUCT_SIZE(struct xfs_cud_log_format,        16);
>         XFS_CHECK_STRUCT_SIZE(struct xfs_map_extent,            32);
> +       XFS_CHECK_STRUCT_SIZE(struct xfs_phys_extent,           16);
>  
>         XFS_CHECK_OFFSET(struct xfs_bui_log_format,
> bui_extents,        16);
> +       XFS_CHECK_OFFSET(struct xfs_cui_log_format,
> cui_extents,        16);
>  
>         /*
>          * The v5 superblock format extended several v4 header
> structures with
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index 7e97bf19793d..24cf4c64ebaa 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -622,28 +622,18 @@ static const struct xfs_item_ops
> xfs_cui_item_ops = {
>         .iop_relog      = xfs_cui_item_relog,
>  };
>  
> -/*
> - * Copy an CUI format buffer from the given buf, and into the
> destination
> - * CUI format structure.  The CUI/CUD items were designed not to
> need any
> - * special alignment handling.
> - */
> -static int
> +static inline void
>  xfs_cui_copy_format(
> -       struct xfs_log_iovec            *buf,
> -       struct xfs_cui_log_format       *dst_cui_fmt)
> +       struct xfs_cui_log_format       *dst,
> +       const struct xfs_cui_log_format *src)
>  {
> -       struct xfs_cui_log_format       *src_cui_fmt;
> -       uint                            len;
> +       unsigned int                    i;
>  
> -       src_cui_fmt = buf->i_addr;
> -       len = xfs_cui_log_format_sizeof(src_cui_fmt->cui_nextents);
> +       memcpy(dst, src, offsetof(struct xfs_cui_log_format,
> cui_extents));
>  
> -       if (buf->i_len == len) {
> -               memcpy(dst_cui_fmt, src_cui_fmt, len);
> -               return 0;
> -       }
> -       XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> -       return -EFSCORRUPTED;
> +       for (i = 0; i < src->cui_nextents; i++)
> +               memcpy(&dst->cui_extents[i], &src->cui_extents[i],
> +                               sizeof(struct xfs_phys_extent));
>  }
>  
>  /*
> @@ -660,19 +650,26 @@ xlog_recover_cui_commit_pass2(
>         struct xlog_recover_item        *item,
>         xfs_lsn_t                       lsn)
>  {
> -       int                             error;
>         struct xfs_mount                *mp = log->l_mp;
>         struct xfs_cui_log_item         *cuip;
>         struct xfs_cui_log_format       *cui_formatp;
> +       size_t                          len;
>  
>         cui_formatp = item->ri_buf[0].i_addr;
>  
> +       if (item->ri_buf[0].i_len < xfs_cui_log_format_sizeof(0)) {
> +               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log-
> >l_mp);
> +               return -EFSCORRUPTED;
> +       }
> +
> +       len = xfs_cui_log_format_sizeof(cui_formatp->cui_nextents);
> +       if (item->ri_buf[0].i_len != len) {
> +               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log-
> >l_mp);
> +               return -EFSCORRUPTED;
> +       }
> +
>         cuip = xfs_cui_init(mp, cui_formatp->cui_nextents);
> -       error = xfs_cui_copy_format(&item->ri_buf[0], &cuip-
> >cui_format);
> -       if (error) {
> -               xfs_cui_item_free(cuip);
> -               return error;
> -       }
> +       xfs_cui_copy_format(&cuip->cui_format, cui_formatp);
>         atomic_set(&cuip->cui_next_extent, cui_formatp-
> >cui_nextents);
>         /*
>          * Insert the intent into the AIL directly and drop one
> reference so
> 

Re: [PATCH 4/6] xfs: fix memcpy fortify errors in RUI log format copying

[Allison Henderson]

On Mon, 2022-10-24 at 14:32 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of
> memcpy.  Since we're already fixing problems with BUI item copying,
> we
> should fix it everything else.
> 
> Refactor the xfs_rui_copy_format function to handle the copying of
> the
> head and the flex array members separately.  While we're at it, fix a
> minor validation deficiency in the recovery function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Looks good to me
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/xfs_ondisk.h    |    3 ++
>  fs/xfs/xfs_rmap_item.c |   58 ++++++++++++++++++++++----------------
> ----------
>  2 files changed, 30 insertions(+), 31 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index e20d2844b0c5..19c1df00b48e 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -138,11 +138,14 @@ xfs_check_ondisk_structs(void)
>         XFS_CHECK_STRUCT_SIZE(struct xfs_bud_log_format,        16);
>         XFS_CHECK_STRUCT_SIZE(struct xfs_cui_log_format,        16);
>         XFS_CHECK_STRUCT_SIZE(struct xfs_cud_log_format,        16);
> +       XFS_CHECK_STRUCT_SIZE(struct xfs_rui_log_format,        16);
> +       XFS_CHECK_STRUCT_SIZE(struct xfs_rud_log_format,        16);
>         XFS_CHECK_STRUCT_SIZE(struct xfs_map_extent,            32);
>         XFS_CHECK_STRUCT_SIZE(struct xfs_phys_extent,           16);
>  
>         XFS_CHECK_OFFSET(struct xfs_bui_log_format,
> bui_extents,        16);
>         XFS_CHECK_OFFSET(struct xfs_cui_log_format,
> cui_extents,        16);
> +       XFS_CHECK_OFFSET(struct xfs_rui_log_format,
> rui_extents,        16);
>  
>         /*
>          * The v5 superblock format extended several v4 header
> structures with
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index fef92e02f3bb..27047e73f582 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -155,31 +155,6 @@ xfs_rui_init(
>         return ruip;
>  }
>  
> -/*
> - * Copy an RUI format buffer from the given buf, and into the
> destination
> - * RUI format structure.  The RUI/RUD items were designed not to
> need any
> - * special alignment handling.
> - */
> -STATIC int
> -xfs_rui_copy_format(
> -       struct xfs_log_iovec            *buf,
> -       struct xfs_rui_log_format       *dst_rui_fmt)
> -{
> -       struct xfs_rui_log_format       *src_rui_fmt;
> -       uint                            len;
> -
> -       src_rui_fmt = buf->i_addr;
> -       len = xfs_rui_log_format_sizeof(src_rui_fmt->rui_nextents);
> -
> -       if (buf->i_len != len) {
> -               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> -               return -EFSCORRUPTED;
> -       }
> -
> -       memcpy(dst_rui_fmt, src_rui_fmt, len);
> -       return 0;
> -}
> -
>  static inline struct xfs_rud_log_item *RUD_ITEM(struct xfs_log_item
> *lip)
>  {
>         return container_of(lip, struct xfs_rud_log_item, rud_item);
> @@ -652,6 +627,20 @@ static const struct xfs_item_ops
> xfs_rui_item_ops = {
>         .iop_relog      = xfs_rui_item_relog,
>  };
>  
> +static inline void
> +xfs_rui_copy_format(
> +       struct xfs_rui_log_format       *dst,
> +       const struct xfs_rui_log_format *src)
> +{
> +       unsigned int                    i;
> +
> +       memcpy(dst, src, offsetof(struct xfs_rui_log_format,
> rui_extents));
> +
> +       for (i = 0; i < src->rui_nextents; i++)
> +               memcpy(&dst->rui_extents[i], &src->rui_extents[i],
> +                               sizeof(struct xfs_map_extent));
> +}
> +
>  /*
>   * This routine is called to create an in-core extent rmap update
>   * item from the rui format structure which was logged on disk.
> @@ -666,19 +655,26 @@ xlog_recover_rui_commit_pass2(
>         struct xlog_recover_item        *item,
>         xfs_lsn_t                       lsn)
>  {
> -       int                             error;
>         struct xfs_mount                *mp = log->l_mp;
>         struct xfs_rui_log_item         *ruip;
>         struct xfs_rui_log_format       *rui_formatp;
> +       size_t                          len;
>  
>         rui_formatp = item->ri_buf[0].i_addr;
>  
> +       if (item->ri_buf[0].i_len < xfs_rui_log_format_sizeof(0)) {
> +               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log-
> >l_mp);
> +               return -EFSCORRUPTED;
> +       }
> +
> +       len = xfs_rui_log_format_sizeof(rui_formatp->rui_nextents);
> +       if (item->ri_buf[0].i_len != len) {
> +               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log-
> >l_mp);
> +               return -EFSCORRUPTED;
> +       }
> +
>         ruip = xfs_rui_init(mp, rui_formatp->rui_nextents);
> -       error = xfs_rui_copy_format(&item->ri_buf[0], &ruip-
> >rui_format);
> -       if (error) {
> -               xfs_rui_item_free(ruip);
> -               return error;
> -       }
> +       xfs_rui_copy_format(&ruip->rui_format, rui_formatp);
>         atomic_set(&ruip->rui_next_extent, rui_formatp-
> >rui_nextents);
>         /*
>          * Insert the intent into the AIL directly and drop one
> reference so
> 

Re: [PATCH 5/6] xfs: fix memcpy fortify errors in EFI log format copying

[Allison Henderson]

On Mon, 2022-10-24 at 14:32 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of
> memcpy.  Since we're already fixing problems with BUI item copying,
> we
> should fix it everything else.
> 
> An extra difficulty here is that the ef[id]_extents arrays are
> declared
> as single-element arrays.  This is not the convention for flex arrays
> in
> the modern kernel, and it causes all manner of problems with static
> checking tools, since they often cannot tell the difference between a
> single element array and a flex array.
> 
> So for starters, change those array[1] declarations to array[]
> declarations to signal that they are proper flex arrays and adjust
> all
> the "size-1" expressions to fit the new declaration style.
> 
> Next, refactor the xfs_efi_copy_format function to handle the copying
> of
> the head and the flex array members separately.  While we're at it,
> fix
> a minor validation deficiency in the recovery function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_log_format.h |   12 ++++++------
>  fs/xfs/xfs_extfree_item.c      |   31 +++++++++++++++++++++---------
> -
>  fs/xfs/xfs_ondisk.h            |   11 +++++++----
>  fs/xfs/xfs_super.c             |    4 ++--
>  4 files changed, 36 insertions(+), 22 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_log_format.h
> b/fs/xfs/libxfs/xfs_log_format.h
> index b351b9dc6561..2f41fa8477c9 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -613,7 +613,7 @@ typedef struct xfs_efi_log_format {
>         uint16_t                efi_size;       /* size of this item
> */
>         uint32_t                efi_nextents;   /* # extents to free
> */
>         uint64_t                efi_id;         /* efi identifier */
> -       xfs_extent_t            efi_extents[1]; /* array of extents
> to free */
> +       xfs_extent_t            efi_extents[];  /* array of extents
> to free */
>  } xfs_efi_log_format_t;
>  
>  typedef struct xfs_efi_log_format_32 {
> @@ -621,7 +621,7 @@ typedef struct xfs_efi_log_format_32 {
>         uint16_t                efi_size;       /* size of this item
> */
>         uint32_t                efi_nextents;   /* # extents to free
> */
>         uint64_t                efi_id;         /* efi identifier */
> -       xfs_extent_32_t         efi_extents[1]; /* array of extents
> to free */
> +       xfs_extent_32_t         efi_extents[];  /* array of extents
> to free */
>  } __attribute__((packed)) xfs_efi_log_format_32_t;
>  
>  typedef struct xfs_efi_log_format_64 {
> @@ -629,7 +629,7 @@ typedef struct xfs_efi_log_format_64 {
>         uint16_t                efi_size;       /* size of this item
> */
>         uint32_t                efi_nextents;   /* # extents to free
> */
>         uint64_t                efi_id;         /* efi identifier */
> -       xfs_extent_64_t         efi_extents[1]; /* array of extents
> to free */
> +       xfs_extent_64_t         efi_extents[];  /* array of extents
> to free */
>  } xfs_efi_log_format_64_t;
>  
>  /*
> @@ -642,7 +642,7 @@ typedef struct xfs_efd_log_format {
>         uint16_t                efd_size;       /* size of this item
> */
>         uint32_t                efd_nextents;   /* # of extents freed
> */
>         uint64_t                efd_efi_id;     /* id of
> corresponding efi */
> -       xfs_extent_t            efd_extents[1]; /* array of extents
> freed */
> +       xfs_extent_t            efd_extents[];  /* array of extents
> freed */
>  } xfs_efd_log_format_t;
>  
>  typedef struct xfs_efd_log_format_32 {
> @@ -650,7 +650,7 @@ typedef struct xfs_efd_log_format_32 {
>         uint16_t                efd_size;       /* size of this item
> */
>         uint32_t                efd_nextents;   /* # of extents freed
> */
>         uint64_t                efd_efi_id;     /* id of
> corresponding efi */
> -       xfs_extent_32_t         efd_extents[1]; /* array of extents
> freed */
> +       xfs_extent_32_t         efd_extents[];  /* array of extents
> freed */
>  } __attribute__((packed)) xfs_efd_log_format_32_t;
>  
>  typedef struct xfs_efd_log_format_64 {
> @@ -658,7 +658,7 @@ typedef struct xfs_efd_log_format_64 {
>         uint16_t                efd_size;       /* size of this item
> */
>         uint32_t                efd_nextents;   /* # of extents freed
> */
>         uint64_t                efd_efi_id;     /* id of
> corresponding efi */
> -       xfs_extent_64_t         efd_extents[1]; /* array of extents
> freed */
> +       xfs_extent_64_t         efd_extents[];  /* array of extents
> freed */
>  } xfs_efd_log_format_64_t;
>  
>  /*
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 27ccfcd82f04..466cc5c5cd33 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -76,7 +76,7 @@ xfs_efi_item_sizeof(
>         struct xfs_efi_log_item *efip)
>  {
>         return sizeof(struct xfs_efi_log_format) +
> -              (efip->efi_format.efi_nextents - 1) *
> sizeof(xfs_extent_t);
> +              efip->efi_format.efi_nextents * sizeof(xfs_extent_t);
Did we want to try and avoid using typedefs?  I notice that seems to
come up a lot in reviews.  Otherwise the rest looks good.

Allison

>  }
>  
>  STATIC void
> @@ -160,7 +160,7 @@ xfs_efi_init(
>         ASSERT(nextents > 0);
>         if (nextents > XFS_EFI_MAX_FAST_EXTENTS) {
>                 size = (uint)(sizeof(struct xfs_efi_log_item) +
> -                       ((nextents - 1) * sizeof(xfs_extent_t)));
> +                       (nextents * sizeof(xfs_extent_t)));
>                 efip = kmem_zalloc(size, 0);
>         } else {
>                 efip = kmem_cache_zalloc(xfs_efi_cache,
> @@ -189,14 +189,19 @@ xfs_efi_copy_format(xfs_log_iovec_t *buf,
> xfs_efi_log_format_t *dst_efi_fmt)
>         xfs_efi_log_format_t *src_efi_fmt = buf->i_addr;
>         uint i;
>         uint len = sizeof(xfs_efi_log_format_t) +
> -               (src_efi_fmt->efi_nextents - 1) *
> sizeof(xfs_extent_t);
> +               src_efi_fmt->efi_nextents * sizeof(xfs_extent_t);
>         uint len32 = sizeof(xfs_efi_log_format_32_t) +
> -               (src_efi_fmt->efi_nextents - 1) *
> sizeof(xfs_extent_32_t);
> +               src_efi_fmt->efi_nextents * sizeof(xfs_extent_32_t);
>         uint len64 = sizeof(xfs_efi_log_format_64_t) +
> -               (src_efi_fmt->efi_nextents - 1) *
> sizeof(xfs_extent_64_t);
> +               src_efi_fmt->efi_nextents * sizeof(xfs_extent_64_t);
>  
>         if (buf->i_len == len) {
> -               memcpy((char *)dst_efi_fmt, (char*)src_efi_fmt, len);
> +               memcpy(dst_efi_fmt, src_efi_fmt,
> +                      offsetof(struct xfs_efi_log_format,
> efi_extents));
> +               for (i = 0; i < src_efi_fmt->efi_nextents; i++)
> +                       memcpy(&dst_efi_fmt->efi_extents[i],
> +                              &src_efi_fmt->efi_extents[i],
> +                              sizeof(struct xfs_extent));
>                 return 0;
>         } else if (buf->i_len == len32) {
>                 xfs_efi_log_format_32_t *src_efi_fmt_32 = buf-
> >i_addr;
> @@ -256,7 +261,7 @@ xfs_efd_item_sizeof(
>         struct xfs_efd_log_item *efdp)
>  {
>         return sizeof(xfs_efd_log_format_t) +
> -              (efdp->efd_format.efd_nextents - 1) *
> sizeof(xfs_extent_t);
> +              efdp->efd_format.efd_nextents * sizeof(xfs_extent_t);
>  }
>  
>  STATIC void
> @@ -341,7 +346,7 @@ xfs_trans_get_efd(
>  
>         if (nextents > XFS_EFD_MAX_FAST_EXTENTS) {
>                 efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) +
> -                               (nextents - 1) * sizeof(struct
> xfs_extent),
> +                               nextents * sizeof(struct xfs_extent),
>                                 0);
>         } else {
>                 efdp = kmem_cache_zalloc(xfs_efd_cache,
> @@ -733,6 +738,12 @@ xlog_recover_efi_commit_pass2(
>  
>         efi_formatp = item->ri_buf[0].i_addr;
>  
> +       if (item->ri_buf[0].i_len <
> +                       offsetof(struct xfs_efi_log_format,
> efi_extents)) {
> +               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log-
> >l_mp);
> +               return -EFSCORRUPTED;
> +       }
> +
>         efip = xfs_efi_init(mp, efi_formatp->efi_nextents);
>         error = xfs_efi_copy_format(&item->ri_buf[0], &efip-
> >efi_format);
>         if (error) {
> @@ -772,9 +783,9 @@ xlog_recover_efd_commit_pass2(
>  
>         efd_formatp = item->ri_buf[0].i_addr;
>         ASSERT((item->ri_buf[0].i_len ==
> (sizeof(xfs_efd_log_format_32_t) +
> -               ((efd_formatp->efd_nextents - 1) *
> sizeof(xfs_extent_32_t)))) ||
> +               (efd_formatp->efd_nextents *
> sizeof(xfs_extent_32_t)))) ||
>                (item->ri_buf[0].i_len ==
> (sizeof(xfs_efd_log_format_64_t) +
> -               ((efd_formatp->efd_nextents - 1) *
> sizeof(xfs_extent_64_t)))));
> +               (efd_formatp->efd_nextents *
> sizeof(xfs_extent_64_t)))));
>  
>         xlog_recover_release_intent(log, XFS_LI_EFI, efd_formatp-
> >efd_efi_id);
>         return 0;
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index 19c1df00b48e..9737b5a9f405 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -118,10 +118,10 @@ xfs_check_ondisk_structs(void)
>         /* 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);
> -       XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32,     28);
> -       XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64,     32);
> +       XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_32,     16);
> +       XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_64,     16);
> +       XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32,     16);
> +       XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64,     16);
>         XFS_CHECK_STRUCT_SIZE(struct xfs_extent_32,             12);
>         XFS_CHECK_STRUCT_SIZE(struct xfs_extent_64,             16);
>         XFS_CHECK_STRUCT_SIZE(struct xfs_log_dinode,            176);
> @@ -146,6 +146,9 @@ xfs_check_ondisk_structs(void)
>         XFS_CHECK_OFFSET(struct xfs_bui_log_format,
> bui_extents,        16);
>         XFS_CHECK_OFFSET(struct xfs_cui_log_format,
> cui_extents,        16);
>         XFS_CHECK_OFFSET(struct xfs_rui_log_format,
> rui_extents,        16);
> +       XFS_CHECK_OFFSET(struct xfs_efi_log_format,
> efi_extents,        16);
> +       XFS_CHECK_OFFSET(struct xfs_efi_log_format_32,
> efi_extents,     16);
> +       XFS_CHECK_OFFSET(struct xfs_efi_log_format_64,
> efi_extents,     16);
>  
>         /*
>          * The v5 superblock format extended several v4 header
> structures with
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index f029c6702dda..8485e3b37ca0 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -2029,7 +2029,7 @@ xfs_init_caches(void)
>  
>         xfs_efd_cache = kmem_cache_create("xfs_efd_item",
>                                         (sizeof(struct
> xfs_efd_log_item) +
> -                                       (XFS_EFD_MAX_FAST_EXTENTS -
> 1) *
> +                                       XFS_EFD_MAX_FAST_EXTENTS *
>                                         sizeof(struct xfs_extent)),
>                                         0, 0, NULL);
>         if (!xfs_efd_cache)
> @@ -2037,7 +2037,7 @@ xfs_init_caches(void)
>  
>         xfs_efi_cache = kmem_cache_create("xfs_efi_item",
>                                          (sizeof(struct
> xfs_efi_log_item) +
> -                                        (XFS_EFI_MAX_FAST_EXTENTS -
> 1) *
> +                                        XFS_EFI_MAX_FAST_EXTENTS *
>                                          sizeof(struct xfs_extent)),
>                                          0, 0, NULL);
>         if (!xfs_efi_cache)
> 

Re: [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic

[Allison Henderson]

On Mon, 2022-10-24 at 14:33 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Refactor all the open-coded sizeof logic for EFI/EFD log item and log
> format structures into common helper functions whose names reflect
> the
> struct names.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Looks ok to me
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_log_format.h |   48 ++++++++++++++++++++++++++++
>  fs/xfs/xfs_extfree_item.c      |   69 ++++++++++++------------------
> ----------
>  fs/xfs/xfs_extfree_item.h      |   16 +++++++++
>  fs/xfs/xfs_super.c             |   12 ++-----
>  4 files changed, 88 insertions(+), 57 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_log_format.h
> b/fs/xfs/libxfs/xfs_log_format.h
> index 2f41fa8477c9..f13e0809dc63 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -616,6 +616,14 @@ typedef struct xfs_efi_log_format {
>         xfs_extent_t            efi_extents[];  /* array of extents
> to free */
>  } xfs_efi_log_format_t;
>  
> +static inline size_t
> +xfs_efi_log_format_sizeof(
> +       unsigned int            nr)
> +{
> +       return sizeof(struct xfs_efi_log_format) +
> +                       nr * sizeof(struct xfs_extent);
> +}
> +
>  typedef struct xfs_efi_log_format_32 {
>         uint16_t                efi_type;       /* efi log item type
> */
>         uint16_t                efi_size;       /* size of this item
> */
> @@ -624,6 +632,14 @@ typedef struct xfs_efi_log_format_32 {
>         xfs_extent_32_t         efi_extents[];  /* array of extents
> to free */
>  } __attribute__((packed)) xfs_efi_log_format_32_t;
>  
> +static inline size_t
> +xfs_efi_log_format32_sizeof(
> +       unsigned int            nr)
> +{
> +       return sizeof(struct xfs_efi_log_format_32) +
> +                       nr * sizeof(struct xfs_extent_32);
> +}
> +
>  typedef struct xfs_efi_log_format_64 {
>         uint16_t                efi_type;       /* efi log item type
> */
>         uint16_t                efi_size;       /* size of this item
> */
> @@ -632,6 +648,14 @@ typedef struct xfs_efi_log_format_64 {
>         xfs_extent_64_t         efi_extents[];  /* array of extents
> to free */
>  } xfs_efi_log_format_64_t;
>  
> +static inline size_t
> +xfs_efi_log_format64_sizeof(
> +       unsigned int            nr)
> +{
> +       return sizeof(struct xfs_efi_log_format_64) +
> +                       nr * sizeof(struct xfs_extent_64);
> +}
> +
>  /*
>   * This is the structure used to lay out an efd log item in the
>   * log.  The efd_extents array is a variable size array whose
> @@ -645,6 +669,14 @@ typedef struct xfs_efd_log_format {
>         xfs_extent_t            efd_extents[];  /* array of extents
> freed */
>  } xfs_efd_log_format_t;
>  
> +static inline size_t
> +xfs_efd_log_format_sizeof(
> +       unsigned int            nr)
> +{
> +       return sizeof(struct xfs_efd_log_format) +
> +                       nr * sizeof(struct xfs_extent);
> +}
> +
>  typedef struct xfs_efd_log_format_32 {
>         uint16_t                efd_type;       /* efd log item type
> */
>         uint16_t                efd_size;       /* size of this item
> */
> @@ -653,6 +685,14 @@ typedef struct xfs_efd_log_format_32 {
>         xfs_extent_32_t         efd_extents[];  /* array of extents
> freed */
>  } __attribute__((packed)) xfs_efd_log_format_32_t;
>  
> +static inline size_t
> +xfs_efd_log_format32_sizeof(
> +       unsigned int            nr)
> +{
> +       return sizeof(struct xfs_efd_log_format_32) +
> +                       nr * sizeof(struct xfs_extent_32);
> +}
> +
>  typedef struct xfs_efd_log_format_64 {
>         uint16_t                efd_type;       /* efd log item type
> */
>         uint16_t                efd_size;       /* size of this item
> */
> @@ -661,6 +701,14 @@ typedef struct xfs_efd_log_format_64 {
>         xfs_extent_64_t         efd_extents[];  /* array of extents
> freed */
>  } xfs_efd_log_format_64_t;
>  
> +static inline size_t
> +xfs_efd_log_format64_sizeof(
> +       unsigned int            nr)
> +{
> +       return sizeof(struct xfs_efd_log_format_64) +
> +                       nr * sizeof(struct xfs_extent_64);
> +}
> +
>  /*
>   * RUI/RUD (reverse mapping) log format definitions
>   */
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 466cc5c5cd33..bffb2b91e39a 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -66,27 +66,16 @@ xfs_efi_release(
>         xfs_efi_item_free(efip);
>  }
>  
> -/*
> - * This returns the number of iovecs needed to log the given efi
> item.
> - * We only need 1 iovec for an efi item.  It just logs the
> efi_log_format
> - * structure.
> - */
> -static inline int
> -xfs_efi_item_sizeof(
> -       struct xfs_efi_log_item *efip)
> -{
> -       return sizeof(struct xfs_efi_log_format) +
> -              efip->efi_format.efi_nextents * sizeof(xfs_extent_t);
> -}
> -
>  STATIC void
>  xfs_efi_item_size(
>         struct xfs_log_item     *lip,
>         int                     *nvecs,
>         int                     *nbytes)
>  {
> +       struct xfs_efi_log_item *efip = EFI_ITEM(lip);
> +
>         *nvecs += 1;
> -       *nbytes += xfs_efi_item_sizeof(EFI_ITEM(lip));
> +       *nbytes += xfs_efi_log_format_sizeof(efip-
> >efi_format.efi_nextents);
>  }
>  
>  /*
> @@ -112,7 +101,7 @@ xfs_efi_item_format(
>  
>         xlog_copy_iovec(lv, &vecp, XLOG_REG_TYPE_EFI_FORMAT,
>                         &efip->efi_format,
> -                       xfs_efi_item_sizeof(efip));
> +                       xfs_efi_log_format_sizeof(efip-
> >efi_format.efi_nextents));
>  }
>  
>  
> @@ -155,13 +144,11 @@ xfs_efi_init(
>  
>  {
>         struct xfs_efi_log_item *efip;
> -       uint                    size;
>  
>         ASSERT(nextents > 0);
>         if (nextents > XFS_EFI_MAX_FAST_EXTENTS) {
> -               size = (uint)(sizeof(struct xfs_efi_log_item) +
> -                       (nextents * sizeof(xfs_extent_t)));
> -               efip = kmem_zalloc(size, 0);
> +               efip = kmem_zalloc(xfs_efi_log_item_sizeof(nextents),
> +                               GFP_KERNEL | __GFP_NOFAIL);
>         } else {
>                 efip = kmem_cache_zalloc(xfs_efi_cache,
>                                          GFP_KERNEL | __GFP_NOFAIL);
> @@ -188,12 +175,9 @@ xfs_efi_copy_format(xfs_log_iovec_t *buf,
> xfs_efi_log_format_t *dst_efi_fmt)
>  {
>         xfs_efi_log_format_t *src_efi_fmt = buf->i_addr;
>         uint i;
> -       uint len = sizeof(xfs_efi_log_format_t) +
> -               src_efi_fmt->efi_nextents * sizeof(xfs_extent_t);
> -       uint len32 = sizeof(xfs_efi_log_format_32_t) +
> -               src_efi_fmt->efi_nextents * sizeof(xfs_extent_32_t);
> -       uint len64 = sizeof(xfs_efi_log_format_64_t) +
> -               src_efi_fmt->efi_nextents * sizeof(xfs_extent_64_t);
> +       uint len = xfs_efi_log_format_sizeof(src_efi_fmt-
> >efi_nextents);
> +       uint len32 = xfs_efi_log_format32_sizeof(src_efi_fmt-
> >efi_nextents);
> +       uint len64 = xfs_efi_log_format64_sizeof(src_efi_fmt-
> >efi_nextents);
>  
>         if (buf->i_len == len) {
>                 memcpy(dst_efi_fmt, src_efi_fmt,
> @@ -251,27 +235,16 @@ xfs_efd_item_free(struct xfs_efd_log_item
> *efdp)
>                 kmem_cache_free(xfs_efd_cache, efdp);
>  }
>  
> -/*
> - * This returns the number of iovecs needed to log the given efd
> item.
> - * We only need 1 iovec for an efd item.  It just logs the
> efd_log_format
> - * structure.
> - */
> -static inline int
> -xfs_efd_item_sizeof(
> -       struct xfs_efd_log_item *efdp)
> -{
> -       return sizeof(xfs_efd_log_format_t) +
> -              efdp->efd_format.efd_nextents * sizeof(xfs_extent_t);
> -}
> -
>  STATIC void
>  xfs_efd_item_size(
>         struct xfs_log_item     *lip,
>         int                     *nvecs,
>         int                     *nbytes)
>  {
> +       struct xfs_efd_log_item *efdp = EFD_ITEM(lip);
> +
>         *nvecs += 1;
> -       *nbytes += xfs_efd_item_sizeof(EFD_ITEM(lip));
> +       *nbytes += xfs_efd_log_format_sizeof(efdp-
> >efd_format.efd_nextents);
>  }
>  
>  /*
> @@ -296,7 +269,7 @@ xfs_efd_item_format(
>  
>         xlog_copy_iovec(lv, &vecp, XLOG_REG_TYPE_EFD_FORMAT,
>                         &efdp->efd_format,
> -                       xfs_efd_item_sizeof(efdp));
> +                       xfs_efd_log_format_sizeof(efdp-
> >efd_format.efd_nextents));
>  }
>  
>  /*
> @@ -345,9 +318,8 @@ xfs_trans_get_efd(
>         ASSERT(nextents > 0);
>  
>         if (nextents > XFS_EFD_MAX_FAST_EXTENTS) {
> -               efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) +
> -                               nextents * sizeof(struct xfs_extent),
> -                               0);
> +               efdp = kmem_zalloc(xfs_efd_log_item_sizeof(nextents),
> +                               GFP_KERNEL | __GFP_NOFAIL);
>         } else {
>                 efdp = kmem_cache_zalloc(xfs_efd_cache,
>                                         GFP_KERNEL | __GFP_NOFAIL);
> @@ -738,8 +710,7 @@ xlog_recover_efi_commit_pass2(
>  
>         efi_formatp = item->ri_buf[0].i_addr;
>  
> -       if (item->ri_buf[0].i_len <
> -                       offsetof(struct xfs_efi_log_format,
> efi_extents)) {
> +       if (item->ri_buf[0].i_len < xfs_efi_log_format_sizeof(0)) {
>                 XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log-
> >l_mp);
>                 return -EFSCORRUPTED;
>         }
> @@ -782,10 +753,10 @@ xlog_recover_efd_commit_pass2(
>         struct xfs_efd_log_format       *efd_formatp;
>  
>         efd_formatp = item->ri_buf[0].i_addr;
> -       ASSERT((item->ri_buf[0].i_len ==
> (sizeof(xfs_efd_log_format_32_t) +
> -               (efd_formatp->efd_nextents *
> sizeof(xfs_extent_32_t)))) ||
> -              (item->ri_buf[0].i_len ==
> (sizeof(xfs_efd_log_format_64_t) +
> -               (efd_formatp->efd_nextents *
> sizeof(xfs_extent_64_t)))));
> +       ASSERT(item->ri_buf[0].i_len == xfs_efd_log_format32_sizeof(
> +                                               efd_formatp-
> >efd_nextents) ||
> +              item->ri_buf[0].i_len == xfs_efd_log_format64_sizeof(
> +                                               efd_formatp-
> >efd_nextents));
>  
>         xlog_recover_release_intent(log, XFS_LI_EFI, efd_formatp-
> >efd_efi_id);
>         return 0;
> diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
> index 186d0f2137f1..da6a5afa607c 100644
> --- a/fs/xfs/xfs_extfree_item.h
> +++ b/fs/xfs/xfs_extfree_item.h
> @@ -52,6 +52,14 @@ struct xfs_efi_log_item {
>         xfs_efi_log_format_t    efi_format;
>  };
>  
> +static inline size_t
> +xfs_efi_log_item_sizeof(
> +       unsigned int            nr)
> +{
> +       return offsetof(struct xfs_efi_log_item, efi_format) +
> +                       xfs_efi_log_format_sizeof(nr);
> +}
> +
>  /*
>   * This is the "extent free done" log item.  It is used to log
>   * the fact that some extents earlier mentioned in an efi item
> @@ -64,6 +72,14 @@ struct xfs_efd_log_item {
>         xfs_efd_log_format_t    efd_format;
>  };
>  
> +static inline size_t
> +xfs_efd_log_item_sizeof(
> +       unsigned int            nr)
> +{
> +       return offsetof(struct xfs_efd_log_item, efd_format) +
> +                       xfs_efd_log_format_sizeof(nr);
> +}
> +
>  /*
>   * Max number of extents in fast allocation path.
>   */
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 8485e3b37ca0..ee4b429a2f2c 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -2028,18 +2028,14 @@ xfs_init_caches(void)
>                 goto out_destroy_trans_cache;
>  
>         xfs_efd_cache = kmem_cache_create("xfs_efd_item",
> -                                       (sizeof(struct
> xfs_efd_log_item) +
> -                                       XFS_EFD_MAX_FAST_EXTENTS *
> -                                       sizeof(struct xfs_extent)),
> -                                       0, 0, NULL);
> +                       xfs_efd_log_item_sizeof(XFS_EFD_MAX_FAST_EXTE
> NTS),
> +                       0, 0, NULL);
>         if (!xfs_efd_cache)
>                 goto out_destroy_buf_item_cache;
>  
>         xfs_efi_cache = kmem_cache_create("xfs_efi_item",
> -                                        (sizeof(struct
> xfs_efi_log_item) +
> -                                        XFS_EFI_MAX_FAST_EXTENTS *
> -                                        sizeof(struct xfs_extent)),
> -                                        0, 0, NULL);
> +                       xfs_efi_log_item_sizeof(XFS_EFI_MAX_FAST_EXTE
> NTS),
> +                       0, 0, NULL);
>         if (!xfs_efi_cache)
>                 goto out_destroy_efd_cache;
>  
> 

Re: [PATCH 5/6] xfs: fix memcpy fortify errors in EFI log format copying

[Darrick J. Wong]

On Tue, Oct 25, 2022 at 08:54:25PM +0000, Allison Henderson wrote:
> On Mon, 2022-10-24 at 14:32 -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of
> > memcpy.  Since we're already fixing problems with BUI item copying,
> > we
> > should fix it everything else.
> > 
> > An extra difficulty here is that the ef[id]_extents arrays are
> > declared
> > as single-element arrays.  This is not the convention for flex arrays
> > in
> > the modern kernel, and it causes all manner of problems with static
> > checking tools, since they often cannot tell the difference between a
> > single element array and a flex array.
> > 
> > So for starters, change those array[1] declarations to array[]
> > declarations to signal that they are proper flex arrays and adjust
> > all
> > the "size-1" expressions to fit the new declaration style.
> > 
> > Next, refactor the xfs_efi_copy_format function to handle the copying
> > of
> > the head and the flex array members separately.  While we're at it,
> > fix
> > a minor validation deficiency in the recovery function.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_log_format.h |   12 ++++++------
> >  fs/xfs/xfs_extfree_item.c      |   31 +++++++++++++++++++++---------
> > -
> >  fs/xfs/xfs_ondisk.h            |   11 +++++++----
> >  fs/xfs/xfs_super.c             |    4 ++--
> >  4 files changed, 36 insertions(+), 22 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_log_format.h
> > b/fs/xfs/libxfs/xfs_log_format.h
> > index b351b9dc6561..2f41fa8477c9 100644
> > --- a/fs/xfs/libxfs/xfs_log_format.h
> > +++ b/fs/xfs/libxfs/xfs_log_format.h
> > @@ -613,7 +613,7 @@ typedef struct xfs_efi_log_format {
> >         uint16_t                efi_size;       /* size of this item
> > */
> >         uint32_t                efi_nextents;   /* # extents to free
> > */
> >         uint64_t                efi_id;         /* efi identifier */
> > -       xfs_extent_t            efi_extents[1]; /* array of extents
> > to free */
> > +       xfs_extent_t            efi_extents[];  /* array of extents
> > to free */
> >  } xfs_efi_log_format_t;
> >  
> >  typedef struct xfs_efi_log_format_32 {
> > @@ -621,7 +621,7 @@ typedef struct xfs_efi_log_format_32 {
> >         uint16_t                efi_size;       /* size of this item
> > */
> >         uint32_t                efi_nextents;   /* # extents to free
> > */
> >         uint64_t                efi_id;         /* efi identifier */
> > -       xfs_extent_32_t         efi_extents[1]; /* array of extents
> > to free */
> > +       xfs_extent_32_t         efi_extents[];  /* array of extents
> > to free */
> >  } __attribute__((packed)) xfs_efi_log_format_32_t;
> >  
> >  typedef struct xfs_efi_log_format_64 {
> > @@ -629,7 +629,7 @@ typedef struct xfs_efi_log_format_64 {
> >         uint16_t                efi_size;       /* size of this item
> > */
> >         uint32_t                efi_nextents;   /* # extents to free
> > */
> >         uint64_t                efi_id;         /* efi identifier */
> > -       xfs_extent_64_t         efi_extents[1]; /* array of extents
> > to free */
> > +       xfs_extent_64_t         efi_extents[];  /* array of extents
> > to free */
> >  } xfs_efi_log_format_64_t;
> >  
> >  /*
> > @@ -642,7 +642,7 @@ typedef struct xfs_efd_log_format {
> >         uint16_t                efd_size;       /* size of this item
> > */
> >         uint32_t                efd_nextents;   /* # of extents freed
> > */
> >         uint64_t                efd_efi_id;     /* id of
> > corresponding efi */
> > -       xfs_extent_t            efd_extents[1]; /* array of extents
> > freed */
> > +       xfs_extent_t            efd_extents[];  /* array of extents
> > freed */
> >  } xfs_efd_log_format_t;
> >  
> >  typedef struct xfs_efd_log_format_32 {
> > @@ -650,7 +650,7 @@ typedef struct xfs_efd_log_format_32 {
> >         uint16_t                efd_size;       /* size of this item
> > */
> >         uint32_t                efd_nextents;   /* # of extents freed
> > */
> >         uint64_t                efd_efi_id;     /* id of
> > corresponding efi */
> > -       xfs_extent_32_t         efd_extents[1]; /* array of extents
> > freed */
> > +       xfs_extent_32_t         efd_extents[];  /* array of extents
> > freed */
> >  } __attribute__((packed)) xfs_efd_log_format_32_t;
> >  
> >  typedef struct xfs_efd_log_format_64 {
> > @@ -658,7 +658,7 @@ typedef struct xfs_efd_log_format_64 {
> >         uint16_t                efd_size;       /* size of this item
> > */
> >         uint32_t                efd_nextents;   /* # of extents freed
> > */
> >         uint64_t                efd_efi_id;     /* id of
> > corresponding efi */
> > -       xfs_extent_64_t         efd_extents[1]; /* array of extents
> > freed */
> > +       xfs_extent_64_t         efd_extents[];  /* array of extents
> > freed */
> >  } xfs_efd_log_format_64_t;
> >  
> >  /*
> > diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> > index 27ccfcd82f04..466cc5c5cd33 100644
> > --- a/fs/xfs/xfs_extfree_item.c
> > +++ b/fs/xfs/xfs_extfree_item.c
> > @@ -76,7 +76,7 @@ xfs_efi_item_sizeof(
> >         struct xfs_efi_log_item *efip)
> >  {
> >         return sizeof(struct xfs_efi_log_format) +
> > -              (efip->efi_format.efi_nextents - 1) *
> > sizeof(xfs_extent_t);
> > +              efip->efi_format.efi_nextents * sizeof(xfs_extent_t);
> Did we want to try and avoid using typedefs?  I notice that seems to
> come up a lot in reviews.  Otherwise the rest looks good.

Yes, but I kill off these typedef usages in the next patch by creating
the sizeof helpers.

--D

> Allison
> 
> >  }
> >  
> >  STATIC void
> > @@ -160,7 +160,7 @@ xfs_efi_init(
> >         ASSERT(nextents > 0);
> >         if (nextents > XFS_EFI_MAX_FAST_EXTENTS) {
> >                 size = (uint)(sizeof(struct xfs_efi_log_item) +
> > -                       ((nextents - 1) * sizeof(xfs_extent_t)));
> > +                       (nextents * sizeof(xfs_extent_t)));
> >                 efip = kmem_zalloc(size, 0);
> >         } else {
> >                 efip = kmem_cache_zalloc(xfs_efi_cache,
> > @@ -189,14 +189,19 @@ xfs_efi_copy_format(xfs_log_iovec_t *buf,
> > xfs_efi_log_format_t *dst_efi_fmt)
> >         xfs_efi_log_format_t *src_efi_fmt = buf->i_addr;
> >         uint i;
> >         uint len = sizeof(xfs_efi_log_format_t) +
> > -               (src_efi_fmt->efi_nextents - 1) *
> > sizeof(xfs_extent_t);
> > +               src_efi_fmt->efi_nextents * sizeof(xfs_extent_t);
> >         uint len32 = sizeof(xfs_efi_log_format_32_t) +
> > -               (src_efi_fmt->efi_nextents - 1) *
> > sizeof(xfs_extent_32_t);
> > +               src_efi_fmt->efi_nextents * sizeof(xfs_extent_32_t);
> >         uint len64 = sizeof(xfs_efi_log_format_64_t) +
> > -               (src_efi_fmt->efi_nextents - 1) *
> > sizeof(xfs_extent_64_t);
> > +               src_efi_fmt->efi_nextents * sizeof(xfs_extent_64_t);
> >  
> >         if (buf->i_len == len) {
> > -               memcpy((char *)dst_efi_fmt, (char*)src_efi_fmt, len);
> > +               memcpy(dst_efi_fmt, src_efi_fmt,
> > +                      offsetof(struct xfs_efi_log_format,
> > efi_extents));
> > +               for (i = 0; i < src_efi_fmt->efi_nextents; i++)
> > +                       memcpy(&dst_efi_fmt->efi_extents[i],
> > +                              &src_efi_fmt->efi_extents[i],
> > +                              sizeof(struct xfs_extent));
> >                 return 0;
> >         } else if (buf->i_len == len32) {
> >                 xfs_efi_log_format_32_t *src_efi_fmt_32 = buf-
> > >i_addr;
> > @@ -256,7 +261,7 @@ xfs_efd_item_sizeof(
> >         struct xfs_efd_log_item *efdp)
> >  {
> >         return sizeof(xfs_efd_log_format_t) +
> > -              (efdp->efd_format.efd_nextents - 1) *
> > sizeof(xfs_extent_t);
> > +              efdp->efd_format.efd_nextents * sizeof(xfs_extent_t);
> >  }
> >  
> >  STATIC void
> > @@ -341,7 +346,7 @@ xfs_trans_get_efd(
> >  
> >         if (nextents > XFS_EFD_MAX_FAST_EXTENTS) {
> >                 efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) +
> > -                               (nextents - 1) * sizeof(struct
> > xfs_extent),
> > +                               nextents * sizeof(struct xfs_extent),
> >                                 0);
> >         } else {
> >                 efdp = kmem_cache_zalloc(xfs_efd_cache,
> > @@ -733,6 +738,12 @@ xlog_recover_efi_commit_pass2(
> >  
> >         efi_formatp = item->ri_buf[0].i_addr;
> >  
> > +       if (item->ri_buf[0].i_len <
> > +                       offsetof(struct xfs_efi_log_format,
> > efi_extents)) {
> > +               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log-
> > >l_mp);
> > +               return -EFSCORRUPTED;
> > +       }
> > +
> >         efip = xfs_efi_init(mp, efi_formatp->efi_nextents);
> >         error = xfs_efi_copy_format(&item->ri_buf[0], &efip-
> > >efi_format);
> >         if (error) {
> > @@ -772,9 +783,9 @@ xlog_recover_efd_commit_pass2(
> >  
> >         efd_formatp = item->ri_buf[0].i_addr;
> >         ASSERT((item->ri_buf[0].i_len ==
> > (sizeof(xfs_efd_log_format_32_t) +
> > -               ((efd_formatp->efd_nextents - 1) *
> > sizeof(xfs_extent_32_t)))) ||
> > +               (efd_formatp->efd_nextents *
> > sizeof(xfs_extent_32_t)))) ||
> >                (item->ri_buf[0].i_len ==
> > (sizeof(xfs_efd_log_format_64_t) +
> > -               ((efd_formatp->efd_nextents - 1) *
> > sizeof(xfs_extent_64_t)))));
> > +               (efd_formatp->efd_nextents *
> > sizeof(xfs_extent_64_t)))));
> >  
> >         xlog_recover_release_intent(log, XFS_LI_EFI, efd_formatp-
> > >efd_efi_id);
> >         return 0;
> > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> > index 19c1df00b48e..9737b5a9f405 100644
> > --- a/fs/xfs/xfs_ondisk.h
> > +++ b/fs/xfs/xfs_ondisk.h
> > @@ -118,10 +118,10 @@ xfs_check_ondisk_structs(void)
> >         /* 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);
> > -       XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32,     28);
> > -       XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64,     32);
> > +       XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_32,     16);
> > +       XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_64,     16);
> > +       XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32,     16);
> > +       XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64,     16);
> >         XFS_CHECK_STRUCT_SIZE(struct xfs_extent_32,             12);
> >         XFS_CHECK_STRUCT_SIZE(struct xfs_extent_64,             16);
> >         XFS_CHECK_STRUCT_SIZE(struct xfs_log_dinode,            176);
> > @@ -146,6 +146,9 @@ xfs_check_ondisk_structs(void)
> >         XFS_CHECK_OFFSET(struct xfs_bui_log_format,
> > bui_extents,        16);
> >         XFS_CHECK_OFFSET(struct xfs_cui_log_format,
> > cui_extents,        16);
> >         XFS_CHECK_OFFSET(struct xfs_rui_log_format,
> > rui_extents,        16);
> > +       XFS_CHECK_OFFSET(struct xfs_efi_log_format,
> > efi_extents,        16);
> > +       XFS_CHECK_OFFSET(struct xfs_efi_log_format_32,
> > efi_extents,     16);
> > +       XFS_CHECK_OFFSET(struct xfs_efi_log_format_64,
> > efi_extents,     16);
> >  
> >         /*
> >          * The v5 superblock format extended several v4 header
> > structures with
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index f029c6702dda..8485e3b37ca0 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -2029,7 +2029,7 @@ xfs_init_caches(void)
> >  
> >         xfs_efd_cache = kmem_cache_create("xfs_efd_item",
> >                                         (sizeof(struct
> > xfs_efd_log_item) +
> > -                                       (XFS_EFD_MAX_FAST_EXTENTS -
> > 1) *
> > +                                       XFS_EFD_MAX_FAST_EXTENTS *
> >                                         sizeof(struct xfs_extent)),
> >                                         0, 0, NULL);
> >         if (!xfs_efd_cache)
> > @@ -2037,7 +2037,7 @@ xfs_init_caches(void)
> >  
> >         xfs_efi_cache = kmem_cache_create("xfs_efi_item",
> >                                          (sizeof(struct
> > xfs_efi_log_item) +
> > -                                        (XFS_EFI_MAX_FAST_EXTENTS -
> > 1) *
> > +                                        XFS_EFI_MAX_FAST_EXTENTS *
> >                                          sizeof(struct xfs_extent)),
> >                                          0, 0, NULL);
> >         if (!xfs_efi_cache)
> > 
> 

Re: [PATCH 1/6] xfs: fix validation in attr log item recovery

[Dave Chinner]

On Mon, Oct 24, 2022 at 02:32:37PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Before we start fixing all the complaints about memcpy'ing log items
> around, let's fix some inadequate validation in the xattr log item
> recovery code and get rid of the (now trivial) copy_format function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_attr_item.c |   54 ++++++++++++++++++++----------------------------
>  1 file changed, 23 insertions(+), 31 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index cf5ce607dc05..ee8f678a10a1 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -245,28 +245,6 @@ xfs_attri_init(
>  	return attrip;
>  }
>  
> -/*
> - * Copy an attr format buffer from the given buf, and into the destination attr
> - * format structure.
> - */
> -STATIC int
> -xfs_attri_copy_format(
> -	struct xfs_log_iovec		*buf,
> -	struct xfs_attri_log_format	*dst_attr_fmt)
> -{
> -	struct xfs_attri_log_format	*src_attr_fmt = buf->i_addr;
> -	size_t				len;
> -
> -	len = sizeof(struct xfs_attri_log_format);
> -	if (buf->i_len != len) {
> -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> -		return -EFSCORRUPTED;
> -	}
> -
> -	memcpy((char *)dst_attr_fmt, (char *)src_attr_fmt, len);
> -	return 0;
> -}
> -
>  static inline struct xfs_attrd_log_item *ATTRD_ITEM(struct xfs_log_item *lip)
>  {
>  	return container_of(lip, struct xfs_attrd_log_item, attrd_item);
> @@ -731,24 +709,44 @@ xlog_recover_attri_commit_pass2(
>  	struct xfs_attri_log_nameval	*nv;
>  	const void			*attr_value = NULL;
>  	const void			*attr_name;
> -	int                             error;
> +	size_t				len;
>  
>  	attri_formatp = item->ri_buf[0].i_addr;
>  	attr_name = item->ri_buf[1].i_addr;
>  
>  	/* Validate xfs_attri_log_format before the large memory allocation */
> +	len = sizeof(struct xfs_attri_log_format);
> +	if (item->ri_buf[0].i_len != len) {
> +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> +		return -EFSCORRUPTED;
> +	}

I can't help but think these should use XFS_CORRPUPTION_ERROR() so
that we get a dump of the corrupt log format structure along with
error message.

Regardless, the change looks good - validating the name/value region
sizes before we allocate and copy them is a good idea. :)

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/6] xfs: fix memcpy fortify errors in BUI log format copying

[Dave Chinner]

On Mon, Oct 24, 2022 at 02:32:42PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of
> memcpy.  Unfortunately, it doesn't handle flex arrays correctly:
> 
> ------------[ cut here ]------------
> memcpy: detected field-spanning write (size 48) of single field "dst_bui_fmt" at fs/xfs/xfs_bmap_item.c:628 (size 16)
> 
> Fix this by refactoring the xfs_bui_copy_format function to handle the
> copying of the head and the flex array members separately.  While we're
> at it, fix a minor validation deficiency in the recovery function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/6] xfs: fix memcpy fortify errors in CUI log format copying

[Dave Chinner]

On Mon, Oct 24, 2022 at 02:32:48PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of
> memcpy.  Since we're already fixing problems with BUI item copying, we
> should fix it everything else.
> 
> Refactor the xfs_cui_copy_format function to handle the copying of the
> head and the flex array members separately.  While we're at it, fix a
> minor validation deficiency in the recovery function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

LGTM.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/6] xfs: fix memcpy fortify errors in RUI log format copying

[Dave Chinner]

On Mon, Oct 24, 2022 at 02:32:54PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of
> memcpy.  Since we're already fixing problems with BUI item copying, we
> should fix it everything else.
> 
> Refactor the xfs_rui_copy_format function to handle the copying of the
> head and the flex array members separately.  While we're at it, fix a
> minor validation deficiency in the recovery function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_ondisk.h    |    3 ++
>  fs/xfs/xfs_rmap_item.c |   58 ++++++++++++++++++++++--------------------------
>  2 files changed, 30 insertions(+), 31 deletions(-)

Lokks fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 5/6] xfs: fix memcpy fortify errors in EFI log format copying

[Dave Chinner]

On Mon, Oct 24, 2022 at 02:32:59PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of
> memcpy.  Since we're already fixing problems with BUI item copying, we
> should fix it everything else.
> 
> An extra difficulty here is that the ef[id]_extents arrays are declared
> as single-element arrays.  This is not the convention for flex arrays in
> the modern kernel, and it causes all manner of problems with static
> checking tools, since they often cannot tell the difference between a
> single element array and a flex array.
> 
> So for starters, change those array[1] declarations to array[]
> declarations to signal that they are proper flex arrays and adjust all
> the "size-1" expressions to fit the new declaration style.
> 
> Next, refactor the xfs_efi_copy_format function to handle the copying of
> the head and the flex array members separately.  While we're at it, fix
> a minor validation deficiency in the recovery function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Yes, it could be broken up into smaller changes.

Yes, this could all be reworked like Christoph previously proposed.

Yes, there are more cleanups that could be done.

Yes, there are other ways to get to the same point.

No, I don't think it is worth the effort to do it differently.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/6] xfs: fix validation in attr log item recovery

[Darrick J. Wong]

On Wed, Oct 26, 2022 at 08:19:27AM +1100, Dave Chinner wrote:
> On Mon, Oct 24, 2022 at 02:32:37PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Before we start fixing all the complaints about memcpy'ing log items
> > around, let's fix some inadequate validation in the xattr log item
> > recovery code and get rid of the (now trivial) copy_format function.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_attr_item.c |   54 ++++++++++++++++++++----------------------------
> >  1 file changed, 23 insertions(+), 31 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> > index cf5ce607dc05..ee8f678a10a1 100644
> > --- a/fs/xfs/xfs_attr_item.c
> > +++ b/fs/xfs/xfs_attr_item.c
> > @@ -245,28 +245,6 @@ xfs_attri_init(
> >  	return attrip;
> >  }
> >  
> > -/*
> > - * Copy an attr format buffer from the given buf, and into the destination attr
> > - * format structure.
> > - */
> > -STATIC int
> > -xfs_attri_copy_format(
> > -	struct xfs_log_iovec		*buf,
> > -	struct xfs_attri_log_format	*dst_attr_fmt)
> > -{
> > -	struct xfs_attri_log_format	*src_attr_fmt = buf->i_addr;
> > -	size_t				len;
> > -
> > -	len = sizeof(struct xfs_attri_log_format);
> > -	if (buf->i_len != len) {
> > -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> > -		return -EFSCORRUPTED;
> > -	}
> > -
> > -	memcpy((char *)dst_attr_fmt, (char *)src_attr_fmt, len);
> > -	return 0;
> > -}
> > -
> >  static inline struct xfs_attrd_log_item *ATTRD_ITEM(struct xfs_log_item *lip)
> >  {
> >  	return container_of(lip, struct xfs_attrd_log_item, attrd_item);
> > @@ -731,24 +709,44 @@ xlog_recover_attri_commit_pass2(
> >  	struct xfs_attri_log_nameval	*nv;
> >  	const void			*attr_value = NULL;
> >  	const void			*attr_name;
> > -	int                             error;
> > +	size_t				len;
> >  
> >  	attri_formatp = item->ri_buf[0].i_addr;
> >  	attr_name = item->ri_buf[1].i_addr;
> >  
> >  	/* Validate xfs_attri_log_format before the large memory allocation */
> > +	len = sizeof(struct xfs_attri_log_format);
> > +	if (item->ri_buf[0].i_len != len) {
> > +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> > +		return -EFSCORRUPTED;
> > +	}
> 
> I can't help but think these should use XFS_CORRPUPTION_ERROR() so
> that we get a dump of the corrupt log format structure along with
> error message.

That is a good idea.  I will tack a new patch on the end to make that
conversion.

--D

> Regardless, the change looks good - validating the name/value region
> sizes before we allocate and copy them is a good idea. :)
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic

[Dave Chinner]

On Mon, Oct 24, 2022 at 02:33:05PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Refactor all the open-coded sizeof logic for EFI/EFD log item and log
> format structures into common helper functions whose names reflect the
> struct names.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_log_format.h |   48 ++++++++++++++++++++++++++++
>  fs/xfs/xfs_extfree_item.c      |   69 ++++++++++++----------------------------
>  fs/xfs/xfs_extfree_item.h      |   16 +++++++++
>  fs/xfs/xfs_super.c             |   12 ++-----
>  4 files changed, 88 insertions(+), 57 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index 2f41fa8477c9..f13e0809dc63 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -616,6 +616,14 @@ typedef struct xfs_efi_log_format {
>  	xfs_extent_t		efi_extents[];	/* array of extents to free */
>  } xfs_efi_log_format_t;
>  
> +static inline size_t
> +xfs_efi_log_format_sizeof(
> +	unsigned int		nr)
> +{
> +	return sizeof(struct xfs_efi_log_format) +
> +			nr * sizeof(struct xfs_extent);
> +}

FWIW, I'm not really a fan of placing inline code in the on-disk
format definition headers because combining code and type
definitions eventually leads to dependency hell.

I'm going to say it's OK for these functions to be placed here
because they have no external dependencies and are directly related
to the on-disk structures, but I think we need to be careful about
how much code we include into this header as opposed to the type
specific header files (such as fs/xfs/xfs_extfree_item.h)...

> @@ -345,9 +318,8 @@ xfs_trans_get_efd(
>  	ASSERT(nextents > 0);
>  
>  	if (nextents > XFS_EFD_MAX_FAST_EXTENTS) {
> -		efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) +
> -				nextents * sizeof(struct xfs_extent),
> -				0);
> +		efdp = kmem_zalloc(xfs_efd_log_item_sizeof(nextents),
> +				GFP_KERNEL | __GFP_NOFAIL);

That looks like a broken kmem->kmalloc conversion. Did you mean to
convert this to allocation to use kzalloc() at the same time?

Everything else looks ok, so with the above fixed up

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic

[Darrick J. Wong]

On Wed, Oct 26, 2022 at 09:05:43AM +1100, Dave Chinner wrote:
> On Mon, Oct 24, 2022 at 02:33:05PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Refactor all the open-coded sizeof logic for EFI/EFD log item and log
> > format structures into common helper functions whose names reflect the
> > struct names.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_log_format.h |   48 ++++++++++++++++++++++++++++
> >  fs/xfs/xfs_extfree_item.c      |   69 ++++++++++++----------------------------
> >  fs/xfs/xfs_extfree_item.h      |   16 +++++++++
> >  fs/xfs/xfs_super.c             |   12 ++-----
> >  4 files changed, 88 insertions(+), 57 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> > index 2f41fa8477c9..f13e0809dc63 100644
> > --- a/fs/xfs/libxfs/xfs_log_format.h
> > +++ b/fs/xfs/libxfs/xfs_log_format.h
> > @@ -616,6 +616,14 @@ typedef struct xfs_efi_log_format {
> >  	xfs_extent_t		efi_extents[];	/* array of extents to free */
> >  } xfs_efi_log_format_t;
> >  
> > +static inline size_t
> > +xfs_efi_log_format_sizeof(
> > +	unsigned int		nr)
> > +{
> > +	return sizeof(struct xfs_efi_log_format) +
> > +			nr * sizeof(struct xfs_extent);
> > +}
> 
> FWIW, I'm not really a fan of placing inline code in the on-disk
> format definition headers because combining code and type
> definitions eventually leads to dependency hell.
> 
> I'm going to say it's OK for these functions to be placed here
> because they have no external dependencies and are directly related
> to the on-disk structures, but I think we need to be careful about
> how much code we include into this header as opposed to the type
> specific header files (such as fs/xfs/xfs_extfree_item.h)...
> 
> > @@ -345,9 +318,8 @@ xfs_trans_get_efd(
> >  	ASSERT(nextents > 0);
> >  
> >  	if (nextents > XFS_EFD_MAX_FAST_EXTENTS) {
> > -		efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) +
> > -				nextents * sizeof(struct xfs_extent),
> > -				0);
> > +		efdp = kmem_zalloc(xfs_efd_log_item_sizeof(nextents),
> > +				GFP_KERNEL | __GFP_NOFAIL);
> 
> That looks like a broken kmem->kmalloc conversion. Did you mean to
> convert this to allocation to use kzalloc() at the same time?

Oops, yeah.

> Everything else looks ok, so with the above fixed up
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

Thanks!

--D

> 
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic

[Dave Chinner]

On Tue, Oct 25, 2022 at 03:08:26PM -0700, Darrick J. Wong wrote:
> On Wed, Oct 26, 2022 at 09:05:43AM +1100, Dave Chinner wrote:
> > On Mon, Oct 24, 2022 at 02:33:05PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Refactor all the open-coded sizeof logic for EFI/EFD log item and log
> > > format structures into common helper functions whose names reflect the
> > > struct names.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  fs/xfs/libxfs/xfs_log_format.h |   48 ++++++++++++++++++++++++++++
> > >  fs/xfs/xfs_extfree_item.c      |   69 ++++++++++++----------------------------
> > >  fs/xfs/xfs_extfree_item.h      |   16 +++++++++
> > >  fs/xfs/xfs_super.c             |   12 ++-----
> > >  4 files changed, 88 insertions(+), 57 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> > > index 2f41fa8477c9..f13e0809dc63 100644
> > > --- a/fs/xfs/libxfs/xfs_log_format.h
> > > +++ b/fs/xfs/libxfs/xfs_log_format.h
> > > @@ -616,6 +616,14 @@ typedef struct xfs_efi_log_format {
> > >  	xfs_extent_t		efi_extents[];	/* array of extents to free */
> > >  } xfs_efi_log_format_t;
> > >  
> > > +static inline size_t
> > > +xfs_efi_log_format_sizeof(
> > > +	unsigned int		nr)
> > > +{
> > > +	return sizeof(struct xfs_efi_log_format) +
> > > +			nr * sizeof(struct xfs_extent);
> > > +}
> > 
> > FWIW, I'm not really a fan of placing inline code in the on-disk
> > format definition headers because combining code and type
> > definitions eventually leads to dependency hell.
> > 
> > I'm going to say it's OK for these functions to be placed here
> > because they have no external dependencies and are directly related
> > to the on-disk structures, but I think we need to be careful about
> > how much code we include into this header as opposed to the type
> > specific header files (such as fs/xfs/xfs_extfree_item.h)...
> > 
> > > @@ -345,9 +318,8 @@ xfs_trans_get_efd(
> > >  	ASSERT(nextents > 0);
> > >  
> > >  	if (nextents > XFS_EFD_MAX_FAST_EXTENTS) {
> > > -		efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) +
> > > -				nextents * sizeof(struct xfs_extent),
> > > -				0);
> > > +		efdp = kmem_zalloc(xfs_efd_log_item_sizeof(nextents),
> > > +				GFP_KERNEL | __GFP_NOFAIL);
> > 
> > That looks like a broken kmem->kmalloc conversion. Did you mean to
> > convert this to allocation to use kzalloc() at the same time?
> 
> Oops, yeah.

FWIW, I just went back an looked at the efip allocation and you did
the same thing there, I just didn't notice it on the first pass....

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com