[PATCH 0/9] tlsf: use 8-byte alignment for normal malloc allocations

[PATCH 0/9] tlsf: use 8-byte alignment for normal malloc allocations

[Ahmad Fatoum]

TLSF currently uses only 4-byte alignment on 32-bit platforms, which isn't
enough for ldrd/strd on ARMv7. This series reworks TLSF a bit, so we always
have at least 8 byte alignment.  dlmalloc already has 8 byte alignment
minimum, so nothing to do there.

While this fixes real issues like what Enrico ran into, I'd suggest we only
this be taken into next only after v2022.10.0 is tagged, so this can get
some more testing exposure in the mean time.

Ahmad Fatoum (9):
  test: include <linux/printk.h>
  tlsf: use bselftest for testing ffs/fls
  tlsf: ensure malloc pool is aligned
  tlsf: fix sizeof(size_t) == sizeof(void *) assumption
  tlsf: decouple maximum allocation size from sizeof(size_t)
  tlsf: use 8-byte alignment for normal malloc allocations
  common: malloc: ensure alignment is always at least 8 byte
  test: self: refactor to allow alignment check
  test: self: malloc: fix memory leaks

 common/Kconfig        |  5 +++
 common/dlmalloc.c     |  3 ++
 common/dummy_malloc.c |  2 +-
 common/tlsf.c         | 72 ++++++++++++++++-----------------
 include/bselftest.h   |  1 +
 test/self/malloc.c    | 92 ++++++++++++++++++++++++++++++++-----------
 6 files changed, 114 insertions(+), 61 deletions(-)

-- 
2.30.2

[PATCH 1/9] test: include <linux/printk.h>

[Ahmad Fatoum]

We use pr_print logging in bselftest.h, so include the header that
defines these macros.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 include/bselftest.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/bselftest.h b/include/bselftest.h
index f03c803b6553..58d54c0728ce 100644
--- a/include/bselftest.h
+++ b/include/bselftest.h
@@ -4,6 +4,7 @@
 
 #include <linux/compiler.h>
 #include <linux/list.h>
+#include <linux/printk.h>
 #include <init.h>
 
 enum bselftest_group {
-- 
2.30.2

[PATCH 2/9] tlsf: use bselftest for testing ffs/fls

[Ahmad Fatoum]

The driver has two instances of #ifdef _DEBUG, but when the symbol is
actually defined, we get a build error because of the old-style C
function definition. Fix this and while at it, turn it into a
bselftest.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/tlsf.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/common/tlsf.c b/common/tlsf.c
index 3ca58e3abbfb..8dbb41077cad 100644
--- a/common/tlsf.c
+++ b/common/tlsf.c
@@ -1,14 +1,19 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
+#define pr_fmt(fmt) "tlsf: " fmt
+
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <tlsf.h>
+#include <bselftest.h>
 #include "tlsfbits.h"
 #include <linux/kasan.h>
 
 #define CHAR_BIT	8
 
+BSELFTEST_GLOBALS();
+
 #ifndef CONFIG_KASAN
 #define __memcpy memcpy
 #endif
@@ -870,11 +875,13 @@ void tlsf_remove_pool(tlsf_t tlsf, pool_t pool)
 ** TLSF main interface.
 */
 
-#ifdef _DEBUG
-int test_ffs_fls()
+static int test_tlfs_ffs_fls(void)
 {
-	/* Verify ffs/fls work properly. */
 	int rv = 0;
+
+	total_tests = 8;
+
+	/* Verify ffs/fls work properly. */
 	rv += (tlsf_ffs(0) == -1) ? 0 : 0x1;
 	rv += (tlsf_fls(0) == -1) ? 0 : 0x2;
 	rv += (tlsf_ffs(1) == 0) ? 0 : 0x4;
@@ -885,28 +892,21 @@ int test_ffs_fls()
 	rv += (tlsf_fls(0x7FFFFFFF) == 30) ? 0 : 0x80;
 
 #if defined (TLSF_64BIT)
+	total_tests += 3;
 	rv += (tlsf_fls_sizet(0x80000000) == 31) ? 0 : 0x100;
 	rv += (tlsf_fls_sizet(0x100000000) == 32) ? 0 : 0x200;
 	rv += (tlsf_fls_sizet(0xffffffffffffffff) == 63) ? 0 : 0x400;
+#else
+	skipped_tests += 3;
 #endif
 
-	if (rv)
-	{
-		printf("test_ffs_fls: %x ffs/fls tests failed.\n", rv);
-	}
-	return rv;
+	failed_tests = rv;
+	return 0;
 }
-#endif
+bselftest(core, test_tlfs_ffs_fls);
 
 tlsf_t tlsf_create(void* mem)
 {
-#ifdef _DEBUG
-	if (test_ffs_fls())
-	{
-		return 0;
-	}
-#endif
-
 	if (((tlsfptr_t)mem % ALIGN_SIZE) != 0)
 	{
 		printf("tlsf_create: Memory must be aligned to %u bytes.\n",
-- 
2.30.2

[PATCH 3/9] tlsf: ensure malloc pool is aligned

[Ahmad Fatoum]

The struct control_t describing a pool is allocated at its very start
and then directly followed by the first block. To ensure the first block
is suitably aligned, align_up the size in tlsf_size(). So far, TLSF on
32-bit and 64-bit happened to be aligned, so this introduces no
functional change just yet. With upcoming changes to the block header to
increase alignment on 32-bit systems, this realignment will become required.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/tlsf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/tlsf.c b/common/tlsf.c
index 8dbb41077cad..f8892dafbb7f 100644
--- a/common/tlsf.c
+++ b/common/tlsf.c
@@ -770,11 +770,11 @@ int tlsf_check_pool(pool_t pool)
 
 /*
 ** Size of the TLSF structures in a given memory block passed to
-** tlsf_create, equal to the size of a control_t
+** tlsf_create, equal to aligned size of a control_t
 */
 size_t tlsf_size(void)
 {
-	return sizeof(control_t);
+	return align_up(sizeof(control_t), ALIGN_SIZE);
 }
 
 size_t tlsf_align_size(void)
-- 
2.30.2

[PATCH 4/9] tlsf: fix sizeof(size_t) == sizeof(void *) assumption

[Ahmad Fatoum]

TLSF struct block_header_t doesn't describe a single block, but
instead its first member covers the previous block:

                              .~~~~~~~~~~~~~~~~~~~.
                              |  prev_phys_block  |
    End of previous block --> |———————————————————| <-- Start of a free block
                              |       size        |
                              |— — — — — — — — — —|
                              | < Start of Data > |
                              '———————————————————'

This works because if the previous block is free, there is no harm in
using its last word to store the prev_phys_block.

We thus need pointer arithmetic to:

  - arrive from start of data to size, i.e. decrement offset
    by sizeof(size_t)

  - arrive from size to prev_phys_block, i.e. decrement offset
    by sizeof(struct block_header_t *)

Across the TLSF implementation, we conflate the two though and use
block_header_shift to mean both.  This works as long as
sizeof(size_t) == sizeof(struct block_header_t *), which is true
for both 32-bit and 64-bit configuration currently.

To facilitate having an 8-byte minimum allocation alignment for 32-bit
systems as well, we will increase sizeof(struct block_header_t::size)
to 8 bytes, which will break the implicit assumption. Fix it by adding
an additional const block_header_shift and use it where appropriate.

No functional change just yet.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/tlsf.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/common/tlsf.c b/common/tlsf.c
index f8892dafbb7f..83d469ae0a25 100644
--- a/common/tlsf.c
+++ b/common/tlsf.c
@@ -146,11 +146,12 @@ static const size_t block_header_prev_free_bit = 1 << 1;
 ** The size of the block header exposed to used blocks is the size field.
 ** The prev_phys_block field is stored *inside* the previous free block.
 */
+static const size_t block_header_shift = offsetof(block_header_t, size);
 static const size_t block_header_overhead = sizeof(size_t);
 
 /* User data starts directly after the size field in a used block. */
 static const size_t block_start_offset =
-	offsetof(block_header_t, size) + sizeof(size_t);
+	block_header_shift + block_header_overhead;
 
 /*
 ** A free block must be large enough to store its header minus the size of
@@ -258,7 +259,7 @@ static block_header_t* block_prev(const block_header_t* block)
 static block_header_t* block_next(const block_header_t* block)
 {
 	block_header_t* next = offset_to_block(block_to_ptr(block),
-		block_size(block) - block_header_overhead);
+		block_size(block) - block_header_shift);
 	tlsf_assert(!block_is_last(block));
 	return next;
 }
@@ -469,7 +470,7 @@ static block_header_t* block_split(block_header_t* block, size_t size)
 {
 	/* Calculate the amount of space left in the remaining block. */
 	block_header_t* remaining =
-		offset_to_block(block_to_ptr(block), size - block_header_overhead);
+		offset_to_block(block_to_ptr(block), size - block_header_shift);
 
 	const size_t remain_size = block_size(block) - (size + block_header_overhead);
 
@@ -735,7 +736,7 @@ void tlsf_walk_pool(pool_t pool, tlsf_walker walker, void* user)
 {
 	tlsf_walker pool_walker = walker ? walker : default_walker;
 	block_header_t* block =
-		offset_to_block(pool, -(int)block_header_overhead);
+		offset_to_block(pool, -(int)block_header_shift);
 
 	while (block && !block_is_last(block))
 	{
@@ -841,7 +842,7 @@ pool_t tlsf_add_pool(tlsf_t tlsf, void* mem, size_t bytes)
 	** so that the prev_phys_block field falls outside of the pool -
 	** it will never be used.
 	*/
-	block = offset_to_block(mem, -(tlsfptr_t)block_header_overhead);
+	block = offset_to_block(mem, -(tlsfptr_t)block_header_shift);
 	block_set_size(block, pool_bytes);
 	block_set_free(block);
 	block_set_prev_used(block);
@@ -859,7 +860,7 @@ pool_t tlsf_add_pool(tlsf_t tlsf, void* mem, size_t bytes)
 void tlsf_remove_pool(tlsf_t tlsf, pool_t pool)
 {
 	control_t* control = tlsf_cast(control_t*, tlsf);
-	block_header_t* block = offset_to_block(pool, -(int)block_header_overhead);
+	block_header_t* block = offset_to_block(pool, -(int)block_header_shift);
 
 	int fl = 0, sl = 0;
 
@@ -982,7 +983,7 @@ void* tlsf_memalign(tlsf_t tlsf, size_t align, size_t size)
 	block = block_locate_free(control, aligned_size);
 
 	/* This can't be a static assert. */
-	tlsf_assert(sizeof(block_header_t) == block_size_min + block_header_overhead);
+	tlsf_assert(sizeof(block_header_t) == block_size_min + block_header_shift);
 
 	if (block)
 	{
-- 
2.30.2

[PATCH 5/9] tlsf: decouple maximum allocation size from sizeof(size_t)

[Ahmad Fatoum]

Previous commit ensures that the first block is aligned to ALIGN_SIZE
and the implementation already ensured that sizes are rounded up to
multiples of ALIGN_SIZE.

However, each block starts with a size_t holding the block size. On
systems with sizeof(size_t) == 4, this means even if ALIGN_SIZE were
8, we would end up with an unaligned buffer.

The straight-forward fix for that is to increase the TLSF per-block
overhead to be 8 bytes per allocation, even on 32-bit systems.
That way alignment is naturally maintained. Prepare for this by
replacing references to the block size size_t type with new
tlsf_size_t.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/tlsf.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/common/tlsf.c b/common/tlsf.c
index 83d469ae0a25..16682435e492 100644
--- a/common/tlsf.c
+++ b/common/tlsf.c
@@ -95,10 +95,12 @@ enum tlsf_private
 #define tlsf_static_assert(exp) \
 	typedef char _tlsf_glue(static_assert, __LINE__) [(exp) ? 1 : -1]
 
+typedef size_t tlsf_size_t;
+
 /* This code has been tested on 32- and 64-bit (LP/LLP) architectures. */
 tlsf_static_assert(sizeof(int) * CHAR_BIT == 32);
-tlsf_static_assert(sizeof(size_t) * CHAR_BIT >= 32);
-tlsf_static_assert(sizeof(size_t) * CHAR_BIT <= 64);
+tlsf_static_assert(sizeof(tlsf_size_t) * CHAR_BIT >= 32);
+tlsf_static_assert(sizeof(tlsf_size_t) * CHAR_BIT <= 64);
 
 /* SL_INDEX_COUNT must be <= number of bits in sl_bitmap's storage type. */
 tlsf_static_assert(sizeof(unsigned int) * CHAR_BIT >= SL_INDEX_COUNT);
@@ -126,7 +128,7 @@ typedef struct block_header_t
 	struct block_header_t* prev_phys_block;
 
 	/* The size of this block, excluding the block header. */
-	size_t size;
+	tlsf_size_t size;
 
 	/* Next and previous free blocks. */
 	struct block_header_t* next_free;
@@ -147,7 +149,7 @@ static const size_t block_header_prev_free_bit = 1 << 1;
 ** The prev_phys_block field is stored *inside* the previous free block.
 */
 static const size_t block_header_shift = offsetof(block_header_t, size);
-static const size_t block_header_overhead = sizeof(size_t);
+static const size_t block_header_overhead = sizeof(tlsf_size_t);
 
 /* User data starts directly after the size field in a used block. */
 static const size_t block_start_offset =
@@ -989,7 +991,7 @@ void* tlsf_memalign(tlsf_t tlsf, size_t align, size_t size)
 	{
 		void* ptr = block_to_ptr(block);
 		void* aligned = align_ptr(ptr, align);
-		size_t gap = tlsf_cast(size_t,
+		tlsf_size_t gap = tlsf_cast(tlsf_size_t,
 			tlsf_cast(tlsfptr_t, aligned) - tlsf_cast(tlsfptr_t, ptr));
 
 		/* If gap size is too small, offset to next aligned boundary. */
@@ -1001,7 +1003,7 @@ void* tlsf_memalign(tlsf_t tlsf, size_t align, size_t size)
 				tlsf_cast(tlsfptr_t, aligned) + offset);
 
 			aligned = align_ptr(next_aligned, align);
-			gap = tlsf_cast(size_t,
+			gap = tlsf_cast(tlsf_size_t,
 				tlsf_cast(tlsfptr_t, aligned) - tlsf_cast(tlsfptr_t, ptr));
 		}
 
-- 
2.30.2

[PATCH 6/9] tlsf: use 8-byte alignment for normal malloc allocations

[Ahmad Fatoum]

The current alignment of 4 bytes is too low. Access to 64-bit data via
ldrd/strd requires at least an eight byte alignment:

  | Prior to ARMv6, if the memory address is not 64-bit aligned, the
  | data read from memory is UNPREDICTABLE. Alignment checking (taking
  | a data abort), and support for a big-endian (BE-32) data format are
  | implementation options.

We already have at least an 8 byte alignment for dlmalloc, so have TLSF
follow suit.

Reported-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
Link: https://lore.barebox.org/barebox/ly7d1z1qvs.fsf@ensc-pc.intern.sigma-chemnitz.de/
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/tlsf.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/common/tlsf.c b/common/tlsf.c
index 16682435e492..561635dfd3f3 100644
--- a/common/tlsf.c
+++ b/common/tlsf.c
@@ -35,13 +35,8 @@ enum tlsf_public
 /* Private constants: do not modify. */
 enum tlsf_private
 {
-#if defined (TLSF_64BIT)
 	/* All allocation sizes and addresses are aligned to 8 bytes. */
 	ALIGN_SIZE_LOG2 = 3,
-#else
-	/* All allocation sizes and addresses are aligned to 4 bytes. */
-	ALIGN_SIZE_LOG2 = 2,
-#endif
 	ALIGN_SIZE = (1 << ALIGN_SIZE_LOG2),
 
 	/*
@@ -95,7 +90,7 @@ enum tlsf_private
 #define tlsf_static_assert(exp) \
 	typedef char _tlsf_glue(static_assert, __LINE__) [(exp) ? 1 : -1]
 
-typedef size_t tlsf_size_t;
+typedef uint64_t tlsf_size_t;
 
 /* This code has been tested on 32- and 64-bit (LP/LLP) architectures. */
 tlsf_static_assert(sizeof(int) * CHAR_BIT == 32);
-- 
2.30.2

[PATCH 7/9] common: malloc: ensure alignment is always at least 8 byte

[Ahmad Fatoum]

We used to have following alignments:
		32-bit CPU	64-bit CPU
   dummy	8 bytes		 8 bytes
   dlmalloc	8 bytes		16 bytes
   tlsf		4 bytes		 8 bytes

With recent change to TLSF, we now always have at least 8 bytes as
alignment. To make this clearer, define a new CONFIG_MALLOC_ALIGNMENT
and either use it as the alignment (as done for dummy) or add static
asserts to ensure we have at least this alignment.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/Kconfig        | 5 +++++
 common/dlmalloc.c     | 3 +++
 common/dummy_malloc.c | 2 +-
 common/tlsf.c         | 2 ++
 4 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/common/Kconfig b/common/Kconfig
index 43dd92b08a3d..758f3c29286f 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -276,6 +276,11 @@ config MALLOC_SIZE
 	hex
 	default 0x400000
 	prompt "malloc area size"
+
+config MALLOC_ALIGNMENT
+	hex
+	default 8
+
 endmenu
 
 config BROKEN
diff --git a/common/dlmalloc.c b/common/dlmalloc.c
index ae10c9ae30dd..16ea3cafb624 100644
--- a/common/dlmalloc.c
+++ b/common/dlmalloc.c
@@ -3,6 +3,7 @@
 #include <malloc.h>
 #include <string.h>
 #include <memory.h>
+#include <linux/build_bug.h>
 
 #include <stdio.h>
 #include <module.h>
@@ -884,6 +885,8 @@ static mbinptr av_[NAV * 2 + 2] = {
 
 /*  Other static bookkeeping data */
 
+static_assert(MALLOC_ALIGNMENT >= CONFIG_MALLOC_ALIGNMENT);
+
 /* variables holding tunable values */
 #ifndef __BAREBOX__
 static unsigned long trim_threshold = DEFAULT_TRIM_THRESHOLD;
diff --git a/common/dummy_malloc.c b/common/dummy_malloc.c
index d99b5059cf91..7a96afec76e0 100644
--- a/common/dummy_malloc.c
+++ b/common/dummy_malloc.c
@@ -23,7 +23,7 @@ void *memalign(size_t alignment, size_t bytes)
 
 void *malloc(size_t size)
 {
-	return memalign(8, size);
+	return memalign(CONFIG_MALLOC_ALIGNMENT, size);
 }
 
 void free(void *ptr)
diff --git a/common/tlsf.c b/common/tlsf.c
index 561635dfd3f3..d5debbc2a3da 100644
--- a/common/tlsf.c
+++ b/common/tlsf.c
@@ -103,6 +103,8 @@ tlsf_static_assert(sizeof(unsigned int) * CHAR_BIT >= SL_INDEX_COUNT);
 /* Ensure we've properly tuned our sizes. */
 tlsf_static_assert(ALIGN_SIZE == SMALL_BLOCK_SIZE / SL_INDEX_COUNT);
 
+tlsf_static_assert(ALIGN_SIZE >= CONFIG_MALLOC_ALIGNMENT);
+
 /*
 ** Data structures and associated constants.
 */
-- 
2.30.2

[PATCH 8/9] test: self: refactor to allow alignment check

[Ahmad Fatoum]

Have The expect_alloc_* functions currently only know whether the
pointer is NULL or not. Have them get the full pointer value and return
it instead.

No functional change.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 test/self/malloc.c | 78 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 57 insertions(+), 21 deletions(-)

diff --git a/test/self/malloc.c b/test/self/malloc.c
index c25b416b9751..6b3002ff0996 100644
--- a/test/self/malloc.c
+++ b/test/self/malloc.c
@@ -7,26 +7,62 @@
 #include <malloc.h>
 #include <memory.h>
 #include <linux/sizes.h>
+#include <linux/bitops.h>
 
 BSELFTEST_GLOBALS();
 
-static void __expect(bool cond, bool expect,
+#define get_alignment(val) \
+	BIT(__builtin_constant_p(val) ?  __builtin_ffsll(val) : __ffs64(val))
+
+static_assert(get_alignment(0x1)	!= 1);
+static_assert(get_alignment(0x2)	!= 2);
+static_assert(get_alignment(0x3)	!= 1);
+static_assert(get_alignment(0x4)	!= 4);
+static_assert(get_alignment(0x5)	!= 1);
+static_assert(get_alignment(0x6)	!= 2);
+static_assert(get_alignment(0x8)	!= 8);
+static_assert(get_alignment(0x99)	!= 0x1);
+static_assert(get_alignment(0xDEADBEE0)	!= 0x10);
+
+static bool __expect_cond(bool cond, bool expect,
+			  const char *condstr, const char *func, int line)
+{
+	total_tests++;
+	if (cond == expect)
+		return true;
+
+	failed_tests++;
+	printf("%s:%d: %s to %s\n", func, line,
+	       expect ? "failed" : "unexpectedly succeeded",
+	       condstr);
+	return false;
+
+}
+
+static void *__expect(void *ptr, bool expect,
 		     const char *condstr, const char *func, int line)
 {
+	bool ok;
 	total_tests++;
-	if (cond != expect) {
-		failed_tests++;
-		printf("%s:%d: %s to %s\n", func, line,
-		       expect ? "failed" : "unexpectedly succeeded",
-		       condstr);
+
+	ok = __expect_cond(ptr != NULL, expect, condstr, func, line);
+	if (ok && ptr) {
+		unsigned alignment = get_alignment((uintptr_t)ptr);
+		if (alignment < CONFIG_MALLOC_ALIGNMENT) {
+			failed_tests++;
+			printf("%s:%d: invalid alignment of %u in %s = %p\n", func, line,
+			       alignment, condstr, ptr);
+		}
 	}
+
+	return ptr;
 }
 
-#define expect_alloc_ok(cond) \
-	__expect((cond), true, #cond, __func__, __LINE__)
+#define expect_alloc_ok(ptr) \
+	__expect((ptr), true, #ptr, __func__, __LINE__)
 
-#define expect_alloc_fail(cond) \
-	__expect((cond), false, #cond, __func__, __LINE__)
+#define expect_alloc_fail(ptr) \
+	__expect((ptr), false, #ptr, __func__, __LINE__)
 
 static void test_malloc(void)
 {
@@ -45,14 +81,14 @@ static void test_malloc(void)
 		mem_malloc_size = 0;
 	}
 
-	expect_alloc_ok(p = malloc(1));
+	p = expect_alloc_ok(malloc(1));
 	free(p);
 
 	if (mem_malloc_size) {
 		expect_alloc_fail(malloc(SIZE_MAX));
 
 		if (0xf0000000 > mem_malloc_size) {
-			expect_alloc_fail((tmp = malloc(0xf0000000)));
+			tmp = expect_alloc_fail(malloc(0xf0000000));
 			free(tmp);
 		}
 	} else {
@@ -60,22 +96,22 @@ static void test_malloc(void)
 	}
 
 	p = realloc(NULL, 1);
-	expect_alloc_ok(p = realloc(NULL, 1));
+	p = expect_alloc_ok(realloc(NULL, 1));
 
 	*p = 0x42;
 
-	expect_alloc_ok(tmp = realloc(p, 2));
+	tmp = expect_alloc_ok(realloc(p, 2));
 
 	p = tmp;
-	__expect(*p == 0x42, true, "reread after realloc", __func__, __LINE__);
+	__expect_cond(*p == 0x42, true, "reread after realloc", __func__, __LINE__);
 
 	if (mem_malloc_size) {
-		expect_alloc_fail(tmp = realloc(p, mem_malloc_size));
+		tmp = expect_alloc_fail(realloc(p, mem_malloc_size));
 
 		if (0xf0000000 > mem_malloc_size)
-			expect_alloc_fail((tmp = realloc(p, 0xf0000000)));
+			tmp = expect_alloc_fail(realloc(p, 0xf0000000));
 
-		expect_alloc_fail(tmp = realloc(p, SIZE_MAX));
+		tmp = expect_alloc_fail(realloc(p, SIZE_MAX));
 
 	} else {
 		skipped_tests += 3;
@@ -83,9 +119,9 @@ static void test_malloc(void)
 
 	free(p);
 
-	expect_alloc_ok(p = malloc(0));
-	expect_alloc_ok(tmp = malloc(0));
+	p = expect_alloc_ok(malloc(0));
+	tmp = expect_alloc_ok(malloc(0));
 
-	__expect(p != tmp, true, "allocate distinct 0-size buffers", __func__, __LINE__);
+	__expect_cond(p != tmp, true, "allocate distinct 0-size buffers", __func__, __LINE__);
 }
 bselftest(core, test_malloc);
-- 
2.30.2

[PATCH 9/9] test: self: malloc: fix memory leaks

[Ahmad Fatoum]

The test shouldn't leak memory, even if it fails. Fix the leaks.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 test/self/malloc.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/test/self/malloc.c b/test/self/malloc.c
index 6b3002ff0996..47c225ac6a10 100644
--- a/test/self/malloc.c
+++ b/test/self/malloc.c
@@ -85,7 +85,8 @@ static void test_malloc(void)
 	free(p);
 
 	if (mem_malloc_size) {
-		expect_alloc_fail(malloc(SIZE_MAX));
+		tmp = expect_alloc_fail(malloc(SIZE_MAX));
+		free(tmp);
 
 		if (0xf0000000 > mem_malloc_size) {
 			tmp = expect_alloc_fail(malloc(0xf0000000));
@@ -95,7 +96,7 @@ static void test_malloc(void)
 		skipped_tests += 2;
 	}
 
-	p = realloc(NULL, 1);
+	free(realloc(NULL, 1));
 	p = expect_alloc_ok(realloc(NULL, 1));
 
 	*p = 0x42;
@@ -107,11 +108,15 @@ static void test_malloc(void)
 
 	if (mem_malloc_size) {
 		tmp = expect_alloc_fail(realloc(p, mem_malloc_size));
+		free(tmp);
 
-		if (0xf0000000 > mem_malloc_size)
+		if (0xf0000000 > mem_malloc_size) {
 			tmp = expect_alloc_fail(realloc(p, 0xf0000000));
+			free(tmp);
+		}
 
 		tmp = expect_alloc_fail(realloc(p, SIZE_MAX));
+		free(tmp);
 
 	} else {
 		skipped_tests += 3;
@@ -123,5 +128,8 @@ static void test_malloc(void)
 	tmp = expect_alloc_ok(malloc(0));
 
 	__expect_cond(p != tmp, true, "allocate distinct 0-size buffers", __func__, __LINE__);
+
+	free(p);
+	free(tmp);
 }
 bselftest(core, test_malloc);
-- 
2.30.2

Re: [PATCH 0/9] tlsf: use 8-byte alignment for normal malloc allocations

[Enrico Scholz]

Ahmad Fatoum <a.fatoum@pengutronix.de> writes:

> TLSF currently uses only 4-byte alignment on 32-bit platforms, which isn't
> enough for ldrd/strd on ARMv7. This series reworks TLSF a bit, so we always
> have at least 8 byte alignment.  dlmalloc already has 8 byte alignment
> minimum, so nothing to do there.

I am wondering whether alignment should be increased on 64 bit archs to
16 bytes as well.  ARMv8 spec [1] says

| exclusive pair access must be aligned to twice the data size, that is,
| 128 bits for a pair of 64-bit values.

A github issue [2] mentions this alignment too.


> While this fixes real issues like what Enrico ran into, I'd suggest we only
> this be taken into next only after v2022.10.0 is tagged,

This is ok for me; the issue disappeared with reverting the zstd patch.



Enrico

Footnotes:
[1]  https://developer.arm.com/documentation/den0024/a/An-Introduction-to-the-ARMv8-Instruction-Sets/The-ARMv8-instruction-sets/Addressing

[2]  https://github.com/mattconte/tlsf/issues/16

-- 
SIGMA Chemnitz GmbH       Registergericht:   Amtsgericht Chemnitz HRB 1750
Am Erlenwald 13           Geschaeftsfuehrer: Grit Freitag, Frank Pyritz
09128 Chemnitz

Re: [PATCH 0/9] tlsf: use 8-byte alignment for normal malloc allocations

[Ahmad Fatoum]

Hello Enrico,

On 04.10.22 18:23, Enrico Scholz wrote:
> Ahmad Fatoum <a.fatoum@pengutronix.de> writes:
> 
>> TLSF currently uses only 4-byte alignment on 32-bit platforms, which isn't
>> enough for ldrd/strd on ARMv7. This series reworks TLSF a bit, so we always
>> have at least 8 byte alignment.  dlmalloc already has 8 byte alignment
>> minimum, so nothing to do there.
> 
> I am wondering whether alignment should be increased on 64 bit archs to
> 16 bytes as well.  ARMv8 spec [1] says
> 
> | exclusive pair access must be aligned to twice the data size, that is,
> | 128 bits for a pair of 64-bit values.
> 
> A github issue [2] mentions this alignment too.

Your quote is addressing exclusive pair access, which I think is something
a compiler would generate for lock-free __int128 access, not something that
we should encounter in barebox.

But yes, I believe we should have 16-byte alignment on 64-bit systems with
TLSF as we already do with dlmalloc, if only to be on the safe side.

This is unfortunately more complicated than what this PR does with 32-bit,
so I am leaving that as future exercise (or until we have a real world
example where this is required in barebox).

>> While this fixes real issues like what Enrico ran into, I'd suggest we only
>> this be taken into next only after v2022.10.0 is tagged,
> 
> This is ok for me; the issue disappeared with reverting the zstd patch.

Feel free to give it a test with the zstd patch reverted though. :)

Thanks,
Ahmad

> 
> 
> 
> Enrico
> 
> Footnotes:
> [1]  https://developer.arm.com/documentation/den0024/a/An-Introduction-to-the-ARMv8-Instruction-Sets/The-ARMv8-instruction-sets/Addressing
> 
> [2]  https://github.com/mattconte/tlsf/issues/16
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 0/9] tlsf: use 8-byte alignment for normal malloc allocations

[Sascha Hauer]

On Tue, Oct 04, 2022 at 05:53:58PM +0200, Ahmad Fatoum wrote:
> TLSF currently uses only 4-byte alignment on 32-bit platforms, which isn't
> enough for ldrd/strd on ARMv7. This series reworks TLSF a bit, so we always
> have at least 8 byte alignment.  dlmalloc already has 8 byte alignment
> minimum, so nothing to do there.
> 
> While this fixes real issues like what Enrico ran into, I'd suggest we only
> this be taken into next only after v2022.10.0 is tagged, so this can get
> some more testing exposure in the mean time.
> 
> Ahmad Fatoum (9):
>   test: include <linux/printk.h>
>   tlsf: use bselftest for testing ffs/fls
>   tlsf: ensure malloc pool is aligned
>   tlsf: fix sizeof(size_t) == sizeof(void *) assumption
>   tlsf: decouple maximum allocation size from sizeof(size_t)
>   tlsf: use 8-byte alignment for normal malloc allocations
>   common: malloc: ensure alignment is always at least 8 byte
>   test: self: refactor to allow alignment check
>   test: self: malloc: fix memory leaks

Applied, thanks

Sascha

> 
>  common/Kconfig        |  5 +++
>  common/dlmalloc.c     |  3 ++
>  common/dummy_malloc.c |  2 +-
>  common/tlsf.c         | 72 ++++++++++++++++-----------------
>  include/bselftest.h   |  1 +
>  test/self/malloc.c    | 92 ++++++++++++++++++++++++++++++++-----------
>  6 files changed, 114 insertions(+), 61 deletions(-)
> 
> -- 
> 2.30.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 0/9] tlsf: use 8-byte alignment for normal malloc allocations

[Ahmad Fatoum]

On 14.10.22 10:54, Sascha Hauer wrote:
> On Tue, Oct 04, 2022 at 05:53:58PM +0200, Ahmad Fatoum wrote:
>> TLSF currently uses only 4-byte alignment on 32-bit platforms, which isn't
>> enough for ldrd/strd on ARMv7. This series reworks TLSF a bit, so we always
>> have at least 8 byte alignment.  dlmalloc already has 8 byte alignment
>> minimum, so nothing to do there.
>>
>> While this fixes real issues like what Enrico ran into, I'd suggest we only
>> this be taken into next only after v2022.10.0 is tagged, so this can get
>> some more testing exposure in the mean time.
>>
>> Ahmad Fatoum (9):
>>   test: include <linux/printk.h>
>>   tlsf: use bselftest for testing ffs/fls
>>   tlsf: ensure malloc pool is aligned
>>   tlsf: fix sizeof(size_t) == sizeof(void *) assumption
>>   tlsf: decouple maximum allocation size from sizeof(size_t)
>>   tlsf: use 8-byte alignment for normal malloc allocations
>>   common: malloc: ensure alignment is always at least 8 byte
>>   test: self: refactor to allow alignment check
>>   test: self: malloc: fix memory leaks
> 
> Applied, thanks

Please drop series again. Reported breaking boot on i.MX6Q.
Need to revisit.

> 
> Sascha
> 
>>
>>  common/Kconfig        |  5 +++
>>  common/dlmalloc.c     |  3 ++
>>  common/dummy_malloc.c |  2 +-
>>  common/tlsf.c         | 72 ++++++++++++++++-----------------
>>  include/bselftest.h   |  1 +
>>  test/self/malloc.c    | 92 ++++++++++++++++++++++++++++++++-----------
>>  6 files changed, 114 insertions(+), 61 deletions(-)
>>
>> -- 
>> 2.30.2
>>
>>
>>
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |