This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 1/4] Add framework for tunables
- From: Siddhesh Poyarekar <siddhesh at sourceware dot org>
- To: Florian Weimer <fweimer at redhat dot com>, libc-alpha at sourceware dot org
- Cc: carlos at redhat dot com, adhemerval dot zanella at linaro dot org
- Date: Wed, 28 Dec 2016 02:29:28 +0530
- Subject: Re: [PATCH 1/4] Add framework for tunables
- Authentication-results: sourceware.org; auth=none
- References: <1479285306-11684-1-git-send-email-siddhesh@sourceware.org> <1479285306-11684-2-git-send-email-siddhesh@sourceware.org> <37ef7178-e83c-8318-4983-acfb3f422562@redhat.com>
- Reply-to: 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