diff mbox series

[3/5] linux-user: Use MAP_FIXED_NOREPLACE for do_brk()

Message ID 20230731080317.112658-4-akihiko.odaki@daynix.com
State New
Headers show
Series linux-user: brk/mmap fixes | expand

Commit Message

Akihiko Odaki July 31, 2023, 8:03 a.m. UTC
MAP_FIXED_NOREPLACE can ensure the mapped address is fixed without
concerning that the new mapping overwrites something else.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 linux-user/syscall.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

Comments

Peter Maydell July 31, 2023, 11:44 a.m. UTC | #1
On Mon, 31 Jul 2023 at 09:04, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> MAP_FIXED_NOREPLACE can ensure the mapped address is fixed without
> concerning that the new mapping overwrites something else.

MAP_FIXED_NOREPLACE only came in with Linux 4.17. So
I think we still need to handle the "mapped address
is not the one we asked for" error condition, because
it can happen on older host kernels that ignore the
MAP_FIXED_NOREPLACE flag.

thanks
-- PMM
Akihiko Odaki July 31, 2023, 1:32 p.m. UTC | #2
On 2023/07/31 20:44, Peter Maydell wrote:
> On Mon, 31 Jul 2023 at 09:04, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> MAP_FIXED_NOREPLACE can ensure the mapped address is fixed without
>> concerning that the new mapping overwrites something else.
> 
> MAP_FIXED_NOREPLACE only came in with Linux 4.17. So
> I think we still need to handle the "mapped address
> is not the one we asked for" error condition, because
> it can happen on older host kernels that ignore the
> MAP_FIXED_NOREPLACE flag.
> 
> thanks
> -- PMM

MAP_FIXED_NOREPLACE is substituted with MAP_FIXED before passing to the 
host with patch 1. The NOREPLACE constraint is still ensured by 
inspecting the guest page table.

Regards,
Akihiko Odaki
Peter Maydell July 31, 2023, 1:45 p.m. UTC | #3
On Mon, 31 Jul 2023 at 14:32, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2023/07/31 20:44, Peter Maydell wrote:
> > On Mon, 31 Jul 2023 at 09:04, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> MAP_FIXED_NOREPLACE can ensure the mapped address is fixed without
> >> concerning that the new mapping overwrites something else.
> >
> > MAP_FIXED_NOREPLACE only came in with Linux 4.17. So
> > I think we still need to handle the "mapped address
> > is not the one we asked for" error condition, because
> > it can happen on older host kernels that ignore the
> > MAP_FIXED_NOREPLACE flag.

> MAP_FIXED_NOREPLACE is substituted with MAP_FIXED before passing to the
> host with patch 1. The NOREPLACE constraint is still ensured by
> inspecting the guest page table.

Oh, I see, this patch is a call to target_mmap(), not host
mmap(). Sorry, I misread that.

thanks
-- PMM
Richard Henderson July 31, 2023, 3:44 p.m. UTC | #4
On 7/31/23 06:32, Akihiko Odaki wrote:
> On 2023/07/31 20:44, Peter Maydell wrote:
>> On Mon, 31 Jul 2023 at 09:04, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>
>>> MAP_FIXED_NOREPLACE can ensure the mapped address is fixed without
>>> concerning that the new mapping overwrites something else.
>>
>> MAP_FIXED_NOREPLACE only came in with Linux 4.17. So
>> I think we still need to handle the "mapped address
>> is not the one we asked for" error condition, because
>> it can happen on older host kernels that ignore the
>> MAP_FIXED_NOREPLACE flag.
>>
>> thanks
>> -- PMM
> 
> MAP_FIXED_NOREPLACE is substituted with MAP_FIXED before passing to the host with patch 1. 
> The NOREPLACE constraint is still ensured by inspecting the guest page table.

Won't work for 64-bit guests.


r~
diff mbox series

Patch

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index b9d2ec02f9..ac429a185a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -854,17 +854,12 @@  abi_long do_brk(abi_ulong brk_val)
         return target_brk;
     }
 
-    /* We need to allocate more memory after the brk... Note that
-     * we don't use MAP_FIXED because that will map over the top of
-     * any existing mapping (like the one with the host libc or qemu
-     * itself); instead we treat "mapped but at wrong address" as
-     * a failure and unmap again.
-     */
     if (new_host_brk_page > brk_page) {
         new_alloc_size = new_host_brk_page - brk_page;
         mapped_addr = target_mmap(brk_page, new_alloc_size,
-                                  PROT_READ|PROT_WRITE,
-                                  MAP_ANON|MAP_PRIVATE, 0, 0);
+                                  PROT_READ | PROT_WRITE,
+                                  MAP_FIXED_NOREPLACE | MAP_ANON | MAP_PRIVATE,
+                                  0, 0);
     } else {
         new_alloc_size = 0;
         mapped_addr = brk_page;
@@ -883,12 +878,6 @@  abi_long do_brk(abi_ulong brk_val)
         target_brk = brk_val;
         brk_page = new_host_brk_page;
         return target_brk;
-    } else if (mapped_addr != -1) {
-        /* Mapped but at wrong address, meaning there wasn't actually
-         * enough space for this brk.
-         */
-        target_munmap(mapped_addr, new_alloc_size);
-        mapped_addr = -1;
     }
 
 #if defined(TARGET_ALPHA)