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

[Florian Weimer <fweimer@redhat.com>]

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.

> +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”?

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.

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

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

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

> +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?

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

> +/* 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.

> +/* 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?

> 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

> +/* 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.

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

Thanks,
Florian