* [PATCH][next] xfs: Replace one-element arrays with flexible-array members
@ 2021-03-02 15:05 Gustavo A. R. Silva
2021-03-09 17:42 ` Darrick J. Wong
0 siblings, 1 reply; 11+ messages in thread
From: Gustavo A. R. Silva @ 2021-03-02 15:05 UTC (permalink / raw)
To: Darrick J. Wong, Dave Chinner
Cc: linux-xfs, linux-kernel, Gustavo A. R. Silva, linux-hardening
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Refactor the code according to the use of flexible-array members in
multiple structures, instead of one-element arrays. Also, make use of
the new struct_size() helper to properly calculate the size of multiple
structures that contain flexible-array members.
Below are the results of running xfstests for groups shutdown and log
with the following configuration in local.config:
export TEST_DEV=/dev/sda3
export TEST_DIR=/mnt/test
export SCRATCH_DEV=/dev/sda4
export SCRATCH_MNT=/mnt/scratch
The size for both partitions /dev/sda3 and /dev/sda4 is 25GB.
These are the results of running ./check -g shutdown and ./check -g
log, respectively, on 5.11.0-next-20210301 kernel _without_ this patch
applied:
gustavo@beefy:~/git/xfstests$ time sudo ./check -g shutdown 2>&1 | tee xfs-shutdown-no-patch.out
FSTYP -- xfs (debug)
PLATFORM -- Linux/x86_64 beefy 5.11.0-next-20210301 #4 SMP Mon Mar 1 05:42:06 CST 2021
MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
MOUNT_OPTIONS -- /dev/sda4 /mnt/scratch
generic/042 2s ... 2s
generic/043 13s ... 13s
generic/044 14s ... 14s
generic/045 18s ... 18s
generic/046 14s ... 14s
generic/047 20s ... 22s
generic/048 51s ... 50s
generic/049 9s ... 8s
generic/050 2s ... 1s
generic/051 76s ... 75s
generic/052 1s ... 2s
generic/054 31s ... 30s
generic/055 16s ... 21s
generic/388 89s ... 75s
generic/392 5s ... 5s
generic/417 11s ... 11s
generic/461 21s ... 22s
generic/468 3s ... 3s
generic/474 1s ... 1s
generic/475 86s ... 93s
generic/505 2s ... 2s
generic/506 2s ... 2s
generic/507 [not run] file system doesn't support chattr +AsSu
generic/508 1s ... 1s
generic/530 10s ... 10s
generic/536 3s ... 2s
generic/599 1s ... 1s
generic/622 23s ... 24s
xfs/051 18s ... 18s
xfs/079 15s ... 14s
xfs/121 6s ... 16s
xfs/181 12s ... 15s
xfs/212 1s ... 3s
Ran: generic/042 generic/043 generic/044 generic/045 generic/046 generic/047 generic/048 generic/049 generic/050 generic/051 generic/052 generic/054 generic/055 generic/388 generic/392 generic/417 generic/461 generic/468 generic/474 generic/475 generic/505 generic/506 generic/507 generic/508 generic/530 generic/536 generic/599 generic/622 xfs/051 xfs/079 xfs/121 xfs/181 xfs/212
Not run: generic/507
Passed all 33 tests
gustavo@beefy:~/git/xfstests$ time sudo ./check -g log 2>&1 | tee xfs-log-no-patch.out
FSTYP -- xfs (debug)
PLATFORM -- Linux/x86_64 beefy 5.11.0-next-20210301 #4 SMP Mon Mar 1 05:42:06 CST 2021
MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
MOUNT_OPTIONS -- /dev/sda4 /mnt/scratch
generic/034 1s ... 1s
generic/039 1s ... 1s
generic/040 5s ... 5s
generic/041 7s ... 7s
generic/043 13s ... 13s
generic/044 14s ... 14s
generic/045 18s ... 18s
generic/046 14s ... 14s
generic/051 76s ... 76s
generic/052 1s ... 2s
generic/054 30s ... 48s
generic/055 23s ... 15s
generic/056 1s ... 1s
generic/057 1s ... 1s
generic/059 2s ... 2s
generic/065 1s ... 1s
generic/066 1s ... 2s
generic/073 1s ... 1s
generic/090 1s ... 1s
generic/101 1s ... 1s
generic/104 1s ... 1s
generic/106 1s ... 1s
generic/107 1s ... 1s
generic/177 1s ... 1s
generic/311 67s ... 67s
generic/321 2s ... 2s
generic/322 1s ... 2s
generic/325 1s ... 1s
generic/335 1s ... 1s
generic/336 2s ... 1s
generic/341 1s ... 1s
generic/342 1s ... 1s
generic/343 1s ... 1s
generic/376 1s ... 1s
generic/388 75s ... 97s
generic/417 15s ... 12s
generic/455 [not run] This test requires a valid $LOGWRITES_DEV
generic/457 [not run] This test requires a valid $LOGWRITES_DEV
generic/475 88s ... 89s
generic/479 2s ... 2s
generic/480 1s ... 1s
generic/481 1s ... 1s
generic/483 7s ... 6s
generic/489 1s ... 1s
generic/498 1s ... 1s
generic/501 61s ... 63s
generic/502 1s ... 1s
generic/509 1s ... 1s
generic/510 1s ... 1s
generic/512 1s ... 1s
generic/520 16s ... 17s
generic/526 2s ... 1s
generic/527 0s ... 1s
generic/534 0s ... 1s
generic/535 2s ... 1s
generic/546 3s ... 3s
generic/547 2s ... 2s
generic/552 1s ... 1s
generic/557 1s ... 2s
generic/588 2s ... 1s
shared/002 9s ... 9s
xfs/011 18s ... 18s
xfs/029 1s ... 1s
xfs/051 18s ... 18s
xfs/057 23s ... 23s
xfs/079 14s ... 14s
xfs/095 1s ... 1s
xfs/119 3s ... 3s
xfs/121 6s ... 6s
xfs/141 14s ... 26s
xfs/181 14s ... 14s
xfs/216 3s ... 2s
xfs/217 2s ... 2s
xfs/439 [not run] test requires XFS bug_on_assert to be off, turn it off to run the test
Ran: generic/034 generic/039 generic/040 generic/041 generic/043 generic/044 generic/045 generic/046 generic/051 generic/052 generic/054 generic/055 generic/056 generic/057 generic/059 generic/065 generic/066 generic/073 generic/090 generic/101 generic/104 generic/106 generic/107 generic/177 generic/311 generic/321 generic/322 generic/325 generic/335 generic/336 generic/341 generic/342 generic/343 generic/376 generic/388 generic/417 generic/455 generic/457 generic/475 generic/479 generic/480 generic/481 generic/483 generic/489 generic/498 generic/501 generic/502 generic/509 generic/510 generic/512 generic/520 generic/526 generic/527 generic/534 generic/535 generic/546 generic/547 generic/552 generic/557 generic/588 shared/002 xfs/011 xfs/029 xfs/051 xfs/057 xfs/079 xfs/095 xfs/119 xfs/121 xfs/141 xfs/181 xfs/216 xfs/217 xfs/439
Not run: generic/455 generic/457 xfs/439
Passed all 74 tests
These are the results of running ./check -g shutdown and ./check -g
log, respectively, on 5.11.0-xfstests-patched-next-20210301+ kernel
_with_ this patch applied on top:
gustavo@beefy:~/git/xfstests$ time sudo ./check -g shutdown 2>&1 | tee xfs-shutdown-patched.out
FSTYP -- xfs (debug)
PLATFORM -- Linux/x86_64 beefy 5.11.0-xfstests-patched-next-20210301+ #10 SMP Mon Mar 1 17:36:20 CST 2021
MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
MOUNT_OPTIONS -- /dev/sda4 /mnt/scratch
generic/042 2s ... 3s
generic/043 13s ... 12s
generic/044 14s ... 15s
generic/045 18s ... 18s
generic/046 14s ... 15s
generic/047 21s ... 21s
generic/048 51s ... 51s
generic/049 9s ... 8s
generic/050 1s ... 2s
generic/051 76s ... 77s
generic/052 6s ... 2s
generic/054 35s ... 48s
generic/055 16s ... 17s
generic/388 77s ... 73s
generic/392 7s ... 5s
generic/417 13s ... 11s
generic/461 22s ... 21s
generic/468 2s ... 3s
generic/474 2s ... 1s
generic/475 90s ... 96s
generic/505 3s ... 3s
generic/506 1s ... 2s
generic/507 [not run] file system doesn't support chattr +AsSu
generic/508 2s ... 10s
generic/530 9s ... 13s
generic/536 2s ... 1s
generic/599 1s ... 1s
generic/622 24s ... 25s
xfs/051 18s ... 18s
xfs/079 14s ... 13s
xfs/121 6s ... 8s
xfs/181 11s ... 14s
xfs/212 2s ... 1s
Ran: generic/042 generic/043 generic/044 generic/045 generic/046 generic/047 generic/048 generic/049 generic/050 generic/051 generic/052 generic/054 generic/055 generic/388 generic/392 generic/417 generic/461 generic/468 generic/474 generic/475 generic/505 generic/506 generic/507 generic/508 generic/530 generic/536 generic/599 generic/622 xfs/051 xfs/079 xfs/121 xfs/181 xfs/212
Not run: generic/507
Passed all 33 tests
gustavo@beefy:~/git/xfstests$ time sudo ./check -g log 2>&1 | tee xfs-log-patched.out
FSTYP -- xfs (debug)
PLATFORM -- Linux/x86_64 beefy 5.11.0-xfstests-patched-next-20210301+ #10 SMP Mon Mar 1 17:36:20 CST 2021
MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
MOUNT_OPTIONS -- /dev/sda4 /mnt/scratch
generic/034 1s ... 1s
generic/039 1s ... 1s
generic/040 5s ... 5s
generic/041 7s ... 7s
generic/043 12s ... 13s
generic/044 15s ... 15s
generic/045 18s ... 18s
generic/046 15s ... 14s
generic/051 77s ... 76s
generic/052 2s ... 1s
generic/054 48s ... 29s
generic/055 17s ... 16s
generic/056 1s ... 1s
generic/057 1s ... 1s
generic/059 2s ... 2s
generic/065 1s ... 1s
generic/066 1s ... 1s
generic/073 1s ... 1s
generic/090 1s ... 1s
generic/101 1s ... 1s
generic/104 1s ... 1s
generic/106 1s ... 1s
generic/107 1s ... 1s
generic/177 1s ... 1s
generic/311 68s ... 68s
generic/321 2s ... 2s
generic/322 1s ... 1s
generic/325 2s ... 1s
generic/335 1s ... 1s
generic/336 1s ... 1s
generic/341 1s ... 1s
generic/342 1s ... 1s
generic/343 1s ... 1s
generic/376 1s ... 1s
generic/388 73s ... 83s
generic/417 11s ... 11s
generic/455 [not run] This test requires a valid $LOGWRITES_DEV
generic/457 [not run] This test requires a valid $LOGWRITES_DEV
generic/475 96s ... 83s
generic/479 2s
generic/480 2s ... 1s
generic/481 0s ... 1s
generic/483 7s ... 7s
generic/489 1s ... 1s
generic/498 1s ... 1s
generic/501 63s ... 61s
generic/502 1s ... 1s
generic/509 1s ... 1s
generic/510 1s ... 1s
generic/512 1s ... 1s
generic/520 16s ... 16s
generic/526 1s ... 1s
generic/527 1s ... 1s
generic/534 1s ... 1s
generic/535 1s ... 1s
generic/546 4s ... 4s
generic/547 2s ... 2s
generic/552 1s ... 1s
generic/557 1s ... 1s
generic/588 2s ... 1s
shared/002 8s ... 9s
xfs/011 18s ... 18s
xfs/029 1s ... 1s
xfs/051 18s ... 18s
xfs/057 23s ... 24s
xfs/079 13s ... 14s
xfs/095 2s ... 1s
xfs/119 3s ... 3s
xfs/121 8s ... 6s
xfs/141 18s ... 12s
xfs/181 14s ... 12s
xfs/216 3s ... 2s
xfs/217 2s ... 2s
xfs/439 [not run] test requires XFS bug_on_assert to be off, turn it off to run the test
Ran: generic/034 generic/039 generic/040 generic/041 generic/043 generic/044 generic/045 generic/046 generic/051 generic/052 generic/054 generic/055 generic/056 generic/057 generic/059 generic/065 generic/066 generic/073 generic/090 generic/101 generic/104 generic/106 generic/107 generic/177 generic/311 generic/321 generic/322 generic/325 generic/335 generic/336 generic/341 generic/342 generic/343 generic/376 generic/388 generic/417 generic/455 generic/457 generic/475 generic/479 generic/480 generic/481 generic/483 generic/489 generic/498 generic/501 generic/502 generic/509 generic/510 generic/512 generic/520 generic/526 generic/527 generic/534 generic/535 generic/546 generic/547 generic/552 generic/557 generic/588 shared/002 xfs/011 xfs/029 xfs/051 xfs/057 xfs/079 xfs/095 xfs/119 xfs/121 xfs/141 xfs/181 xfs/216 xfs/217 xfs/439
Not run: generic/455 generic/457 xfs/439
Passed all 74 tests
Notice that the test results before and after this patch is applied are
identical and successful. Other tests might need to be run in order to
verify everything is working as expected. For such tests, the intervention
of the maintainers might be needed.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays
Link: https://github.com/KSPP/linux/issues/79
Build-tested-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/lkml/603e3177.WsvTXRpcLn5wdYW6%25lkp@intel.com/
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
fs/xfs/libxfs/xfs_log_format.h | 12 +++++------
fs/xfs/xfs_extfree_item.c | 39 +++++++++++++++-------------------
fs/xfs/xfs_ondisk.h | 8 +++----
3 files changed, 27 insertions(+), 32 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
index 8bd00da6d2a4..9934a465b441 100644
--- a/fs/xfs/libxfs/xfs_log_format.h
+++ b/fs/xfs/libxfs/xfs_log_format.h
@@ -574,7 +574,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 {
@@ -582,7 +582,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 {
@@ -590,7 +590,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;
/*
@@ -603,7 +603,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 {
@@ -611,7 +611,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 {
@@ -619,7 +619,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 93223ebb3372..5ed3ea93071c 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -73,8 +73,8 @@ 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 - 1) * sizeof(xfs_extent_t);
+ return struct_size(&efip->efi_format, efi_extents,
+ efip->efi_format.efi_nextents);
}
STATIC void
@@ -153,17 +153,14 @@ 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 - 1) * sizeof(xfs_extent_t)));
- efip = kmem_zalloc(size, 0);
- } else {
+ if (nextents > XFS_EFI_MAX_FAST_EXTENTS)
+ efip = kmem_zalloc(struct_size(efip, efi_format.efi_extents,
+ nextents), 0);
+ else
efip = kmem_cache_zalloc(xfs_efi_zone,
GFP_KERNEL | __GFP_NOFAIL);
- }
xfs_log_item_init(mp, &efip->efi_item, XFS_LI_EFI, &xfs_efi_item_ops);
efip->efi_format.efi_nextents = nextents;
@@ -186,12 +183,12 @@ 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);
+ size_t len = struct_size(src_efi_fmt, efi_extents,
+ src_efi_fmt->efi_nextents);
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);
@@ -254,7 +251,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
@@ -328,14 +325,12 @@ xfs_trans_get_efd(
ASSERT(nextents > 0);
- if (nextents > XFS_EFD_MAX_FAST_EXTENTS) {
- efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) +
- (nextents - 1) * sizeof(struct xfs_extent),
- 0);
- } else {
+ if (nextents > XFS_EFD_MAX_FAST_EXTENTS)
+ efdp = kmem_zalloc(struct_size(efip, efi_format.efi_extents,
+ nextents), 0);
+ else
efdp = kmem_cache_zalloc(xfs_efd_zone,
GFP_KERNEL | __GFP_NOFAIL);
- }
xfs_log_item_init(tp->t_mountp, &efdp->efd_item, XFS_LI_EFD,
&xfs_efd_item_ops);
@@ -747,9 +742,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 0aa87c210104..f58e0510385a 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);
--
2.27.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH][next] xfs: Replace one-element arrays with flexible-array members
2021-03-02 15:05 [PATCH][next] xfs: Replace one-element arrays with flexible-array members Gustavo A. R. Silva
@ 2021-03-09 17:42 ` Darrick J. Wong
2021-03-09 19:57 ` Gustavo A. R. Silva
0 siblings, 1 reply; 11+ messages in thread
From: Darrick J. Wong @ 2021-03-09 17:42 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Dave Chinner, linux-xfs, linux-kernel, linux-hardening
On Tue, Mar 02, 2021 at 09:05:58AM -0600, Gustavo A. R. Silva wrote:
> There is a regular need in the kernel to provide a way to declare having
> a dynamically sized set of trailing elements in a structure. Kernel code
> should always use “flexible array members”[1] for these cases. The older
> style of one-element or zero-length arrays should no longer be used[2].
>
> Refactor the code according to the use of flexible-array members in
> multiple structures, instead of one-element arrays. Also, make use of
> the new struct_size() helper to properly calculate the size of multiple
> structures that contain flexible-array members.
>
> Below are the results of running xfstests for groups shutdown and log
> with the following configuration in local.config:
>
> export TEST_DEV=/dev/sda3
> export TEST_DIR=/mnt/test
> export SCRATCH_DEV=/dev/sda4
> export SCRATCH_MNT=/mnt/scratch
>
> The size for both partitions /dev/sda3 and /dev/sda4 is 25GB.
>
> These are the results of running ./check -g shutdown and ./check -g
> log, respectively, on 5.11.0-next-20210301 kernel _without_ this patch
> applied:
>
> gustavo@beefy:~/git/xfstests$ time sudo ./check -g shutdown 2>&1 | tee xfs-shutdown-no-patch.out
> FSTYP -- xfs (debug)
> PLATFORM -- Linux/x86_64 beefy 5.11.0-next-20210301 #4 SMP Mon Mar 1 05:42:06 CST 2021
> MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
> MOUNT_OPTIONS -- /dev/sda4 /mnt/scratch
>
> generic/042 2s ... 2s
> generic/043 13s ... 13s
> generic/044 14s ... 14s
> generic/045 18s ... 18s
> generic/046 14s ... 14s
> generic/047 20s ... 22s
> generic/048 51s ... 50s
> generic/049 9s ... 8s
> generic/050 2s ... 1s
> generic/051 76s ... 75s
> generic/052 1s ... 2s
> generic/054 31s ... 30s
> generic/055 16s ... 21s
> generic/388 89s ... 75s
> generic/392 5s ... 5s
> generic/417 11s ... 11s
> generic/461 21s ... 22s
> generic/468 3s ... 3s
> generic/474 1s ... 1s
> generic/475 86s ... 93s
> generic/505 2s ... 2s
> generic/506 2s ... 2s
> generic/507 [not run] file system doesn't support chattr +AsSu
> generic/508 1s ... 1s
> generic/530 10s ... 10s
> generic/536 3s ... 2s
> generic/599 1s ... 1s
> generic/622 23s ... 24s
> xfs/051 18s ... 18s
> xfs/079 15s ... 14s
> xfs/121 6s ... 16s
> xfs/181 12s ... 15s
> xfs/212 1s ... 3s
> Ran: generic/042 generic/043 generic/044 generic/045 generic/046 generic/047 generic/048 generic/049 generic/050 generic/051 generic/052 generic/054 generic/055 generic/388 generic/392 generic/417 generic/461 generic/468 generic/474 generic/475 generic/505 generic/506 generic/507 generic/508 generic/530 generic/536 generic/599 generic/622 xfs/051 xfs/079 xfs/121 xfs/181 xfs/212
> Not run: generic/507
> Passed all 33 tests
>
> gustavo@beefy:~/git/xfstests$ time sudo ./check -g log 2>&1 | tee xfs-log-no-patch.out
> FSTYP -- xfs (debug)
> PLATFORM -- Linux/x86_64 beefy 5.11.0-next-20210301 #4 SMP Mon Mar 1 05:42:06 CST 2021
> MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
> MOUNT_OPTIONS -- /dev/sda4 /mnt/scratch
>
> generic/034 1s ... 1s
> generic/039 1s ... 1s
> generic/040 5s ... 5s
> generic/041 7s ... 7s
> generic/043 13s ... 13s
> generic/044 14s ... 14s
> generic/045 18s ... 18s
> generic/046 14s ... 14s
> generic/051 76s ... 76s
> generic/052 1s ... 2s
> generic/054 30s ... 48s
> generic/055 23s ... 15s
> generic/056 1s ... 1s
> generic/057 1s ... 1s
> generic/059 2s ... 2s
> generic/065 1s ... 1s
> generic/066 1s ... 2s
> generic/073 1s ... 1s
> generic/090 1s ... 1s
> generic/101 1s ... 1s
> generic/104 1s ... 1s
> generic/106 1s ... 1s
> generic/107 1s ... 1s
> generic/177 1s ... 1s
> generic/311 67s ... 67s
> generic/321 2s ... 2s
> generic/322 1s ... 2s
> generic/325 1s ... 1s
> generic/335 1s ... 1s
> generic/336 2s ... 1s
> generic/341 1s ... 1s
> generic/342 1s ... 1s
> generic/343 1s ... 1s
> generic/376 1s ... 1s
> generic/388 75s ... 97s
> generic/417 15s ... 12s
> generic/455 [not run] This test requires a valid $LOGWRITES_DEV
> generic/457 [not run] This test requires a valid $LOGWRITES_DEV
> generic/475 88s ... 89s
> generic/479 2s ... 2s
> generic/480 1s ... 1s
> generic/481 1s ... 1s
> generic/483 7s ... 6s
> generic/489 1s ... 1s
> generic/498 1s ... 1s
> generic/501 61s ... 63s
> generic/502 1s ... 1s
> generic/509 1s ... 1s
> generic/510 1s ... 1s
> generic/512 1s ... 1s
> generic/520 16s ... 17s
> generic/526 2s ... 1s
> generic/527 0s ... 1s
> generic/534 0s ... 1s
> generic/535 2s ... 1s
> generic/546 3s ... 3s
> generic/547 2s ... 2s
> generic/552 1s ... 1s
> generic/557 1s ... 2s
> generic/588 2s ... 1s
> shared/002 9s ... 9s
> xfs/011 18s ... 18s
> xfs/029 1s ... 1s
> xfs/051 18s ... 18s
> xfs/057 23s ... 23s
> xfs/079 14s ... 14s
> xfs/095 1s ... 1s
> xfs/119 3s ... 3s
> xfs/121 6s ... 6s
> xfs/141 14s ... 26s
> xfs/181 14s ... 14s
> xfs/216 3s ... 2s
> xfs/217 2s ... 2s
> xfs/439 [not run] test requires XFS bug_on_assert to be off, turn it off to run the test
> Ran: generic/034 generic/039 generic/040 generic/041 generic/043 generic/044 generic/045 generic/046 generic/051 generic/052 generic/054 generic/055 generic/056 generic/057 generic/059 generic/065 generic/066 generic/073 generic/090 generic/101 generic/104 generic/106 generic/107 generic/177 generic/311 generic/321 generic/322 generic/325 generic/335 generic/336 generic/341 generic/342 generic/343 generic/376 generic/388 generic/417 generic/455 generic/457 generic/475 generic/479 generic/480 generic/481 generic/483 generic/489 generic/498 generic/501 generic/502 generic/509 generic/510 generic/512 generic/520 generic/526 generic/527 generic/534 generic/535 generic/546 generic/547 generic/552 generic/557 generic/588 shared/002 xfs/011 xfs/029 xfs/051 xfs/057 xfs/079 xfs/095 xfs/119 xfs/121 xfs/141 xfs/181 xfs/216 xfs/217 xfs/439
> Not run: generic/455 generic/457 xfs/439
> Passed all 74 tests
>
> These are the results of running ./check -g shutdown and ./check -g
> log, respectively, on 5.11.0-xfstests-patched-next-20210301+ kernel
> _with_ this patch applied on top:
>
> gustavo@beefy:~/git/xfstests$ time sudo ./check -g shutdown 2>&1 | tee xfs-shutdown-patched.out
> FSTYP -- xfs (debug)
> PLATFORM -- Linux/x86_64 beefy 5.11.0-xfstests-patched-next-20210301+ #10 SMP Mon Mar 1 17:36:20 CST 2021
> MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
> MOUNT_OPTIONS -- /dev/sda4 /mnt/scratch
>
> generic/042 2s ... 3s
> generic/043 13s ... 12s
> generic/044 14s ... 15s
> generic/045 18s ... 18s
> generic/046 14s ... 15s
> generic/047 21s ... 21s
> generic/048 51s ... 51s
> generic/049 9s ... 8s
> generic/050 1s ... 2s
> generic/051 76s ... 77s
> generic/052 6s ... 2s
> generic/054 35s ... 48s
> generic/055 16s ... 17s
> generic/388 77s ... 73s
> generic/392 7s ... 5s
> generic/417 13s ... 11s
> generic/461 22s ... 21s
> generic/468 2s ... 3s
> generic/474 2s ... 1s
> generic/475 90s ... 96s
> generic/505 3s ... 3s
> generic/506 1s ... 2s
> generic/507 [not run] file system doesn't support chattr +AsSu
> generic/508 2s ... 10s
> generic/530 9s ... 13s
> generic/536 2s ... 1s
> generic/599 1s ... 1s
> generic/622 24s ... 25s
> xfs/051 18s ... 18s
> xfs/079 14s ... 13s
> xfs/121 6s ... 8s
> xfs/181 11s ... 14s
> xfs/212 2s ... 1s
> Ran: generic/042 generic/043 generic/044 generic/045 generic/046 generic/047 generic/048 generic/049 generic/050 generic/051 generic/052 generic/054 generic/055 generic/388 generic/392 generic/417 generic/461 generic/468 generic/474 generic/475 generic/505 generic/506 generic/507 generic/508 generic/530 generic/536 generic/599 generic/622 xfs/051 xfs/079 xfs/121 xfs/181 xfs/212
> Not run: generic/507
> Passed all 33 tests
>
> gustavo@beefy:~/git/xfstests$ time sudo ./check -g log 2>&1 | tee xfs-log-patched.out
> FSTYP -- xfs (debug)
> PLATFORM -- Linux/x86_64 beefy 5.11.0-xfstests-patched-next-20210301+ #10 SMP Mon Mar 1 17:36:20 CST 2021
> MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
> MOUNT_OPTIONS -- /dev/sda4 /mnt/scratch
>
> generic/034 1s ... 1s
> generic/039 1s ... 1s
> generic/040 5s ... 5s
> generic/041 7s ... 7s
> generic/043 12s ... 13s
> generic/044 15s ... 15s
> generic/045 18s ... 18s
> generic/046 15s ... 14s
> generic/051 77s ... 76s
> generic/052 2s ... 1s
> generic/054 48s ... 29s
> generic/055 17s ... 16s
> generic/056 1s ... 1s
> generic/057 1s ... 1s
> generic/059 2s ... 2s
> generic/065 1s ... 1s
> generic/066 1s ... 1s
> generic/073 1s ... 1s
> generic/090 1s ... 1s
> generic/101 1s ... 1s
> generic/104 1s ... 1s
> generic/106 1s ... 1s
> generic/107 1s ... 1s
> generic/177 1s ... 1s
> generic/311 68s ... 68s
> generic/321 2s ... 2s
> generic/322 1s ... 1s
> generic/325 2s ... 1s
> generic/335 1s ... 1s
> generic/336 1s ... 1s
> generic/341 1s ... 1s
> generic/342 1s ... 1s
> generic/343 1s ... 1s
> generic/376 1s ... 1s
> generic/388 73s ... 83s
> generic/417 11s ... 11s
> generic/455 [not run] This test requires a valid $LOGWRITES_DEV
> generic/457 [not run] This test requires a valid $LOGWRITES_DEV
> generic/475 96s ... 83s
> generic/479 2s
> generic/480 2s ... 1s
> generic/481 0s ... 1s
> generic/483 7s ... 7s
> generic/489 1s ... 1s
> generic/498 1s ... 1s
> generic/501 63s ... 61s
> generic/502 1s ... 1s
> generic/509 1s ... 1s
> generic/510 1s ... 1s
> generic/512 1s ... 1s
> generic/520 16s ... 16s
> generic/526 1s ... 1s
> generic/527 1s ... 1s
> generic/534 1s ... 1s
> generic/535 1s ... 1s
> generic/546 4s ... 4s
> generic/547 2s ... 2s
> generic/552 1s ... 1s
> generic/557 1s ... 1s
> generic/588 2s ... 1s
> shared/002 8s ... 9s
> xfs/011 18s ... 18s
> xfs/029 1s ... 1s
> xfs/051 18s ... 18s
> xfs/057 23s ... 24s
> xfs/079 13s ... 14s
> xfs/095 2s ... 1s
> xfs/119 3s ... 3s
> xfs/121 8s ... 6s
> xfs/141 18s ... 12s
> xfs/181 14s ... 12s
> xfs/216 3s ... 2s
> xfs/217 2s ... 2s
> xfs/439 [not run] test requires XFS bug_on_assert to be off, turn it off to run the test
> Ran: generic/034 generic/039 generic/040 generic/041 generic/043 generic/044 generic/045 generic/046 generic/051 generic/052 generic/054 generic/055 generic/056 generic/057 generic/059 generic/065 generic/066 generic/073 generic/090 generic/101 generic/104 generic/106 generic/107 generic/177 generic/311 generic/321 generic/322 generic/325 generic/335 generic/336 generic/341 generic/342 generic/343 generic/376 generic/388 generic/417 generic/455 generic/457 generic/475 generic/479 generic/480 generic/481 generic/483 generic/489 generic/498 generic/501 generic/502 generic/509 generic/510 generic/512 generic/520 generic/526 generic/527 generic/534 generic/535 generic/546 generic/547 generic/552 generic/557 generic/588 shared/002 xfs/011 xfs/029 xfs/051 xfs/057 xfs/079 xfs/095 xfs/119 xfs/121 xfs/141 xfs/181 xfs/216 xfs/217 xfs/439
> Not run: generic/455 generic/457 xfs/439
> Passed all 74 tests
>
> Notice that the test results before and after this patch is applied are
> identical and successful. Other tests might need to be run in order to
> verify everything is working as expected. For such tests, the intervention
> of the maintainers might be needed.
>
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2] https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays
>
> Link: https://github.com/KSPP/linux/issues/79
> Build-tested-by: kernel test robot <lkp@intel.com>
> Link: https://lore.kernel.org/lkml/603e3177.WsvTXRpcLn5wdYW6%25lkp@intel.com/
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> fs/xfs/libxfs/xfs_log_format.h | 12 +++++------
> fs/xfs/xfs_extfree_item.c | 39 +++++++++++++++-------------------
> fs/xfs/xfs_ondisk.h | 8 +++----
> 3 files changed, 27 insertions(+), 32 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index 8bd00da6d2a4..9934a465b441 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -574,7 +574,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 {
> @@ -582,7 +582,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 {
> @@ -590,7 +590,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;
>
> /*
> @@ -603,7 +603,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 {
> @@ -611,7 +611,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 {
> @@ -619,7 +619,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 93223ebb3372..5ed3ea93071c 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -73,8 +73,8 @@ 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 - 1) * sizeof(xfs_extent_t);
> + return struct_size(&efip->efi_format, efi_extents,
> + efip->efi_format.efi_nextents);
> }
>
> STATIC void
> @@ -153,17 +153,14 @@ 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 - 1) * sizeof(xfs_extent_t)));
> - efip = kmem_zalloc(size, 0);
> - } else {
> + if (nextents > XFS_EFI_MAX_FAST_EXTENTS)
> + efip = kmem_zalloc(struct_size(efip, efi_format.efi_extents,
> + nextents), 0);
> + else
> efip = kmem_cache_zalloc(xfs_efi_zone,
> GFP_KERNEL | __GFP_NOFAIL);
> - }
>
> xfs_log_item_init(mp, &efip->efi_item, XFS_LI_EFI, &xfs_efi_item_ops);
> efip->efi_format.efi_nextents = nextents;
> @@ -186,12 +183,12 @@ 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);
> + size_t len = struct_size(src_efi_fmt, efi_extents,
> + src_efi_fmt->efi_nextents);
> 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);
Shouldn't these be converted to struct_size() too?
It seems to work all right for casted NULL pointers, and then we get all
the typechecking and multiplication overflow checking, e.g.:
size_t len64 = struct_size((struct xfs_efi_log_format_32 *)NULL,
efi_extents src_efi_fmt->efi_nextents);
--D
> 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);
> @@ -254,7 +251,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
> @@ -328,14 +325,12 @@ xfs_trans_get_efd(
>
> ASSERT(nextents > 0);
>
> - if (nextents > XFS_EFD_MAX_FAST_EXTENTS) {
> - efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) +
> - (nextents - 1) * sizeof(struct xfs_extent),
> - 0);
> - } else {
> + if (nextents > XFS_EFD_MAX_FAST_EXTENTS)
> + efdp = kmem_zalloc(struct_size(efip, efi_format.efi_extents,
> + nextents), 0);
> + else
> efdp = kmem_cache_zalloc(xfs_efd_zone,
> GFP_KERNEL | __GFP_NOFAIL);
> - }
>
> xfs_log_item_init(tp->t_mountp, &efdp->efd_item, XFS_LI_EFD,
> &xfs_efd_item_ops);
> @@ -747,9 +742,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 0aa87c210104..f58e0510385a 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);
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][next] xfs: Replace one-element arrays with flexible-array members
2021-03-09 17:42 ` Darrick J. Wong
@ 2021-03-09 19:57 ` Gustavo A. R. Silva
2021-03-09 21:26 ` Darrick J. Wong
0 siblings, 1 reply; 11+ messages in thread
From: Gustavo A. R. Silva @ 2021-03-09 19:57 UTC (permalink / raw)
To: Darrick J. Wong, Gustavo A. R. Silva
Cc: Dave Chinner, linux-xfs, linux-kernel, linux-hardening
Hi Darrick,
Please, see my comments below...
On 3/9/21 11:42, Darrick J. Wong wrote:
> On Tue, Mar 02, 2021 at 09:05:58AM -0600, Gustavo A. R. Silva wrote:
>> There is a regular need in the kernel to provide a way to declare having
>> a dynamically sized set of trailing elements in a structure. Kernel code
>> should always use “flexible array members”[1] for these cases. The older
>> style of one-element or zero-length arrays should no longer be used[2].
>>
>> Refactor the code according to the use of flexible-array members in
>> multiple structures, instead of one-element arrays. Also, make use of
>> the new struct_size() helper to properly calculate the size of multiple
>> structures that contain flexible-array members.
>>
>> Below are the results of running xfstests for groups shutdown and log
>> with the following configuration in local.config:
>>
>> export TEST_DEV=/dev/sda3
>> export TEST_DIR=/mnt/test
>> export SCRATCH_DEV=/dev/sda4
>> export SCRATCH_MNT=/mnt/scratch
>>
>> The size for both partitions /dev/sda3 and /dev/sda4 is 25GB.
>>
>> These are the results of running ./check -g shutdown and ./check -g
>> log, respectively, on 5.11.0-next-20210301 kernel _without_ this patch
>> applied:
>>
>> gustavo@beefy:~/git/xfstests$ time sudo ./check -g shutdown 2>&1 | tee xfs-shutdown-no-patch.out
>> FSTYP -- xfs (debug)
>> PLATFORM -- Linux/x86_64 beefy 5.11.0-next-20210301 #4 SMP Mon Mar 1 05:42:06 CST 2021
>> MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
>> MOUNT_OPTIONS -- /dev/sda4 /mnt/scratch
>>
>> generic/042 2s ... 2s
>> generic/043 13s ... 13s
>> generic/044 14s ... 14s
>> generic/045 18s ... 18s
>> generic/046 14s ... 14s
>> generic/047 20s ... 22s
>> generic/048 51s ... 50s
>> generic/049 9s ... 8s
>> generic/050 2s ... 1s
>> generic/051 76s ... 75s
>> generic/052 1s ... 2s
>> generic/054 31s ... 30s
>> generic/055 16s ... 21s
>> generic/388 89s ... 75s
>> generic/392 5s ... 5s
>> generic/417 11s ... 11s
>> generic/461 21s ... 22s
>> generic/468 3s ... 3s
>> generic/474 1s ... 1s
>> generic/475 86s ... 93s
>> generic/505 2s ... 2s
>> generic/506 2s ... 2s
>> generic/507 [not run] file system doesn't support chattr +AsSu
>> generic/508 1s ... 1s
>> generic/530 10s ... 10s
>> generic/536 3s ... 2s
>> generic/599 1s ... 1s
>> generic/622 23s ... 24s
>> xfs/051 18s ... 18s
>> xfs/079 15s ... 14s
>> xfs/121 6s ... 16s
>> xfs/181 12s ... 15s
>> xfs/212 1s ... 3s
>> Ran: generic/042 generic/043 generic/044 generic/045 generic/046 generic/047 generic/048 generic/049 generic/050 generic/051 generic/052 generic/054 generic/055 generic/388 generic/392 generic/417 generic/461 generic/468 generic/474 generic/475 generic/505 generic/506 generic/507 generic/508 generic/530 generic/536 generic/599 generic/622 xfs/051 xfs/079 xfs/121 xfs/181 xfs/212
>> Not run: generic/507
>> Passed all 33 tests
>>
>> gustavo@beefy:~/git/xfstests$ time sudo ./check -g log 2>&1 | tee xfs-log-no-patch.out
>> FSTYP -- xfs (debug)
>> PLATFORM -- Linux/x86_64 beefy 5.11.0-next-20210301 #4 SMP Mon Mar 1 05:42:06 CST 2021
>> MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
>> MOUNT_OPTIONS -- /dev/sda4 /mnt/scratch
>>
>> generic/034 1s ... 1s
>> generic/039 1s ... 1s
>> generic/040 5s ... 5s
>> generic/041 7s ... 7s
>> generic/043 13s ... 13s
>> generic/044 14s ... 14s
>> generic/045 18s ... 18s
>> generic/046 14s ... 14s
>> generic/051 76s ... 76s
>> generic/052 1s ... 2s
>> generic/054 30s ... 48s
>> generic/055 23s ... 15s
>> generic/056 1s ... 1s
>> generic/057 1s ... 1s
>> generic/059 2s ... 2s
>> generic/065 1s ... 1s
>> generic/066 1s ... 2s
>> generic/073 1s ... 1s
>> generic/090 1s ... 1s
>> generic/101 1s ... 1s
>> generic/104 1s ... 1s
>> generic/106 1s ... 1s
>> generic/107 1s ... 1s
>> generic/177 1s ... 1s
>> generic/311 67s ... 67s
>> generic/321 2s ... 2s
>> generic/322 1s ... 2s
>> generic/325 1s ... 1s
>> generic/335 1s ... 1s
>> generic/336 2s ... 1s
>> generic/341 1s ... 1s
>> generic/342 1s ... 1s
>> generic/343 1s ... 1s
>> generic/376 1s ... 1s
>> generic/388 75s ... 97s
>> generic/417 15s ... 12s
>> generic/455 [not run] This test requires a valid $LOGWRITES_DEV
>> generic/457 [not run] This test requires a valid $LOGWRITES_DEV
>> generic/475 88s ... 89s
>> generic/479 2s ... 2s
>> generic/480 1s ... 1s
>> generic/481 1s ... 1s
>> generic/483 7s ... 6s
>> generic/489 1s ... 1s
>> generic/498 1s ... 1s
>> generic/501 61s ... 63s
>> generic/502 1s ... 1s
>> generic/509 1s ... 1s
>> generic/510 1s ... 1s
>> generic/512 1s ... 1s
>> generic/520 16s ... 17s
>> generic/526 2s ... 1s
>> generic/527 0s ... 1s
>> generic/534 0s ... 1s
>> generic/535 2s ... 1s
>> generic/546 3s ... 3s
>> generic/547 2s ... 2s
>> generic/552 1s ... 1s
>> generic/557 1s ... 2s
>> generic/588 2s ... 1s
>> shared/002 9s ... 9s
>> xfs/011 18s ... 18s
>> xfs/029 1s ... 1s
>> xfs/051 18s ... 18s
>> xfs/057 23s ... 23s
>> xfs/079 14s ... 14s
>> xfs/095 1s ... 1s
>> xfs/119 3s ... 3s
>> xfs/121 6s ... 6s
>> xfs/141 14s ... 26s
>> xfs/181 14s ... 14s
>> xfs/216 3s ... 2s
>> xfs/217 2s ... 2s
>> xfs/439 [not run] test requires XFS bug_on_assert to be off, turn it off to run the test
>> Ran: generic/034 generic/039 generic/040 generic/041 generic/043 generic/044 generic/045 generic/046 generic/051 generic/052 generic/054 generic/055 generic/056 generic/057 generic/059 generic/065 generic/066 generic/073 generic/090 generic/101 generic/104 generic/106 generic/107 generic/177 generic/311 generic/321 generic/322 generic/325 generic/335 generic/336 generic/341 generic/342 generic/343 generic/376 generic/388 generic/417 generic/455 generic/457 generic/475 generic/479 generic/480 generic/481 generic/483 generic/489 generic/498 generic/501 generic/502 generic/509 generic/510 generic/512 generic/520 generic/526 generic/527 generic/534 generic/535 generic/546 generic/547 generic/552 generic/557 generic/588 shared/002 xfs/011 xfs/029 xfs/051 xfs/057 xfs/079 xfs/095 xfs/119 xfs/121 xfs/141 xfs/181 xfs/216 xfs/217 xfs/439
>> Not run: generic/455 generic/457 xfs/439
>> Passed all 74 tests
>>
>> These are the results of running ./check -g shutdown and ./check -g
>> log, respectively, on 5.11.0-xfstests-patched-next-20210301+ kernel
>> _with_ this patch applied on top:
>>
>> gustavo@beefy:~/git/xfstests$ time sudo ./check -g shutdown 2>&1 | tee xfs-shutdown-patched.out
>> FSTYP -- xfs (debug)
>> PLATFORM -- Linux/x86_64 beefy 5.11.0-xfstests-patched-next-20210301+ #10 SMP Mon Mar 1 17:36:20 CST 2021
>> MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
>> MOUNT_OPTIONS -- /dev/sda4 /mnt/scratch
>>
>> generic/042 2s ... 3s
>> generic/043 13s ... 12s
>> generic/044 14s ... 15s
>> generic/045 18s ... 18s
>> generic/046 14s ... 15s
>> generic/047 21s ... 21s
>> generic/048 51s ... 51s
>> generic/049 9s ... 8s
>> generic/050 1s ... 2s
>> generic/051 76s ... 77s
>> generic/052 6s ... 2s
>> generic/054 35s ... 48s
>> generic/055 16s ... 17s
>> generic/388 77s ... 73s
>> generic/392 7s ... 5s
>> generic/417 13s ... 11s
>> generic/461 22s ... 21s
>> generic/468 2s ... 3s
>> generic/474 2s ... 1s
>> generic/475 90s ... 96s
>> generic/505 3s ... 3s
>> generic/506 1s ... 2s
>> generic/507 [not run] file system doesn't support chattr +AsSu
>> generic/508 2s ... 10s
>> generic/530 9s ... 13s
>> generic/536 2s ... 1s
>> generic/599 1s ... 1s
>> generic/622 24s ... 25s
>> xfs/051 18s ... 18s
>> xfs/079 14s ... 13s
>> xfs/121 6s ... 8s
>> xfs/181 11s ... 14s
>> xfs/212 2s ... 1s
>> Ran: generic/042 generic/043 generic/044 generic/045 generic/046 generic/047 generic/048 generic/049 generic/050 generic/051 generic/052 generic/054 generic/055 generic/388 generic/392 generic/417 generic/461 generic/468 generic/474 generic/475 generic/505 generic/506 generic/507 generic/508 generic/530 generic/536 generic/599 generic/622 xfs/051 xfs/079 xfs/121 xfs/181 xfs/212
>> Not run: generic/507
>> Passed all 33 tests
>>
>> gustavo@beefy:~/git/xfstests$ time sudo ./check -g log 2>&1 | tee xfs-log-patched.out
>> FSTYP -- xfs (debug)
>> PLATFORM -- Linux/x86_64 beefy 5.11.0-xfstests-patched-next-20210301+ #10 SMP Mon Mar 1 17:36:20 CST 2021
>> MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
>> MOUNT_OPTIONS -- /dev/sda4 /mnt/scratch
>>
>> generic/034 1s ... 1s
>> generic/039 1s ... 1s
>> generic/040 5s ... 5s
>> generic/041 7s ... 7s
>> generic/043 12s ... 13s
>> generic/044 15s ... 15s
>> generic/045 18s ... 18s
>> generic/046 15s ... 14s
>> generic/051 77s ... 76s
>> generic/052 2s ... 1s
>> generic/054 48s ... 29s
>> generic/055 17s ... 16s
>> generic/056 1s ... 1s
>> generic/057 1s ... 1s
>> generic/059 2s ... 2s
>> generic/065 1s ... 1s
>> generic/066 1s ... 1s
>> generic/073 1s ... 1s
>> generic/090 1s ... 1s
>> generic/101 1s ... 1s
>> generic/104 1s ... 1s
>> generic/106 1s ... 1s
>> generic/107 1s ... 1s
>> generic/177 1s ... 1s
>> generic/311 68s ... 68s
>> generic/321 2s ... 2s
>> generic/322 1s ... 1s
>> generic/325 2s ... 1s
>> generic/335 1s ... 1s
>> generic/336 1s ... 1s
>> generic/341 1s ... 1s
>> generic/342 1s ... 1s
>> generic/343 1s ... 1s
>> generic/376 1s ... 1s
>> generic/388 73s ... 83s
>> generic/417 11s ... 11s
>> generic/455 [not run] This test requires a valid $LOGWRITES_DEV
>> generic/457 [not run] This test requires a valid $LOGWRITES_DEV
>> generic/475 96s ... 83s
>> generic/479 2s
>> generic/480 2s ... 1s
>> generic/481 0s ... 1s
>> generic/483 7s ... 7s
>> generic/489 1s ... 1s
>> generic/498 1s ... 1s
>> generic/501 63s ... 61s
>> generic/502 1s ... 1s
>> generic/509 1s ... 1s
>> generic/510 1s ... 1s
>> generic/512 1s ... 1s
>> generic/520 16s ... 16s
>> generic/526 1s ... 1s
>> generic/527 1s ... 1s
>> generic/534 1s ... 1s
>> generic/535 1s ... 1s
>> generic/546 4s ... 4s
>> generic/547 2s ... 2s
>> generic/552 1s ... 1s
>> generic/557 1s ... 1s
>> generic/588 2s ... 1s
>> shared/002 8s ... 9s
>> xfs/011 18s ... 18s
>> xfs/029 1s ... 1s
>> xfs/051 18s ... 18s
>> xfs/057 23s ... 24s
>> xfs/079 13s ... 14s
>> xfs/095 2s ... 1s
>> xfs/119 3s ... 3s
>> xfs/121 8s ... 6s
>> xfs/141 18s ... 12s
>> xfs/181 14s ... 12s
>> xfs/216 3s ... 2s
>> xfs/217 2s ... 2s
>> xfs/439 [not run] test requires XFS bug_on_assert to be off, turn it off to run the test
>> Ran: generic/034 generic/039 generic/040 generic/041 generic/043 generic/044 generic/045 generic/046 generic/051 generic/052 generic/054 generic/055 generic/056 generic/057 generic/059 generic/065 generic/066 generic/073 generic/090 generic/101 generic/104 generic/106 generic/107 generic/177 generic/311 generic/321 generic/322 generic/325 generic/335 generic/336 generic/341 generic/342 generic/343 generic/376 generic/388 generic/417 generic/455 generic/457 generic/475 generic/479 generic/480 generic/481 generic/483 generic/489 generic/498 generic/501 generic/502 generic/509 generic/510 generic/512 generic/520 generic/526 generic/527 generic/534 generic/535 generic/546 generic/547 generic/552 generic/557 generic/588 shared/002 xfs/011 xfs/029 xfs/051 xfs/057 xfs/079 xfs/095 xfs/119 xfs/121 xfs/141 xfs/181 xfs/216 xfs/217 xfs/439
>> Not run: generic/455 generic/457 xfs/439
>> Passed all 74 tests
>>
>> Notice that the test results before and after this patch is applied are
>> identical and successful. Other tests might need to be run in order to
>> verify everything is working as expected. For such tests, the intervention
>> of the maintainers might be needed.
>>
>> [1] https://en.wikipedia.org/wiki/Flexible_array_member
>> [2] https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays
>>
>> Link: https://github.com/KSPP/linux/issues/79
>> Build-tested-by: kernel test robot <lkp@intel.com>
>> Link: https://lore.kernel.org/lkml/603e3177.WsvTXRpcLn5wdYW6%25lkp@intel.com/
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>> fs/xfs/libxfs/xfs_log_format.h | 12 +++++------
>> fs/xfs/xfs_extfree_item.c | 39 +++++++++++++++-------------------
>> fs/xfs/xfs_ondisk.h | 8 +++----
>> 3 files changed, 27 insertions(+), 32 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
>> index 8bd00da6d2a4..9934a465b441 100644
>> --- a/fs/xfs/libxfs/xfs_log_format.h
>> +++ b/fs/xfs/libxfs/xfs_log_format.h
>> @@ -574,7 +574,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 {
>> @@ -582,7 +582,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 {
>> @@ -590,7 +590,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;
>>
>> /*
>> @@ -603,7 +603,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 {
>> @@ -611,7 +611,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 {
>> @@ -619,7 +619,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 93223ebb3372..5ed3ea93071c 100644
>> --- a/fs/xfs/xfs_extfree_item.c
>> +++ b/fs/xfs/xfs_extfree_item.c
>> @@ -73,8 +73,8 @@ 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 - 1) * sizeof(xfs_extent_t);
>> + return struct_size(&efip->efi_format, efi_extents,
>> + efip->efi_format.efi_nextents);
>> }
>>
>> STATIC void
>> @@ -153,17 +153,14 @@ 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 - 1) * sizeof(xfs_extent_t)));
>> - efip = kmem_zalloc(size, 0);
>> - } else {
>> + if (nextents > XFS_EFI_MAX_FAST_EXTENTS)
>> + efip = kmem_zalloc(struct_size(efip, efi_format.efi_extents,
>> + nextents), 0);
>> + else
>> efip = kmem_cache_zalloc(xfs_efi_zone,
>> GFP_KERNEL | __GFP_NOFAIL);
>> - }
>>
>> xfs_log_item_init(mp, &efip->efi_item, XFS_LI_EFI, &xfs_efi_item_ops);
>> efip->efi_format.efi_nextents = nextents;
>> @@ -186,12 +183,12 @@ 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);
>> + size_t len = struct_size(src_efi_fmt, efi_extents,
>> + src_efi_fmt->efi_nextents);
>> 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);
>
> Shouldn't these be converted to struct_size() too?
>
> It seems to work all right for casted NULL pointers, and then we get all
> the typechecking and multiplication overflow checking, e.g.:
>
> size_t len64 = struct_size((struct xfs_efi_log_format_32 *)NULL,
> efi_extents src_efi_fmt->efi_nextents);
Yeah; in that case, what do you think about casting 0, instead of NULL:
uint len32 = struct_size((xfs_efi_log_format_32_t *)0, efi_extents,
src_efi_fmt->efi_nextents);
uint len64 = struct_size((xfs_efi_log_format_64_t *)0, efi_extents,
src_efi_fmt->efi_nextents);
Thanks
--
Gustavo
>
> --D
>
>> 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);
>> @@ -254,7 +251,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
>> @@ -328,14 +325,12 @@ xfs_trans_get_efd(
>>
>> ASSERT(nextents > 0);
>>
>> - if (nextents > XFS_EFD_MAX_FAST_EXTENTS) {
>> - efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) +
>> - (nextents - 1) * sizeof(struct xfs_extent),
>> - 0);
>> - } else {
>> + if (nextents > XFS_EFD_MAX_FAST_EXTENTS)
>> + efdp = kmem_zalloc(struct_size(efip, efi_format.efi_extents,
>> + nextents), 0);
>> + else
>> efdp = kmem_cache_zalloc(xfs_efd_zone,
>> GFP_KERNEL | __GFP_NOFAIL);
>> - }
>>
>> xfs_log_item_init(tp->t_mountp, &efdp->efd_item, XFS_LI_EFD,
>> &xfs_efd_item_ops);
>> @@ -747,9 +742,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 0aa87c210104..f58e0510385a 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);
>> --
>> 2.27.0
>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][next] xfs: Replace one-element arrays with flexible-array members
2021-03-09 19:57 ` Gustavo A. R. Silva
@ 2021-03-09 21:26 ` Darrick J. Wong
2021-03-09 22:03 ` Gustavo A. R. Silva
0 siblings, 1 reply; 11+ messages in thread
From: Darrick J. Wong @ 2021-03-09 21:26 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Gustavo A. R. Silva, Dave Chinner, linux-xfs, linux-kernel,
linux-hardening
On Tue, Mar 09, 2021 at 01:57:36PM -0600, Gustavo A. R. Silva wrote:
> Hi Darrick,
>
> Please, see my comments below...
>
> On 3/9/21 11:42, Darrick J. Wong wrote:
> > On Tue, Mar 02, 2021 at 09:05:58AM -0600, Gustavo A. R. Silva wrote:
> >> There is a regular need in the kernel to provide a way to declare having
> >> a dynamically sized set of trailing elements in a structure. Kernel code
> >> should always use “flexible array members”[1] for these cases. The older
> >> style of one-element or zero-length arrays should no longer be used[2].
> >>
> >> Refactor the code according to the use of flexible-array members in
> >> multiple structures, instead of one-element arrays. Also, make use of
> >> the new struct_size() helper to properly calculate the size of multiple
> >> structures that contain flexible-array members.
> >>
> >> Below are the results of running xfstests for groups shutdown and log
> >> with the following configuration in local.config:
> >>
> >> export TEST_DEV=/dev/sda3
> >> export TEST_DIR=/mnt/test
> >> export SCRATCH_DEV=/dev/sda4
> >> export SCRATCH_MNT=/mnt/scratch
> >>
> >> The size for both partitions /dev/sda3 and /dev/sda4 is 25GB.
> >>
> >> These are the results of running ./check -g shutdown and ./check -g
> >> log, respectively, on 5.11.0-next-20210301 kernel _without_ this patch
> >> applied:
> >>
> >> gustavo@beefy:~/git/xfstests$ time sudo ./check -g shutdown 2>&1 | tee xfs-shutdown-no-patch.out
> >> FSTYP -- xfs (debug)
> >> PLATFORM -- Linux/x86_64 beefy 5.11.0-next-20210301 #4 SMP Mon Mar 1 05:42:06 CST 2021
> >> MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
> >> MOUNT_OPTIONS -- /dev/sda4 /mnt/scratch
> >>
> >> generic/042 2s ... 2s
> >> generic/043 13s ... 13s
> >> generic/044 14s ... 14s
> >> generic/045 18s ... 18s
> >> generic/046 14s ... 14s
> >> generic/047 20s ... 22s
> >> generic/048 51s ... 50s
> >> generic/049 9s ... 8s
> >> generic/050 2s ... 1s
> >> generic/051 76s ... 75s
> >> generic/052 1s ... 2s
> >> generic/054 31s ... 30s
> >> generic/055 16s ... 21s
> >> generic/388 89s ... 75s
> >> generic/392 5s ... 5s
> >> generic/417 11s ... 11s
> >> generic/461 21s ... 22s
> >> generic/468 3s ... 3s
> >> generic/474 1s ... 1s
> >> generic/475 86s ... 93s
> >> generic/505 2s ... 2s
> >> generic/506 2s ... 2s
> >> generic/507 [not run] file system doesn't support chattr +AsSu
> >> generic/508 1s ... 1s
> >> generic/530 10s ... 10s
> >> generic/536 3s ... 2s
> >> generic/599 1s ... 1s
> >> generic/622 23s ... 24s
> >> xfs/051 18s ... 18s
> >> xfs/079 15s ... 14s
> >> xfs/121 6s ... 16s
> >> xfs/181 12s ... 15s
> >> xfs/212 1s ... 3s
> >> Ran: generic/042 generic/043 generic/044 generic/045 generic/046 generic/047 generic/048 generic/049 generic/050 generic/051 generic/052 generic/054 generic/055 generic/388 generic/392 generic/417 generic/461 generic/468 generic/474 generic/475 generic/505 generic/506 generic/507 generic/508 generic/530 generic/536 generic/599 generic/622 xfs/051 xfs/079 xfs/121 xfs/181 xfs/212
> >> Not run: generic/507
> >> Passed all 33 tests
> >>
> >> gustavo@beefy:~/git/xfstests$ time sudo ./check -g log 2>&1 | tee xfs-log-no-patch.out
> >> FSTYP -- xfs (debug)
> >> PLATFORM -- Linux/x86_64 beefy 5.11.0-next-20210301 #4 SMP Mon Mar 1 05:42:06 CST 2021
> >> MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
> >> MOUNT_OPTIONS -- /dev/sda4 /mnt/scratch
> >>
> >> generic/034 1s ... 1s
> >> generic/039 1s ... 1s
> >> generic/040 5s ... 5s
> >> generic/041 7s ... 7s
> >> generic/043 13s ... 13s
> >> generic/044 14s ... 14s
> >> generic/045 18s ... 18s
> >> generic/046 14s ... 14s
> >> generic/051 76s ... 76s
> >> generic/052 1s ... 2s
> >> generic/054 30s ... 48s
> >> generic/055 23s ... 15s
> >> generic/056 1s ... 1s
> >> generic/057 1s ... 1s
> >> generic/059 2s ... 2s
> >> generic/065 1s ... 1s
> >> generic/066 1s ... 2s
> >> generic/073 1s ... 1s
> >> generic/090 1s ... 1s
> >> generic/101 1s ... 1s
> >> generic/104 1s ... 1s
> >> generic/106 1s ... 1s
> >> generic/107 1s ... 1s
> >> generic/177 1s ... 1s
> >> generic/311 67s ... 67s
> >> generic/321 2s ... 2s
> >> generic/322 1s ... 2s
> >> generic/325 1s ... 1s
> >> generic/335 1s ... 1s
> >> generic/336 2s ... 1s
> >> generic/341 1s ... 1s
> >> generic/342 1s ... 1s
> >> generic/343 1s ... 1s
> >> generic/376 1s ... 1s
> >> generic/388 75s ... 97s
> >> generic/417 15s ... 12s
> >> generic/455 [not run] This test requires a valid $LOGWRITES_DEV
> >> generic/457 [not run] This test requires a valid $LOGWRITES_DEV
> >> generic/475 88s ... 89s
> >> generic/479 2s ... 2s
> >> generic/480 1s ... 1s
> >> generic/481 1s ... 1s
> >> generic/483 7s ... 6s
> >> generic/489 1s ... 1s
> >> generic/498 1s ... 1s
> >> generic/501 61s ... 63s
> >> generic/502 1s ... 1s
> >> generic/509 1s ... 1s
> >> generic/510 1s ... 1s
> >> generic/512 1s ... 1s
> >> generic/520 16s ... 17s
> >> generic/526 2s ... 1s
> >> generic/527 0s ... 1s
> >> generic/534 0s ... 1s
> >> generic/535 2s ... 1s
> >> generic/546 3s ... 3s
> >> generic/547 2s ... 2s
> >> generic/552 1s ... 1s
> >> generic/557 1s ... 2s
> >> generic/588 2s ... 1s
> >> shared/002 9s ... 9s
> >> xfs/011 18s ... 18s
> >> xfs/029 1s ... 1s
> >> xfs/051 18s ... 18s
> >> xfs/057 23s ... 23s
> >> xfs/079 14s ... 14s
> >> xfs/095 1s ... 1s
> >> xfs/119 3s ... 3s
> >> xfs/121 6s ... 6s
> >> xfs/141 14s ... 26s
> >> xfs/181 14s ... 14s
> >> xfs/216 3s ... 2s
> >> xfs/217 2s ... 2s
> >> xfs/439 [not run] test requires XFS bug_on_assert to be off, turn it off to run the test
> >> Ran: generic/034 generic/039 generic/040 generic/041 generic/043 generic/044 generic/045 generic/046 generic/051 generic/052 generic/054 generic/055 generic/056 generic/057 generic/059 generic/065 generic/066 generic/073 generic/090 generic/101 generic/104 generic/106 generic/107 generic/177 generic/311 generic/321 generic/322 generic/325 generic/335 generic/336 generic/341 generic/342 generic/343 generic/376 generic/388 generic/417 generic/455 generic/457 generic/475 generic/479 generic/480 generic/481 generic/483 generic/489 generic/498 generic/501 generic/502 generic/509 generic/510 generic/512 generic/520 generic/526 generic/527 generic/534 generic/535 generic/546 generic/547 generic/552 generic/557 generic/588 shared/002 xfs/011 xfs/029 xfs/051 xfs/057 xfs/079 xfs/095 xfs/119 xfs/121 xfs/141 xfs/181 xfs/216 xfs/217 xfs/439
> >> Not run: generic/455 generic/457 xfs/439
> >> Passed all 74 tests
> >>
> >> These are the results of running ./check -g shutdown and ./check -g
> >> log, respectively, on 5.11.0-xfstests-patched-next-20210301+ kernel
> >> _with_ this patch applied on top:
> >>
> >> gustavo@beefy:~/git/xfstests$ time sudo ./check -g shutdown 2>&1 | tee xfs-shutdown-patched.out
> >> FSTYP -- xfs (debug)
> >> PLATFORM -- Linux/x86_64 beefy 5.11.0-xfstests-patched-next-20210301+ #10 SMP Mon Mar 1 17:36:20 CST 2021
> >> MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
> >> MOUNT_OPTIONS -- /dev/sda4 /mnt/scratch
> >>
> >> generic/042 2s ... 3s
> >> generic/043 13s ... 12s
> >> generic/044 14s ... 15s
> >> generic/045 18s ... 18s
> >> generic/046 14s ... 15s
> >> generic/047 21s ... 21s
> >> generic/048 51s ... 51s
> >> generic/049 9s ... 8s
> >> generic/050 1s ... 2s
> >> generic/051 76s ... 77s
> >> generic/052 6s ... 2s
> >> generic/054 35s ... 48s
> >> generic/055 16s ... 17s
> >> generic/388 77s ... 73s
> >> generic/392 7s ... 5s
> >> generic/417 13s ... 11s
> >> generic/461 22s ... 21s
> >> generic/468 2s ... 3s
> >> generic/474 2s ... 1s
> >> generic/475 90s ... 96s
> >> generic/505 3s ... 3s
> >> generic/506 1s ... 2s
> >> generic/507 [not run] file system doesn't support chattr +AsSu
> >> generic/508 2s ... 10s
> >> generic/530 9s ... 13s
> >> generic/536 2s ... 1s
> >> generic/599 1s ... 1s
> >> generic/622 24s ... 25s
> >> xfs/051 18s ... 18s
> >> xfs/079 14s ... 13s
> >> xfs/121 6s ... 8s
> >> xfs/181 11s ... 14s
> >> xfs/212 2s ... 1s
> >> Ran: generic/042 generic/043 generic/044 generic/045 generic/046 generic/047 generic/048 generic/049 generic/050 generic/051 generic/052 generic/054 generic/055 generic/388 generic/392 generic/417 generic/461 generic/468 generic/474 generic/475 generic/505 generic/506 generic/507 generic/508 generic/530 generic/536 generic/599 generic/622 xfs/051 xfs/079 xfs/121 xfs/181 xfs/212
> >> Not run: generic/507
> >> Passed all 33 tests
> >>
> >> gustavo@beefy:~/git/xfstests$ time sudo ./check -g log 2>&1 | tee xfs-log-patched.out
> >> FSTYP -- xfs (debug)
> >> PLATFORM -- Linux/x86_64 beefy 5.11.0-xfstests-patched-next-20210301+ #10 SMP Mon Mar 1 17:36:20 CST 2021
> >> MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
> >> MOUNT_OPTIONS -- /dev/sda4 /mnt/scratch
> >>
> >> generic/034 1s ... 1s
> >> generic/039 1s ... 1s
> >> generic/040 5s ... 5s
> >> generic/041 7s ... 7s
> >> generic/043 12s ... 13s
> >> generic/044 15s ... 15s
> >> generic/045 18s ... 18s
> >> generic/046 15s ... 14s
> >> generic/051 77s ... 76s
> >> generic/052 2s ... 1s
> >> generic/054 48s ... 29s
> >> generic/055 17s ... 16s
> >> generic/056 1s ... 1s
> >> generic/057 1s ... 1s
> >> generic/059 2s ... 2s
> >> generic/065 1s ... 1s
> >> generic/066 1s ... 1s
> >> generic/073 1s ... 1s
> >> generic/090 1s ... 1s
> >> generic/101 1s ... 1s
> >> generic/104 1s ... 1s
> >> generic/106 1s ... 1s
> >> generic/107 1s ... 1s
> >> generic/177 1s ... 1s
> >> generic/311 68s ... 68s
> >> generic/321 2s ... 2s
> >> generic/322 1s ... 1s
> >> generic/325 2s ... 1s
> >> generic/335 1s ... 1s
> >> generic/336 1s ... 1s
> >> generic/341 1s ... 1s
> >> generic/342 1s ... 1s
> >> generic/343 1s ... 1s
> >> generic/376 1s ... 1s
> >> generic/388 73s ... 83s
> >> generic/417 11s ... 11s
> >> generic/455 [not run] This test requires a valid $LOGWRITES_DEV
> >> generic/457 [not run] This test requires a valid $LOGWRITES_DEV
> >> generic/475 96s ... 83s
> >> generic/479 2s
> >> generic/480 2s ... 1s
> >> generic/481 0s ... 1s
> >> generic/483 7s ... 7s
> >> generic/489 1s ... 1s
> >> generic/498 1s ... 1s
> >> generic/501 63s ... 61s
> >> generic/502 1s ... 1s
> >> generic/509 1s ... 1s
> >> generic/510 1s ... 1s
> >> generic/512 1s ... 1s
> >> generic/520 16s ... 16s
> >> generic/526 1s ... 1s
> >> generic/527 1s ... 1s
> >> generic/534 1s ... 1s
> >> generic/535 1s ... 1s
> >> generic/546 4s ... 4s
> >> generic/547 2s ... 2s
> >> generic/552 1s ... 1s
> >> generic/557 1s ... 1s
> >> generic/588 2s ... 1s
> >> shared/002 8s ... 9s
> >> xfs/011 18s ... 18s
> >> xfs/029 1s ... 1s
> >> xfs/051 18s ... 18s
> >> xfs/057 23s ... 24s
> >> xfs/079 13s ... 14s
> >> xfs/095 2s ... 1s
> >> xfs/119 3s ... 3s
> >> xfs/121 8s ... 6s
> >> xfs/141 18s ... 12s
> >> xfs/181 14s ... 12s
> >> xfs/216 3s ... 2s
> >> xfs/217 2s ... 2s
> >> xfs/439 [not run] test requires XFS bug_on_assert to be off, turn it off to run the test
> >> Ran: generic/034 generic/039 generic/040 generic/041 generic/043 generic/044 generic/045 generic/046 generic/051 generic/052 generic/054 generic/055 generic/056 generic/057 generic/059 generic/065 generic/066 generic/073 generic/090 generic/101 generic/104 generic/106 generic/107 generic/177 generic/311 generic/321 generic/322 generic/325 generic/335 generic/336 generic/341 generic/342 generic/343 generic/376 generic/388 generic/417 generic/455 generic/457 generic/475 generic/479 generic/480 generic/481 generic/483 generic/489 generic/498 generic/501 generic/502 generic/509 generic/510 generic/512 generic/520 generic/526 generic/527 generic/534 generic/535 generic/546 generic/547 generic/552 generic/557 generic/588 shared/002 xfs/011 xfs/029 xfs/051 xfs/057 xfs/079 xfs/095 xfs/119 xfs/121 xfs/141 xfs/181 xfs/216 xfs/217 xfs/439
> >> Not run: generic/455 generic/457 xfs/439
> >> Passed all 74 tests
> >>
> >> Notice that the test results before and after this patch is applied are
> >> identical and successful. Other tests might need to be run in order to
> >> verify everything is working as expected. For such tests, the intervention
> >> of the maintainers might be needed.
> >>
> >> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> >> [2] https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays
> >>
> >> Link: https://github.com/KSPP/linux/issues/79
> >> Build-tested-by: kernel test robot <lkp@intel.com>
> >> Link: https://lore.kernel.org/lkml/603e3177.WsvTXRpcLn5wdYW6%25lkp@intel.com/
> >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> >> ---
> >> fs/xfs/libxfs/xfs_log_format.h | 12 +++++------
> >> fs/xfs/xfs_extfree_item.c | 39 +++++++++++++++-------------------
> >> fs/xfs/xfs_ondisk.h | 8 +++----
> >> 3 files changed, 27 insertions(+), 32 deletions(-)
> >>
> >> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> >> index 8bd00da6d2a4..9934a465b441 100644
> >> --- a/fs/xfs/libxfs/xfs_log_format.h
> >> +++ b/fs/xfs/libxfs/xfs_log_format.h
> >> @@ -574,7 +574,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 {
> >> @@ -582,7 +582,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 {
> >> @@ -590,7 +590,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;
> >>
> >> /*
> >> @@ -603,7 +603,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 {
> >> @@ -611,7 +611,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 {
> >> @@ -619,7 +619,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 93223ebb3372..5ed3ea93071c 100644
> >> --- a/fs/xfs/xfs_extfree_item.c
> >> +++ b/fs/xfs/xfs_extfree_item.c
> >> @@ -73,8 +73,8 @@ 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 - 1) * sizeof(xfs_extent_t);
> >> + return struct_size(&efip->efi_format, efi_extents,
> >> + efip->efi_format.efi_nextents);
> >> }
> >>
> >> STATIC void
> >> @@ -153,17 +153,14 @@ 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 - 1) * sizeof(xfs_extent_t)));
> >> - efip = kmem_zalloc(size, 0);
> >> - } else {
> >> + if (nextents > XFS_EFI_MAX_FAST_EXTENTS)
> >> + efip = kmem_zalloc(struct_size(efip, efi_format.efi_extents,
> >> + nextents), 0);
> >> + else
> >> efip = kmem_cache_zalloc(xfs_efi_zone,
> >> GFP_KERNEL | __GFP_NOFAIL);
> >> - }
> >>
> >> xfs_log_item_init(mp, &efip->efi_item, XFS_LI_EFI, &xfs_efi_item_ops);
> >> efip->efi_format.efi_nextents = nextents;
> >> @@ -186,12 +183,12 @@ 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);
> >> + size_t len = struct_size(src_efi_fmt, efi_extents,
> >> + src_efi_fmt->efi_nextents);
> >> 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);
> >
> > Shouldn't these be converted to struct_size() too?
> >
> > It seems to work all right for casted NULL pointers, and then we get all
> > the typechecking and multiplication overflow checking, e.g.:
> >
> > size_t len64 = struct_size((struct xfs_efi_log_format_32 *)NULL,
> > efi_extents src_efi_fmt->efi_nextents);
>
> Yeah; in that case, what do you think about casting 0, instead of NULL:
>
> uint len32 = struct_size((xfs_efi_log_format_32_t *)0, efi_extents,
> src_efi_fmt->efi_nextents);
> uint len64 = struct_size((xfs_efi_log_format_64_t *)0, efi_extents,
> src_efi_fmt->efi_nextents);
I don't have a preference either way, either here or for the half-dozen
more of these scattered elsewhere in the file.
--D
> Thanks
> --
> Gustavo
>
> >
> > --D
> >
> >> 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);
> >> @@ -254,7 +251,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
> >> @@ -328,14 +325,12 @@ xfs_trans_get_efd(
> >>
> >> ASSERT(nextents > 0);
> >>
> >> - if (nextents > XFS_EFD_MAX_FAST_EXTENTS) {
> >> - efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) +
> >> - (nextents - 1) * sizeof(struct xfs_extent),
> >> - 0);
> >> - } else {
> >> + if (nextents > XFS_EFD_MAX_FAST_EXTENTS)
> >> + efdp = kmem_zalloc(struct_size(efip, efi_format.efi_extents,
> >> + nextents), 0);
> >> + else
> >> efdp = kmem_cache_zalloc(xfs_efd_zone,
> >> GFP_KERNEL | __GFP_NOFAIL);
> >> - }
> >>
> >> xfs_log_item_init(tp->t_mountp, &efdp->efd_item, XFS_LI_EFD,
> >> &xfs_efd_item_ops);
> >> @@ -747,9 +742,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 0aa87c210104..f58e0510385a 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);
> >> --
> >> 2.27.0
> >>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][next] xfs: Replace one-element arrays with flexible-array members
2021-03-09 21:26 ` Darrick J. Wong
@ 2021-03-09 22:03 ` Gustavo A. R. Silva
0 siblings, 0 replies; 11+ messages in thread
From: Gustavo A. R. Silva @ 2021-03-09 22:03 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Gustavo A. R. Silva, Dave Chinner, linux-xfs, linux-kernel,
linux-hardening
On 3/9/21 15:26, Darrick J. Wong wrote:
>>> It seems to work all right for casted NULL pointers, and then we get all
>>> the typechecking and multiplication overflow checking, e.g.:
>>>
>>> size_t len64 = struct_size((struct xfs_efi_log_format_32 *)NULL,
>>> efi_extents src_efi_fmt->efi_nextents);
>> Yeah; in that case, what do you think about casting 0, instead of NULL:
>>
>> uint len32 = struct_size((xfs_efi_log_format_32_t *)0, efi_extents,
>> src_efi_fmt->efi_nextents);
>> uint len64 = struct_size((xfs_efi_log_format_64_t *)0, efi_extents,
>> src_efi_fmt->efi_nextents);
> I don't have a preference either way, either here or for the half-dozen
> more of these scattered elsewhere in the file.
OK. I'll send v2, shortly
Thanks for the feedback!
--
Gustavo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][next] xfs: Replace one-element arrays with flexible-array members
2023-02-03 17:53 ` Kees Cook
@ 2023-02-06 19:17 ` Gustavo A. R. Silva
0 siblings, 0 replies; 11+ messages in thread
From: Gustavo A. R. Silva @ 2023-02-06 19:17 UTC (permalink / raw)
To: Kees Cook, Gustavo A. R. Silva
Cc: Darrick J. Wong, linux-xfs, linux-kernel, linux-hardening, Dave Chinner
On 2/3/23 11:53, Kees Cook wrote:
> On Thu, Feb 02, 2023 at 07:24:50PM -0600, Gustavo A. R. Silva wrote:
>> One-element arrays are deprecated, and we are replacing them with flexible
>> array members instead. So, replace one-element arrays with flexible-array
>> members in structures xfs_attr_leaf_name_local and
>> xfs_attr_leaf_name_remote.
>>
>> The only binary differences reported after the changes are all like
>> these:
>>
>> fs/xfs/libxfs/xfs_attr_leaf.o
>> _@@ -435,7 +435,7 @@
>> 3b8: movzbl 0x2(%rbx),%eax
>> 3bc: rol $0x8,%bp
>> 3c0: movzwl %bp,%ebp
>> - 3c3: lea 0x2(%rax,%rbp,1),%ebx
>> + 3c3: lea 0x3(%rax,%rbp,1),%ebx
>> 3c7: call 3cc <xfs_attr_leaf_entsize+0x8c>
>> 3c8: R_X86_64_PLT32 __tsan_func_exit-0x4
>> 3cc: or $0x3,%ebx
>> _@@ -454,7 +454,7 @@
>> 3ea: movzbl 0x8(%rbx),%ebx
>> 3ee: call 3f3 <xfs_attr_leaf_entsize+0xb3>
>> 3ef: R_X86_64_PLT32 __tsan_func_exit-0x4
>> - 3f3: add $0xa,%ebx
>> + 3f3: add $0xb,%ebx
>> 3f6: or $0x3,%ebx
>> 3f9: add $0x1,%ebx
>> 3fc: mov %ebx,%eax
>>
>> similar changes in fs/xfs/scrub/attr.o and fs/xfs/xfs.o object files.
>
> I usually turn off the sanitizers for the A/B build comparisons to make
Oh yes! that's a good point. I'll see that they are turned off next time. :)
> it easier to read the results. It looks like it _grew_ in size here,
> though?
Yep; I'm sorry I got it wrong. :/ I had it right in the beginning, then after
reading the code once again just before sending out a version of this patch
with only the flex-array transformations, I noticed the entsize functions and
the "sizeof(struct-with-one-element-array) - 1" and I forgot about the padding,
removed the "- 1" and got a bit confused with my build-tests.
I'll send v2 with my original changes... the flex-array transformations, only.
--
Gustavo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][next] xfs: Replace one-element arrays with flexible-array members
2023-02-05 22:51 ` Dave Chinner
@ 2023-02-06 0:21 ` Gustavo A. R. Silva
0 siblings, 0 replies; 11+ messages in thread
From: Gustavo A. R. Silva @ 2023-02-06 0:21 UTC (permalink / raw)
To: Dave Chinner, Gustavo A. R. Silva
Cc: Darrick J. Wong, linux-xfs, linux-kernel, linux-hardening, Kees Cook
On 2/5/23 16:51, Dave Chinner wrote:
> On Thu, Feb 02, 2023 at 07:24:50PM -0600, Gustavo A. R. Silva wrote:
>> One-element arrays are deprecated, and we are replacing them with flexible
>> array members instead. So, replace one-element arrays with flexible-array
>> members in structures xfs_attr_leaf_name_local and
>> xfs_attr_leaf_name_remote.
>>
>> The only binary differences reported after the changes are all like
>> these:
>>
>> fs/xfs/libxfs/xfs_attr_leaf.o
>> _@@ -435,7 +435,7 @@
>> 3b8: movzbl 0x2(%rbx),%eax
>> 3bc: rol $0x8,%bp
>> 3c0: movzwl %bp,%ebp
>> - 3c3: lea 0x2(%rax,%rbp,1),%ebx
>> + 3c3: lea 0x3(%rax,%rbp,1),%ebx
>> 3c7: call 3cc <xfs_attr_leaf_entsize+0x8c>
>> 3c8: R_X86_64_PLT32 __tsan_func_exit-0x4
>> 3cc: or $0x3,%ebx
>> _@@ -454,7 +454,7 @@
>> 3ea: movzbl 0x8(%rbx),%ebx
>> 3ee: call 3f3 <xfs_attr_leaf_entsize+0xb3>
>> 3ef: R_X86_64_PLT32 __tsan_func_exit-0x4
>> - 3f3: add $0xa,%ebx
>> + 3f3: add $0xb,%ebx
>> 3f6: or $0x3,%ebx
>> 3f9: add $0x1,%ebx
>> 3fc: mov %ebx,%eax
>>
>> similar changes in fs/xfs/scrub/attr.o and fs/xfs/xfs.o object files.
>
> That seems like a red flag to me - an off-by-one change in the
> compiled code that calculates of the on-disk size of a structure as
> a result of an in-memory structure change just smells like a bug.
Ughh..
You're right. I somehow got confused between the moment I first
build-tested this in my build machine and after a final last-minute
review I did on the machine from which I ultimately send the patches
out.
More comments below...
>
> How did you test this change?
>
>> And the reason for this is because of the round_up() macro called in
>> functions xfs_attr_leaf_entsize_remote() and xfs_attr_leaf_entsize_local(),
>> which is compensanting for the one-byte reduction in size (due to the
>> flex-array transformation) of structures xfs_attr_leaf_name_remote and
>> xfs_attr_leaf_name_local. So, sizes remain the same before and after
>> changes.
>
> I'm not sure that is true. Before this change:
Yeah; this in fact was a final last-minute review I did before sending out
the patch, and it was when I noticed the round_up() macro was doing something
quite idiomatic when it comes to calculating the sizes of structures containing
one-element arrays. People usually subtract the sizeof(type-of-one-element)
from the sizeof(struct-with-one-element-array) when they perform other
calculations. And in this case as the sizeof(type-of-one-element) is one byte,
at the moment I thought that subtraction was because of that, and then when I
build-tested that final change, I totally forgot about the padding (I had
actually noticed it when I modified the structure definitions :/) and now I
see I got all confused.
>
> sizeof(xfs_attr_leaf_name_local_t) = 4
> sizeof(xfs_attr_leaf_name_remote_t) = 12
>
> After this change:
>
> sizeof(xfs_attr_leaf_name_local_t) = 4
> sizeof(xfs_attr_leaf_name_remote_t) = 12
Yes; in fact I noticed that. :/
>
> i.e. no change because the structures aren't defined as packed
> structures. Hence the compiler pads them to out to 4 byte alignment
> naturally regardless of the flex array definition. pahole on x86-64
> also confirms that the (padded) size of the structure is not
> changed.
Yep; I actually was going to include the pahole output for both structures
in the changelog text, but I decided not to do it at the last minute as
I didn't see it necessary because, as you pointed out, the sizes before
and after the flex-array transformations are the same.
>
> However, the on-disk structure it is being used to decode is packed,
> and we're only using pointer arithmetic to pull the location of the
> name/value pairs out of the buffer to copy them - it's the structure
> size calculations that actually define the size of the structures
> for a given name length, not the sizeof() value or the flex array
> definitions...
>
>> This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
>> routines on memcpy() and help us make progress towards globally
>> enabling -fstrict-flex-arrays=3 [1].
>>
>> Link: https://github.com/KSPP/linux/issues/79
>> Link: https://github.com/KSPP/linux/issues/251
>> Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1]
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>> fs/xfs/libxfs/xfs_da_format.h | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
>> index 25e2841084e1..e1e62ebb0c44 100644
>> --- a/fs/xfs/libxfs/xfs_da_format.h
>> +++ b/fs/xfs/libxfs/xfs_da_format.h
>> @@ -620,14 +620,14 @@ typedef struct xfs_attr_leaf_entry { /* sorted on key, not name */
>> typedef struct xfs_attr_leaf_name_local {
>> __be16 valuelen; /* number of bytes in value */
>> __u8 namelen; /* length of name bytes */
>> - __u8 nameval[1]; /* name/value bytes */
>> + __u8 nameval[]; /* name/value bytes */
>> } xfs_attr_leaf_name_local_t;
>>
>> typedef struct xfs_attr_leaf_name_remote {
>> __be32 valueblk; /* block number of value bytes */
>> __be32 valuelen; /* number of bytes in value */
>> __u8 namelen; /* length of name bytes */
>> - __u8 name[1]; /* name bytes */
>> + __u8 name[]; /* name bytes */
>> } xfs_attr_leaf_name_remote_t;
>>
>> typedef struct xfs_attr_leafblock {
>> @@ -747,13 +747,13 @@ xfs_attr3_leaf_name_local(xfs_attr_leafblock_t *leafp, int idx)
>> */
>> static inline int xfs_attr_leaf_entsize_remote(int nlen)
>> {
>> - return round_up(sizeof(struct xfs_attr_leaf_name_remote) - 1 +
>> + return round_up(sizeof(struct xfs_attr_leaf_name_remote) +
>> nlen, XFS_ATTR_LEAF_NAME_ALIGN);
>> }
>
> To be honest, the actual padding and alignment calculations are
> kinda whacky because that's the way they were defined back in 1995.
> And, well, once set in the on-disk format, it can't easily be
> changed. FYI, here's the original definition from 1995:
>
> #define XFS_ATTR_LEAF_ENTSIZE_REMOTE(nlen) /* space for remote struct */ \
> (((sizeof(xfs_attr_leaf_name_remote_t)-1 + (nlen)) +3)&~0x3)
>
> So apart using round_up and defines instead of magic numbers, the
> current calculation is unchanged from the original definition.
>
> AFAICT, the modification you are proposing above breaks this because the
> sizeof(xfs_attr_leaf_name_remote) result has not changed with the
> change of the structure definition.
>
> e.g. if namelen = 17, before we had:
>
> size = round_up(12 - 1 + 17, 4)
> = round_up(28, 4)
> = 28
>
> Which is correct because the on-disk format is packed:
>
> 0 4 89 12 20 26 28
> +---+---++--+-------+-----+-+-----....
> |---------------| 17 bytes of name.
> |-| 2 bytes of padding
> |-----.... Next attr record.
>
> We end up with 2 bytes of padded between the end of the name and the
> start of the next attribute record in the block.
>
> But after this patch, now we calculate the size as:
>
> size = round_up(12 + 17, 4)
> = round_up(29, 4)
> = 32
>
> Which is a different result, and would result in incorrect parsing
> of the attribute records in the buffer. Hence I don't think it is
> valid to be changing the entsize calculations like this if sizeof()
> is not changing results.
Yep; you're right.
>
> Which comes back to my original question: how did you test this?
I compared the generated object files in fs/xfs/, fs/xfs/scrub/ and
fs/xfs/libxfs/ before and after the changes with something like
these[1]:
ARGS=--disassemble --demangle --reloc --no-show-raw-insn --section=.text
for i in $(cd $OUT/xfs/before && echo *.o); do echo $i; diff -u <(objdump $ARGS $OUT/xfs/before/$i | sed "0,/^Disassembly/d") <(objdump $ARGS $OUT/xfs/after/$i
| sed "0,/^Disassembly/d"); done
where of course the generated object files before the changes are
located in OUT/xfs/before/ and the ones after changes in $OUT/xfs/after/
I just double-checked and, indeed, the changes I mentioned in the
changelog text only show up when I modify the entsize functions.
So, because of the padding, the flex-array transformations don't
actually affect the sizes of the involved structures. So, it seems
that change is enough and is the correct one.
I really appreciate your comments and feedback, Dave. And I'm sorry
for the confusion.
Thank you!
--
Gustavo
[1] https://outflux.net/blog/archives/2022/06/24/finding-binary-differences/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][next] xfs: Replace one-element arrays with flexible-array members
2023-02-03 1:24 Gustavo A. R. Silva
2023-02-03 17:53 ` Kees Cook
2023-02-03 21:32 ` Darrick J. Wong
@ 2023-02-05 22:51 ` Dave Chinner
2023-02-06 0:21 ` Gustavo A. R. Silva
2 siblings, 1 reply; 11+ messages in thread
From: Dave Chinner @ 2023-02-05 22:51 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Darrick J. Wong, linux-xfs, linux-kernel, linux-hardening
On Thu, Feb 02, 2023 at 07:24:50PM -0600, Gustavo A. R. Silva wrote:
> One-element arrays are deprecated, and we are replacing them with flexible
> array members instead. So, replace one-element arrays with flexible-array
> members in structures xfs_attr_leaf_name_local and
> xfs_attr_leaf_name_remote.
>
> The only binary differences reported after the changes are all like
> these:
>
> fs/xfs/libxfs/xfs_attr_leaf.o
> _@@ -435,7 +435,7 @@
> 3b8: movzbl 0x2(%rbx),%eax
> 3bc: rol $0x8,%bp
> 3c0: movzwl %bp,%ebp
> - 3c3: lea 0x2(%rax,%rbp,1),%ebx
> + 3c3: lea 0x3(%rax,%rbp,1),%ebx
> 3c7: call 3cc <xfs_attr_leaf_entsize+0x8c>
> 3c8: R_X86_64_PLT32 __tsan_func_exit-0x4
> 3cc: or $0x3,%ebx
> _@@ -454,7 +454,7 @@
> 3ea: movzbl 0x8(%rbx),%ebx
> 3ee: call 3f3 <xfs_attr_leaf_entsize+0xb3>
> 3ef: R_X86_64_PLT32 __tsan_func_exit-0x4
> - 3f3: add $0xa,%ebx
> + 3f3: add $0xb,%ebx
> 3f6: or $0x3,%ebx
> 3f9: add $0x1,%ebx
> 3fc: mov %ebx,%eax
>
> similar changes in fs/xfs/scrub/attr.o and fs/xfs/xfs.o object files.
That seems like a red flag to me - an off-by-one change in the
compiled code that calculates of the on-disk size of a structure as
a result of an in-memory structure change just smells like a bug.
How did you test this change?
> And the reason for this is because of the round_up() macro called in
> functions xfs_attr_leaf_entsize_remote() and xfs_attr_leaf_entsize_local(),
> which is compensanting for the one-byte reduction in size (due to the
> flex-array transformation) of structures xfs_attr_leaf_name_remote and
> xfs_attr_leaf_name_local. So, sizes remain the same before and after
> changes.
I'm not sure that is true. Before this change:
sizeof(xfs_attr_leaf_name_local_t) = 4
sizeof(xfs_attr_leaf_name_remote_t) = 12
After this change:
sizeof(xfs_attr_leaf_name_local_t) = 4
sizeof(xfs_attr_leaf_name_remote_t) = 12
i.e. no change because the structures aren't defined as packed
structures. Hence the compiler pads them to out to 4 byte alignment
naturally regardless of the flex array definition. pahole on x86-64
also confirms that the (padded) size of the structure is not
changed.
However, the on-disk structure it is being used to decode is packed,
and we're only using pointer arithmetic to pull the location of the
name/value pairs out of the buffer to copy them - it's the structure
size calculations that actually define the size of the structures
for a given name length, not the sizeof() value or the flex array
definitions...
> This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
> routines on memcpy() and help us make progress towards globally
> enabling -fstrict-flex-arrays=3 [1].
>
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/KSPP/linux/issues/251
> Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1]
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> fs/xfs/libxfs/xfs_da_format.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
> index 25e2841084e1..e1e62ebb0c44 100644
> --- a/fs/xfs/libxfs/xfs_da_format.h
> +++ b/fs/xfs/libxfs/xfs_da_format.h
> @@ -620,14 +620,14 @@ typedef struct xfs_attr_leaf_entry { /* sorted on key, not name */
> typedef struct xfs_attr_leaf_name_local {
> __be16 valuelen; /* number of bytes in value */
> __u8 namelen; /* length of name bytes */
> - __u8 nameval[1]; /* name/value bytes */
> + __u8 nameval[]; /* name/value bytes */
> } xfs_attr_leaf_name_local_t;
>
> typedef struct xfs_attr_leaf_name_remote {
> __be32 valueblk; /* block number of value bytes */
> __be32 valuelen; /* number of bytes in value */
> __u8 namelen; /* length of name bytes */
> - __u8 name[1]; /* name bytes */
> + __u8 name[]; /* name bytes */
> } xfs_attr_leaf_name_remote_t;
>
> typedef struct xfs_attr_leafblock {
> @@ -747,13 +747,13 @@ xfs_attr3_leaf_name_local(xfs_attr_leafblock_t *leafp, int idx)
> */
> static inline int xfs_attr_leaf_entsize_remote(int nlen)
> {
> - return round_up(sizeof(struct xfs_attr_leaf_name_remote) - 1 +
> + return round_up(sizeof(struct xfs_attr_leaf_name_remote) +
> nlen, XFS_ATTR_LEAF_NAME_ALIGN);
> }
To be honest, the actual padding and alignment calculations are
kinda whacky because that's the way they were defined back in 1995.
And, well, once set in the on-disk format, it can't easily be
changed. FYI, here's the original definition from 1995:
#define XFS_ATTR_LEAF_ENTSIZE_REMOTE(nlen) /* space for remote struct */ \
(((sizeof(xfs_attr_leaf_name_remote_t)-1 + (nlen)) +3)&~0x3)
So apart using round_up and defines instead of magic numbers, the
current calculation is unchanged from the original definition.
AFAICT, the modification you are proposing above breaks this because the
sizeof(xfs_attr_leaf_name_remote) result has not changed with the
change of the structure definition.
e.g. if namelen = 17, before we had:
size = round_up(12 - 1 + 17, 4)
= round_up(28, 4)
= 28
Which is correct because the on-disk format is packed:
0 4 89 12 20 26 28
+---+---++--+-------+-----+-+-----....
|---------------| 17 bytes of name.
|-| 2 bytes of padding
|-----.... Next attr record.
We end up with 2 bytes of padded between the end of the name and the
start of the next attribute record in the block.
But after this patch, now we calculate the size as:
size = round_up(12 + 17, 4)
= round_up(29, 4)
= 32
Which is a different result, and would result in incorrect parsing
of the attribute records in the buffer. Hence I don't think it is
valid to be changing the entsize calculations like this if sizeof()
is not changing results.
Which comes back to my original question: how did you test this?
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][next] xfs: Replace one-element arrays with flexible-array members
2023-02-03 1:24 Gustavo A. R. Silva
2023-02-03 17:53 ` Kees Cook
@ 2023-02-03 21:32 ` Darrick J. Wong
2023-02-05 22:51 ` Dave Chinner
2 siblings, 0 replies; 11+ messages in thread
From: Darrick J. Wong @ 2023-02-03 21:32 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: linux-xfs, linux-kernel, linux-hardening
On Thu, Feb 02, 2023 at 07:24:50PM -0600, Gustavo A. R. Silva wrote:
> One-element arrays are deprecated, and we are replacing them with flexible
> array members instead. So, replace one-element arrays with flexible-array
> members in structures xfs_attr_leaf_name_local and
> xfs_attr_leaf_name_remote.
>
> The only binary differences reported after the changes are all like
> these:
>
> fs/xfs/libxfs/xfs_attr_leaf.o
> _@@ -435,7 +435,7 @@
> 3b8: movzbl 0x2(%rbx),%eax
> 3bc: rol $0x8,%bp
> 3c0: movzwl %bp,%ebp
> - 3c3: lea 0x2(%rax,%rbp,1),%ebx
> + 3c3: lea 0x3(%rax,%rbp,1),%ebx
> 3c7: call 3cc <xfs_attr_leaf_entsize+0x8c>
> 3c8: R_X86_64_PLT32 __tsan_func_exit-0x4
> 3cc: or $0x3,%ebx
> _@@ -454,7 +454,7 @@
> 3ea: movzbl 0x8(%rbx),%ebx
> 3ee: call 3f3 <xfs_attr_leaf_entsize+0xb3>
> 3ef: R_X86_64_PLT32 __tsan_func_exit-0x4
> - 3f3: add $0xa,%ebx
> + 3f3: add $0xb,%ebx
> 3f6: or $0x3,%ebx
> 3f9: add $0x1,%ebx
> 3fc: mov %ebx,%eax
>
> similar changes in fs/xfs/scrub/attr.o and fs/xfs/xfs.o object files.
>
> And the reason for this is because of the round_up() macro called in
> functions xfs_attr_leaf_entsize_remote() and xfs_attr_leaf_entsize_local(),
> which is compensanting for the one-byte reduction in size (due to the
> flex-array transformation) of structures xfs_attr_leaf_name_remote and
> xfs_attr_leaf_name_local. So, sizes remain the same before and after
> changes.
>
> This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
> routines on memcpy() and help us make progress towards globally
> enabling -fstrict-flex-arrays=3 [1].
>
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/KSPP/linux/issues/251
> Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1]
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> fs/xfs/libxfs/xfs_da_format.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
> index 25e2841084e1..e1e62ebb0c44 100644
> --- a/fs/xfs/libxfs/xfs_da_format.h
> +++ b/fs/xfs/libxfs/xfs_da_format.h
> @@ -620,14 +620,14 @@ typedef struct xfs_attr_leaf_entry { /* sorted on key, not name */
> typedef struct xfs_attr_leaf_name_local {
> __be16 valuelen; /* number of bytes in value */
> __u8 namelen; /* length of name bytes */
> - __u8 nameval[1]; /* name/value bytes */
> + __u8 nameval[]; /* name/value bytes */
> } xfs_attr_leaf_name_local_t;
>
> typedef struct xfs_attr_leaf_name_remote {
> __be32 valueblk; /* block number of value bytes */
> __be32 valuelen; /* number of bytes in value */
> __u8 namelen; /* length of name bytes */
> - __u8 name[1]; /* name bytes */
> + __u8 name[]; /* name bytes */
Does the large comment about m68k problems in xfs_ondisk.h need updating
here?
--D
> } xfs_attr_leaf_name_remote_t;
>
> typedef struct xfs_attr_leafblock {
> @@ -747,13 +747,13 @@ xfs_attr3_leaf_name_local(xfs_attr_leafblock_t *leafp, int idx)
> */
> static inline int xfs_attr_leaf_entsize_remote(int nlen)
> {
> - return round_up(sizeof(struct xfs_attr_leaf_name_remote) - 1 +
> + return round_up(sizeof(struct xfs_attr_leaf_name_remote) +
> nlen, XFS_ATTR_LEAF_NAME_ALIGN);
> }
>
> static inline int xfs_attr_leaf_entsize_local(int nlen, int vlen)
> {
> - return round_up(sizeof(struct xfs_attr_leaf_name_local) - 1 +
> + return round_up(sizeof(struct xfs_attr_leaf_name_local) +
> nlen + vlen, XFS_ATTR_LEAF_NAME_ALIGN);
> }
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][next] xfs: Replace one-element arrays with flexible-array members
2023-02-03 1:24 Gustavo A. R. Silva
@ 2023-02-03 17:53 ` Kees Cook
2023-02-06 19:17 ` Gustavo A. R. Silva
2023-02-03 21:32 ` Darrick J. Wong
2023-02-05 22:51 ` Dave Chinner
2 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2023-02-03 17:53 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Darrick J. Wong, linux-xfs, linux-kernel, linux-hardening
On Thu, Feb 02, 2023 at 07:24:50PM -0600, Gustavo A. R. Silva wrote:
> One-element arrays are deprecated, and we are replacing them with flexible
> array members instead. So, replace one-element arrays with flexible-array
> members in structures xfs_attr_leaf_name_local and
> xfs_attr_leaf_name_remote.
>
> The only binary differences reported after the changes are all like
> these:
>
> fs/xfs/libxfs/xfs_attr_leaf.o
> _@@ -435,7 +435,7 @@
> 3b8: movzbl 0x2(%rbx),%eax
> 3bc: rol $0x8,%bp
> 3c0: movzwl %bp,%ebp
> - 3c3: lea 0x2(%rax,%rbp,1),%ebx
> + 3c3: lea 0x3(%rax,%rbp,1),%ebx
> 3c7: call 3cc <xfs_attr_leaf_entsize+0x8c>
> 3c8: R_X86_64_PLT32 __tsan_func_exit-0x4
> 3cc: or $0x3,%ebx
> _@@ -454,7 +454,7 @@
> 3ea: movzbl 0x8(%rbx),%ebx
> 3ee: call 3f3 <xfs_attr_leaf_entsize+0xb3>
> 3ef: R_X86_64_PLT32 __tsan_func_exit-0x4
> - 3f3: add $0xa,%ebx
> + 3f3: add $0xb,%ebx
> 3f6: or $0x3,%ebx
> 3f9: add $0x1,%ebx
> 3fc: mov %ebx,%eax
>
> similar changes in fs/xfs/scrub/attr.o and fs/xfs/xfs.o object files.
I usually turn off the sanitizers for the A/B build comparisons to make
it easier to read the results. It looks like it _grew_ in size here,
though?
> And the reason for this is because of the round_up() macro called in
> functions xfs_attr_leaf_entsize_remote() and xfs_attr_leaf_entsize_local(),
> which is compensanting for the one-byte reduction in size (due to the
> flex-array transformation) of structures xfs_attr_leaf_name_remote and
> xfs_attr_leaf_name_local. So, sizes remain the same before and after
> changes.
>
> This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
> routines on memcpy() and help us make progress towards globally
> enabling -fstrict-flex-arrays=3 [1].
>
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/KSPP/linux/issues/251
> Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1]
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
If xfstests pass, this seems good to me. Thanks!
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH][next] xfs: Replace one-element arrays with flexible-array members
@ 2023-02-03 1:24 Gustavo A. R. Silva
2023-02-03 17:53 ` Kees Cook
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Gustavo A. R. Silva @ 2023-02-03 1:24 UTC (permalink / raw)
To: Darrick J. Wong
Cc: linux-xfs, linux-kernel, Gustavo A. R. Silva, linux-hardening
One-element arrays are deprecated, and we are replacing them with flexible
array members instead. So, replace one-element arrays with flexible-array
members in structures xfs_attr_leaf_name_local and
xfs_attr_leaf_name_remote.
The only binary differences reported after the changes are all like
these:
fs/xfs/libxfs/xfs_attr_leaf.o
_@@ -435,7 +435,7 @@
3b8: movzbl 0x2(%rbx),%eax
3bc: rol $0x8,%bp
3c0: movzwl %bp,%ebp
- 3c3: lea 0x2(%rax,%rbp,1),%ebx
+ 3c3: lea 0x3(%rax,%rbp,1),%ebx
3c7: call 3cc <xfs_attr_leaf_entsize+0x8c>
3c8: R_X86_64_PLT32 __tsan_func_exit-0x4
3cc: or $0x3,%ebx
_@@ -454,7 +454,7 @@
3ea: movzbl 0x8(%rbx),%ebx
3ee: call 3f3 <xfs_attr_leaf_entsize+0xb3>
3ef: R_X86_64_PLT32 __tsan_func_exit-0x4
- 3f3: add $0xa,%ebx
+ 3f3: add $0xb,%ebx
3f6: or $0x3,%ebx
3f9: add $0x1,%ebx
3fc: mov %ebx,%eax
similar changes in fs/xfs/scrub/attr.o and fs/xfs/xfs.o object files.
And the reason for this is because of the round_up() macro called in
functions xfs_attr_leaf_entsize_remote() and xfs_attr_leaf_entsize_local(),
which is compensanting for the one-byte reduction in size (due to the
flex-array transformation) of structures xfs_attr_leaf_name_remote and
xfs_attr_leaf_name_local. So, sizes remain the same before and after
changes.
This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
routines on memcpy() and help us make progress towards globally
enabling -fstrict-flex-arrays=3 [1].
Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/KSPP/linux/issues/251
Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1]
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
fs/xfs/libxfs/xfs_da_format.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
index 25e2841084e1..e1e62ebb0c44 100644
--- a/fs/xfs/libxfs/xfs_da_format.h
+++ b/fs/xfs/libxfs/xfs_da_format.h
@@ -620,14 +620,14 @@ typedef struct xfs_attr_leaf_entry { /* sorted on key, not name */
typedef struct xfs_attr_leaf_name_local {
__be16 valuelen; /* number of bytes in value */
__u8 namelen; /* length of name bytes */
- __u8 nameval[1]; /* name/value bytes */
+ __u8 nameval[]; /* name/value bytes */
} xfs_attr_leaf_name_local_t;
typedef struct xfs_attr_leaf_name_remote {
__be32 valueblk; /* block number of value bytes */
__be32 valuelen; /* number of bytes in value */
__u8 namelen; /* length of name bytes */
- __u8 name[1]; /* name bytes */
+ __u8 name[]; /* name bytes */
} xfs_attr_leaf_name_remote_t;
typedef struct xfs_attr_leafblock {
@@ -747,13 +747,13 @@ xfs_attr3_leaf_name_local(xfs_attr_leafblock_t *leafp, int idx)
*/
static inline int xfs_attr_leaf_entsize_remote(int nlen)
{
- return round_up(sizeof(struct xfs_attr_leaf_name_remote) - 1 +
+ return round_up(sizeof(struct xfs_attr_leaf_name_remote) +
nlen, XFS_ATTR_LEAF_NAME_ALIGN);
}
static inline int xfs_attr_leaf_entsize_local(int nlen, int vlen)
{
- return round_up(sizeof(struct xfs_attr_leaf_name_local) - 1 +
+ return round_up(sizeof(struct xfs_attr_leaf_name_local) +
nlen + vlen, XFS_ATTR_LEAF_NAME_ALIGN);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-02-06 19:17 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-02 15:05 [PATCH][next] xfs: Replace one-element arrays with flexible-array members Gustavo A. R. Silva
2021-03-09 17:42 ` Darrick J. Wong
2021-03-09 19:57 ` Gustavo A. R. Silva
2021-03-09 21:26 ` Darrick J. Wong
2021-03-09 22:03 ` Gustavo A. R. Silva
2023-02-03 1:24 Gustavo A. R. Silva
2023-02-03 17:53 ` Kees Cook
2023-02-06 19:17 ` Gustavo A. R. Silva
2023-02-03 21:32 ` Darrick J. Wong
2023-02-05 22:51 ` Dave Chinner
2023-02-06 0:21 ` Gustavo A. R. Silva
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).