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

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.
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. | #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. | #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. | #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. | #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. | #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. | #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 --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);
     }
 }