Re: [PATCH] linux-user: Add emulation for MADV_WIPEONFORK and MADV_KEEPONFORK in madvise()

[Ilya Leoshkevich]

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 <deller@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.

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.

>       *
>       * 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?



With the MADV_DONTNEED comment change:

Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>