Re: [PATCH 3/4] linux-user: Fix mmap2() syscall on 32-bit targets to allow file mapping beyond 4GB

[Richard Henderson <richard.henderson@linaro.org>]

On 7/7/23 21:04, Helge Deller wrote:
> On 7/7/23 21:47, Richard Henderson wrote:
>> On 7/7/23 14:19, Helge Deller wrote:
>>> The mmap2() syscall allows 32-bit guests to specify the offset into a
>>> file in page units (instead of bytes, as done by mmap(2)).
>>> On physical machines this allows 32-bit applications to map such parts
>>> of large files which are stored beyond the 4GB limit.
>>>
>>> Allow the same behaviour when emulating 32-bit guests with qemu.
>>>
>>> For that switch the mmap2() function to always take an abi_ullong
>>> (64-bit) offset parameter for target_mmap() and mmap_frag() to avoid an
>>> arithmetical overflow when shifing a 32-bit offset parameter by
>>> 12 bits (=PAGE_SHIFT) and thus possibly overflow the abi_ulong (32-bit)
>>> type.
>>>
>>> Signed-off-by: Helge Deller<deller@gmx.de>
>>> ---
>>>   linux-user/mmap.c      | 9 +++++----
>>>   linux-user/syscall.c   | 2 +-
>>>   linux-user/user-mmap.h | 2 +-
>>>   3 files changed, 7 insertions(+), 6 deletions(-)
>>
>> https://patchew.org/QEMU/20230630132159.376995-1-richard.henderson@linaro.org/20230630132159.376995-12-richard.henderson@linaro.org/
>>
>> Wherein I use the host off_t (which must be 64-bits).
> 
> I like your patch.
> But wouldn't it be better to use off64_t instead of off_t just to make
> clear that this is a 64bit int?

No, I don't think so.  That's the point of _FILE_OFFSET_BITS=64.

> And this part:
> -                          arg5, arg6 << MMAP_SHIFT);
> +                          arg5, (off_t)(abi_ulong)arg6 << MMAP_SHIFT);
> maybe should become (with brackets): ?
> +                          arg5, ((off64_t)(abi_ulong)arg6) << MMAP_SHIFT);

Why would you add useless parenthesis?
At some point everyone should know C operator precedence...


r~