[PATCH] linux-user: make sure brk(0) returns a page-aligned value

[PATCH] linux-user: make sure brk(0) returns a page-aligned value

[Andreas Schwab]

Fixes: 86f04735ac ("linux-user: Fix brk() to release pages")
Signed-off-by: Andreas Schwab <schwab@suse.de>
---
 linux-user/syscall.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 08162cc966..e8a17377f5 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -805,7 +805,7 @@ static abi_ulong brk_page;
 
 void target_set_brk(abi_ulong new_brk)
 {
-    target_brk = new_brk;
+    target_brk = TARGET_PAGE_ALIGN(new_brk);
     brk_page = HOST_PAGE_ALIGN(target_brk);
 }
 
-- 
2.41.0


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] linux-user: make sure brk(0) returns a page-aligned value

[Richard Henderson]

On 7/6/23 12:34, Andreas Schwab wrote:
> Fixes: 86f04735ac ("linux-user: Fix brk() to release pages")
> Signed-off-by: Andreas Schwab <schwab@suse.de>
> ---
>   linux-user/syscall.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 08162cc966..e8a17377f5 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -805,7 +805,7 @@ static abi_ulong brk_page;
>   
>   void target_set_brk(abi_ulong new_brk)
>   {
> -    target_brk = new_brk;
> +    target_brk = TARGET_PAGE_ALIGN(new_brk);
>       brk_page = HOST_PAGE_ALIGN(target_brk);
>   }
>   

It make sense, since that's how do_brk aligns things.
I'm curious why this error might have produced host memory clobbering, but I'm not going 
to debug that.

Queuing for tcg/linux-user.


r~

Re: [PATCH] linux-user: make sure brk(0) returns a page-aligned value

[Helge Deller]

On 7/8/23 19:26, Richard Henderson wrote:
> On 7/6/23 12:34, Andreas Schwab wrote:
>> Fixes: 86f04735ac ("linux-user: Fix brk() to release pages")
>> Signed-off-by: Andreas Schwab <schwab@suse.de>
>> ---
>>   linux-user/syscall.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 08162cc966..e8a17377f5 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -805,7 +805,7 @@ static abi_ulong brk_page;
>>   void target_set_brk(abi_ulong new_brk)
>>   {
>> -    target_brk = new_brk;
>> +    target_brk = TARGET_PAGE_ALIGN(new_brk);
>>       brk_page = HOST_PAGE_ALIGN(target_brk);
>>   }
>
> It make sense, since that's how do_brk aligns things.

Yes, patch looks good.
I haven't tested, but it seems it adjusts the initial brk(0) value
only to make sure that it's target page aligned.
Maybe the title should be: ?
linux-user: make sure the initial brk(0) is page-aligned

> I'm curious why this error might have produced host memory clobbering, but I'm not going to debug that.

I don't believe that this un-alignment triggers host memory clobbering either.

Helge

Re: [PATCH] linux-user: make sure brk(0) returns a page-aligned value

[Helge Deller]

On 7/8/23 23:36, Helge Deller wrote:
> On 7/8/23 19:26, Richard Henderson wrote:
>> On 7/6/23 12:34, Andreas Schwab wrote:
>>> Fixes: 86f04735ac ("linux-user: Fix brk() to release pages")
>>> Signed-off-by: Andreas Schwab <schwab@suse.de>
>>> ---
>>>   linux-user/syscall.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index 08162cc966..e8a17377f5 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -805,7 +805,7 @@ static abi_ulong brk_page;
>>>   void target_set_brk(abi_ulong new_brk)
>>>   {
>>> -    target_brk = new_brk;
>>> +    target_brk = TARGET_PAGE_ALIGN(new_brk);
>>>       brk_page = HOST_PAGE_ALIGN(target_brk);
>>>   }
...
>> I'm curious why this error might have produced host memory clobbering, but I'm not going to debug that.
> I don't believe that this un-alignment triggers host memory clobbering either.

See my follow-up in the other mail threadd:
"Re: [RISC-V] ERROR:../accel/tcg/cpu-exec.c:1028:cpu_exec_setjmp: assertion failed: (cpu == current_cpu)"

Helge

Re: [PATCH] linux-user: make sure brk(0) returns a page-aligned value

[Helge Deller]

On 7/8/23 23:36, Helge Deller wrote:
> On 7/8/23 19:26, Richard Henderson wrote:
>> On 7/6/23 12:34, Andreas Schwab wrote:
>>> Fixes: 86f04735ac ("linux-user: Fix brk() to release pages")
>>> Signed-off-by: Andreas Schwab <schwab@suse.de>
>>> ---
>>>   linux-user/syscall.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index 08162cc966..e8a17377f5 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -805,7 +805,7 @@ static abi_ulong brk_page;
>>>   void target_set_brk(abi_ulong new_brk)
>>>   {
>>> -    target_brk = new_brk;
>>> +    target_brk = TARGET_PAGE_ALIGN(new_brk);
>>>       brk_page = HOST_PAGE_ALIGN(target_brk);
>>>   }
>>
>> It make sense, since that's how do_brk aligns things.
>
> Yes, patch looks good.
> I haven't tested, but it seems it adjusts the initial brk(0) value
> only to make sure that it's target page aligned.
> Maybe the title should be: ?
> linux-user: make sure the initial brk(0) is page-aligned

Another bug report regarding the non-aligned brk()...

See:
https://github.com/upx/upx/issues/683
https://gitlab.com/qemu-project/qemu/-/issues/1756

Helge