[PATCH v2 14/14] fs: Fix a confusing error about overlapping regions
Heinrich Schuchardt
xypron.glpk at gmx.de
Fri Sep 1 08:23:36 CEST 2023
On 9/1/23 03:13, Simon Glass wrote:
> When lmb runs out of space in its internal tables, it gives errors on
> every fs load operation. This is horribly confusing, as the poor user
> tries different memory regions one at a time.
>
> Use the updated API to check the error code and print a helpful message.
> Also, allow the operation to proceed.
>
> Update the tests accordingly.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
> (no changes since v1)
>
> fs/fs.c | 7 ++++++-
> include/lmb.h | 19 ++++++++++++++++++-
> lib/lmb.c | 20 +++++++++++---------
> test/lib/lmb.c | 42 ++++++++++++++++++++----------------------
> 4 files changed, 55 insertions(+), 33 deletions(-)
>
> diff --git a/fs/fs.c b/fs/fs.c
> index 2b815b1db0fe..1a84cb410bf9 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -569,8 +569,13 @@ static int fs_read_lmb_check(const char *filename, ulong addr, loff_t offset,
> lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob);
> lmb_dump_all(&lmb);
>
> - if (lmb_alloc_addr(&lmb, addr, read_len) == addr)
> + ret = lmb_alloc_addr(&lmb, addr, read_len, NULL);
> + if (!ret)
> return 0;
> + if (ret == -E2BIG) {
> + log_warning("Reservation tables exhausted: see CONFIG_LMB_USE_MAX_REGIONS\n");
> + return 0;
This is dangerous:
You cannot check if a load operation will override protected memory but
still continue.
Please, return -E2BIG here.
Best regards
Heinrich
> + }
>
> log_err("** Reading file would overwrite reserved memory **\n");
> return -ENOSPC;
> diff --git a/include/lmb.h b/include/lmb.h
> index c81716b3f1d1..f28a872c78d0 100644
> --- a/include/lmb.h
> +++ b/include/lmb.h
> @@ -114,7 +114,24 @@ phys_addr_t lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align,
> phys_addr_t max_addr);
> phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align,
> phys_addr_t max_addr);
> -phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base, phys_size_t size);
> +
> +/**
> + * lmb_alloc_addr() - Allocate memory within a region
> + *
> + * Try to allocate a specific address range: must be in defined memory but not
> + * reserved
> + *
> + * @lmb: the logical memory block struct
> + * @base: base address of the memory region
> + * @size: size of the memory region
> + * @addrp: if non-NULL, returns the allocated address, on success
> + * Return: 0 if OK, -EPERM if the memory is already allocated, -E2BIG if
> + * there is not enough room in the reservation tables, so
> + * CONFIG_LMB_USE_MAX_REGIONS should be increased
> + */
> +int lmb_alloc_addr(struct lmb *lmb, phys_addr_t base, phys_size_t size,
> + phys_addr_t *addrp);
> +
> phys_size_t lmb_get_free_size(struct lmb *lmb, phys_addr_t addr);
>
> /**
> diff --git a/lib/lmb.c b/lib/lmb.c
> index fd6326cbf7fe..c3d14db07e45 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -252,7 +252,7 @@ void lmb_init_and_reserve_range(struct lmb *lmb, phys_addr_t base,
> * @size: size of the memory region to add
> * @flags: flags for this new memory region
> * Returns: 0 if OK, -EBUSY if an existing enveloping region has different
> - * flags, -EPERM if there is an existing non-adjacent region, -ENOSPC if there
> + * flags, -EPERM if there is an existing non-adjacent region, -E2BIG if there
> * is no more room in the list of regions, ekse region number that was coalesced
> * with this one
> **/
> @@ -318,7 +318,7 @@ static int lmb_add_area_flags(struct lmb_region *rgn, phys_addr_t base,
> if (coalesced)
> return coalesced;
> if (rgn->cnt >= rgn->max)
> - return -ENOSPC;
> + return -E2BIG;
>
> /* Couldn't coalesce the LMB, so add it to the sorted table. */
> for (i = rgn->cnt-1; i >= 0; i--) {
> @@ -511,11 +511,8 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align,
> return 0;
> }
>
> -/*
> - * Try to allocate a specific address range: must be in defined memory but not
> - * reserved
> - */
> -phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base, phys_size_t size)
> +int lmb_alloc_addr(struct lmb *lmb, phys_addr_t base, phys_size_t size,
> + phys_addr_t *addrp)
> {
> int area;
>
> @@ -529,9 +526,14 @@ phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base, phys_size_t size)
> if (lmb_addrs_overlap(lmb->memory.area[area].base,
> lmb->memory.area[area].size,
> base + size - 1, 1)) {
> + int ret;
> +
> /* ok, reserve the memory */
> - if (lmb_reserve(lmb, base, size) >= 0)
> - return base;
> + ret = lmb_reserve(lmb, base, size);
> + if (ret < 0)
> + return ret;
> + if (addrp)
> + *addrp = base;
> }
> }
>
> diff --git a/test/lib/lmb.c b/test/lib/lmb.c
> index b665d8dcdeb4..c8dbdb3b73eb 100644
> --- a/test/lib/lmb.c
> +++ b/test/lib/lmb.c
> @@ -497,22 +497,22 @@ static int test_alloc_addr(struct unit_test_state *uts, const phys_addr_t ram)
> alloc_addr_b, 0x10000, alloc_addr_c, 0x10000);
>
> /* allocate blocks */
> - a = lmb_alloc_addr(&lmb, ram, alloc_addr_a - ram);
> + ut_assertok(lmb_alloc_addr(&lmb, ram, alloc_addr_a - ram, &a));
> ut_asserteq(a, ram);
> ASSERT_LMB(&lmb, ram, ram_size, 3, ram, 0x8010000,
> alloc_addr_b, 0x10000, alloc_addr_c, 0x10000);
> - b = lmb_alloc_addr(&lmb, alloc_addr_a + 0x10000,
> - alloc_addr_b - alloc_addr_a - 0x10000);
> + ut_assertok(lmb_alloc_addr(&lmb, alloc_addr_a + 0x10000,
> + alloc_addr_b - alloc_addr_a - 0x10000, &b));
> ut_asserteq(b, alloc_addr_a + 0x10000);
> ASSERT_LMB(&lmb, ram, ram_size, 2, ram, 0x10010000,
> alloc_addr_c, 0x10000, 0, 0);
> - c = lmb_alloc_addr(&lmb, alloc_addr_b + 0x10000,
> - alloc_addr_c - alloc_addr_b - 0x10000);
> + ut_assertok(lmb_alloc_addr(&lmb, alloc_addr_b + 0x10000,
> + alloc_addr_c - alloc_addr_b - 0x10000, &c));
> ut_asserteq(c, alloc_addr_b + 0x10000);
> ASSERT_LMB(&lmb, ram, ram_size, 1, ram, 0x18010000,
> 0, 0, 0, 0);
> - d = lmb_alloc_addr(&lmb, alloc_addr_c + 0x10000,
> - ram_end - alloc_addr_c - 0x10000);
> + ut_assertok(lmb_alloc_addr(&lmb, alloc_addr_c + 0x10000,
> + ram_end - alloc_addr_c - 0x10000, &d));
> ut_asserteq(d, alloc_addr_c + 0x10000);
> ASSERT_LMB(&lmb, ram, ram_size, 1, ram, ram_size,
> 0, 0, 0, 0);
> @@ -528,7 +528,7 @@ static int test_alloc_addr(struct unit_test_state *uts, const phys_addr_t ram)
>
> /* allocate at 3 points in free range */
>
> - d = lmb_alloc_addr(&lmb, ram_end - 4, 4);
> + ut_assertok(lmb_alloc_addr(&lmb, ram_end - 4, 4, &d));
> ut_asserteq(d, ram_end - 4);
> ASSERT_LMB(&lmb, ram, ram_size, 2, ram, 0x18010000,
> d, 4, 0, 0);
> @@ -537,7 +537,7 @@ static int test_alloc_addr(struct unit_test_state *uts, const phys_addr_t ram)
> ASSERT_LMB(&lmb, ram, ram_size, 1, ram, 0x18010000,
> 0, 0, 0, 0);
>
> - d = lmb_alloc_addr(&lmb, ram_end - 128, 4);
> + ut_assertok(lmb_alloc_addr(&lmb, ram_end - 128, 4, &d));
> ut_asserteq(d, ram_end - 128);
> ASSERT_LMB(&lmb, ram, ram_size, 2, ram, 0x18010000,
> d, 4, 0, 0);
> @@ -546,7 +546,7 @@ static int test_alloc_addr(struct unit_test_state *uts, const phys_addr_t ram)
> ASSERT_LMB(&lmb, ram, ram_size, 1, ram, 0x18010000,
> 0, 0, 0, 0);
>
> - d = lmb_alloc_addr(&lmb, alloc_addr_c + 0x10000, 4);
> + ut_assertok(lmb_alloc_addr(&lmb, alloc_addr_c + 0x10000, 4, &d));
> ut_asserteq(d, alloc_addr_c + 0x10000);
> ASSERT_LMB(&lmb, ram, ram_size, 1, ram, 0x18010004,
> 0, 0, 0, 0);
> @@ -560,19 +560,19 @@ static int test_alloc_addr(struct unit_test_state *uts, const phys_addr_t ram)
> ut_asserteq(ret, 0);
> ASSERT_LMB(&lmb, ram, ram_size, 1, ram + 0x8000000, 0x10010000,
> 0, 0, 0, 0);
> - d = lmb_alloc_addr(&lmb, ram, 4);
> + ut_assertok(lmb_alloc_addr(&lmb, ram, 4, &d));
> ut_asserteq(d, ram);
> ASSERT_LMB(&lmb, ram, ram_size, 2, d, 4,
> ram + 0x8000000, 0x10010000, 0, 0);
>
> /* check that allocating outside memory fails */
> if (ram_end != 0) {
> - ret = lmb_alloc_addr(&lmb, ram_end, 1);
> - ut_asserteq(ret, 0);
> + ut_assertok(lmb_alloc_addr(&lmb, ram_end, 1, &a));
> + ut_asserteq(0x40000000, a);
> }
> if (ram != 0) {
> - ret = lmb_alloc_addr(&lmb, ram - 1, 1);
> - ut_asserteq(ret, 0);
> + ut_assertok(lmb_alloc_addr(&lmb, ram - 1, 1, &a));
> + ut_asserteq(ram, a);
> }
>
> return 0;
> @@ -588,7 +588,7 @@ static int lib_test_lmb_alloc_addr(struct unit_test_state *uts)
> return ret;
>
> /* simulate 512 MiB RAM beginning at 1.5GiB */
> - return test_alloc_addr(uts, 0xE0000000);
> + return test_alloc_addr(uts, 0xe0000000);
> }
>
> DM_TEST(lib_test_lmb_alloc_addr, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
> @@ -699,8 +699,7 @@ static int lib_test_LMB_MAX_AREAS(struct unit_test_state *uts)
>
> /* error for the (CONFIG_LMB_MAX_AREAS + 1) memory regions */
> offset = ram + 2 * (CONFIG_LMB_MAX_AREAS + 1) * ram_size;
> - ret = lmb_add(&lmb, offset, ram_size);
> - ut_asserteq(ret, -1);
> + ut_asserteq(-E2BIG, lmb_add(&lmb, offset, ram_size));
>
> ut_asserteq(lmb.memory.cnt, CONFIG_LMB_MAX_AREAS);
> ut_asserteq(lmb.reserved.cnt, 0);
> @@ -717,8 +716,7 @@ static int lib_test_LMB_MAX_AREAS(struct unit_test_state *uts)
>
> /* error for the 9th reserved blocks */
> offset = ram + 2 * (CONFIG_LMB_MAX_AREAS + 1) * blk_size;
> - ret = lmb_reserve(&lmb, offset, blk_size);
> - ut_asserteq(ret, -1);
> + ut_asserteq(-E2BIG, lmb_reserve(&lmb, offset, blk_size));
>
> ut_asserteq(lmb.memory.cnt, CONFIG_LMB_MAX_AREAS);
> ut_asserteq(lmb.reserved.cnt, CONFIG_LMB_MAX_AREAS);
> @@ -762,8 +760,8 @@ static int lib_test_lmb_flags(struct unit_test_state *uts)
> 0, 0, 0, 0);
>
> /* reserve again, new flag */
> - ret = lmb_reserve_flags(&lmb, 0x40010000, 0x10000, LMB_NONE);
> - ut_asserteq(ret, -1);
> + ut_asserteq(-EBUSY,
> + lmb_reserve_flags(&lmb, 0x40010000, 0x10000, LMB_NONE));
> ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000,
> 0, 0, 0, 0);
>
More information about the U-Boot
mailing list