[PATCH v2 0/2] lmb: Fix reserving the same region multiple times

[PATCH v2 0/2] lmb: Fix reserving the same region multiple times

[Sam Protsenko]

This series is intended to be picked up for the v2025.01 release.

Since commit 1d9aa4a283da ("lmb: Fix the allocation of overlapping
memory areas with !LMB_NONE") the lmb_add_region_flags() returns -1 when
the caller tries to add the already existing region with !LMB_NONE
flags (it was returning 0 before that patch). That causes
boot_fdt_reserve_region() function to print erroneous error messages
when it's called consequently more than one time.

Make lmb_add_region_flags() return -EEXIST when the already added region
with !LMB_NONE flags is being added, and then check that error code in
boot_fdt_reserve_region() to avoid printing the misleading error
messages.

This series fixes all 3 found cases (discussed at [1]) for the false
positive error messages. All unit tests pass in sandbox U-Boot.

Changes in v2:
  - Split the original series by two separate series (cleanups will be
    sent out as a different one)
  - Collected all R-b and A-b tags from review
  - Patch #1: Removed the check for exactly the same region, and return
    -EEXIST in the branch handling overlapping regions instead
  - Reworded the commit messages slightly

[1] https://lists.denx.de/pipermail/u-boot/2024-December/574123.html

Sam Protsenko (2):
  lmb: Return -EEXIST in lmb_add_region_flags() if region already added
  boot: fdt: Handle already reserved memory in boot_fdt_reserve_region()

 boot/image-fdt.c |  2 +-
 lib/lmb.c        | 26 +++++++++++++-------------
 test/lib/lmb.c   |  2 +-
 3 files changed, 15 insertions(+), 15 deletions(-)

-- 
2.39.5

[PATCH v2 1/2] lmb: Return -EEXIST in lmb_add_region_flags() if region already added

[Sam Protsenko]

An attempt to add the already added LMB region using
lmb_add_region_flags() ends up in lmb_addrs_overlap() check, which
eventually leads to either returning 0 if 'flags' is LMB_NONE, or -1
otherwise. It makes it impossible for the user of this function to catch
the case when the region is already added and differentiate it from
regular errors. That in turn may lead to incorrect error handling in the
caller code, like reporting misleading errors or interrupting the normal
code path where it could be treated as the normal case. An example is
boot_fdt_reserve_region() function, which might be called twice (e.g.
during board startup in initr_lmb(), and then during 'booti' command
booting the OS), thus trying to reserve exactly the same memory regions
described in the device tree twice, which produces an error message on
second call.

Return -EEXIST error code in case when the added region exists and it's
not LMB_NONE; for LMB_NONE return 0, to conform to unit tests
(specifically test_alloc_addr() in test/lib/lmb.c) and the preferred
behavior described in commit 1d9aa4a283da ("lmb: Fix the allocation of
overlapping memory areas with !LMB_NONE"). The change of
lmb_add_region_flags() return values is described in the table below:

    Return case                        Pre-1d9   1d9    New
    -----------------------------------------------------------
    Added successfully                    0      0      0
    Failed to add                         -1     -1     -1
    Already added, flags == LMB_NONE      0      0      0
    Already added, flags != LMB_NONE      0      -1     -EEXIST

Rework all affected functions and their documentation. Also fix the
corresponding unit test which checks reserving the same region with the
same flags to account for the changed return value.

No functional change is intended (by this patch itself).

Fixes: 1d9aa4a283da ("lmb: Fix the allocation of overlapping memory areas with !LMB_NONE")
Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - Removed the check for exactly the same region, and return -EEXIST in
    the branch handling overlapping regions instead
  - Reworded the commit message accordingly

 lib/lmb.c      | 26 +++++++++++++-------------
 test/lib/lmb.c |  2 +-
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/lib/lmb.c b/lib/lmb.c
index b03237bc06cb..a695edf70dfa 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -183,8 +183,10 @@ static long lmb_resize_regions(struct alist *lmb_rgn_lst,
  * the function might resize an already existing region or coalesce two
  * adjacent regions.
  *
- *
- * Returns: 0 if the region addition successful, -1 on failure
+ * Return:
+ * * %0		- Added successfully, or it's already added (only if LMB_NONE)
+ * * %-EEXIST	- The region is already added, and flags != LMB_NONE
+ * * %-1	- Failure
  */
 static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base,
 				 phys_size_t size, enum lmb_flags flags)
@@ -217,17 +219,15 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base,
 			coalesced++;
 			break;
 		} else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) {
-			if (flags == LMB_NONE) {
-				ret = lmb_resize_regions(lmb_rgn_lst, i, base,
-							 size);
-				if (ret < 0)
-					return -1;
+			if (flags != LMB_NONE)
+				return -EEXIST;
 
-				coalesced++;
-				break;
-			} else {
+			ret = lmb_resize_regions(lmb_rgn_lst, i, base, size);
+			if (ret < 0)
 				return -1;
-			}
+
+			coalesced++;
+			break;
 		}
 	}
 
@@ -667,7 +667,7 @@ long lmb_add(phys_addr_t base, phys_size_t size)
  *
  * Free up a region of memory.
  *
- * Return: 0 if successful, -1 on failure
+ * Return: 0 if successful, negative error code on failure
  */
 long lmb_free_flags(phys_addr_t base, phys_size_t size,
 		    uint flags)
@@ -818,7 +818,7 @@ static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size,
 				      lmb_memory[rgn].size,
 				      base + size - 1, 1)) {
 			/* ok, reserve the memory */
-			if (lmb_reserve_flags(base, size, flags) >= 0)
+			if (!lmb_reserve_flags(base, size, flags))
 				return base;
 		}
 	}
diff --git a/test/lib/lmb.c b/test/lib/lmb.c
index 0bd29e2a4fe7..48c3c966f8f2 100644
--- a/test/lib/lmb.c
+++ b/test/lib/lmb.c
@@ -754,7 +754,7 @@ static int lib_test_lmb_flags(struct unit_test_state *uts)
 
 	/* reserve again, same flag */
 	ret = lmb_reserve_flags(0x40010000, 0x10000, LMB_NOMAP);
-	ut_asserteq(ret, -1L);
+	ut_asserteq(ret, -EEXIST);
 	ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x10000,
 		   0, 0, 0, 0);
 
-- 
2.39.5

Re: [PATCH v2 1/2] lmb: Return -EEXIST in lmb_add_region_flags() if region already added

[Ilias Apalodimas]

Thanks Sam!

On Wed, 11 Dec 2024 at 04:17, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> An attempt to add the already added LMB region using
> lmb_add_region_flags() ends up in lmb_addrs_overlap() check, which
> eventually leads to either returning 0 if 'flags' is LMB_NONE, or -1
> otherwise. It makes it impossible for the user of this function to catch
> the case when the region is already added and differentiate it from
> regular errors. That in turn may lead to incorrect error handling in the
> caller code, like reporting misleading errors or interrupting the normal
> code path where it could be treated as the normal case. An example is
> boot_fdt_reserve_region() function, which might be called twice (e.g.
> during board startup in initr_lmb(), and then during 'booti' command
> booting the OS), thus trying to reserve exactly the same memory regions
> described in the device tree twice, which produces an error message on
> second call.
>
> Return -EEXIST error code in case when the added region exists and it's
> not LMB_NONE; for LMB_NONE return 0, to conform to unit tests
> (specifically test_alloc_addr() in test/lib/lmb.c) and the preferred
> behavior described in commit 1d9aa4a283da ("lmb: Fix the allocation of
> overlapping memory areas with !LMB_NONE"). The change of
> lmb_add_region_flags() return values is described in the table below:
>
>     Return case                        Pre-1d9   1d9    New
>     -----------------------------------------------------------
>     Added successfully                    0      0      0
>     Failed to add                         -1     -1     -1
>     Already added, flags == LMB_NONE      0      0      0
>     Already added, flags != LMB_NONE      0      -1     -EEXIST
>
> Rework all affected functions and their documentation. Also fix the
> corresponding unit test which checks reserving the same region with the
> same flags to account for the changed return value.
>
> No functional change is intended (by this patch itself).
>
> Fixes: 1d9aa4a283da ("lmb: Fix the allocation of overlapping memory areas with !LMB_NONE")
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - Removed the check for exactly the same region, and return -EEXIST in
>     the branch handling overlapping regions instead
>   - Reworded the commit message accordingly
>
>  lib/lmb.c      | 26 +++++++++++++-------------
>  test/lib/lmb.c |  2 +-
>  2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/lib/lmb.c b/lib/lmb.c
> index b03237bc06cb..a695edf70dfa 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -183,8 +183,10 @@ static long lmb_resize_regions(struct alist *lmb_rgn_lst,
>   * the function might resize an already existing region or coalesce two
>   * adjacent regions.
>   *
> - *
> - * Returns: 0 if the region addition successful, -1 on failure
> + * Return:
> + * * %0                - Added successfully, or it's already added (only if LMB_NONE)
> + * * %-EEXIST  - The region is already added, and flags != LMB_NONE
> + * * %-1       - Failure
>   */
>  static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base,
>                                  phys_size_t size, enum lmb_flags flags)
> @@ -217,17 +219,15 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base,
>                         coalesced++;
>                         break;
>                 } else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) {
> -                       if (flags == LMB_NONE) {
> -                               ret = lmb_resize_regions(lmb_rgn_lst, i, base,
> -                                                        size);
> -                               if (ret < 0)
> -                                       return -1;
> +                       if (flags != LMB_NONE)
> +                               return -EEXIST;
>
> -                               coalesced++;
> -                               break;
> -                       } else {
> +                       ret = lmb_resize_regions(lmb_rgn_lst, i, base, size);
> +                       if (ret < 0)
>                                 return -1;
> -                       }
> +
> +                       coalesced++;
> +                       break;
>                 }
>         }
>
> @@ -667,7 +667,7 @@ long lmb_add(phys_addr_t base, phys_size_t size)
>   *
>   * Free up a region of memory.
>   *
> - * Return: 0 if successful, -1 on failure
> + * Return: 0 if successful, negative error code on failure
>   */
>  long lmb_free_flags(phys_addr_t base, phys_size_t size,
>                     uint flags)
> @@ -818,7 +818,7 @@ static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size,
>                                       lmb_memory[rgn].size,
>                                       base + size - 1, 1)) {
>                         /* ok, reserve the memory */
> -                       if (lmb_reserve_flags(base, size, flags) >= 0)
> +                       if (!lmb_reserve_flags(base, size, flags))
>                                 return base;
>                 }
>         }
> diff --git a/test/lib/lmb.c b/test/lib/lmb.c
> index 0bd29e2a4fe7..48c3c966f8f2 100644
> --- a/test/lib/lmb.c
> +++ b/test/lib/lmb.c
> @@ -754,7 +754,7 @@ static int lib_test_lmb_flags(struct unit_test_state *uts)
>
>         /* reserve again, same flag */
>         ret = lmb_reserve_flags(0x40010000, 0x10000, LMB_NOMAP);
> -       ut_asserteq(ret, -1L);
> +       ut_asserteq(ret, -EEXIST);
>         ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x10000,
>                    0, 0, 0, 0);
>
> --
> 2.39.5
>

Tom this needs to go in -master for the release
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH v2 2/2] boot: fdt: Handle already reserved memory in boot_fdt_reserve_region()

[Sam Protsenko]

The boot_fdt_add_mem_rsv_regions() function can be called twice, e.g.
first time during the board init (as a part of LMB init), and then when
booting the OS with 'booti' command:

    lmb_add_region_flags
    lmb_reserve_flags
    boot_fdt_reserve_region
    boot_fdt_add_mem_rsv_regions
               ^
               |
               +-----------------------+
               | (1)                   | (2)
    lmb_reserve_common        image_setup_linux
    lmb_init                  ...
    initr_lmb                 do_booti
    board_init_r              'booti'

That consequently leads to the attempt of reserving the same memory
areas (described in the 'reserved-memory' dts node) in LMB. The
lmb_add_region_flags() returns -EEXIST error code in such cases, but
boot_fdt_reserve_region() handles all negative error codes as a failure
to reserve fdt memory region, printing corresponding error messages,
which are essentially harmless, but misleading. For example, this is the
output of 'booti' command on E850-96 board:

    => booti $loadaddr - $fdtaddr
    ...
    ERROR: reserving fdt memory region failed
           (addr=bab00000 size=5500000 flags=2)
    ERROR: reserving fdt memory region failed
           (addr=f0000000 size=200000 flags=4)
    ...
    Starting kernel ...

The mentioned false positive error messages are observed starting with
commit 1d9aa4a283da ("lmb: Fix the allocation of overlapping memory
areas with !LMB_NONE"), which removes the check for the already added
memory regions in lmb_add_region_flags(), making it return -1 for
!LMB_NONE cases. Another commit 827dee587b75 ("fdt: lmb: add reserved
regions as no-overwrite") changes flags used for reserving memory in
boot_fdt_add_mem_rsv_regions() from LMB_NONE to LMB_NOOVERWRITE. So
together with the patch mentioned earlier, it makes
lmb_add_region_flags() return -1 when called from
boot_fdt_reserve_region().

Since then, the different patch was implemented, returning -EEXIST error
code in described cases, which is:

   lmb: Return -EEXIST in lmb_add_region_flags() if region already added

Handle -EEXIST error code as a normal (successful) case in
lmb_reserve_flags() and don't print any messages.

Fixes: 1d9aa4a283da ("lmb: Fix the allocation of overlapping memory areas with !LMB_NONE")
Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Changes in v2:
  - Added R-b tag from Ilias
  - Reworded the commit message a bit, reflecting changes in the lmb patch

 boot/image-fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/boot/image-fdt.c b/boot/image-fdt.c
index 3d5b6f9e2dc7..73c43c30684f 100644
--- a/boot/image-fdt.c
+++ b/boot/image-fdt.c
@@ -77,7 +77,7 @@ static void boot_fdt_reserve_region(u64 addr, u64 size, enum lmb_flags flags)
 		debug("   reserving fdt memory region: addr=%llx size=%llx flags=%x\n",
 		      (unsigned long long)addr,
 		      (unsigned long long)size, flags);
-	} else {
+	} else if (ret != -EEXIST) {
 		puts("ERROR: reserving fdt memory region failed ");
 		printf("(addr=%llx size=%llx flags=%x)\n",
 		       (unsigned long long)addr,
-- 
2.39.5

Re: [PATCH v2 0/2] lmb: Fix reserving the same region multiple times

[Michal Simek]

st 11. 12. 2024 v 3:17 odesílatel Sam Protsenko
<semen.protsenko@linaro.org> napsal:
>
> This series is intended to be picked up for the v2025.01 release.
>
> Since commit 1d9aa4a283da ("lmb: Fix the allocation of overlapping
> memory areas with !LMB_NONE") the lmb_add_region_flags() returns -1 when
> the caller tries to add the already existing region with !LMB_NONE
> flags (it was returning 0 before that patch). That causes
> boot_fdt_reserve_region() function to print erroneous error messages
> when it's called consequently more than one time.
>
> Make lmb_add_region_flags() return -EEXIST when the already added region
> with !LMB_NONE flags is being added, and then check that error code in
> boot_fdt_reserve_region() to avoid printing the misleading error
> messages.
>
> This series fixes all 3 found cases (discussed at [1]) for the false
> positive error messages. All unit tests pass in sandbox U-Boot.
>
> Changes in v2:
>   - Split the original series by two separate series (cleanups will be
>     sent out as a different one)
>   - Collected all R-b and A-b tags from review
>   - Patch #1: Removed the check for exactly the same region, and return
>     -EEXIST in the branch handling overlapping regions instead
>   - Reworded the commit messages slightly
>
> [1] https://lists.denx.de/pipermail/u-boot/2024-December/574123.html
>
> Sam Protsenko (2):
>   lmb: Return -EEXIST in lmb_add_region_flags() if region already added
>   boot: fdt: Handle already reserved memory in boot_fdt_reserve_region()
>
>  boot/image-fdt.c |  2 +-
>  lib/lmb.c        | 26 +++++++++++++-------------
>  test/lib/lmb.c   |  2 +-
>  3 files changed, 15 insertions(+), 15 deletions(-)
>
> --
> 2.39.5
>

Tested-by: Michal Simek <michal.simek@amd.com>

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

Re: [PATCH v2 0/2] lmb: Fix reserving the same region multiple times

[Tom Rini]

On Tue, 10 Dec 2024 20:17:00 -0600, Sam Protsenko wrote:

> This series is intended to be picked up for the v2025.01 release.
> 
> Since commit 1d9aa4a283da ("lmb: Fix the allocation of overlapping
> memory areas with !LMB_NONE") the lmb_add_region_flags() returns -1 when
> the caller tries to add the already existing region with !LMB_NONE
> flags (it was returning 0 before that patch). That causes
> boot_fdt_reserve_region() function to print erroneous error messages
> when it's called consequently more than one time.
> 
> [...]

Applied to u-boot/master, thanks!

-- 
Tom