Patchwork linux-user: Fix stale tbs after mmap

login
register
mail settings
Submitter Alexander Graf
Date May 7, 2012, 11:38 a.m.
Message ID <BA0ED813-AF4C-4FB4-AEFB-CB1B802DF76D@suse.de>
Download mbox | patch
Permalink /patch/157295/
State New
Headers show

Comments

Alexander Graf - May 7, 2012, 11:38 a.m.
On 07.05.2012, at 13:32, Alexander Graf wrote:

> 
> On 07.05.2012, at 12:37, Peter Maydell wrote:
> 
>> On 7 May 2012 10:30, Alexander Graf <agraf@suse.de> wrote:
>>> @@ -587,6 +587,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>>>    page_dump(stdout);
>>>    printf("\n");
>>> #endif
>>> +    tb_invalidate_phys_page_range(start, start + len, 0);
>>>    mmap_unlock();
>>>    return start;
>> 
>> The comment at the top of tb_invalidate_phys_page_range() says
>> "start and end must refer to the same physical page" -- is it
>> out of date or does that not apply to user-mode?
>> 
>> Do you need to also invalidate the range on munmap() and
>> mprotect-to-not-executable in order to correctly fault on
>> the case of:
>> map something
>> execute it
>> unmap it
>> try to execute it again
>> 
>> ? (haven't tested that case but it seems like it might be an issue)
> 
> Yeah, the issue does exist:

And the below patch on top of my revised patch fixes it. The question is whether we still need to flush on mmap() then?


Alex
Peter Maydell - May 7, 2012, 12:15 p.m.
On 7 May 2012 12:38, Alexander Graf <agraf@suse.de> wrote:
> And the below patch on top of my revised patch fixes it.
> The question is whether we still need to flush on mmap() then?

IIRC the kernel will let you MAP_FIXED mmap a file to an
address that's already mmap'd for something else without
having to munmap the previous mapping. So yes, we still
need to flush on mmap I think.

Also I think you need mprotect() for the case of:
  mmap(...,PROT_EXEC)
  execute
  mprotect(not PROT_EXEC)
  try to execute again

-- PMM
Peter Maydell - May 11, 2012, 3:46 p.m.
On 7 May 2012 12:38, Alexander Graf <agraf@suse.de> wrote:
>
> On 07.05.2012, at 13:32, Alexander Graf wrote:
>
>>
>> On 07.05.2012, at 12:37, Peter Maydell wrote:
>>
>>> On 7 May 2012 10:30, Alexander Graf <agraf@suse.de> wrote:
>>>> @@ -587,6 +587,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>>>>    page_dump(stdout);
>>>>    printf("\n");
>>>> #endif
>>>> +    tb_invalidate_phys_page_range(start, start + len, 0);
>>>>    mmap_unlock();
>>>>    return start;
>>>
>>> The comment at the top of tb_invalidate_phys_page_range() says
>>> "start and end must refer to the same physical page" -- is it
>>> out of date or does that not apply to user-mode?
>>>
>>> Do you need to also invalidate the range on munmap() and
>>> mprotect-to-not-executable in order to correctly fault on
>>> the case of:
>>> map something
>>> execute it
>>> unmap it
>>> try to execute it again
>>>
>>> ? (haven't tested that case but it seems like it might be an issue)
>>
>> Yeah, the issue does exist:
>
> And the below patch on top of my revised patch fixes it.

I think these two patches look correct (and as you pointed out
on irc I was wrong about mprotect, which effectively already
handles flushing the tb if needed). If you can roll them together
into a single patch with a commit message and signed-off-by
you can add my Reviewed-by: tag to it.

thanks
-- PMM
Alexander Graf - May 11, 2012, 4 p.m.
On 11.05.2012, at 17:46, Peter Maydell wrote:

> On 7 May 2012 12:38, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 07.05.2012, at 13:32, Alexander Graf wrote:
>> 
>>> 
>>> On 07.05.2012, at 12:37, Peter Maydell wrote:
>>> 
>>>> On 7 May 2012 10:30, Alexander Graf <agraf@suse.de> wrote:
>>>>> @@ -587,6 +587,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>>>>>    page_dump(stdout);
>>>>>    printf("\n");
>>>>> #endif
>>>>> +    tb_invalidate_phys_page_range(start, start + len, 0);
>>>>>    mmap_unlock();
>>>>>    return start;
>>>> 
>>>> The comment at the top of tb_invalidate_phys_page_range() says
>>>> "start and end must refer to the same physical page" -- is it
>>>> out of date or does that not apply to user-mode?
>>>> 
>>>> Do you need to also invalidate the range on munmap() and
>>>> mprotect-to-not-executable in order to correctly fault on
>>>> the case of:
>>>> map something
>>>> execute it
>>>> unmap it
>>>> try to execute it again
>>>> 
>>>> ? (haven't tested that case but it seems like it might be an issue)
>>> 
>>> Yeah, the issue does exist:
>> 
>> And the below patch on top of my revised patch fixes it.
> 
> I think these two patches look correct (and as you pointed out
> on irc I was wrong about mprotect, which effectively already
> handles flushing the tb if needed). If you can roll them together
> into a single patch with a commit message and signed-off-by
> you can add my Reviewed-by: tag to it.

Well, if we invalidate on unmap, do we need to invalidate on mmap() still? Or is only invalidating on unmap enough? Maybe when you use fixed addresses... hrm

Ah whatever, let's just flush everywhere now and then optimize later.


Alex

Patch

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 3611deb..bb4e752 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -690,8 +690,10 @@  int target_munmap(abi_ulong start, abi_ulong len)
         }
     }
 
-    if (ret == 0)
+    if (ret == 0) {
         page_set_flags(start, start + len, 0);
+        tb_invalidate_phys_range(start, start + len, 0);
+    }
     mmap_unlock();
     return ret;
 }