Re: [PATCH] posix: Fix large mmap64 offset for mips64n32 (BZ#24699)

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 03/07/2019 10:08, Thomas De Schampheleire wrote:
> El mar., 2 jul. 2019 a las 18:43, Adhemerval Zanella
> (<adhemerval.zanella@linaro.org>) escribió:
>>
>> Ping, I have added it as a release blocker for 2.30.
>>
>> On 19/06/2019 11:13, Adhemerval Zanella wrote:
>>> The fix for BZ#21270 (commit 158d5fa0e19) added a mask to avoid offset larger
>>> than 1^44 to be used along __NR_mmap2.  However mips64n32 users __NR_mmap,
>>> as mips64n64, but still defines off_t as old non-LFS type (other ILP32, such
>>> x32, defines off_t being equal to off64_t).  This leads to use the same
>>> mask meant only for __NR_mmap2 call for __NR_mmap, thus limiting the maximum
>>> offset it can use with mmap64.
>>>
>>> This patch fixes by setting the high mask only for __NR_mmap2 usage. The
>>> posix/tst-mmap-offset.c already tests it and also fails for mips64n32. The
>>> patch also change the test to check for an arch-specific header that defines
>>> the maximum supported offset.
>>>
>>> Checked on x86_64-linux-gnu, i686-linux-gnu, and I also tests tst-mmap-offset
>>> on qemu simulated mips64 with kernel 3.2.0 kernel for both mips-linux-gnu and
>>> mips64-n32-linux-gnu.
>>>
>>>       [BZ #24699]
>>>       * posix/tst-mmap-offset.c: Mention BZ #24699.
>>>       (do_test_bz21270): Rename to do_test_large_offset and use
>>>       mmap64_maximum_offset to check for maximum expected offset value.
>>>       * sysdeps/generic/mmap_info.h: New file.
>>>       * sysdeps/unix/sysv/linux/mips/mmap_info.h: Likewise.
>>>       * sysdeps/unix/sysv/linux/mmap64.c (MMAP_OFF_HIGH_MASK): Define iff
>>>       __NR_mmap2 is used.
>>> ---
>>>  posix/tst-mmap-offset.c                  |  9 +++++----
>>>  sysdeps/generic/mmap_info.h              | 16 ++++++++++++++++
>>>  sysdeps/unix/sysv/linux/mips/mmap_info.h | 13 +++++++++++++
>>>  sysdeps/unix/sysv/linux/mmap64.c         |  9 ++++++++-
>>>  4 files changed, 42 insertions(+), 5 deletions(-)
>>>  create mode 100644 sysdeps/generic/mmap_info.h
>>>  create mode 100644 sysdeps/unix/sysv/linux/mips/mmap_info.h
>>>
>>> diff --git a/posix/tst-mmap-offset.c b/posix/tst-mmap-offset.c
>>> index 676c52845f..d4e53c3e67 100644
>>> --- a/posix/tst-mmap-offset.c
>>> +++ b/posix/tst-mmap-offset.c
>>> @@ -1,4 +1,4 @@
>>> -/* BZ #18877 and #21270 mmap offset test.
>>> +/* BZ #18877, BZ #21270, and BZ #24699 mmap offset test.
>>>
>>>     Copyright (C) 2015-2019 Free Software Foundation, Inc.
>>>     This file is part of the GNU C Library.
>>> @@ -24,6 +24,7 @@
>>>  #include <unistd.h>
>>>  #include <errno.h>
>>>  #include <sys/mman.h>
>>> +#include <mmap_info.h>
>>>
>>>  #include <support/check.h>
>>>
>>> @@ -76,7 +77,7 @@ do_test_bz18877 (void)
>>>
>>>  /* Check if invalid offset are handled correctly by mmap.  */
>>>  static int
>>> -do_test_bz21270 (void)
>>> +do_test_large_offset (void)
>>>  {
>>>    /* For architectures with sizeof (off_t) < sizeof (off64_t) mmap is
>>>       implemented with __SYS_mmap2 syscall and the offset is represented in
>>> @@ -90,7 +91,7 @@ do_test_bz21270 (void)
>>>    const size_t length = 4096;
>>>
>>>    void *addr = mmap64 (NULL, length, prot, flags, fd, offset);
>>> -  if (sizeof (off_t) < sizeof (off64_t))
>>> +  if (mmap64_maximum_offset (page_shift) < UINT64_MAX)
>>>      {
>>>        if ((addr != MAP_FAILED) && (errno != EINVAL))
>>>       FAIL_RET ("mmap succeed");
>>> @@ -110,7 +111,7 @@ do_test (void)
>>>    int ret = 0;
>>>
>>>    ret += do_test_bz18877 ();
>>> -  ret += do_test_bz21270 ();
>>> +  ret += do_test_large_offset ();
>>>
>>>    return ret;
>>>  }
>>> diff --git a/sysdeps/generic/mmap_info.h b/sysdeps/generic/mmap_info.h
>>> new file mode 100644
>>> index 0000000000..9a3315a8fe
>>> --- /dev/null
>>> +++ b/sysdeps/generic/mmap_info.h
>>> @@ -0,0 +1,16 @@
>>> +/* As default architectures with sizeof (off_t) < sizeof (off64_t) the mmap is
>>> +   implemented with __SYS_mmap2 syscall and the offset is represented in
>>> +   multiples of page size.  For offset larger than
>>> +   '1 << (page_shift + 8 * sizeof (off_t))' (that is, 1<<44 on system with
>>> +   page size of 4096 bytes) the system call silently truncates the offset.
>>> +   For this case, glibc mmap implementation returns EINVAL.  */
>>> +
>>> +/* Return the maximum value expected as offset argument in mmap64 call.  */
>>> +uint64_t static inline
>>> +mmap64_maximum_offset (long int page_shift)
>>> +{
>>> +  if (sizeof (off_t) < sizeof (off64_t))
>>> +    return (UINT64_C(1) << (page_shift + (8 * sizeof (off_t)))) - 1;
>>> +  else
>>> +    return UINT64_MAX;
>>> +}
>>> diff --git a/sysdeps/unix/sysv/linux/mips/mmap_info.h b/sysdeps/unix/sysv/linux/mips/mmap_info.h
>>> new file mode 100644
>>> index 0000000000..6fd3c3d954
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/mips/mmap_info.h
>>> @@ -0,0 +1,13 @@
>>> +/* mips64n32 uses __NR_mmap for mmap64 while still having sizeof (off_t)
>>> +   smaller than sizeof (off64_t).  So it allows mapping large offsets
>>> +   using mmap64 than 32-bit archs which uses __NR_mmap2.  */
>>> +
>>> +uint64_t static inline
>>> +mmap64_maximum_offset (long int page_shift)
>>> +{
>>> +#if _MIPS_SIM == _ABIN32 || _MIPS_SIM == _ABI64
>>> +  return UINT64_MAX;
>>> +#else
>>> +  return (UINT64_C(1) << (page_shift + (8 * sizeof (off_t)))) - 1;
>>> +#endif
>>> +}
>>> diff --git a/sysdeps/unix/sysv/linux/mmap64.c b/sysdeps/unix/sysv/linux/mmap64.c
>>> index cb56540119..671db2b654 100644
>>> --- a/sysdeps/unix/sysv/linux/mmap64.c
>>> +++ b/sysdeps/unix/sysv/linux/mmap64.c
>>> @@ -23,11 +23,18 @@
>>>  #include <sysdep.h>
>>>  #include <mmap_internal.h>
>>>
>>> +#ifdef __NR_mmap2
>>>  /* To avoid silent truncation of offset when using mmap2, do not accept
>>>     offset larger than 1 << (page_shift + off_t bits).  For archictures with
>>>     32 bits off_t and page size of 4096 it would be 1^44.  */
>>> -#define MMAP_OFF_HIGH_MASK \
>>> +# define MMAP_OFF_HIGH_MASK \
>>>    ((-(MMAP2_PAGE_UNIT << 1) << (8 * sizeof (off_t) - 1)))
>>> +#else
>>> +/* Some ABIs might use __NR_mmap while having sizeof (off_t) smaller than
>>> +   sizeof (off64_t) (currently only MIPS64n32).  For this case just set
>>> +   zero the higher bits so mmap with large offset does not fail.  */
>>> +# define MMAP_OFF_HIGH_MASK  0x0
>>> +#endif
>>>
>>>  #define MMAP_OFF_MASK (MMAP_OFF_HIGH_MASK | MMAP_OFF_LOW_MASK)
>>>
>>>
> 
> 
> I have tested this patch successfully. My test program compiled for
> MIPS64n32 now works correctly, and did not so without the patch. When
> compiling for MIPS64n64, nothing changed (as expected) and everything
> works too.
> 
> So from a user perspective, this is good to be applied.
> 
> Thanks!
> Thomas
> 

Thanks for checking on it, if no one opposes I will commit this shortly.