Linux: Use mmap instead of malloc in dirent/tst-getdents64
diff mbox series

Message ID 87a7e2p01f.fsf@oldenburg2.str.redhat.com
State New
Headers show
Series
  • Linux: Use mmap instead of malloc in dirent/tst-getdents64
Related show

Commit Message

Florian Weimer June 28, 2019, 8:49 a.m. UTC
malloc dirties the entire allocated memory region due to M_PERTURB
in the test harness.

2019-06-28  Florian Weimer  <fweimer@redhat.com>

	* sysdeps/unix/sysv/linux/tst-getdents64.c (large_buffer_checks):
	Use mmap instead of malloc.  malloc with M_PERTURB writes to the
	entire allocated memory range.

Comments

Adhemerval Zanella June 28, 2019, 11:22 a.m. UTC | #1
On 28/06/2019 05:49, Florian Weimer wrote:
> malloc dirties the entire allocated memory region due to M_PERTURB
> in the test harness.
> 
> 2019-06-28  Florian Weimer  <fweimer@redhat.com>
> 
> 	* sysdeps/unix/sysv/linux/tst-getdents64.c (large_buffer_checks):
> 	Use mmap instead of malloc.  malloc with M_PERTURB writes to the
> 	entire allocated memory range.

LGTM.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> 
> diff --git a/sysdeps/unix/sysv/linux/tst-getdents64.c b/sysdeps/unix/sysv/linux/tst-getdents64.c
> index 24e77e04d8..8a28e6c67c 100644
> --- a/sysdeps/unix/sysv/linux/tst-getdents64.c
> +++ b/sysdeps/unix/sysv/linux/tst-getdents64.c
> @@ -27,6 +27,7 @@
>  #include <support/check.h>
>  #include <support/support.h>
>  #include <support/xunistd.h>
> +#include <sys/mman.h>
>  #include <unistd.h>
>  
>  /* Called by large_buffer_checks below.  */
> @@ -53,8 +54,13 @@ large_buffer_checks (int fd)
>    size_t large_buffer_size;
>    if (!__builtin_add_overflow (UINT_MAX, 2, &large_buffer_size))
>      {
> -      char *large_buffer = malloc (large_buffer_size);
> -      if (large_buffer == NULL)
> +      int flags = MAP_ANONYMOUS | MAP_PRIVATE;
> +#ifdef MAP_NORESERVE
> +      flags |= MAP_NORESERVE;
> +#endif
> +      void *large_buffer = mmap (NULL, large_buffer_size,
> +                                 PROT_READ | PROT_WRITE, flags, -1, 0);
> +      if (large_buffer == MAP_FAILED)

Should we really skip the test instead of using xmmap? The only case I think of
that it might fail is if kernel can't allocate bookeeping memory for the page
table itself, but I really think it unlikely (sanitizer usually allocates a
lot of VMA as well).

>          printf ("warning: could not allocate %zu bytes of memory,"
>                  " subtests skipped\n", large_buffer_size);
>        else
> @@ -65,8 +71,8 @@ large_buffer_checks (int fd)
>            large_buffer_check (fd, large_buffer, UINT_MAX);
>            large_buffer_check (fd, large_buffer, (size_t) UINT_MAX + 1);
>            large_buffer_check (fd, large_buffer, (size_t) UINT_MAX + 2);
> +          xmunmap (large_buffer, large_buffer_size);
>          }
> -      free (large_buffer);
>      }
>  }
>  
>
Florian Weimer June 28, 2019, 12:04 p.m. UTC | #2
* Adhemerval Zanella:

> On 28/06/2019 05:49, Florian Weimer wrote:
>> malloc dirties the entire allocated memory region due to M_PERTURB
>> in the test harness.
>> 
>> 2019-06-28  Florian Weimer  <fweimer@redhat.com>
>> 
>> 	* sysdeps/unix/sysv/linux/tst-getdents64.c (large_buffer_checks):
>> 	Use mmap instead of malloc.  malloc with M_PERTURB writes to the
>> 	entire allocated memory range.
>
> LGTM.
>
> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

Thanks!

>> diff --git a/sysdeps/unix/sysv/linux/tst-getdents64.c b/sysdeps/unix/sysv/linux/tst-getdents64.c
>> index 24e77e04d8..8a28e6c67c 100644
>> --- a/sysdeps/unix/sysv/linux/tst-getdents64.c
>> +++ b/sysdeps/unix/sysv/linux/tst-getdents64.c
>> @@ -27,6 +27,7 @@
>>  #include <support/check.h>
>>  #include <support/support.h>
>>  #include <support/xunistd.h>
>> +#include <sys/mman.h>
>>  #include <unistd.h>
>>  
>>  /* Called by large_buffer_checks below.  */
>> @@ -53,8 +54,13 @@ large_buffer_checks (int fd)
>>    size_t large_buffer_size;
>>    if (!__builtin_add_overflow (UINT_MAX, 2, &large_buffer_size))
>>      {
>> -      char *large_buffer = malloc (large_buffer_size);
>> -      if (large_buffer == NULL)
>> +      int flags = MAP_ANONYMOUS | MAP_PRIVATE;
>> +#ifdef MAP_NORESERVE
>> +      flags |= MAP_NORESERVE;
>> +#endif
>> +      void *large_buffer = mmap (NULL, large_buffer_size,
>> +                                 PROT_READ | PROT_WRITE, flags, -1, 0);
>> +      if (large_buffer == MAP_FAILED)
>
> Should we really skip the test instead of using xmmap? The only case I think of
> that it might fail is if kernel can't allocate bookeeping memory for the page
> table itself, but I really think it unlikely (sanitizer usually allocates a
> lot of VMA as well).

MAP_NORESERVE is a misnomer; it does not do what it says.  It disables
the overcommit heuristics for vm.overcommit_memory=0, but not for
vm.overcommit_memory=2.  mmap can also fail due to resource limits on
address space, not residential memory.  So I think the code above is the
best we can do.

Thanks,
Florian
Adhemerval Zanella June 28, 2019, 2:35 p.m. UTC | #3
On 28/06/2019 09:04, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 28/06/2019 05:49, Florian Weimer wrote:
>>> malloc dirties the entire allocated memory region due to M_PERTURB
>>> in the test harness.
>>>
>>> 2019-06-28  Florian Weimer  <fweimer@redhat.com>
>>>
>>> 	* sysdeps/unix/sysv/linux/tst-getdents64.c (large_buffer_checks):
>>> 	Use mmap instead of malloc.  malloc with M_PERTURB writes to the
>>> 	entire allocated memory range.
>>
>> LGTM.
>>
>> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> 
> Thanks!
> 
>>> diff --git a/sysdeps/unix/sysv/linux/tst-getdents64.c b/sysdeps/unix/sysv/linux/tst-getdents64.c
>>> index 24e77e04d8..8a28e6c67c 100644
>>> --- a/sysdeps/unix/sysv/linux/tst-getdents64.c
>>> +++ b/sysdeps/unix/sysv/linux/tst-getdents64.c
>>> @@ -27,6 +27,7 @@
>>>  #include <support/check.h>
>>>  #include <support/support.h>
>>>  #include <support/xunistd.h>
>>> +#include <sys/mman.h>
>>>  #include <unistd.h>
>>>  
>>>  /* Called by large_buffer_checks below.  */
>>> @@ -53,8 +54,13 @@ large_buffer_checks (int fd)
>>>    size_t large_buffer_size;
>>>    if (!__builtin_add_overflow (UINT_MAX, 2, &large_buffer_size))
>>>      {
>>> -      char *large_buffer = malloc (large_buffer_size);
>>> -      if (large_buffer == NULL)
>>> +      int flags = MAP_ANONYMOUS | MAP_PRIVATE;
>>> +#ifdef MAP_NORESERVE
>>> +      flags |= MAP_NORESERVE;
>>> +#endif
>>> +      void *large_buffer = mmap (NULL, large_buffer_size,
>>> +                                 PROT_READ | PROT_WRITE, flags, -1, 0);
>>> +      if (large_buffer == MAP_FAILED)
>>
>> Should we really skip the test instead of using xmmap? The only case I think of
>> that it might fail is if kernel can't allocate bookeeping memory for the page
>> table itself, but I really think it unlikely (sanitizer usually allocates a
>> lot of VMA as well).
> 
> MAP_NORESERVE is a misnomer; it does not do what it says.  It disables
> the overcommit heuristics for vm.overcommit_memory=0, but not for
> vm.overcommit_memory=2.  mmap can also fail due to resource limits on
> address space, not residential memory.  So I think the code above is the
> best we can do.

Alright, but my point is for usual 64-bit architecture (which the test
are actually exercised) mapping 4GB of memory shouldn't really be a
problem. The change looks ok, I would just like to make is more explicit
that the tests passed, but not all sub-tests ran.
Florian Weimer June 28, 2019, 2:52 p.m. UTC | #4
* Adhemerval Zanella:

> Alright, but my point is for usual 64-bit architecture (which the test
> are actually exercised) mapping 4GB of memory shouldn't really be a
> problem.

Our z/VM guests have 2 GiB RAM assigned to them.

> The change looks ok, I would just like to make is more explicit
> that the tests passed, but not all sub-tests ran.

It's just a warning, not FAIL_UNSUPPORTED:

      if (large_buffer == MAP_FAILED)
        printf ("warning: could not allocate %zu bytes of memory,"
                " subtests skipped\n", large_buffer_size);

If you don't like that, we'd have to split it into a separate test, I
think.

Thanks,
Florian
Adhemerval Zanella June 28, 2019, 4:12 p.m. UTC | #5
On 28/06/2019 11:52, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> Alright, but my point is for usual 64-bit architecture (which the test
>> are actually exercised) mapping 4GB of memory shouldn't really be a
>> problem.
> 
> Our z/VM guests have 2 GiB RAM assigned to them.
> 
>> The change looks ok, I would just like to make is more explicit
>> that the tests passed, but not all sub-tests ran.
> 
> It's just a warning, not FAIL_UNSUPPORTED:
> 
>       if (large_buffer == MAP_FAILED)
>         printf ("warning: could not allocate %zu bytes of memory,"
>                 " subtests skipped\n", large_buffer_size);
> 
> If you don't like that, we'd have to split it into a separate test, I
> think.

I think we are ok for now, maybe in future we might want to add a new
test return code (SKIP or something).
Rafal Luzynski June 29, 2019, 7 p.m. UTC | #6
28.06.2019 10:49 Florian Weimer <fweimer@redhat.com> wrote:
> 
> malloc dirties the entire allocated memory region due to M_PERTURB
> in the test harness.
> 
> 2019-06-28  Florian Weimer  <fweimer@redhat.com>
> 
> 	* sysdeps/unix/sysv/linux/tst-getdents64.c (large_buffer_checks):
> 	Use mmap instead of malloc.  malloc with M_PERTURB writes to the
> 	entire allocated memory range.

Thank you, Florian.  I would like to confirm that now my make check
runs correctly.

Regards,

Rafal

Patch
diff mbox series

diff --git a/sysdeps/unix/sysv/linux/tst-getdents64.c b/sysdeps/unix/sysv/linux/tst-getdents64.c
index 24e77e04d8..8a28e6c67c 100644
--- a/sysdeps/unix/sysv/linux/tst-getdents64.c
+++ b/sysdeps/unix/sysv/linux/tst-getdents64.c
@@ -27,6 +27,7 @@ 
 #include <support/check.h>
 #include <support/support.h>
 #include <support/xunistd.h>
+#include <sys/mman.h>
 #include <unistd.h>
 
 /* Called by large_buffer_checks below.  */
@@ -53,8 +54,13 @@  large_buffer_checks (int fd)
   size_t large_buffer_size;
   if (!__builtin_add_overflow (UINT_MAX, 2, &large_buffer_size))
     {
-      char *large_buffer = malloc (large_buffer_size);
-      if (large_buffer == NULL)
+      int flags = MAP_ANONYMOUS | MAP_PRIVATE;
+#ifdef MAP_NORESERVE
+      flags |= MAP_NORESERVE;
+#endif
+      void *large_buffer = mmap (NULL, large_buffer_size,
+                                 PROT_READ | PROT_WRITE, flags, -1, 0);
+      if (large_buffer == MAP_FAILED)
         printf ("warning: could not allocate %zu bytes of memory,"
                 " subtests skipped\n", large_buffer_size);
       else
@@ -65,8 +71,8 @@  large_buffer_checks (int fd)
           large_buffer_check (fd, large_buffer, UINT_MAX);
           large_buffer_check (fd, large_buffer, (size_t) UINT_MAX + 1);
           large_buffer_check (fd, large_buffer, (size_t) UINT_MAX + 2);
+          xmunmap (large_buffer, large_buffer_size);
         }
-      free (large_buffer);
     }
 }