nptl: Fix MADV_GUARD_INSTALL logic for thread without guard page (BZ 33356)release/2.42/master

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>
(cherry picked from commit 855bfa2566bbefefa27c516b344df58a75824a5c)

3 files changed, 59 insertions, 25 deletions

diff --git a/NEWS b/NEWS
index 06c27a8..ed3c114 100644
--- a/NEWS
+++ b/NEWS
@@ -13,6 +13,8 @@ The following bugs were resolved with this release:
   [32994] stdlib: resolve a double lock init issue after fork
   [33234] Use TLS initial-exec model for __libc_tsd_CTYPE_* thread variables
   [33245] nptl: nptl: error in internal cancellation syscall handling
+  [33356] nptl: creating thread stack with guardsize 0 can erroneously
+    conclude MADV_GUARD_INSTALL is available
   [33361] nss: Group merge does not react to ERANGE during merge
 
 Version 2.42
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 800ca89..fb8a60a 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -240,7 +240,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
@@ -258,11 +258,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 e3e06df..1c73d3f 100644
--- 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>
@@ -202,7 +203,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);
@@ -227,7 +228,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);
@@ -250,18 +251,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);
@@ -277,9 +269,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);
@@ -299,7 +300,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);
@@ -320,7 +321,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);
@@ -346,21 +347,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;