RFC: tunables failure indications...

RFC: tunables failure indications...

[DJ Delorie]

Re: https://sourceware.org/bugzilla/show_bug.cgi?id=25035
"sbrk() failure handled poorly in tunables_strdup"

The current failure mode for tunables is: if we can't allocate memory,
we delete the tunables env var and pretend it didn't exist, without
telling the user anything happened.

I think we should do better, but I don't know what.

IMHO tunables are "hints" but there should be *some* feedback when a
tunable can't be honored because of internal failures (vs tunables
that don't apply to the current arch, etc).

In the BZ noted above, where *sbrk* fails, I'm inclined to just
SIGSEGV because... well... *sbrk* just failed at the start of a
program.  Something has gone horribly wrong, even if the program
continues to run normally after that (it happens in our own testsuite
sometimes).

The helper functions in malloc return error codes for invalid values,
which the tunables harness completely ignores.  Not even a printf.

So... what is a reasonable thing for the user to expect, when a
tunable they specify cannot be honored?

Re: RFC: tunables failure indications...

[Siddhesh Poyarekar]

On 06/12/19 4:14 am, DJ Delorie wrote:
> Re: https://sourceware.org/bugzilla/show_bug.cgi?id=25035
> "sbrk() failure handled poorly in tunables_strdup"

As I understand it, there seem to be a few different problems in that bug:

1. The env gets updated even when strdup returns NULL, which is a bug
2. Tunables may get ignored on internal errors
3. Tunables may may get on invalid input
4. brk failed

> The current failure mode for tunables is: if we can't allocate memory,
> we delete the tunables env var and pretend it didn't exist, without
> telling the user anything happened.

That was a deliberate design decision back then because tunables were
considered non-critical, i.e. they're not guaranteed to work in all
situations and as such, shouldn't hamper core functionality just because
it couldn't work.  So problem (2) and (3) are by design.

> I think we should do better, but I don't know what.
> 
> IMHO tunables are "hints" but there should be *some* feedback when a
> tunable can't be honored because of internal failures (vs tunables
> that don't apply to the current arch, etc).
> 
> In the BZ noted above, where *sbrk* fails, I'm inclined to just
> SIGSEGV because... well... *sbrk* just failed at the start of a
> program.  Something has gone horribly wrong, even if the program
> continues to run normally after that (it happens in our own testsuite
> sometimes).

That's a fair point.  I would agree with a patch that fails
catastrophically on a failed brk or similar internal errors as opposed
to invalid values from users.  That is, we make (2) fail with an error exit.

> The helper functions in malloc return error codes for invalid values,
> which the tunables harness completely ignores.  Not even a printf.

This replicates what we used to do earlier when setting the
MALLOC_MMAP_MAX_, etc environment variables and it follows the same
principle that it shouldn't hamper core functionality.  Maybe there's a
valid use case to have debug messages (when built with a flag set for
example) that get explicitly enabled to dump warnings on invalid tunable
values.

As for (4), that's super-odd even with randomization.  Is it possible
that somehow brk with ASLR enabled fails when the new brk end is not
page aligned?

Siddhesh

Re: RFC: tunables failure indications...

[Carlos O'Donell]

On 12/6/19 3:32 AM, Siddhesh Poyarekar wrote:
> On 06/12/19 4:14 am, DJ Delorie wrote:
>> Re: https://sourceware.org/bugzilla/show_bug.cgi?id=25035
>> "sbrk() failure handled poorly in tunables_strdup"
> 
> As I understand it, there seem to be a few different problems in that bug:
> 
> 1. The env gets updated even when strdup returns NULL, which is a bug
> 2. Tunables may get ignored on internal errors
> 3. Tunables may may get on invalid input
> 4. brk failed
> 
>> The current failure mode for tunables is: if we can't allocate memory,
>> we delete the tunables env var and pretend it didn't exist, without
>> telling the user anything happened.
> 
> That was a deliberate design decision back then because tunables were
> considered non-critical, i.e. they're not guaranteed to work in all
> situations and as such, shouldn't hamper core functionality just because
> it couldn't work.  So problem (2) and (3) are by design.
> 
>> I think we should do better, but I don't know what.
>>
>> IMHO tunables are "hints" but there should be *some* feedback when a
>> tunable can't be honored because of internal failures (vs tunables
>> that don't apply to the current arch, etc).
>>
>> In the BZ noted above, where *sbrk* fails, I'm inclined to just
>> SIGSEGV because... well... *sbrk* just failed at the start of a
>> program.  Something has gone horribly wrong, even if the program
>> continues to run normally after that (it happens in our own testsuite
>> sometimes).
> 
> That's a fair point.  I would agree with a patch that fails
> catastrophically on a failed brk or similar internal errors as opposed
> to invalid values from users.  That is, we make (2) fail with an error exit.

I agree. Not having enough memory to carry out operations like reading
the tunables should be a critical failure.

The tunables are environment variables so they will naturally be limited
to the size of the usuable environment, and so this is all normally
expected behaviour.

>> The helper functions in malloc return error codes for invalid values,
>> which the tunables harness completely ignores.  Not even a printf.
> 
> This replicates what we used to do earlier when setting the
> MALLOC_MMAP_MAX_, etc environment variables and it follows the same
> principle that it shouldn't hamper core functionality.  Maybe there's a
> valid use case to have debug messages (when built with a flag set for
> example) that get explicitly enabled to dump warnings on invalid tunable
> values.

Tunables are inherently functionality that the loader is brining online
during process startup.

It may be sensible to add a LD_DEBUG=tunables to that list and print more
verbose tunables processing information?

> As for (4), that's super-odd even with randomization.  Is it possible
> that somehow brk with ASLR enabled fails when the new brk end is not
> page aligned?

No, in the case DJ and I were looking at this was a static vs. ASLR vs.
kernel VMA layout issue where we still have kernel issues on less
maintstream architectures. We still need to fail safe in those cases
and I think we should just shut the process down with appropriate error
messages.

-- 
Cheers,
Carlos.

Re: RFC: tunables failure indications...

[Siddhesh Poyarekar]

On 10/12/19 9:55 pm, Carlos O'Donell wrote:
> Tunables are inherently functionality that the loader is brining online
> during process startup.
> 
> It may be sensible to add a LD_DEBUG=tunables to that list and print more
> verbose tunables processing information?

That sounds reasonable.

>> As for (4), that's super-odd even with randomization.  Is it possible
>> that somehow brk with ASLR enabled fails when the new brk end is not
>> page aligned?
> 
> No, in the case DJ and I were looking at this was a static vs. ASLR vs.
> kernel VMA layout issue where we still have kernel issues on less
> maintstream architectures. We still need to fail safe in those cases
> and I think we should just shut the process down with appropriate error
> messages.

OK, I was just wondering if this was a kernel bug.  Either ways,
shutting down with an error sounds good, preferably in the ld.so way
(i.e. write to stderr and _exit()) rather than a forced segfault.

Siddhesh

Re: RFC: tunables failure indications...

[DJ Delorie]

Siddhesh Poyarekar <siddhesh@gotplt.org> writes:
>> No, in the case DJ and I were looking at this was a static vs. ASLR vs.
>> kernel VMA layout issue where we still have kernel issues on less
>> maintstream architectures. We still need to fail safe in those cases
>> and I think we should just shut the process down with appropriate error
>> messages.
>
> OK, I was just wondering if this was a kernel bug.  Either ways,
> shutting down with an error sounds good, preferably in the ld.so way
> (i.e. write to stderr and _exit()) rather than a forced segfault.

How about this?

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index e625ac1a7d..b55d677aee 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -45,12 +45,15 @@ tunables_strdup (const char *in)
   while (in[i++] != '\0');
   char *out = __sbrk (i);
 
-  /* FIXME: In reality if the allocation fails, __sbrk will crash attempting to
-     set the thread-local errno since the TCB has not yet been set up.  This
-     needs to be fixed with an __sbrk implementation that does not set
-     errno.  */
+  /* For most of the tunables code, we ignore user errors.  However,
+     this is a system error - and running out of memory at program
+     startup should be reported, so we do.  */
   if (out == (void *)-1)
-    return NULL;
+    {
+#define SBRKMSG "sbrk() failure while processing tunables"
+      write (2, SBRKMSG, sizeof(SBRKMSG) - 1);
+      _exit (1);
+    }
 
   i--;
 

Re: RFC: tunables failure indications...

[Siddhesh Poyarekar]

On 12/12/19 3:30 am, DJ Delorie wrote:
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index e625ac1a7d..b55d677aee 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -45,12 +45,15 @@ tunables_strdup (const char *in)
>    while (in[i++] != '\0');
>    char *out = __sbrk (i);
>  
> -  /* FIXME: In reality if the allocation fails, __sbrk will crash attempting to
> -     set the thread-local errno since the TCB has not yet been set up.  This
> -     needs to be fixed with an __sbrk implementation that does not set
> -     errno.  */
> +  /* For most of the tunables code, we ignore user errors.  However,
> +     this is a system error - and running out of memory at program
> +     startup should be reported, so we do.  */
>    if (out == (void *)-1)
> -    return NULL;
> +    {
> +#define SBRKMSG "sbrk() failure while processing tunables"
> +      write (2, SBRKMSG, sizeof(SBRKMSG) - 1);
> +      _exit (1);

I think this could be a _dl_fatal_printf() or similar.

Siddhesh

Re: RFC: tunables failure indications...

[DJ Delorie]

Siddhesh Poyarekar <siddhesh@gotplt.org> writes:
> I think this could be a _dl_fatal_printf() or similar.

_dl_fatal_printf() is for ld.so; tunables are in the main library.  I
did look around a bit but couldn't find something obvious that would
work before stdio was initialized, and without a working malloc.

Re: RFC: tunables failure indications...

[Siddhesh Poyarekar]

On 12/12/19 9:33 am, DJ Delorie wrote:
> Siddhesh Poyarekar <siddhesh@gotplt.org> writes:
>> I think this could be a _dl_fatal_printf() or similar.
> 
> _dl_fatal_printf() is for ld.so; tunables are in the main library.  I
> did look around a bit but couldn't find something obvious that would
> work before stdio was initialized, and without a working malloc.
> 

True, the tunables are in the main library, but the tunables framework
itself (i.e. dl-tunables.c) is part of the dynamic linker.
_dl_fatal_printf and everything it calls should have been linked into ld.so.

Siddhesh

Re: RFC: tunables failure indications...

[Carlos O'Donell]

On 12/11/19 11:08 PM, Siddhesh Poyarekar wrote:
> On 12/12/19 9:33 am, DJ Delorie wrote:
>> Siddhesh Poyarekar <siddhesh@gotplt.org> writes:
>>> I think this could be a _dl_fatal_printf() or similar.
>>
>> _dl_fatal_printf() is for ld.so; tunables are in the main library.  I
>> did look around a bit but couldn't find something obvious that would
>> work before stdio was initialized, and without a working malloc.
>>
> 
> True, the tunables are in the main library, but the tunables framework
> itself (i.e. dl-tunables.c) is part of the dynamic linker.
> _dl_fatal_printf and everything it calls should have been linked into ld.so.

Agreed, it should be possible to use _dl_fatal_printf.

Does it not work?

-- 
Cheers,
Carlos.

Re: RFC: tunables failure indications...

[DJ Delorie]

?

From d64d1ddf3d79a1e51612f8e4ae3ecfd020246275 Mon Sep 17 00:00:00 2001
From: DJ Delorie <dj@redhat.com>
Date: Fri, 13 Dec 2019 13:36:58 -0500
Subject: tunables: report sbrk() failure


diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index e625ac1a7d..3a3f8096c2 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -45,12 +45,14 @@ tunables_strdup (const char *in)
   while (in[i++] != '\0');
   char *out = __sbrk (i);
 
-  /* FIXME: In reality if the allocation fails, __sbrk will crash attempting to
-     set the thread-local errno since the TCB has not yet been set up.  This
-     needs to be fixed with an __sbrk implementation that does not set
-     errno.  */
+  /* For most of the tunables code, we ignore user errors.  However,
+     this is a system error - and running out of memory at program
+     startup should be reported, so we do.  */
   if (out == (void *)-1)
-    return NULL;
+    {
+      _dl_fatal_printf("sbrk() failure while processing tunables\n");
+      _exit (1);
+    }
 
   i--;
 

Re: RFC: tunables failure indications...

[Carlos O'Donell]

On 12/13/19 1:48 PM, DJ Delorie wrote:
> 
> ?
> 

Almost. _dl_fatal_printf is noreturn and calls _exit (127).

So all you need is the call to _dl_fatal_printf.

> From d64d1ddf3d79a1e51612f8e4ae3ecfd020246275 Mon Sep 17 00:00:00 2001
> From: DJ Delorie <dj@redhat.com>
> Date: Fri, 13 Dec 2019 13:36:58 -0500
> Subject: tunables: report sbrk() failure
> 
> 
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index e625ac1a7d..3a3f8096c2 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -45,12 +45,14 @@ tunables_strdup (const char *in)
>    while (in[i++] != '\0');
>    char *out = __sbrk (i);
>  
> -  /* FIXME: In reality if the allocation fails, __sbrk will crash attempting to
> -     set the thread-local errno since the TCB has not yet been set up.  This
> -     needs to be fixed with an __sbrk implementation that does not set
> -     errno.  */
> +  /* For most of the tunables code, we ignore user errors.  However,
> +     this is a system error - and running out of memory at program
> +     startup should be reported, so we do.  */
>    if (out == (void *)-1)
> -    return NULL;
> +    {
> +      _dl_fatal_printf("sbrk() failure while processing tunables\n");
> +      _exit (1);
> +    }
>  
>    i--;
>  
> 


-- 
Cheers,
Carlos.

Re: RFC: tunables failure indications...

[DJ Delorie]

"Carlos O'Donell" <codonell@redhat.com> writes:
> Almost. _dl_fatal_printf is noreturn and calls _exit (127).

Done.

From b50d6a577bf2f118923ce0e1b159c4d659815405 Mon Sep 17 00:00:00 2001
From: DJ Delorie <dj@redhat.com>
Date: Fri, 13 Dec 2019 13:36:58 -0500
Subject: tunables: report sbrk() failure


diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index e625ac1a7d..f0d7d42770 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -45,12 +45,11 @@ tunables_strdup (const char *in)
   while (in[i++] != '\0');
   char *out = __sbrk (i);
 
-  /* FIXME: In reality if the allocation fails, __sbrk will crash attempting to
-     set the thread-local errno since the TCB has not yet been set up.  This
-     needs to be fixed with an __sbrk implementation that does not set
-     errno.  */
+  /* For most of the tunables code, we ignore user errors.  However,
+     this is a system error - and running out of memory at program
+     startup should be reported, so we do.  */
   if (out == (void *)-1)
-    return NULL;
+    _dl_fatal_printf ("sbrk() failure while processing tunables\n");
 
   i--;
 

Re: RFC: tunables failure indications...

[Carlos O'Donell]

On 12/13/19 3:21 PM, DJ Delorie wrote:
> "Carlos O'Donell" <codonell@redhat.com> writes:
>> Almost. _dl_fatal_printf is noreturn and calls _exit (127).
> 
> Done.
> 
> From b50d6a577bf2f118923ce0e1b159c4d659815405 Mon Sep 17 00:00:00 2001
> From: DJ Delorie <dj@redhat.com>
> Date: Fri, 13 Dec 2019 13:36:58 -0500
> Subject: tunables: report sbrk() failure
> 

LGTM for master if you proved this works by using gdb to set out to void
and confirmed it exits with the right error code and exits via _exit
(had a breakpoint on _exit and it triggers without showing exit on the
call chain).

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index e625ac1a7d..f0d7d42770 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -45,12 +45,11 @@ tunables_strdup (const char *in)
>    while (in[i++] != '\0');
>    char *out = __sbrk (i);
>  
> -  /* FIXME: In reality if the allocation fails, __sbrk will crash attempting to
> -     set the thread-local errno since the TCB has not yet been set up.  This
> -     needs to be fixed with an __sbrk implementation that does not set
> -     errno.  */
> +  /* For most of the tunables code, we ignore user errors.  However,
> +     this is a system error - and running out of memory at program
> +     startup should be reported, so we do.  */
>    if (out == (void *)-1)
> -    return NULL;
> +    _dl_fatal_printf ("sbrk() failure while processing tunables\n");
>  
>    i--;
>  
> 


-- 
Cheers,
Carlos.

Re: RFC: tunables failure indications...

[DJ Delorie]

"Carlos O'Donell" <codonell@redhat.com> writes:
> LGTM for master if you proved this works by using gdb to set out to void
> and confirmed it exits with the right error code and exits via _exit
> (had a breakpoint on _exit and it triggers without showing exit on the
> call chain).

Done and pushed.  Thanks!