Re: RFC: tunables failure indications...

[Carlos O'Donell <codonell at redhat dot com>]

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.