[PATCH] nplt: Fix MADV_GUARD_INSTALL logic for thread without guard page (BZ 33356)

[Florian Weimer]

* Adhemerval Zanella:

> 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.

(nptl typo, as mentioned)

> ---
>  nptl/allocatestack.c | 22 +++++++++++++++++-----
>  nptl/tst-guard1.c    | 31 +++++++++++++++++++------------
>  2 files changed, 36 insertions(+), 17 deletions(-)
>
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index 4c2aacdd10..61838eed8a 100644
> --- 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 parge, so we need

typo: pa[]ge

> +	 check whether the kernel supports guard advise.  */

typo: need [to] check

>        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 e3e06df0fc..2c597a6a34 100644
> --- a/nptl/tst-guard1.c
> +++ b/nptl/tst-guard1.c
> @@ -250,18 +250,9 @@ do_test2 (void)
>    xmunmap (stack, stacksize);
>  }

I'd suggest to run each test after fork, in separate processes, and then
once in the same process, for better test coverage.

> @@ -350,8 +350,15 @@ do_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 setup the guard pages even on kernel without
> +       MADV_GUARD_INSTALL support (BZ 33356).  */

typos: set[s ]up, on [a] kernel

Thanks,
Florian