| Message ID | BA0ED813-AF4C-4FB4-AEFB-CB1B802DF76D@suse.de |
|---|---|
| State | New |
| Headers | show |
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
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
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
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; }