linux-user: Fix stale tbs after mmap

Submitted by Alexander Graf on May 7, 2012, 11:38 a.m.

Details

Message ID BA0ED813-AF4C-4FB4-AEFB-CB1B802DF76D@suse.de
State New
Headers show

Commit Message

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

Comments

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 hide | download patch | download mbox

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;
 }