Fix an issue in the 'fail:' cleanup path of the 'assign_shared_memory()'
function where the use of an unsigned long 'i' with the condition
'--i >= 0' caused an infinite loop. Update the loop to use 'i--',
ensuring correct loop termination.

This change adheres to MISRA C Rule 14.3: "Controlling expressions shall
not be invariant."

Fixes: 72c5fa2208 (device-tree: Move Arm's static-shmem feature to common, 2025-06-03)
Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
---
Changes to v2:
- added Fixes tag
- updated the loop to use 'i--'
- updated commit message accordingly

Link to v1:
https://patchew.org/Xen/d7768394209c65cfef4006c3ffc0a0f5d82c4c18.1756368997.git.dmytro._5Fprokopchuk1@epam.com/
---
 xen/common/device-tree/static-shmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/device-tree/static-shmem.c b/xen/common/device-tree/static-shmem.c
index 8023c0a484..79f23caa77 100644
--- a/xen/common/device-tree/static-shmem.c
+++ b/xen/common/device-tree/static-shmem.c
@@ -185,7 +185,7 @@ static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
     return 0;
 
  fail:
-    while ( --i >= 0 )
+    while ( i-- )
         put_page_nr(page + i, nr_borrowers);
     return ret;
 }
-- 
2.43.0

On 8/28/25 05:39, Dmytro Prokopchuk1 wrote:
> Fix an issue in the 'fail:' cleanup path of the 'assign_shared_memory()'
> function where the use of an unsigned long 'i' with the condition
> '--i >= 0' caused an infinite loop.

I'm not requesting any changes to this patch, but as a separate discussion this
could have been caught with -Wtype-limits:

arch/arm/static-shmem.c: In function ‘assign_shared_memory’:
arch/arm/static-shmem.c:188:17: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
  188 |     while ( --i >= 0 )
      |                 ^~

It might be nice to add this warning flag into the build by default, but there
may be other issues that would need to be addressed first. Having the flag at
least for the release build would be nice, since the debug build may (or may
not) have justifiable occurrences inside of ASSERTs. Again, I don't mean to
increase the scope of work, I'm just making an observation.

If you're curious try it out, add this to your make command:
EXTRA_CFLAGS_XEN_CORE="-Wtype-limits -Wno-error=type-limits"

On 2025-08-28 11:39, Dmytro Prokopchuk1 wrote:
> Fix an issue in the 'fail:' cleanup path of the 
> 'assign_shared_memory()'
> function where the use of an unsigned long 'i' with the condition
> '--i >= 0' caused an infinite loop. Update the loop to use 'i--',
> ensuring correct loop termination.
> 
> This change adheres to MISRA C Rule 14.3: "Controlling expressions 
> shall
> not be invariant."
> 
> Fixes: 72c5fa2208 (device-tree: Move Arm's static-shmem feature to 
> common, 2025-06-03)

The format should be

Fixes: 72c5fa220804 ("device-tree: Move Arm's static-shmem feature to 
common")

can be fixed on commit probably

> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
> ---
> Changes to v2:
> - added Fixes tag
> - updated the loop to use 'i--'
> - updated commit message accordingly
> 
> Link to v1:
> https://patchew.org/Xen/d7768394209c65cfef4006c3ffc0a0f5d82c4c18.1756368997.git.dmytro._5Fprokopchuk1@epam.com/
> ---
>  xen/common/device-tree/static-shmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/device-tree/static-shmem.c 
> b/xen/common/device-tree/static-shmem.c
> index 8023c0a484..79f23caa77 100644
> --- a/xen/common/device-tree/static-shmem.c
> +++ b/xen/common/device-tree/static-shmem.c
> @@ -185,7 +185,7 @@ static int __init assign_shared_memory(struct 
> domain *d, paddr_t gbase,
>      return 0;
> 
>   fail:
> -    while ( --i >= 0 )
> +    while ( i-- )
>          put_page_nr(page + i, nr_borrowers);
>      return ret;
>  }

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

On 28/08/2025 10:47 am, Nicola Vetrini wrote:
> On 2025-08-28 11:39, Dmytro Prokopchuk1 wrote:
>> Fix an issue in the 'fail:' cleanup path of the 'assign_shared_memory()'
>> function where the use of an unsigned long 'i' with the condition
>> '--i >= 0' caused an infinite loop. Update the loop to use 'i--',
>> ensuring correct loop termination.
>>
>> This change adheres to MISRA C Rule 14.3: "Controlling expressions shall
>> not be invariant."
>>
>> Fixes: 72c5fa2208 (device-tree: Move Arm's static-shmem feature to
>> common, 2025-06-03)
>
> The format should be
>
> Fixes: 72c5fa220804 ("device-tree: Move Arm's static-shmem feature to
> common")
>
> can be fixed on commit probably

It can, but that's the wrong Fixes tag.  That commit simply moved the
bad logic.

Commit 041957bad382 ("xen/arm: Add additional reference to owner domain
when the owner is allocated") is the one that introduced the bad expression.

~Andrew