Re: [PATCH 1/4] Add framework for tunables

[Siddhesh Poyarekar <siddhesh at sourceware dot org>]

On Tuesday 27 December 2016 04:38 PM, Florian Weimer wrote:
> On 11/16/2016 09:35 AM, Siddhesh Poyarekar wrote:
>> diff --git a/README.tunables b/README.tunables
> 
>> +The tunable framework allows modules within glibc to register
>> variables that
>> +may be tweaked through an environment variable or an API call.  It
>> aims to
> 
> This seems outdated, change with an API call is no longer supported.

Yeah, that's an old relic.  Fixed

>> +enforce a strict namespace rule to bring consistency to naming of
>> these tunable
>> +environment variables across the project.  This document is a guide
>> for glibc
>> +developers to add tunables to the framework.
>> +
>> +ADDING A NEW TUNABLE
>> +--------------------
>> +
>> +The TOP_NAMESPACE is defined by default as 'glibc' and it may be
>> overridden in
> 
> “The TOP_NAMESPACE macro”?

Fixed.

> 
> It's not clear based on this description if distributions are supposed
> to change the default definition for glibc as a whole (I don't think
> so), or where they are supposed to override the macro.
> 

I'll redo the description a bit.

>> diff --git a/csu/libc-start.c b/csu/libc-start.c
>> index 99c040a..4e5cafa 100644
>> --- a/csu/libc-start.c
>> +++ b/csu/libc-start.c
>> @@ -21,6 +21,11 @@
>>  #include <unistd.h>
>>  #include <ldsodefs.h>
>>  #include <exit-thread.h>
>> +#include <libc-internal.h>
>> +
>> +#if HAVE_TUNABLES
>> +# include <elf/dl-tunables.h>
>> +#endif
> 
> I'd prefer if you could cut down the number of HAVE_TUNABLES conditionals.
> 
>> +  /* Initialize very early so that tunables can use it.  */
>> +  __libc_init_secure ();
>> +
>> +#if HAVE_TUNABLES
>> +  __tunables_init (__environ);
>> +#endif
> 
> For !HAVE_TUNABLES, __tunables_init could be defined an empty inline
> function, so that no code is generated for it.
> 

OK.

>> diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
> 
>> +#ifndef _TUNABLE_TYPES_H_
>> +# define _TUNABLE_TYPES_H_
> 
> No indentation here.
> 
>> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
>> new file mode 100644
>> index 0000000..91340b6
>> --- /dev/null
>> +++ b/elf/dl-tunables.c
>> @@ -0,0 +1,310 @@
>> +/* The tunable framework.  See the README to know how to use the
>> tunable in
> 
> Should be “README.tunables”.

OK.

> Parts of this file should probably go into csu/ because the code runs so
> early during library startup (see my other comments).

OK, I'll need to figure out how to split things up.

>> +static int
>> +tunables_unsetenv (char **ep, const char *name)
> 
> This is copied from elf/dl-environ.c, it seems.  Is it possible to avoid
> creating this copy?

I'm going to eventually drop the dl-environ.c copy by moving the LD_*
variables to tunables.

> 
> Calling either form of unsetenv is problematic because it may make the
> auxiliary vector unreachable on !LIBC_START_MAIN_AUXVEC_ARG platforms
> (which seems to include the majority, so perhaps this is a non-issue).

I'm not sure what the problem is here.  Either way, it doesn't seem like
something the patch introduces, is it?

>> +/* If the value does not validate, don't bother initializing the
>> internal type
>> +   and also take care to clear the recorded string value in STRVAL.  */
>> +#define RETURN_IF_INVALID_RANGE(__cur, __val) \
>> +({                                          \
>> +  __typeof ((__cur)->type.min) min =
>> (__cur)->type.min;                  \
>> +  __typeof ((__cur)->type.max) max =
>> (__cur)->type.max;                  \
>> +  if (min != max && ((__val) < min || (__val) > max))                  \
>> +    return;                                      \
>> +})
> 
> Is it really necessary to turn this into a macro instead of an (inline)
> function?  The return statement could very well be located in the
> caller, IMHO.

I rewrote the function to explicitly check for bounds based on type.

> 
>> +/* Disable a tunable if it is set.  */
>> +static void
>> +disable_tunable (tunable_id_t id, char **envp)
>> +{
>> +  const char *env_alias = tunable_list[id].env_alias;
>> +
>> +  if (env_alias)
>> +    tunables_unsetenv (envp, tunable_list[id].env_alias);
>> +}
> 
> env_alias != NULL.
> 
>> +    default:
>> +      __builtin_unreachable ();
> 
> Should this rather be __bultin_trap?

OK.

> 
>> diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
> 
>> +/* A tunable.  */
>> +struct _tunable
>> +{
> 
>> +  const char *env_alias;        /* The compatibility environment
>> +                       variable name.  */
> 
> Maybe store the variable name as well, as to

I don't understand this, can you please elaborate?

> 
>> +/* Same as TUNABLE_SET_VAL, but also set the callback function to
>> __CB and call
>> +   it.  */
>> +# define TUNABLE_SET_VAL_WITH_CALLBACK(__id,__val,__cb) \
> 
> Comment seems misleading, the callback function isn't really set.

It used to be until I changed that.  I'll fix the comment.

> 
>> diff --git a/elf/rtld.c b/elf/rtld.c
>> index 647661c..eb211a2 100644
>> --- a/elf/rtld.c
>> +++ b/elf/rtld.c
>> @@ -2527,11 +2527,13 @@ process_envvars (enum mode *modep)
>>      }
>>        while (*nextp != '\0');
>>
>> +#if !HAVE_TUNABLES
>>        if (__access ("/etc/suid-debug", F_OK) != 0)
>>      {
>>        unsetenv ("MALLOC_CHECK_");
>>        GLRO(dl_debug_mask) = 0;
>>      }
>> +#endif
> 
> I believe this introduces a security vulnerability.  You must keep the
> setting of dl_debug_mask.

I'll add it back in #ifdef SHARED in disable_tunable.

Siddhesh