From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 60983 invoked by alias); 27 Dec 2016 20:59:35 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 60973 invoked by uid 89); 27 Dec 2016 20:59:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.6 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_NEUTRAL autolearn=no version=3.3.2 spammy=Either, rewrote, H*R:D*sourceware.org, SHARED X-HELO: homiemail-a47.g.dreamhost.com Reply-To: siddhesh@sourceware.org Subject: Re: [PATCH 1/4] Add framework for tunables 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> To: Florian Weimer , libc-alpha@sourceware.org Cc: carlos@redhat.com, adhemerval.zanella@linaro.org From: Siddhesh Poyarekar Message-ID: Date: Tue, 27 Dec 2016 20:59:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <37ef7178-e83c-8318-4983-acfb3f422562@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2016-12/txt/msg01007.txt.bz2 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 >=20 >> +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 >=20 > 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 >=20 > =93The TOP_NAMESPACE macro=94? Fixed. >=20 > 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. >=20 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 >> #include >> #include >> +#include >> + >> +#if HAVE_TUNABLES >> +# include >> +#endif >=20 > I'd prefer if you could cut down the number of HAVE_TUNABLES conditionals. >=20 >> + /* Initialize very early so that tunables can use it. */ >> + __libc_init_secure (); >> + >> +#if HAVE_TUNABLES >> + __tunables_init (__environ); >> +#endif >=20 > For !HAVE_TUNABLES, __tunables_init could be defined an empty inline > function, so that no code is generated for it. >=20 OK. >> diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h >=20 >> +#ifndef _TUNABLE_TYPES_H_ >> +# define _TUNABLE_TYPES_H_ >=20 > No indentation here. >=20 >> 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 >=20 > Should be =93README.tunables=94. 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) >=20 > 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. >=20 > 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 =3D >> (__cur)->type.min; \ >> + __typeof ((__cur)->type.max) max =3D >> (__cur)->type.max; \ >> + if (min !=3D max && ((__val) < min || (__val) > max)) = \ >> + return; \ >> +}) >=20 > 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. >=20 >> +/* Disable a tunable if it is set. */ >> +static void >> +disable_tunable (tunable_id_t id, char **envp) >> +{ >> + const char *env_alias =3D tunable_list[id].env_alias; >> + >> + if (env_alias) >> + tunables_unsetenv (envp, tunable_list[id].env_alias); >> +} >=20 > env_alias !=3D NULL. >=20 >> + default: >> + __builtin_unreachable (); >=20 > Should this rather be __bultin_trap? OK. >=20 >> diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h >=20 >> +/* A tunable. */ >> +struct _tunable >> +{ >=20 >> + const char *env_alias; /* The compatibility environment >> + variable name. */ >=20 > Maybe store the variable name as well, as to I don't understand this, can you please elaborate? >=20 >> +/* 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) \ >=20 > Comment seems misleading, the callback function isn't really set. It used to be until I changed that. I'll fix the comment. >=20 >> 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 !=3D '\0'); >> >> +#if !HAVE_TUNABLES >> if (__access ("/etc/suid-debug", F_OK) !=3D 0) >> { >> unsetenv ("MALLOC_CHECK_"); >> GLRO(dl_debug_mask) =3D 0; >> } >> +#endif >=20 > 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