On 12/12/22 22:16, Ilya Leoshkevich wrote:
On Mon, Dec 12, 2022 at 08:00:45AM +0100, Helge Deller wrote:
Both parameters have a different value on the parisc platform, so first
translate the target value into a host value for usage in the native
madvise() syscall.
Those parameters are often used by security sensitive applications (e.g.
tor browser, boringssl, ...) which expect the call to return a proper
return code on failure, so return -EINVAL if qemu fails to forward the
syscall to the host OS.
Tested with testcase of tor browser when running hppa-linux guest on
x86-64 host.
Signed-off-by: Helge Deller <del...@gmx.de>
diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 10f5079331..c75342108c 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -901,11 +901,25 @@ abi_long target_madvise(abi_ulong start, abi_ulong
len_in, int advice)
return -TARGET_EINVAL;
}
+ /* Translate for some architectures which have different MADV_xxx values */
+ switch (advice) {
+ case TARGET_MADV_DONTNEED: /* alpha */
+ advice = MADV_DONTNEED;
+ break;
+ case TARGET_MADV_WIPEONFORK: /* parisc */
+ advice = MADV_WIPEONFORK;
+ break;
+ case TARGET_MADV_KEEPONFORK: /* parisc */
+ advice = MADV_KEEPONFORK;
+ break;
+ /* we do not care about the other MADV_xxx values yet */
+ }
+
/*
* A straight passthrough may not be safe because qemu sometimes turns
* private file-backed mappings into anonymous mappings.
*
- * This is a hint, so ignoring and returning success is ok.
+ * For MADV_DONTNEED, which is a hint, ignoring and returning success is
ok.
Actually, MADV_DONTNEED is one of the few values, which is not always a
hint - it can be used to e.g. zero out pages.
Right, it _should_ zero out pages and return 0, or otherwise return failure.
I think the problem is that some userspace apps will then sadly break if we
change the current behaviour....
Anyway, in this patch I didn't wanted to touch MAD_DONTNEED.
As the next paragraph states, strictly speaking, MADV_DONTNEED is
currently broken, because it can indeed be ignored without indication
in some cases, but it's still arguably better than not honoring it at
all.
Yep.
*
* This breaks MADV_DONTNEED, completely implementing which is quite
* complicated. However, there is one low-hanging fruit: mappings that are
@@ -913,11 +927,17 @@ abi_long target_madvise(abi_ulong start, abi_ulong
len_in, int advice)
* passthrough is safe, so do it.
*/
mmap_lock();
- if (advice == TARGET_MADV_DONTNEED &&
- can_passthrough_madv_dontneed(start, end)) {
- ret = get_errno(madvise(g2h_untagged(start), len, MADV_DONTNEED));
- if (ret == 0) {
- page_reset_target_data(start, start + len);
+ switch (advice) {
+ case MADV_WIPEONFORK:
+ case MADV_KEEPONFORK:
+ ret = -EINVAL;
+ /* fall through */
+ case MADV_DONTNEED:
+ if (can_passthrough_madv_dontneed(start, end)) {
+ ret = get_errno(madvise(g2h_untagged(start), len, advice));
+ if ((advice == MADV_DONTNEED) && (ret == 0)) {
+ page_reset_target_data(start, start + len);
+ }
}
}
mmap_unlock();
Nit: maybe rename can_passthrough_madv_dontneed() to can_passthrough(),
since now it's used not only for MADV_DONTNEED?
Maybe can_passthrough_madvise() is better?
With the MADV_DONTNEED comment change:
Just for me to understand correctly:
You propose that I shouldn't touch that comment in my followup-patch, right?
That's ok.
Acked-by: Ilya Leoshkevich <i...@linux.ibm.com>
Thanks!
Helge