[PATCH v5 3/4] Add system-wide tunables: Apply tunables part

[Adhemerval Zanella Netto]

On 17/02/26 11:44, Yury Khrustalev wrote:
> On Wed, Feb 21, 2024 at 06:49:50PM -0500, DJ Delorie wrote:
>>
>> Load ld.so.cache and fetch the tunables extension.  Apply
>> those tunables to the current program.  We do not yet apply
>> security policies.
> 
> By "security policies" do you mean "filtering" or something else?
> 
>> diff --git a/elf/Makefile b/elf/Makefile
>> index 42da891f5a..282af88e71 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
>> @@ -91,6 +91,7 @@ dl-routines = \
>>  
>>  ifeq (yes,$(use-ldconfig))
>>  dl-routines += dl-cache
>> +CFLAGS-dl-tunables.c += -DUSE_LDCONFIG
> 
> Shouldn't this be already present in the flags? At least it works without
> this line.
> 
>> ...
>>
>> --- a/elf/dl-cache.c
>> +++ b/elf/dl-cache.c
>> @@ -29,6 +29,7 @@
>>
>> ...
>>
>> +
>> +const struct tunable_header_cached *
>> +_dl_load_cache_tunables (const char **data)
>> +{
>> +  struct cache_extension_all_loaded ext;
>> +
>> +  /* This loads the cache (temporary).  */
>> +  if (_dl_check_ldsocache_needs_loading ())
>> +    _dl_maybe_load_ldsocache ();
>> +
>> +  if (cache_new && cache_new != (void *) -1)
> 
> Can cache_new ever be (void *) -1? Do you mean MAP_FAILED?

The -1 is an internal sentinel used for any mismatched used during cache 
parsing (check _dl_load_cache_lookup).  I think it would be better to wrapper
this logic with more readable wrappers like:

#define DL_CACHE_INVALID ((void*)-1)

static inline bool
dl_cache_valid (struct cache_file_new *)
{
  return cache_new != NULL && cache_new != DL_CACHE_INVALID;
}

> 
>> ...
>>
>> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
>> index 37ade374c6..83808f75e7 100644
>> --- a/elf/dl-tunables.c
>> +++ b/elf/dl-tunables.c
> 
> This seems OK but it needs some good tests to check what happens if the
> tunables in the config file mismatch tunables supported by this Glibc.
> 
>> ...
>>
>> +	      else
>> +		{
>> +		  tunable_initialize (& tunable_list[tid],
>> +				      value, strlen (value));
>> +		}
>> +	    }
>> +	}
>> +    }
>> +#endif
> 
> Nit: probably add
> 
>   // #if defined(SHARED) && defined (USE_LDCONFIG)
> 
> after this #endif since the code block is quite long?
> 
>> ...
>>
>> diff --git a/elf/tunconf.h b/elf/tunconf.h
>> index a6c5f0dd9a..623fb7546d 100644
>> --- a/elf/tunconf.h
>> +++ b/elf/tunconf.h
> 
> OK
>