nptl: Fix MADV_GUARD_INSTALL logic for thread without guard page (BZ 33356)

The main issue is that setup_stack_prot fails to account for cases where
the cached thread stack lacks a guard page, which can cause madvise to
fail. Update the logic to also handle whether MADV_GUARD_INSTALL is
supported when resizing the guard page.

Checked on x86_64-linux-gnu with 6.8.0 and 6.15 kernels.

Reviewed-by: Florian Weimer <fweimer@redhat.com>

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 4c2aacdd10153e8a1728c98687e40b09722c445d..99f56b94dfedb795dedb2095db0e7f1768d9ed1e 100644 (file)
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -234,7 +234,7 @@ setup_stack_prot (char *mem, size_t size, struct pthread *pd,
 /* Update the guard area of the thread stack MEM of size SIZE with the new
    GUARDISZE.  It uses the method defined by PD stack_mode.  */
 static inline bool
-adjust_stack_prot (char *mem, size_t size, const struct pthread *pd,
+adjust_stack_prot (char *mem, size_t size, struct pthread *pd,
                   size_t guardsize, size_t pagesize_m1)
 {
   /* The required guard area is larger than the current one.  For
@@ -252,11 +252,23 @@ adjust_stack_prot (char *mem, size_t size, const struct pthread *pd,
      so use the new guard placement with the new size.  */
   if (guardsize > pd->guardsize)
     {
+      /* There was no need to previously setup a guard page, so we need
+        to check whether the kernel supports guard advise.  */
       char *guard = guard_position (mem, size, guardsize, pd, pagesize_m1);
-      if (pd->stack_mode == ALLOCATE_GUARD_MADV_GUARD)
-       return __madvise (guard, guardsize, MADV_GUARD_INSTALL) == 0;
-      else if (pd->stack_mode == ALLOCATE_GUARD_PROT_NONE)
-       return __mprotect (guard, guardsize, PROT_NONE) == 0;
+      if (atomic_load_relaxed (&allocate_stack_mode)
+         == ALLOCATE_GUARD_MADV_GUARD)
+       {
+         if (__madvise (guard, guardsize, MADV_GUARD_INSTALL) == 0)
+           {
+             pd->stack_mode = ALLOCATE_GUARD_MADV_GUARD;
+             return true;
+           }
+         atomic_store_relaxed (&allocate_stack_mode,
+                               ALLOCATE_GUARD_PROT_NONE);
+       }
+
+      pd->stack_mode = ALLOCATE_GUARD_PROT_NONE;
+      return __mprotect (guard, guardsize, PROT_NONE) == 0;
     }
   /* The current guard area is larger than the required one.  For
      _STACK_GROWS_DOWN is means change the guard as:

diff --git a/nptl/tst-guard1.c b/nptl/tst-guard1.c
index 6732b090171bdc51c6f76708778fb2d03cf709ea..8abdbf44f4f087fe22137c91ded27cd359e20589 100644 (file)
--- a/nptl/tst-guard1.c
+++ b/nptl/tst-guard1.c
@@ -21,6 +21,7 @@
 #include <setjmp.h>
 #include <stackinfo.h>
 #include <stdio.h>
+#include <support/capture_subprocess.h>
 #include <support/check.h>
 #include <support/test-driver.h>
 #include <support/xsignal.h>
@@ -170,7 +171,7 @@ tf (void *closure)
 
 /* Test 1: caller provided stack without guard.  */
 static void
-do_test1 (void)
+do_test1 (void *closure)
 {
   pthread_attr_t attr;
   xpthread_attr_init (&attr);
@@ -195,7 +196,7 @@ do_test1 (void)
 
 /* Test 2: same as 1., but with a guard area.  */
 static void
-do_test2 (void)
+do_test2 (void *closure)
 {
   pthread_attr_t attr;
   xpthread_attr_init (&attr);
@@ -218,18 +219,9 @@ do_test2 (void)
   xmunmap (stack, stacksize);
 }
 
-/* Test 3: pthread_create with default values.  */
+/* Test 3: pthread_create without a guard area.  */
 static void
-do_test3 (void)
-{
-  pthread_t t = xpthread_create (NULL, tf, NULL);
-  void *status = xpthread_join (t);
-  TEST_VERIFY (status == 0);
-}
-
-/* Test 4: pthread_create without a guard area.  */
-static void
-do_test4 (void)
+do_test3 (void *closure)
 {
   pthread_attr_t attr;
   xpthread_attr_init (&attr);
@@ -245,9 +237,18 @@ do_test4 (void)
   xpthread_attr_destroy (&attr);
 }
 
+/* Test 4: pthread_create with default values.  */
+static void
+do_test4 (void *closure)
+{
+  pthread_t t = xpthread_create (NULL, tf, NULL);
+  void *status = xpthread_join (t);
+  TEST_VERIFY (status == 0);
+}
+
 /* Test 5: pthread_create with non default stack and guard size value.  */
 static void
-do_test5 (void)
+do_test5 (void *closure)
 {
   pthread_attr_t attr;
   xpthread_attr_init (&attr);
@@ -267,7 +268,7 @@ do_test5 (void)
    test 3, but with a larger guard area.  The pthread_create will need to
    increase the guard area.  */
 static void
-do_test6 (void)
+do_test6 (void *closure)
 {
   pthread_attr_t attr;
   xpthread_attr_init (&attr);
@@ -288,7 +289,7 @@ do_test6 (void)
    pthread_create should use the cached stack from previous tests, but it
    would require to reduce the guard area.  */
 static void
-do_test7 (void)
+do_test7 (void *closure)
 {
   pthread_t t = xpthread_create (NULL, tf, NULL);
   void *status = xpthread_join (t);
@@ -302,21 +303,40 @@ do_test (void)
 
   static const struct {
     const char *descr;
-    void (*test)(void);
+    void (*test) (void *);
   } tests[] = {
     { "user provided stack without guard", do_test1 },
     { "user provided stack with guard",    do_test2 },
-    { "default attribute",                 do_test3 },
-    { "default attribute without guard",   do_test4 },
+    /* N.B: do_test3 should be before do_test4 to check if a new thread
+       that uses the thread stack previously allocated without a guard
+       page correctly sets up the guard pages even on a kernel without
+       MADV_GUARD_INSTALL support (BZ 33356).  */
+    { "default attribute without guard",   do_test3 },
+    { "default attribute",                 do_test4 },
+    /* Also checks if the guard is correctly removed from the cache thread
+       stack.  */
+    { "default attribute without guard",   do_test3 },
     { "non default stack and guard sizes", do_test5 },
     { "reused stack with larger guard",    do_test6 },
     { "reused stack with smaller guard",   do_test7 },
   };
 
+  /* Run each test with a clean state.  */
+  for (int i = 0; i < array_length (tests); i++)
+    {
+      printf ("debug: fork: test%01d: %s\n", i, tests[i].descr);
+      struct support_capture_subprocess result =
+       support_capture_subprocess (tests[i].test, NULL);
+      support_capture_subprocess_check (&result, tests[i].descr, 0,
+                                       sc_allow_none);
+      support_capture_subprocess_free (&result);
+    }
+
+  /* And now run the same tests along with the thread stack cache.  */
   for (int i = 0; i < array_length (tests); i++)
     {
       printf ("debug: test%01d: %s\n", i, tests[i].descr);
-      tests[i].test();
+      tests[i].test ( NULL);
     }
 
   return 0;