[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH] linux-user/mmap: Return EFAULT for invalid addresses
From: |
Laurent Vivier |
Subject: |
Re: [RFC PATCH] linux-user/mmap: Return EFAULT for invalid addresses |
Date: |
Tue, 16 Feb 2021 17:21:03 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 |
Le 16/02/2021 à 12:49, Richard Purdie a écrit :
> On Sat, 2021-02-13 at 18:40 +0100, Laurent Vivier wrote:
>> Le 08/01/2021 à 18:46, Richard Purdie a écrit :
>>> When using qemu-i386 to run gobject introspection parts of a webkitgtk
>>> build using musl as libc on a 64 bit host, it sits in an infinite loop
>>> of mremap calls of ever decreasing/increasing addresses.
>>>
>>> I suspect something in the musl memory allocation code loops indefinitely
>>> if it only sees ENOMEM and only exits when it hits EFAULT.
>>>
>>> According to the docs, trying to mremap outside the address space
>>> can/should return EFAULT and changing this allows the build to succeed.
>>>
>>> There was previous discussion of this as it used to work before qemu 2.11
>>> and we've carried hacks to work around it since, this appears to be a
>>> better fix of the real issue?
>>>
>>> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org
>>>
>>> Index: qemu-5.2.0/linux-user/mmap.c
>>> ===================================================================
>>> --- qemu-5.2.0.orig/linux-user/mmap.c
>>> +++ qemu-5.2.0/linux-user/mmap.c
>>> @@ -727,7 +727,7 @@ abi_long target_mremap(abi_ulong old_add
>>> !guest_range_valid(new_addr, new_size)) ||
>>> ((flags & MREMAP_MAYMOVE) == 0 &&
>>> !guest_range_valid(old_addr, new_size))) {
>>> - errno = ENOMEM;
>>> + errno = EFAULT;
>>> return -1;
>>> }
>>>
>>>
>>>
>>>
>>
>> I agree with that, the ENOMEM is returned when there is not enough virtual
>> memory (the
>> mmap_find_vma() case).
>>
>> According to the manpage, EFAULT is returned when old_addr and old_addr +
>> old_size is an invalid
>> address space.
>>
>> So:
>>
>> if (!guest_range_valid(old_addr, old_size)) {
>> errno = EFAULT;
>> return -1;
>> }
>>
>> But in the case of new_size and new_addr, it seems the good value to use is
>> EINVAL.
>>
>> So:
>>
>> if (((flags & MREMAP_FIXED) && !guest_range_valid(new_addr, new_size)) ||
>> ((flags & MREMAP_MAYMOVE) == 0 && !guest_range_valid(old_addr,
>> new_size))) {
>> errno = EINVAL;
>> return -1;
>> }
>>
>> Did you try that?
>
> Its taken me a short while to reproduce the test environment but I did
> so and can confirm that using EINVAL works just as well as EFAULT in
> the test case we have. The above would therefore seem to make sense to
> me and would fix the case we found.
Could you send a v2 of your patch with these changes?
Thanks,
Laurent