* [PATCH 1/6] xfs: fix validation in attr log item recovery
2022-10-24 21:32 [PATCHSET 0/6] xfs: fix various problems with log intent item recovery Darrick J. Wong
@ 2022-10-24 21:32 ` Darrick J. Wong
2022-10-25 18:50 ` Kees Cook
` (2 more replies)
2022-10-24 21:32 ` [PATCH 2/6] xfs: fix memcpy fortify errors in BUI log format copying Darrick J. Wong
` (4 subsequent siblings)
5 siblings, 3 replies; 27+ messages in thread
From: Darrick J. Wong @ 2022-10-24 21:32 UTC (permalink / raw)
To: djwong; +Cc: linux-xfs, david
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;
}
/*
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 1/6] xfs: fix validation in attr log item recovery
2022-10-24 21:32 ` [PATCH 1/6] xfs: fix validation in attr log " Darrick J. Wong
@ 2022-10-25 18:50 ` Kees Cook
2022-10-25 20:42 ` Allison Henderson
2022-10-25 21:19 ` Dave Chinner
2 siblings, 0 replies; 27+ messages in thread
From: Kees Cook @ 2022-10-25 18:50 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, david
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] xfs: fix validation in attr log item recovery
2022-10-24 21:32 ` [PATCH 1/6] xfs: fix validation in attr log " Darrick J. Wong
2022-10-25 18:50 ` Kees Cook
@ 2022-10-25 20:42 ` Allison Henderson
2022-10-25 21:19 ` Dave Chinner
2 siblings, 0 replies; 27+ messages in thread
From: Allison Henderson @ 2022-10-25 20:42 UTC (permalink / raw)
To: djwong; +Cc: david, linux-xfs
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;
> }
>
> /*
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/6] xfs: fix validation in attr log item recovery
2022-10-24 21:32 ` [PATCH 1/6] xfs: fix validation in attr log " Darrick J. Wong
2022-10-25 18:50 ` Kees Cook
2022-10-25 20:42 ` Allison Henderson
@ 2022-10-25 21:19 ` Dave Chinner
2022-10-25 22:05 ` Darrick J. Wong
2 siblings, 1 reply; 27+ messages in thread
From: Dave Chinner @ 2022-10-25 21:19 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/6] xfs: fix validation in attr log item recovery
2022-10-25 21:19 ` Dave Chinner
@ 2022-10-25 22:05 ` Darrick J. Wong
0 siblings, 0 replies; 27+ messages in thread
From: Darrick J. Wong @ 2022-10-25 22:05 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/6] xfs: fix memcpy fortify errors in BUI log format copying
2022-10-24 21:32 [PATCHSET 0/6] xfs: fix various problems with log intent item recovery Darrick J. Wong
2022-10-24 21:32 ` [PATCH 1/6] xfs: fix validation in attr log " Darrick J. Wong
@ 2022-10-24 21:32 ` Darrick J. Wong
2022-10-25 18:52 ` Kees Cook
` (2 more replies)
2022-10-24 21:32 ` [PATCH 3/6] xfs: fix memcpy fortify errors in CUI " Darrick J. Wong
` (3 subsequent siblings)
5 siblings, 3 replies; 27+ messages in thread
From: Darrick J. Wong @ 2022-10-24 21:32 UTC (permalink / raw)
To: djwong; +Cc: linux-xfs, david
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 2/6] xfs: fix memcpy fortify errors in BUI log format copying
2022-10-24 21:32 ` [PATCH 2/6] xfs: fix memcpy fortify errors in BUI log format copying Darrick J. Wong
@ 2022-10-25 18:52 ` Kees Cook
2022-10-25 20:47 ` Allison Henderson
2022-10-25 21:34 ` Dave Chinner
2 siblings, 0 replies; 27+ messages in thread
From: Kees Cook @ 2022-10-25 18:52 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, david
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/6] xfs: fix memcpy fortify errors in BUI log format copying
2022-10-24 21:32 ` [PATCH 2/6] xfs: fix memcpy fortify errors in BUI log format copying Darrick J. Wong
2022-10-25 18:52 ` Kees Cook
@ 2022-10-25 20:47 ` Allison Henderson
2022-10-25 21:34 ` Dave Chinner
2 siblings, 0 replies; 27+ messages in thread
From: Allison Henderson @ 2022-10-25 20:47 UTC (permalink / raw)
To: djwong; +Cc: david, linux-xfs
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
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/6] xfs: fix memcpy fortify errors in BUI log format copying
2022-10-24 21:32 ` [PATCH 2/6] xfs: fix memcpy fortify errors in BUI log format copying Darrick J. Wong
2022-10-25 18:52 ` Kees Cook
2022-10-25 20:47 ` Allison Henderson
@ 2022-10-25 21:34 ` Dave Chinner
2 siblings, 0 replies; 27+ messages in thread
From: Dave Chinner @ 2022-10-25 21:34 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/6] xfs: fix memcpy fortify errors in CUI log format copying
2022-10-24 21:32 [PATCHSET 0/6] xfs: fix various problems with log intent item recovery Darrick J. Wong
2022-10-24 21:32 ` [PATCH 1/6] xfs: fix validation in attr log " Darrick J. Wong
2022-10-24 21:32 ` [PATCH 2/6] xfs: fix memcpy fortify errors in BUI log format copying Darrick J. Wong
@ 2022-10-24 21:32 ` Darrick J. Wong
2022-10-25 20:47 ` Allison Henderson
2022-10-25 21:36 ` Dave Chinner
2022-10-24 21:32 ` [PATCH 4/6] xfs: fix memcpy fortify errors in RUI " Darrick J. Wong
` (2 subsequent siblings)
5 siblings, 2 replies; 27+ messages in thread
From: Darrick J. Wong @ 2022-10-24 21:32 UTC (permalink / raw)
To: djwong; +Cc: linux-xfs, david
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 3/6] xfs: fix memcpy fortify errors in CUI log format copying
2022-10-24 21:32 ` [PATCH 3/6] xfs: fix memcpy fortify errors in CUI " Darrick J. Wong
@ 2022-10-25 20:47 ` Allison Henderson
2022-10-25 21:36 ` Dave Chinner
1 sibling, 0 replies; 27+ messages in thread
From: Allison Henderson @ 2022-10-25 20:47 UTC (permalink / raw)
To: djwong; +Cc: david, linux-xfs
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
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 3/6] xfs: fix memcpy fortify errors in CUI log format copying
2022-10-24 21:32 ` [PATCH 3/6] xfs: fix memcpy fortify errors in CUI " Darrick J. Wong
2022-10-25 20:47 ` Allison Henderson
@ 2022-10-25 21:36 ` Dave Chinner
1 sibling, 0 replies; 27+ messages in thread
From: Dave Chinner @ 2022-10-25 21:36 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 4/6] xfs: fix memcpy fortify errors in RUI log format copying
2022-10-24 21:32 [PATCHSET 0/6] xfs: fix various problems with log intent item recovery Darrick J. Wong
` (2 preceding siblings ...)
2022-10-24 21:32 ` [PATCH 3/6] xfs: fix memcpy fortify errors in CUI " Darrick J. Wong
@ 2022-10-24 21:32 ` Darrick J. Wong
2022-10-25 20:49 ` Allison Henderson
2022-10-25 21:37 ` Dave Chinner
2022-10-24 21:32 ` [PATCH 5/6] xfs: fix memcpy fortify errors in EFI " Darrick J. Wong
2022-10-24 21:33 ` [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic Darrick J. Wong
5 siblings, 2 replies; 27+ messages in thread
From: Darrick J. Wong @ 2022-10-24 21:32 UTC (permalink / raw)
To: djwong; +Cc: linux-xfs, david
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 4/6] xfs: fix memcpy fortify errors in RUI log format copying
2022-10-24 21:32 ` [PATCH 4/6] xfs: fix memcpy fortify errors in RUI " Darrick J. Wong
@ 2022-10-25 20:49 ` Allison Henderson
2022-10-25 21:37 ` Dave Chinner
1 sibling, 0 replies; 27+ messages in thread
From: Allison Henderson @ 2022-10-25 20:49 UTC (permalink / raw)
To: djwong; +Cc: david, linux-xfs
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
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 4/6] xfs: fix memcpy fortify errors in RUI log format copying
2022-10-24 21:32 ` [PATCH 4/6] xfs: fix memcpy fortify errors in RUI " Darrick J. Wong
2022-10-25 20:49 ` Allison Henderson
@ 2022-10-25 21:37 ` Dave Chinner
1 sibling, 0 replies; 27+ messages in thread
From: Dave Chinner @ 2022-10-25 21:37 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 5/6] xfs: fix memcpy fortify errors in EFI log format copying
2022-10-24 21:32 [PATCHSET 0/6] xfs: fix various problems with log intent item recovery Darrick J. Wong
` (3 preceding siblings ...)
2022-10-24 21:32 ` [PATCH 4/6] xfs: fix memcpy fortify errors in RUI " Darrick J. Wong
@ 2022-10-24 21:32 ` Darrick J. Wong
2022-10-25 19:08 ` Kees Cook
` (2 more replies)
2022-10-24 21:33 ` [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic Darrick J. Wong
5 siblings, 3 replies; 27+ messages in thread
From: Darrick J. Wong @ 2022-10-24 21:32 UTC (permalink / raw)
To: djwong; +Cc: linux-xfs, david
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)
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 5/6] xfs: fix memcpy fortify errors in EFI log format copying
2022-10-24 21:32 ` [PATCH 5/6] xfs: fix memcpy fortify errors in EFI " Darrick J. Wong
@ 2022-10-25 19:08 ` Kees Cook
2022-10-25 20:54 ` Allison Henderson
2022-10-25 21:47 ` Dave Chinner
2 siblings, 0 replies; 27+ messages in thread
From: Kees Cook @ 2022-10-25 19:08 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, david
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 5/6] xfs: fix memcpy fortify errors in EFI log format copying
2022-10-24 21:32 ` [PATCH 5/6] xfs: fix memcpy fortify errors in EFI " Darrick J. Wong
2022-10-25 19:08 ` Kees Cook
@ 2022-10-25 20:54 ` Allison Henderson
2022-10-25 21:17 ` Darrick J. Wong
2022-10-25 21:47 ` Dave Chinner
2 siblings, 1 reply; 27+ messages in thread
From: Allison Henderson @ 2022-10-25 20:54 UTC (permalink / raw)
To: djwong; +Cc: david, linux-xfs
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)
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 5/6] xfs: fix memcpy fortify errors in EFI log format copying
2022-10-25 20:54 ` Allison Henderson
@ 2022-10-25 21:17 ` Darrick J. Wong
0 siblings, 0 replies; 27+ messages in thread
From: Darrick J. Wong @ 2022-10-25 21:17 UTC (permalink / raw)
To: Allison Henderson; +Cc: david, linux-xfs
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)
> >
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 5/6] xfs: fix memcpy fortify errors in EFI log format copying
2022-10-24 21:32 ` [PATCH 5/6] xfs: fix memcpy fortify errors in EFI " Darrick J. Wong
2022-10-25 19:08 ` Kees Cook
2022-10-25 20:54 ` Allison Henderson
@ 2022-10-25 21:47 ` Dave Chinner
2 siblings, 0 replies; 27+ messages in thread
From: Dave Chinner @ 2022-10-25 21:47 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic
2022-10-24 21:32 [PATCHSET 0/6] xfs: fix various problems with log intent item recovery Darrick J. Wong
` (4 preceding siblings ...)
2022-10-24 21:32 ` [PATCH 5/6] xfs: fix memcpy fortify errors in EFI " Darrick J. Wong
@ 2022-10-24 21:33 ` Darrick J. Wong
2022-10-25 19:14 ` Kees Cook
` (2 more replies)
5 siblings, 3 replies; 27+ messages in thread
From: Darrick J. Wong @ 2022-10-24 21:33 UTC (permalink / raw)
To: djwong; +Cc: linux-xfs, david
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;
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic
2022-10-24 21:33 ` [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic Darrick J. Wong
@ 2022-10-25 19:14 ` Kees Cook
2022-10-25 20:56 ` Allison Henderson
2022-10-25 22:05 ` Dave Chinner
2 siblings, 0 replies; 27+ messages in thread
From: Kees Cook @ 2022-10-25 19:14 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, david
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic
2022-10-24 21:33 ` [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic Darrick J. Wong
2022-10-25 19:14 ` Kees Cook
@ 2022-10-25 20:56 ` Allison Henderson
2022-10-25 22:05 ` Dave Chinner
2 siblings, 0 replies; 27+ messages in thread
From: Allison Henderson @ 2022-10-25 20:56 UTC (permalink / raw)
To: djwong; +Cc: david, linux-xfs
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;
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic
2022-10-24 21:33 ` [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic Darrick J. Wong
2022-10-25 19:14 ` Kees Cook
2022-10-25 20:56 ` Allison Henderson
@ 2022-10-25 22:05 ` Dave Chinner
2022-10-25 22:08 ` Darrick J. Wong
2 siblings, 1 reply; 27+ messages in thread
From: Dave Chinner @ 2022-10-25 22:05 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic
2022-10-25 22:05 ` Dave Chinner
@ 2022-10-25 22:08 ` Darrick J. Wong
2022-10-25 22:22 ` Dave Chinner
0 siblings, 1 reply; 27+ messages in thread
From: Darrick J. Wong @ 2022-10-25 22:08 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic
2022-10-25 22:08 ` Darrick J. Wong
@ 2022-10-25 22:22 ` Dave Chinner
0 siblings, 0 replies; 27+ messages in thread
From: Dave Chinner @ 2022-10-25 22:22 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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
^ permalink raw reply [flat|nested] 27+ messages in thread