[PATCH v7 1/4] Add system-wide tunables: ldconfig part

[Yury Khrustalev]

On Mon, Dec 04, 2023 at 08:48:18PM -0500, DJ Delorie wrote:
> Adds support for reading /etc/tunables.conf
> 
> The file contains one line per tunable, like this:
> 
> glibc.foo.bar=14
> glibc.malloc.more=0
> 
> Additionally, each line can be prefixed with a single character
> that controls overridability by the GLIBC_TUNABLES env var:
> 
> !glibc.foo=0
>    ^ May be made more secure

I think we should not do this. Allowing and forbidding override
via env variable should be enough. It is not possible to define
what "more secure" means unless we require this to be done for
all existing tunables retrospectively. Also, some tunables are
not related to security. In a nutshell, this "may be made more
something" is highly speculative and will only lead to confusion.

> +glibc.foo=0
>    ^ May be overridden

If there is no prefix, does it mean the same as '+'?

> -glibc.foo=0
>    ^ May not be overridden
> 
> Internally, each tunable will later have a default "overridability"
> and logic for what "more secure" means.

This is over-complicating things and I don't see much value in it.

> The tunable cache format allows for a filter to be assigned to
> each tunable, to be used at program start to decide if a tunable
> applies to that program.  No such filters have yet been specified.
> 
> The cache format also stores a pre-parsed value for the tunable, and
> the ID of the tunable, to improve load-time performance.

OK

> ...
>
> diff --git a/elf/Makefile b/elf/Makefile
> index 7f039b5563..6b366df91b 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile

OK

> ...
>
> diff --git a/elf/cache.c b/elf/cache.c
> index a6dc85dc0f..fe8f66366f 100644
> --- a/elf/cache.c
> +++ b/elf/cache.c
> @@ -36,6 +36,7 @@
>  #include <dl-cache.h>
>  #include <version.h>
>  #include <stringtable.h>
> +#include <tunconf.h>
>  
>  /* Used to store library names, paths, and other strings.  */
>  static struct stringtable strings;
> @@ -275,7 +276,8 @@ check_new_cache (struct cache_file_new *cache)
>  
>  /* Print the extension information in *EXT.  */
>  static void
> -print_extensions (struct cache_extension_all_loaded *ext)
> +print_extensions (struct cache_extension_all_loaded *ext,
> +		  const char *cache_data)
>  {
>    if (ext->sections[cache_extension_tag_generator].base != NULL)
>      {
> @@ -284,6 +286,34 @@ print_extensions (struct cache_extension_all_loaded *ext)
>  	      ext->sections[cache_extension_tag_generator].size, stdout);
>        putchar ('\n');
>      }
> +  if (ext->sections[cache_extension_tag_tunables].base != NULL)
> +    {
> +      struct tunable_header_cached *thc;
> +      struct tunable_entry_cached *tec;
> +      int i, count;
> +
> +      thc = (struct tunable_header_cached *)
> +	ext->sections[cache_extension_tag_tunables].base;
> +      tec = thc->tunables;
> +      count = thc->num_tunables;
> +      printf("tunables sig 0x%08x ver 0x%08x count %u\n",
> +	     thc->signature, thc->version, thc->num_tunables);
> +      /* Check that COUNT won't overflow our data block.  */
> +      assert (ext->sections[cache_extension_tag_tunables].base
> +	      + ext->sections[cache_extension_tag_tunables].size
> +	      >= (void *) & tec[count]);
> +      for (i = 0; i < count; ++ i)
> +	{
> +	  printf("  [%d] %s : %s [flags 0x%08x",
> +		 i,
> +		 cache_data + tec[i].name_offset,
> +		 cache_data + tec[i].value_offset,
> +		 tec[i].flags);
> +	  if (tec[i].flag_offset != 0)
> +	    printf(" : %s", cache_data + tec[i].flag_offset);
> +	  printf("]\n");
> +	}
> +    }
>  }

I find the output quite difficult to understand:

  tunables sig 0x7c3ba94f ver 0x01000000 count 2
    [0] glibc.cpu.aarch64_bti (0) : 0 [flags 0x00000000]
    [1] glibc.cpu.aarch64_gcs (1) : 0 [flags 0x00000000]

I would expect to see something like:

    [0] glibc.foo.bar=value (some words or abbreviations for flags)

If we reduce amount of flags as suggested above, we can print something
readable and concise for each entry in the cache.

>  /* Print the whole cache file, if a file contains the new cache format
> @@ -394,7 +424,7 @@ print_cache (const char *cache_name)
>  		       cache_new->libs[i].hwcap, hwcaps_string,
>  		       cache_data + cache_new->libs[i].value);
>  	}
> -      print_extensions (&ext);
> +      print_extensions (&ext, cache_data);

OK

>      }
>    /* Cleanup.  */
>    munmap (cache, cache_size);
> @@ -498,6 +528,28 @@ write_extensions (int fd, uint32_t str_offset,
>        ext->sections[xid].size = hwcaps_size;
>      }
>  
> +  struct tunable_header_cached *tunable_data;
> +  size_t tunable_size;
> +  size_t tunable_aligner = 0;
> +
> +  tunable_data = get_tunconf_ext (str_offset);
> +  if (tunable_data != NULL)
> +    {
> +      uint32_t tunable_offset_ua;
> +      uint32_t tunable_offset;
> +
> +      tunable_size = TUNCONF_SIZE (tunable_data);
> +      tunable_offset_ua = generator_offset + strlen (generator);
> +      tunable_offset = ALIGN_UP (tunable_offset_ua, 8);
> +      tunable_aligner = tunable_offset - tunable_offset_ua;
> +
> +      ++xid;
> +      ext->sections[xid].tag = cache_extension_tag_tunables;
> +      ext->sections[xid].flags = 0;
> +      ext->sections[xid].offset = tunable_offset;
> +      ext->sections[xid].size = tunable_size;
> +    }
> +

OK

>    ++xid;
>    ext->count = xid;
>    assert (xid <= cache_extension_count);
> @@ -509,6 +561,14 @@ write_extensions (int fd, uint32_t str_offset,
>        || write (fd, generator, strlen (generator)) != strlen (generator))
>      error (EXIT_FAILURE, errno, _("Writing of cache extension data failed"));
>  
> +  if (tunable_data)
> +    {
> +      if (write (fd, "        ", tunable_aligner) != tunable_aligner
> +	  || write (fd, tunable_data, tunable_size) != tunable_size)
> +	error (EXIT_FAILURE, errno, _("Writing of cache tunable data failed"));
> +      free (tunable_data);
> +    }
> +

OK

>    free (hwcaps_array);
>    free (ext);
>  }
> @@ -1106,3 +1166,9 @@ out_fail:
>    free (temp_name);
>    free (file_entries);
>  }
> +
> +struct stringtable_entry *
> +cache_store_string (const char *string)
> +{
> +  return stringtable_add (&strings, string);
> +}

OK

> diff --git a/elf/ldconfig.c b/elf/ldconfig.c
> index 070e933df6..11b063eb5c 100644
> --- a/elf/ldconfig.c
> +++ b/elf/ldconfig.c
> @@ -44,12 +44,17 @@
>  #include <dl-cache.h>
>  #include <dl-hwcaps.h>
>  #include <dl-is_dso.h>
> +#include "tunconf.h"

OK
 
>  
>  #ifndef LD_SO_CONF
>  # define LD_SO_CONF SYSCONFDIR "/ld.so.conf"
>  #endif
>  
> +#ifndef TUNABLES_CONF
> +# define TUNABLES_CONF SYSCONFDIR "/tunables.conf"
> +#endif
> +

OK

>  /* Get libc version number.  */
>  #include <version.h>
>  
> @@ -107,9 +112,12 @@ static int opt_ignore_aux_cache;
>  /* Cache file to use.  */
>  static char *cache_file;
>  
> -/* Configuration file.  */
> +/* Configuration file for libraries.  */
>  static const char *config_file;
>  
> +/* Configuration file for tunables.  */
> +static const char *tunconfig_file;
> +

OK

>  /* Name and version of program.  */
>  static void print_version (FILE *stream, struct argp_state *state);
>  void (*argp_program_version_hook) (FILE *, struct argp_state *)
> @@ -127,7 +135,8 @@ static const struct argp_option options[] =
>    { NULL, 'X', NULL, 0, N_("Don't update symbolic links"), 0},
>    { NULL, 'r', N_("ROOT"), 0, N_("Change to and use ROOT as root directory"), 0},
>    { NULL, 'C', N_("CACHE"), 0, N_("Use CACHE as cache file"), 0},
> -  { NULL, 'f', N_("CONF"), 0, N_("Use CONF as configuration file"), 0},
> +  { NULL, 'f', N_("CONF"), 0, N_("Use CONF as configuration file for libraries"), 0},
> +  { NULL, 't', N_("TUNCONF"), 0, N_("Use TUNCONF as configuration file for tunables"), 0},

OK

>    { NULL, 'n', NULL, 0, N_("Only process directories specified on the command line.  Don't build cache."), 0},
>    { NULL, 'l', NULL, 0, N_("Manually link individual libraries."), 0},
>    { "format", 'c', N_("FORMAT"), 0, N_("Format to use: new (default), old, or compat"), 0},
> @@ -164,6 +173,9 @@ parse_opt (int key, char *arg, struct argp_state *state)
>      case 'f':
>        config_file = arg;
>        break;
> +    case 't':
> +      tunconfig_file = arg;
> +      break;

OK

>      case 'i':
>        opt_ignore_aux_cache = 1;
>        break;
> @@ -421,7 +433,7 @@ add_dir_1 (const char *line, const char *from_file, int from_line)
>  }
>  
>  static void
> -add_dir_callback (const char *line, const char *from_file, int from_line)
> +add_dir_callback (char *line, const char *from_file, int from_line)
>  {
>    if (!strncasecmp (line, "hwcap", 5) && isblank (line[5]))
>      error (0, 0, _("%s:%u: hwcap directive ignored"), from_file, from_line);
> @@ -1089,6 +1101,9 @@ main (int argc, char **argv)
>    if (config_file == NULL)
>      config_file = LD_SO_CONF;
>  
> +  if (tunconfig_file == NULL)
> +    tunconfig_file = TUNABLES_CONF;
> +

OK

>    if (opt_print_cache)
>      {
>        if (opt_chroot != NULL)
> @@ -1164,6 +1179,8 @@ main (int argc, char **argv)
>  
>    search_dirs ();
>  
> +  parse_tunconf (tunconfig_file, opt_chroot);
> +

OK

> ...
> diff --git a/elf/tunconf.c b/elf/tunconf.c
> new file mode 100644
> index 0000000000..1b7bf0ac2b
> --- /dev/null
> +++ b/elf/tunconf.c
>
> ...
>
> +
> +/* Declared in chroot_canon.c.  */
> +extern char *chroot_canon (const char *chroot, const char *name);
> +
> +/*----------------------------------------------------------------------*/
> +
> +#ifndef TUNABLES_CONF
> +# define TUNABLES_CONF SYSCONFDIR "/tunables.conf"
> +#endif
> +
> +#ifndef TUNABLES_CACHE
> +# define TUNABLES_CACHE SYSCONFDIR "/tunables.cache"
> +#endif
> +
> +/* Tunable Override Policies.  */
> +typedef enum {
> +  TOP_DEFAULT = 0,	/* let the internal code decide */
> +  TOP_ALLOW,		/* let the environment variable override */

Is there really a difference between DEFAULT and ALLOW? I think this set
of options here makes everything more complicated than it should.

> +  TOP_STRICT,		/* internal code will only allow "stricter" setting */

As I said above, I strongly suggest that we don't do this in this patch
series.

> +  TOP_DENY		/* no override allowed */
> +} TOP;
> +
> +struct tunable_entry_int {
> +  struct stringtable_entry *name;
> +  struct stringtable_entry *value;
> +  TOP top;
> +  int tunable_id;
> +  int value_is_negative:1;
> +  int value_was_parsed:1;
> +  unsigned long long value_ull;
> +  signed long long value_sll;
> +
> +  struct tunable_entry_int *next;
> +};
> +
> +struct tunable_entry_int *entry_list;
> +struct tunable_entry_int **entry_list_next = &entry_list;
> +

OK

>
> ...
>
> diff --git a/elf/tunconf.h b/elf/tunconf.h
> new file mode 100644
> index 0000000000..7578605c5a
> --- /dev/null
> +++ b/elf/tunconf.h
> @@ -0,0 +1,40 @@
> +#define TUNCONF_SIGNATURE		0x7c3ba94f
> +#define TUNCONF_VERSION			0x01000000
> +
> +#define TUNCONF_FLAG_PARSED		0x00000001
> +#define TUNCONF_FLAG_NEGATIVE		0x00000002
> +
> +#define TUNCONF_FLAG_OVERRIDABLE	0x0000000C
> +#define TUNCONF_OVERRIDE_DEFAULT	0x00000000
> +#define TUNCONF_OVERRIDE_ALLOW		0x00000004
> +#define TUNCONF_OVERRIDE_STRICTER	0x00000008
> +#define TUNCONF_OVERRIDE_DENY		0x0000000C
> +
> +#define TUNCONF_FLAG_FILTER		0x0000ff00
> +#define TUNCONF_FILTER_PERPROC		0x00000100

As above, too many flags IMHO.

>
> ...
>
> diff --git a/sysdeps/generic/dl-cache.h b/sysdeps/generic/dl-cache.h
> index 26f0c1e0bd..f438982b6f 100644
> --- a/sysdeps/generic/dl-cache.h
> +++ b/sysdeps/generic/dl-cache.h
> @@ -220,6 +220,12 @@ enum cache_extension_tag
>        size must be a multiple of 4.  */
>     cache_extension_tag_glibc_hwcaps,
>  
> +   /* Array of system-wide tunable information.
> +
> +      For this section, 8-byte alignment is required, and the section
> +      size must be a multiple of 8.  */
> +   cache_extension_tag_tunables,
> +

OK

>
> ...
>
> diff --git a/sysdeps/generic/ldconfig.h b/sysdeps/generic/ldconfig.h
> index 22d0fd0f82..800714659e 100644
> --- a/sysdeps/generic/ldconfig.h
> +++ b/sysdeps/generic/ldconfig.h
> @@ -74,6 +74,8 @@ extern void add_to_cache (const char *path, const char *filename,
>  			  unsigned int isa_level,
>  			  struct glibc_hwcaps_subdirectory *);
>  
> +extern struct stringtable_entry *cache_store_string (const char *string);
> +
>  extern void init_aux_cache (void);
>  
>  extern void load_aux_cache (const char *aux_cache_name);
> @@ -112,8 +114,8 @@ enum opt_format
>  extern enum opt_format opt_format;
>  
>  /* Declared in ldconfig-parse.c */
> -typedef void (*ldconfig_parse_config_cb) (const char *line,
> -					 const char *from_file, int from_line);
> +typedef void (*ldconfig_parse_config_cb) (char *line,
> +					  const char *from_file, int from_line);
>  
>  void ldconfig_parse_config (const char *filename, char *opt_chroot,
>  			    ldconfig_parse_config_cb cb);

OK