| Message ID | 20210502231844.1977630-28-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tcg: Clean up code_gen_buffer allocation | expand |
Richard Henderson <richard.henderson@linaro.org> writes: > 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. > > In this case, instead of changing permissions of N guard pages, > we change permissions of N rwx regions. The same number of > syscalls are required either way. > > [1] https://gist.github.com/hikalium/75ae822466ee4da13cbbe486498a191f > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > tcg/region.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/tcg/region.c b/tcg/region.c > index 604530b902..5e00db4cfb 100644 > --- a/tcg/region.c > +++ b/tcg/region.c > @@ -765,12 +765,15 @@ static int alloc_code_gen_buffer(size_t size, int splitwx, Error **errp) > error_free_or_abort(errp); > } > > - prot = PROT_READ | PROT_WRITE | PROT_EXEC; > + /* > + * macOS 11.2 has a bug (Apple Feedback FB8994773) in which mprotect > + * rejects a permission change from RWX -> NONE when reserving the > + * guard pages later. We can go the other way with the same number > + * of syscalls, so always begin with PROT_NONE. > + */ > + prot = PROT_NONE; > flags = MAP_PRIVATE | MAP_ANONYMOUS; > -#ifdef CONFIG_TCG_INTERPRETER > - /* The tcg interpreter does not need execute permission. */ > - prot = PROT_READ | PROT_WRITE; > -#elif defined(CONFIG_DARWIN) > +#ifdef CONFIG_DARWIN > /* Applicable to both iOS and macOS (Apple Silicon). */ > if (!splitwx) { > flags |= MAP_JIT; > @@ -901,11 +904,7 @@ void tcg_region_init(size_t tb_size, int splitwx, unsigned max_cpus) > } > } I think the comment in tcg_region_init needs to be updated, currently reads: /* * 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. * Work with the page protections set up with the initial mapping. */ need_prot = PAGE_READ | PAGE_WRITE; but now we start with everything at PROT_NONE and are just setting need_prot for the non-guard pages. > if (have_prot != 0) { > - /* > - * 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. > - */ > + /* Guard pages are nice for bug detection but are not essential. */ > (void)qemu_mprotect_none(end, page_size); Isn't the last page already set as PROT_NONE? > } > } -- Alex Bennée
From: Richard Henderson <richard.henderson@linaro.org> > 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. > > In this case, instead of changing permissions of N guard pages, we change > permissions of N rwx regions. The same number of syscalls are required either > way. > > [1] https://gist.github.com/hikalium/75ae822466ee4da13cbbe486498a191f > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > tcg/region.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) Reviewed-by: Luis Pires <luis.pires@eldorado.org.br> -- Luis Pires Instituto de Pesquisas ELDORADO Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
On 6/9/21 4:21 AM, Alex Bennée wrote: > > Richard Henderson <richard.henderson@linaro.org> writes: > >> 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. >> >> In this case, instead of changing permissions of N guard pages, >> we change permissions of N rwx regions. The same number of >> syscalls are required either way. >> >> [1] https://gist.github.com/hikalium/75ae822466ee4da13cbbe486498a191f >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> tcg/region.c | 19 +++++++++---------- >> 1 file changed, 9 insertions(+), 10 deletions(-) >> >> diff --git a/tcg/region.c b/tcg/region.c >> index 604530b902..5e00db4cfb 100644 >> --- a/tcg/region.c >> +++ b/tcg/region.c >> @@ -765,12 +765,15 @@ static int alloc_code_gen_buffer(size_t size, int splitwx, Error **errp) >> error_free_or_abort(errp); >> } >> >> - prot = PROT_READ | PROT_WRITE | PROT_EXEC; >> + /* >> + * macOS 11.2 has a bug (Apple Feedback FB8994773) in which mprotect >> + * rejects a permission change from RWX -> NONE when reserving the >> + * guard pages later. We can go the other way with the same number >> + * of syscalls, so always begin with PROT_NONE. >> + */ >> + prot = PROT_NONE; >> flags = MAP_PRIVATE | MAP_ANONYMOUS; >> -#ifdef CONFIG_TCG_INTERPRETER >> - /* The tcg interpreter does not need execute permission. */ >> - prot = PROT_READ | PROT_WRITE; >> -#elif defined(CONFIG_DARWIN) >> +#ifdef CONFIG_DARWIN >> /* Applicable to both iOS and macOS (Apple Silicon). */ >> if (!splitwx) { >> flags |= MAP_JIT; >> @@ -901,11 +904,7 @@ void tcg_region_init(size_t tb_size, int splitwx, unsigned max_cpus) >> } >> } > > I think the comment in tcg_region_init needs to be updated, currently > reads: > > /* > * 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. > * Work with the page protections set up with the initial mapping. > */ > need_prot = PAGE_READ | PAGE_WRITE; > > but now we start with everything at PROT_NONE and are just setting > need_prot for the non-guard pages. The comment *has* been updated: work with the page protections set up with the initial mapping. Which is *not* always PROT_NONE. See the USE_STATIC_CODE_GEN_BUFFER and _WIN32 copies of alloc_code_gen_buffer. >> if (have_prot != 0) { >> - /* >> - * 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. >> - */ >> + /* Guard pages are nice for bug detection but are not essential. */ >> (void)qemu_mprotect_none(end, page_size); > > Isn't the last page already set as PROT_NONE? No. r~
diff --git a/tcg/region.c b/tcg/region.c index 604530b902..5e00db4cfb 100644 --- a/tcg/region.c +++ b/tcg/region.c @@ -765,12 +765,15 @@ static int alloc_code_gen_buffer(size_t size, int splitwx, Error **errp) error_free_or_abort(errp); } - prot = PROT_READ | PROT_WRITE | PROT_EXEC; + /* + * macOS 11.2 has a bug (Apple Feedback FB8994773) in which mprotect + * rejects a permission change from RWX -> NONE when reserving the + * guard pages later. We can go the other way with the same number + * of syscalls, so always begin with PROT_NONE. + */ + prot = PROT_NONE; flags = MAP_PRIVATE | MAP_ANONYMOUS; -#ifdef CONFIG_TCG_INTERPRETER - /* The tcg interpreter does not need execute permission. */ - prot = PROT_READ | PROT_WRITE; -#elif defined(CONFIG_DARWIN) +#ifdef CONFIG_DARWIN /* Applicable to both iOS and macOS (Apple Silicon). */ if (!splitwx) { flags |= MAP_JIT; @@ -901,11 +904,7 @@ void tcg_region_init(size_t tb_size, int splitwx, unsigned max_cpus) } } if (have_prot != 0) { - /* - * 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. - */ + /* Guard pages are nice for bug detection but are not essential. */ (void)qemu_mprotect_none(end, page_size); } }
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. In this case, instead of changing permissions of N guard pages, we change permissions of N rwx regions. The same number of syscalls are required either way. [1] https://gist.github.com/hikalium/75ae822466ee4da13cbbe486498a191f Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- tcg/region.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) -- 2.25.1