* [PATCH v3 for-6.0 0/2] tcg: Workaround macOS 11.2 mprotect bug
@ 2021-03-20 16:57 Richard Henderson
2021-03-20 16:57 ` [PATCH v3 for-6.0 1/2] tcg: Do not set guard pages on the rx portion of code_gen_buffer Richard Henderson
2021-03-20 16:57 ` [PATCH v3 for-6.0 2/2] tcg: Workaround macOS 11.2 mprotect bug Richard Henderson
0 siblings, 2 replies; 9+ messages in thread
From: Richard Henderson @ 2021-03-20 16:57 UTC (permalink / raw)
To: qemu-devel; +Cc: r.bolshakov, j
My 29 patch series cleaning up buffer allocation really came
a week too late for 6.0. Let me do something quite minimal
instead -- simply ignore the failure to create the guard pages.
r~
Richard Henderson (2):
tcg: Do not set guard pages on the rx portion of code_gen_buffer
tcg: Workaround macOS 11.2 mprotect bug
tcg/tcg.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 for-6.0 1/2] tcg: Do not set guard pages on the rx portion of code_gen_buffer
2021-03-20 16:57 [PATCH v3 for-6.0 0/2] tcg: Workaround macOS 11.2 mprotect bug Richard Henderson
@ 2021-03-20 16:57 ` Richard Henderson
2021-03-22 13:56 ` Roman Bolshakov
2021-03-20 16:57 ` [PATCH v3 for-6.0 2/2] tcg: Workaround macOS 11.2 mprotect bug Richard Henderson
1 sibling, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2021-03-20 16:57 UTC (permalink / raw)
To: qemu-devel; +Cc: r.bolshakov, j
The rw portion of the buffer is the only one in which overruns
can be generated. Allow the rx portion to be more completely
covered by huge pages.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
tcg/tcg.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/tcg/tcg.c b/tcg/tcg.c
index de91bb6e9e..88c9e6f8a4 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -828,7 +828,6 @@ void tcg_region_init(void)
size_t region_size;
size_t n_regions;
size_t i;
- uintptr_t splitwx_diff;
n_regions = tcg_n_regions();
@@ -858,8 +857,11 @@ void tcg_region_init(void)
/* account for that last guard page */
region.end -= page_size;
- /* set guard pages */
- splitwx_diff = tcg_splitwx_diff;
+ /*
+ * Set guard pages in the rw buffer, as that's the one into which
+ * buffer overruns could occur. Do not set guard pages in the rx
+ * buffer -- let that one use hugepages throughout.
+ */
for (i = 0; i < region.n; i++) {
void *start, *end;
int rc;
@@ -867,10 +869,6 @@ void tcg_region_init(void)
tcg_region_bounds(i, &start, &end);
rc = qemu_mprotect_none(end, page_size);
g_assert(!rc);
- if (splitwx_diff) {
- rc = qemu_mprotect_none(end + splitwx_diff, page_size);
- g_assert(!rc);
- }
}
tcg_region_trees_init();
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 for-6.0 2/2] tcg: Workaround macOS 11.2 mprotect bug
2021-03-20 16:57 [PATCH v3 for-6.0 0/2] tcg: Workaround macOS 11.2 mprotect bug Richard Henderson
2021-03-20 16:57 ` [PATCH v3 for-6.0 1/2] tcg: Do not set guard pages on the rx portion of code_gen_buffer Richard Henderson
@ 2021-03-20 16:57 ` Richard Henderson
2021-03-22 10:03 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2021-03-20 16:57 UTC (permalink / raw)
To: qemu-devel; +Cc: r.bolshakov, j
There's a change in mprotect() behaviour [1] in the latest macOS
on M1 and it's not yet clear if it's going to be fixed by Apple.
As a short-term fix, ignore failures setting up the guard pages.
[1] https://gist.github.com/hikalium/75ae822466ee4da13cbbe486498a191f
Buglink: https://bugs.launchpad.net/qemu/+bug/1914849
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
tcg/tcg.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 88c9e6f8a4..1fbe0b686d 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -864,11 +864,15 @@ void tcg_region_init(void)
*/
for (i = 0; i < region.n; i++) {
void *start, *end;
- int rc;
tcg_region_bounds(i, &start, &end);
- rc = qemu_mprotect_none(end, page_size);
- g_assert(!rc);
+
+ /*
+ * macOS 11.2 has a bug (Apple Feedback FB8994773) in which mprotect
+ * rejects a permission change from RWX -> NONE. Guard pages are
+ * nice for bug detection but are not essential; ignore any failure.
+ */
+ (void)qemu_mprotect_none(end, page_size);
}
tcg_region_trees_init();
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 for-6.0 2/2] tcg: Workaround macOS 11.2 mprotect bug
2021-03-20 16:57 ` [PATCH v3 for-6.0 2/2] tcg: Workaround macOS 11.2 mprotect bug Richard Henderson
@ 2021-03-22 10:03 ` Philippe Mathieu-Daudé
2021-03-22 13:47 ` Roman Bolshakov
2021-03-22 15:00 ` Richard Henderson
0 siblings, 2 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-03-22 10:03 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: r.bolshakov, j
On 3/20/21 5:57 PM, Richard Henderson wrote:
> There's a change in mprotect() behaviour [1] in the latest macOS
> on M1 and it's not yet clear if it's going to be fixed by Apple.
>
> As a short-term fix, ignore failures setting up the guard pages.
>
> [1] https://gist.github.com/hikalium/75ae822466ee4da13cbbe486498a191f
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1914849
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> tcg/tcg.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index 88c9e6f8a4..1fbe0b686d 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -864,11 +864,15 @@ void tcg_region_init(void)
> */
> for (i = 0; i < region.n; i++) {
> void *start, *end;
> - int rc;
>
> tcg_region_bounds(i, &start, &end);
> - rc = qemu_mprotect_none(end, page_size);
What about:
#ifdef CONFIG_DARWIN
/* ... */
(void)rc;
#else
> - g_assert(!rc);
#endif
> +
> + /*
> + * macOS 11.2 has a bug (Apple Feedback FB8994773) in which mprotect
> + * rejects a permission change from RWX -> NONE. Guard pages are
> + * nice for bug detection but are not essential; ignore any failure.
> + */
> + (void)qemu_mprotect_none(end, page_size);
> }
>
> tcg_region_trees_init();
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 for-6.0 2/2] tcg: Workaround macOS 11.2 mprotect bug
2021-03-22 10:03 ` Philippe Mathieu-Daudé
@ 2021-03-22 13:47 ` Roman Bolshakov
2021-03-22 15:00 ` Richard Henderson
1 sibling, 0 replies; 9+ messages in thread
From: Roman Bolshakov @ 2021-03-22 13:47 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Richard Henderson, qemu-devel, j
On Mon, Mar 22, 2021 at 11:03:05AM +0100, Philippe Mathieu-Daudé wrote:
> On 3/20/21 5:57 PM, Richard Henderson wrote:
> > There's a change in mprotect() behaviour [1] in the latest macOS
> > on M1 and it's not yet clear if it's going to be fixed by Apple.
> >
> > As a short-term fix, ignore failures setting up the guard pages.
> >
> > [1] https://gist.github.com/hikalium/75ae822466ee4da13cbbe486498a191f
> >
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1914849
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> > tcg/tcg.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/tcg/tcg.c b/tcg/tcg.c
> > index 88c9e6f8a4..1fbe0b686d 100644
> > --- a/tcg/tcg.c
> > +++ b/tcg/tcg.c
> > @@ -864,11 +864,15 @@ void tcg_region_init(void)
> > */
> > for (i = 0; i < region.n; i++) {
> > void *start, *end;
> > - int rc;
> >
> > tcg_region_bounds(i, &start, &end);
> > - rc = qemu_mprotect_none(end, page_size);
>
> What about:
>
> #ifdef CONFIG_DARWIN
>
> /* ... */
> (void)rc;
> #else
>
> > - g_assert(!rc);
>
> #endif
>
> > +
> > + /*
> > + * macOS 11.2 has a bug (Apple Feedback FB8994773) in which mprotect
> > + * rejects a permission change from RWX -> NONE. Guard pages are
> > + * nice for bug detection but are not essential; ignore any failure.
> > + */
> > + (void)qemu_mprotect_none(end, page_size);
> > }
> >
> > tcg_region_trees_init();
> >
>
I agree with Philippe, it's worth to keep the bug detection on non-buggy
platforms.
Otherwise:
Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
Tested-by: Roman Bolshakov <r.bolshakov@yadro.com>
Thanks,
Roman
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 for-6.0 1/2] tcg: Do not set guard pages on the rx portion of code_gen_buffer
2021-03-20 16:57 ` [PATCH v3 for-6.0 1/2] tcg: Do not set guard pages on the rx portion of code_gen_buffer Richard Henderson
@ 2021-03-22 13:56 ` Roman Bolshakov
2021-03-22 15:13 ` Richard Henderson
0 siblings, 1 reply; 9+ messages in thread
From: Roman Bolshakov @ 2021-03-22 13:56 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, j
On Sat, Mar 20, 2021 at 10:57:19AM -0600, Richard Henderson wrote:
> The rw portion of the buffer is the only one in which overruns
> can be generated. Allow the rx portion to be more completely
> covered by huge pages.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> tcg/tcg.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index de91bb6e9e..88c9e6f8a4 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -828,7 +828,6 @@ void tcg_region_init(void)
> size_t region_size;
> size_t n_regions;
> size_t i;
> - uintptr_t splitwx_diff;
>
> n_regions = tcg_n_regions();
>
> @@ -858,8 +857,11 @@ void tcg_region_init(void)
> /* account for that last guard page */
> region.end -= page_size;
>
> - /* set guard pages */
> - splitwx_diff = tcg_splitwx_diff;
> + /*
> + * Set guard pages in the rw buffer, as that's the one into which
> + * buffer overruns could occur. Do not set guard pages in the rx
> + * buffer -- let that one use hugepages throughout.
> + */
> for (i = 0; i < region.n; i++) {
> void *start, *end;
> int rc;
> @@ -867,10 +869,6 @@ void tcg_region_init(void)
> tcg_region_bounds(i, &start, &end);
> rc = qemu_mprotect_none(end, page_size);
> g_assert(!rc);
> - if (splitwx_diff) {
> - rc = qemu_mprotect_none(end + splitwx_diff, page_size);
> - g_assert(!rc);
> - }
> }
>
> tcg_region_trees_init();
> --
> 2.25.1
>
Thanks for fixing the issue, Richard,
I have two questions:
- Should we keep guards pages for rx on all platforms except darwin?
(that would make it similar to what Philippe proposed in the comments
to patch 2).
- What does mean that rx might be covered by huge pages? (perhaps I'm
missing some context)
Otherwise,
Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
Tested-by: Roman Bolshakov <r.bolshakov@yadro.com>
BR,
Roman
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 for-6.0 2/2] tcg: Workaround macOS 11.2 mprotect bug
2021-03-22 10:03 ` Philippe Mathieu-Daudé
2021-03-22 13:47 ` Roman Bolshakov
@ 2021-03-22 15:00 ` Richard Henderson
2021-03-22 15:39 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2021-03-22 15:00 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel; +Cc: r.bolshakov, j
On 3/22/21 4:03 AM, Philippe Mathieu-Daudé wrote:
>> - rc = qemu_mprotect_none(end, page_size);
>
> What about:
>
> #ifdef CONFIG_DARWIN
>
> /* ... */
> (void)rc;
> #else
>
>> - g_assert(!rc);
>
> #endif
What does that buy us, really? It seems like it just clutters the code with
ifdefs.
r~
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 for-6.0 1/2] tcg: Do not set guard pages on the rx portion of code_gen_buffer
2021-03-22 13:56 ` Roman Bolshakov
@ 2021-03-22 15:13 ` Richard Henderson
0 siblings, 0 replies; 9+ messages in thread
From: Richard Henderson @ 2021-03-22 15:13 UTC (permalink / raw)
To: Roman Bolshakov; +Cc: qemu-devel, j
On 3/22/21 7:56 AM, Roman Bolshakov wrote:
> - What does mean that rx might be covered by huge pages? (perhaps I'm
> missing some context)
Huge pages are where a single level (n-1) page table entry covers the entire
span that would be covered by m * level n page table entries.
On x86, this is a 2MB hugepage, vs 512 4kB pages. Most modern cpus support
something similar.
See qemu_madvise(..., QEMU_MADV_HUGEPAGE).
r~
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 for-6.0 2/2] tcg: Workaround macOS 11.2 mprotect bug
2021-03-22 15:00 ` Richard Henderson
@ 2021-03-22 15:39 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-03-22 15:39 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: r.bolshakov, j
On 3/22/21 4:00 PM, Richard Henderson wrote:
> On 3/22/21 4:03 AM, Philippe Mathieu-Daudé wrote:
>>> - rc = qemu_mprotect_none(end, page_size);
>>
>> What about:
>>
>> #ifdef CONFIG_DARWIN
>>
>> /* ... */
>> (void)rc;
>> #else
>>
>>> - g_assert(!rc);
>>
>> #endif
>
> What does that buy us, really? It seems like it just clutters the code
> with ifdefs.
No problem.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-03-22 15:40 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-20 16:57 [PATCH v3 for-6.0 0/2] tcg: Workaround macOS 11.2 mprotect bug Richard Henderson
2021-03-20 16:57 ` [PATCH v3 for-6.0 1/2] tcg: Do not set guard pages on the rx portion of code_gen_buffer Richard Henderson
2021-03-22 13:56 ` Roman Bolshakov
2021-03-22 15:13 ` Richard Henderson
2021-03-20 16:57 ` [PATCH v3 for-6.0 2/2] tcg: Workaround macOS 11.2 mprotect bug Richard Henderson
2021-03-22 10:03 ` Philippe Mathieu-Daudé
2021-03-22 13:47 ` Roman Bolshakov
2021-03-22 15:00 ` Richard Henderson
2021-03-22 15:39 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).