Re: [PATCH v3 11/19] elf: Do not duplicate the GLIBC_TUNABLES string

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 20/11/23 19:44, Siddhesh Poyarekar wrote:
> On 2023-11-06 15:25, Adhemerval Zanella wrote:
>> The tunable parsing duplicates the tunable environment variable so it
>> null-terminates each one since it simplifies the later parsing. It has
>> the drawback of adding another point of failure (__minimal_malloc
>> failing), and the memory copy requires tuning the compiler to avoid mem
>> operations calls.
>>
>> The parsing now tracks the tunable start and its size. The
>> dl-tunable-parse.h adds helper functions to help parsing, like a strcmp
>> that also checks for size and an iterator for suboptions that are
>> comma-separated (used on hwcap parsing by x86, powerpc, and s390x).
>>
>> Since the environment variable is allocated on the stack by the kernel,
>> it is safe to keep the references to the suboptions for later parsing
>> of string tunables (as done by set_hwcaps by multiple architectures).
>>
>> Checked on x86_64-linux-gnu, powerpc64le-linux-gnu, and
>> aarch64-linux-gnu.
>> ---
>>   elf/dl-tunables.c                             |  66 +++----
>>   elf/dl-tunables.h                             |   6 +-
>>   elf/tst-tunables.c                            |  66 ++++++-
>>   sysdeps/generic/dl-tunables-parse.h           | 129 ++++++++++++++
>>   sysdeps/s390/cpu-features.c                   | 167 +++++++-----------
>>   .../unix/sysv/linux/aarch64/cpu-features.c    |  38 ++--
>>   .../unix/sysv/linux/powerpc/cpu-features.c    |  45 ++---
>>   .../sysv/linux/powerpc/tst-hwcap-tunables.c   |   6 +-
>>   sysdeps/x86/Makefile                          |   4 +-
>>   sysdeps/x86/cpu-tunables.c                    | 118 +++++--------
>>   sysdeps/x86/tst-hwcap-tunables.c              | 148 ++++++++++++++++
>>   11 files changed, 517 insertions(+), 276 deletions(-)
>>   create mode 100644 sysdeps/generic/dl-tunables-parse.h
>>   create mode 100644 sysdeps/x86/tst-hwcap-tunables.c
>>
>> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
>> index 6e3e6a2cf8..f406521735 100644
>> --- a/elf/dl-tunables.c
>> +++ b/elf/dl-tunables.c
>> @@ -36,28 +36,6 @@
>>   #define TUNABLES_INTERNAL 1
>>   #include "dl-tunables.h"
>>   -#include <not-errno.h>
>> -
>> -static char *
>> -tunables_strdup (const char *in)
>> -{
>> -  size_t i = 0;
>> -
>> -  while (in[i++] != '\0');
>> -  char *out = __minimal_malloc (i + 1);
>> -
>> -  /* For most of the tunables code, we ignore user errors.  However,
>> -     this is a system error - and running out of memory at program
>> -     startup should be reported, so we do.  */
>> -  if (out == NULL)
>> -    _dl_fatal_printf ("failed to allocate memory to process tunables\n");
>> -
>> -  while (i-- > 0)
>> -    out[i] = in[i];
>> -
>> -  return out;
>> -}
>> -
>>   static char **
>>   get_next_env (char **envp, char **name, size_t *namelen, char **val,
>>             char ***prev_envp)
>> @@ -134,14 +112,14 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp,
>>   /* Validate range of the input value and initialize the tunable CUR if it looks
>>      good.  */
>>   static void
>> -tunable_initialize (tunable_t *cur, const char *strval)
>> +tunable_initialize (tunable_t *cur, const char *strval, size_t len)
>>   {
>> -  tunable_val_t val;
>> +  tunable_val_t val = { 0 };
>>       if (cur->type.type_code != TUNABLE_TYPE_STRING)
>>       val.numval = (tunable_num_t) _dl_strtoul (strval, NULL);
> 
> There's an implicit assumption that strval is NULL terminated for numeric values.  Is that safe?  Maybe all you need to do here is copy strval into a static buffer of size 21 (basically size of the string representing -1ULL) and pass that to _dl_strtoul.

Afaiu the environment variable will always be NULL terminated (although some 
might overlap).  This is due how the kernel will layout the argv/envp on 
process creation at sys_execve:

fs/exec.c

1886 static int do_execveat_common(int fd, struct filename *filename,
1887                               struct user_arg_ptr argv,
1888                               struct user_arg_ptr envp,
1889                               int flags)
1890 {
[...]
1941         retval = copy_strings(bprm->envc, envp, bprm);
1942         if (retval < 0)
1943                 goto out_free;
1944
1945         retval = copy_strings(bprm->argc, argv, bprm);
1946         if (retval < 0)
1947                 goto out_free;
[...]

 518 /*
 519  * 'copy_strings()' copies argument/environment strings from the old
 520  * processes's memory to the new process's stack.  The call to get_user_pages()
 521  * ensures the destination page is created and not swapped out.
 522  */
 523 static int copy_strings(int argc, struct user_arg_ptr argv,
 524                         struct linux_binprm *bprm)
 525 {
[...]
 531         while (argc-- > 0) {
[...]
 536                 ret = -EFAULT;
 537                 str = get_user_arg_ptr(argv, argc);
 538                 if (IS_ERR(str))
 539                         goto out;
 540
 541                 len = strnlen_user(str, MAX_ARG_STRLEN);
 542                 if (!len)
 543                         goto out;

So even if caller construct a bogus argv/envp with non null-terminated
strings, if the kernel can not found a NULL execve will eventually fail 
with EFAULT:

$ cat spawn.c
#include <assert.h>
#include <errno.h>
#include <unistd.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/mman.h>

extern char **environ;

int main (int argc, char *argv[])
{
  pid_t pid = fork ();
  assert (pid != -1);
  if (pid != 0)
    exit (EXIT_FAILURE);

  long int pz = sysconf (_SC_PAGESIZE);
  assert (pz != -1);

  void *buf = mmap (NULL, 2 * pz, PROT_READ | PROT_WRITE,
                    MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
  assert (buf != MAP_FAILED);

  assert (mprotect (buf + pz, pz, PROT_NONE) != -1);

  char *env = (buf + pz) - 1;
  env[0] = 'a';

  char *new_argv[] = {  "./spawn", NULL };
  char *new_envp[] = { env, NULL };
  int r = execve (new_argv[0], new_argv, new_envp);
  printf ("r=%d (errno=%s)\n", r, strerrorname_np (errno));

  return 0;
}
$ gcc -Wall -O2 spawn.c -D_GNU_SOURCE -o spawn && ./spawn
r=-1 (errno=EFAULT)

> 
>>     else
>> -    val.strval = strval;
>> +    val.strval = (struct tunable_str_t) { strval, len };
>>     do_tunable_update_val (cur, &val, NULL, NULL);
>>   }
>>   @@ -158,29 +136,29 @@ struct tunable_toset_t
>>   {
>>     tunable_t *t;
>>     const char *value;
>> +  size_t len;
>>   };
>>     enum { tunables_list_size = array_length (tunable_list) };
>>     /* Parse the tunable string VALSTRING and set TUNABLES with the found tunables
>> -   and their respectibles values.  VALSTRING is a duplicated values,  where
>> -   delimiters ':' are replaced with '\0', so string tunables are null
>> -   terminated.
>> +   and their respectibles values.  The VALSTRING is parsed in place, with the
>> +   tunable start and size recorded in TUNABLES.
>>      Return the number of tunables found (including 0 if the string is empty)
>>      or -1 if for a ill-formatted definition.  */
>>   static int
>> -parse_tunables_string (char *valstring, struct tunable_toset_t *tunables)
>> +parse_tunables_string (const char *valstring, struct tunable_toset_t *tunables)
>>   {
>>     if (valstring == NULL || *valstring == '\0')
>>       return 0;
>>   -  char *p = valstring;
>> +  const char *p = valstring;
>>     bool done = false;
>>     int ntunables = 0;
>>       while (!done)
>>       {
>> -      char *name = p;
>> +      const char *name = p;
>>           /* First, find where the name ends.  */
>>         while (*p != '=' && *p != ':' && *p != '\0')
>> @@ -202,7 +180,7 @@ parse_tunables_string (char *valstring, struct tunable_toset_t *tunables)
>>         /* Skip the '='.  */
>>         p++;
>>   -      char *value = p;
>> +      const char *value = p;
>>           while (*p != '=' && *p != ':' && *p != '\0')
>>       p++;
>> @@ -211,8 +189,6 @@ parse_tunables_string (char *valstring, struct tunable_toset_t *tunables)
>>       return -1;
>>         else if (*p == '\0')
>>       done = true;
>> -      else
>> -    *p++ = '\0';
>>           /* Add the tunable if it exists.  */
>>         for (size_t i = 0; i < tunables_list_size; i++)
>> @@ -221,7 +197,8 @@ parse_tunables_string (char *valstring, struct tunable_toset_t *tunables)
>>           if (tunable_is_name (cur->name, name))
>>           {
>> -          tunables[ntunables++] = (struct tunable_toset_t) { cur, value };
>> +          tunables[ntunables++] =
>> +        (struct tunable_toset_t) { cur, value, p - value };
>>             break;
>>           }
>>       }
>> @@ -231,7 +208,7 @@ parse_tunables_string (char *valstring, struct tunable_toset_t *tunables)
>>   }
>>     static void
>> -parse_tunables (char *valstring)
>> +parse_tunables (const char *valstring)
>>   {
>>     struct tunable_toset_t tunables[tunables_list_size];
>>     int ntunables = parse_tunables_string (valstring, tunables);
>> @@ -243,7 +220,7 @@ parse_tunables (char *valstring)
>>       }
>>       for (int i = 0; i < ntunables; i++)
>> -    tunable_initialize (tunables[i].t, tunables[i].value);
>> +    tunable_initialize (tunables[i].t, tunables[i].value, tunables[i].len);
>>   }
>>     /* Initialize the tunables list from the environment.  For now we only use the
>> @@ -264,9 +241,12 @@ __tunables_init (char **envp)
>>     while ((envp = get_next_env (envp, &envname, &len, &envval,
>>                      &prev_envp)) != NULL)
>>       {
>> +      /* The environment variable is allocated on the stack by the kernel, so
>> +     it is safe to keep the references to the suboptions for later parsing
>> +     of string tunables.  */
>>         if (tunable_is_name ("GLIBC_TUNABLES", envname))
>>       {
>> -      parse_tunables (tunables_strdup (envval));
>> +      parse_tunables (envval);
>>         continue;
>>       }
>>   @@ -284,7 +264,7 @@ __tunables_init (char **envp)
>>         /* We have a match.  Initialize and move on to the next line.  */
>>         if (tunable_is_name (name, envname))
>>           {
>> -          tunable_initialize (cur, envval);
>> +          tunable_initialize (cur, envval, len);
>>             break;
>>           }
>>       }
>> @@ -298,7 +278,7 @@ __tunables_print (void)
>>       {
>>         const tunable_t *cur = &tunable_list[i];
>>         if (cur->type.type_code == TUNABLE_TYPE_STRING
>> -      && cur->val.strval == NULL)
>> +      && cur->val.strval.str == NULL)
>>       _dl_printf ("%s:\n", cur->name);
>>         else
>>       {
>> @@ -324,7 +304,9 @@ __tunables_print (void)
>>                 (size_t) cur->type.max);
>>             break;
>>           case TUNABLE_TYPE_STRING:
>> -          _dl_printf ("%s\n", cur->val.strval);
>> +          _dl_printf ("%.*s\n",
>> +              (int) cur->val.strval.len,
>> +              cur->val.strval.str);
>>             break;
>>           default:
>>             __builtin_unreachable ();
>> @@ -359,7 +341,7 @@ __tunable_get_val (tunable_id_t id, void *valp, tunable_callback_t callback)
>>       }
>>       case TUNABLE_TYPE_STRING:
>>       {
>> -      *((const char **)valp) = cur->val.strval;
>> +      *((struct tunable_str_t **) valp) = &cur->val.strval;
>>         break;
>>       }
>>       default:
>> diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
>> index 45c191e021..0e777d7d37 100644
>> --- a/elf/dl-tunables.h
>> +++ b/elf/dl-tunables.h
>> @@ -30,7 +30,11 @@ typedef intmax_t tunable_num_t;
>>   typedef union
>>   {
>>     tunable_num_t numval;
>> -  const char *strval;
>> +  struct tunable_str_t
>> +  {
>> +    const char *str;
>> +    size_t len;
>> +  } strval;
>>   } tunable_val_t;
>>     typedef void (*tunable_callback_t) (tunable_val_t *);
>> diff --git a/elf/tst-tunables.c b/elf/tst-tunables.c
>> index e1ad44f27c..188345b070 100644
>> --- a/elf/tst-tunables.c
>> +++ b/elf/tst-tunables.c
>> @@ -31,7 +31,8 @@ static int restart;
>>     static const struct test_t
>>   {
>> -  const char *env;
>> +  const char *name;
>> +  const char *value;
>>     int32_t expected_malloc_check;
>>     size_t expected_mmap_threshold;
>>     int32_t expected_perturb;
>> @@ -39,12 +40,14 @@ static const struct test_t
>>   {
>>     /* Expected tunable format.  */
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.check=2",
>>       2,
>>       0,
>>       0,
>>     },
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>>       2,
>>       4096,
>> @@ -52,6 +55,7 @@ static const struct test_t
>>     },
>>     /* Empty tunable are ignored.  */
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.check=2::glibc.malloc.mmap_threshold=4096",
>>       2,
>>       4096,
>> @@ -59,6 +63,7 @@ static const struct test_t
>>     },
>>     /* As well empty values.  */
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.check=:glibc.malloc.mmap_threshold=4096",
>>       0,
>>       4096,
>> @@ -66,18 +71,21 @@ static const struct test_t
>>     },
>>     /* Tunable are processed from left to right, so last one is the one set.  */
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.check=1:glibc.malloc.check=2",
>>       2,
>>       0,
>>       0,
>>     },
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.check=1:glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>>       2,
>>       4096,
>>       0,
>>     },
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096:glibc.malloc.check=1",
>>       1,
>>       4096,
>> @@ -85,12 +93,14 @@ static const struct test_t
>>     },
>>     /* 0x800 is larger than tunable maxval (0xff), so the tunable is unchanged.  */
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.perturb=0x800",
>>       0,
>>       0,
>>       0,
>>     },
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.perturb=0x55",
>>       0,
>>       0,
>> @@ -98,6 +108,7 @@ static const struct test_t
>>     },
>>     /* Out of range values are just ignored.  */
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
>>       0,
>>       4096,
>> @@ -105,24 +116,28 @@ static const struct test_t
>>     },
>>     /* Invalid keys are ignored.  */
>>     {
>> +    "GLIBC_TUNABLES",
>>       ":glibc.malloc.garbage=2:glibc.malloc.check=1",
>>       1,
>>       0,
>>       0,
>>     },
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>>       0,
>>       4096,
>>       0,
>>     },
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.not_valid.check=2:glibc.malloc.mmap_threshold=4096",
>>       0,
>>       4096,
>>       0,
>>     },
>>     {
>> +    "GLIBC_TUNABLES",
>>       "not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>>       0,
>>       4096,
>> @@ -130,24 +145,28 @@ static const struct test_t
>>     },
>>     /* Invalid subkeys are ignored.  */
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096:glibc.malloc.check=2",
>>       2,
>>       0,
>>       0,
>>     },
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.check=4:glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096",
>>       0,
>>       0,
>>       0,
>>     },
>>     {
>> +    "GLIBC_TUNABLES",
>>       "not_valid.malloc.check=2",
>>       0,
>>       0,
>>       0,
>>     },
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.not_valid.check=2",
>>       0,
>>       0,
>> @@ -156,6 +175,7 @@ static const struct test_t
>>     /* An ill-formatted tunable in the for key=key=value will considere the
>>        value as 'key=value' (which can not be parsed as an integer).  */
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096",
>>       0,
>>       0,
>> @@ -163,41 +183,77 @@ static const struct test_t
>>     },
>>     /* Ill-formatted tunables string is not parsed.  */
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
>>       0,
>>       0,
>>       0,
>>     },
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.check=2=2",
>>       0,
>>       0,
>>       0,
>>     },
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.check=2=2:glibc.malloc.mmap_threshold=4096",
>>       0,
>>       0,
>>       0,
>>     },
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.check=2=2:glibc.malloc.check=2",
>>       0,
>>       0,
>>       0,
>>     },
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096=4096",
>>       0,
>>       0,
>>       0,
>>     },
>>     {
>> +    "GLIBC_TUNABLES",
>>       "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096=4096",
>>       0,
>>       0,
>>       0,
>>     },
>> +  /* Also check some tunable aliases.  */
>> +  {
>> +    "MALLOC_CHECK_",
>> +    "2",
>> +    2,
>> +    0,
>> +    0,
>> +  },
>> +  {
>> +    "MALLOC_MMAP_THRESHOLD_",
>> +    "4096",
>> +    0,
>> +    4096,
>> +    0,
>> +  },
>> +  {
>> +    "MALLOC_PERTURB_",
>> +    "0x55",
>> +    0,
>> +    0,
>> +    0x55,
>> +  },
>> +  /* 0x800 is larger than tunable maxval (0xff), so the tunable is unchanged.  */
>> +  {
>> +    "MALLOC_PERTURB_",
>> +    "0x800",
>> +    0,
>> +    0,
>> +    0,
>> +  },
>>   };
>>     static int
>> @@ -245,13 +301,17 @@ do_test (int argc, char *argv[])
>>       {
>>         snprintf (nteststr, sizeof nteststr, "%d", i);
>>   -      printf ("[%d] Spawned test for %s\n", i, tests[i].env);
>> -      setenv ("GLIBC_TUNABLES", tests[i].env, 1);
>> +      printf ("[%d] Spawned test for %s=%s\n",
>> +          i,
>> +          tests[i].name,
>> +          tests[i].value);
>> +      setenv (tests[i].name, tests[i].value, 1);
>>         struct support_capture_subprocess result
>>       = support_capture_subprogram (spargv[0], spargv);
>>         support_capture_subprocess_check (&result, "tst-tunables", 0,
>>                       sc_allow_stderr);
>>         support_capture_subprocess_free (&result);
>> +      unsetenv (tests[i].name);
>>       }
>>       return 0;
>> diff --git a/sysdeps/generic/dl-tunables-parse.h b/sysdeps/generic/dl-tunables-parse.h
>> new file mode 100644
>> index 0000000000..5b399f852d
>> --- /dev/null
>> +++ b/sysdeps/generic/dl-tunables-parse.h
>> @@ -0,0 +1,129 @@
>> +/* Helper functions to handle tunable strings.
>> +   Copyright (C) 2023 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <https://www.gnu.org/licenses/>.  */
>> +
>> +#ifndef _DL_TUNABLES_PARSE_H
>> +#define _DL_TUNABLES_PARSE_H 1
>> +
>> +#include <string.h>
>> +
>> +/* Compare the contents of STRVAL with STR of size LEN.  The STR might not
>> +   be null-terminated.   */
>> +static __always_inline bool
>> +tunable_strcmp (const struct tunable_str_t *strval, const char *str,
>> +        size_t len)
>> +{
>> +  return strval->len == len && memcmp (strval->str, str, len) == 0;
>> +}
>> +#define tunable_strcmp_cte(__tunable, __str) \
>> + tunable_strcmp (&__tunable->strval, __str, sizeof (__str) - 1)
>> +
>> +/*
>> +   Helper functions to iterate over a tunable string composed by multiple
>> +   suboptions separated by commaxi; this is a common pattern for CPU.  Each
>> +   suboptions is return in the form of { address, size } (no null terminated).
>> +   For instance:
>> +
>> +     struct tunable_str_comma_t ts;
>> +     tunable_str_comma_init (&ts, valp);
>> +
>> +     struct tunable_str_t t;
>> +     while (tunable_str_comma_next (&ts, &t))
>> +      {
>> +    _dl_printf ("[%s] %.*s (%d)\n",
>> +            __func__,
>> +            (int) tstr.len,
>> +            tstr.str,
>> +            (int) tstr.len);
>> +
>> +        if (tunable_str_comma_strcmp (&t, opt, opt1_len))
>> +      {
>> +        [...]
>> +      }
>> +    else if (tunable_str_comma_strcmp_cte (&t, "opt2"))
>> +      {
>> +        [...]
>> +      }
>> +      }
>> +*/
>> +
>> +struct tunable_str_comma_state_t
>> +{
>> +  const char *p;
>> +  size_t plen;
>> +  size_t maxplen;
>> +};
>> +
>> +struct tunable_str_comma_t
>> +{
>> +  const char *str;
>> +  size_t len;
>> +  bool disable;
>> +};
>> +
>> +static inline void
>> +tunable_str_comma_init (struct tunable_str_comma_state_t *state,
>> +            tunable_val_t *valp)
>> +{
> 
> Maybe add an assertion here that valp->strval.str is not NULL since all functions assume that?  Or at least a comment somewhere.

An assert won't work because tunable_val_t is an union.  I will add
the comment:

   NB: These function are expected to be called from tunable callback
   functions along with tunable_val_t with string types.

> 
>> +  state->p = valp->strval.str;
>> +  state->plen = 0;
>> +  state->maxplen = valp->strval.len;
>> +}
>> +
>> +static inline bool
>> +tunable_str_comma_next (struct tunable_str_comma_state_t *state,
>> +            struct tunable_str_comma_t *str)
>> +{
>> +  if (*state->p == '\0' || state->plen >= state->maxplen)
>> +    return false;
>> +
>> +  const char *c;
>> +  for (c = state->p; *c != ','; c++, state->plen++)
>> +    if (*c == '\0' || state->plen == state->maxplen)
>> +      break;
>> +
>> +  str->str = state->p;
>> +  str->len = c - state->p;
>> +
>> +  if (str->len > 0)
>> +    {
>> +      str->disable = *str->str == '-';
>> +      if (str->disable)
>> +    {
>> +      str->str = str->str + 1;
>> +      str->len = str->len - 1;
>> +    }
>> +    }
>> +
>> +  state->p = c + 1;
>> +  state->plen++;
>> +
>> +  return true;
>> +}
>> +
>> +/* Compare the contents of T with STR of size LEN.  The STR might not be
>> +   null-terminated.   */
>> +static __always_inline bool
>> +tunable_str_comma_strcmp (const struct tunable_str_comma_t *t, const char *str,
>> +              size_t len)
>> +{
>> +  return t->len == len && memcmp (t->str, str, len) == 0;
>> +}
>> +#define tunable_str_comma_strcmp_cte(__t, __str) \
>> +  tunable_str_comma_strcmp (__t, __str, sizeof (__str) - 1)
>> +
>> +#endif
>> diff --git a/sysdeps/s390/cpu-features.c b/sysdeps/s390/cpu-features.c
>> index 55449ba07f..8b1466fa7b 100644
>> --- a/sysdeps/s390/cpu-features.c
>> +++ b/sysdeps/s390/cpu-features.c
>> @@ -22,6 +22,7 @@
>>   #include <ifunc-memcmp.h>
>>   #include <string.h>
>>   #include <dl-symbol-redir-ifunc.h>
>> +#include <dl-tunables-parse.h>
>>     #define S390_COPY_CPU_FEATURES(SRC_PTR, DEST_PTR)    \
>>     (DEST_PTR)->hwcap = (SRC_PTR)->hwcap;            \
>> @@ -51,33 +52,14 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>>     struct cpu_features cpu_features_curr;
>>     S390_COPY_CPU_FEATURES (cpu_features, &cpu_features_curr);
>>   -  const char *token = valp->strval;
>> -  do
>> +  struct tunable_str_comma_state_t ts;
>> +  tunable_str_comma_init (&ts, valp);
>> +
>> +  struct tunable_str_comma_t t;
>> +  while (tunable_str_comma_next (&ts, &t))
>>       {
>> -      const char *token_end, *feature;
>> -      bool disable;
>> -      size_t token_len;
>> -      size_t feature_len;
>> -
>> -      /* Find token separator or end of string.  */
>> -      for (token_end = token; *token_end != ','; token_end++)
>> -    if (*token_end == '\0')
>> -      break;
>> -
>> -      /* Determine feature.  */
>> -      token_len = token_end - token;
>> -      if (*token == '-')
>> -    {
>> -      disable = true;
>> -      feature = token + 1;
>> -      feature_len = token_len - 1;
>> -    }
>> -      else
>> -    {
>> -      disable = false;
>> -      feature = token;
>> -      feature_len = token_len;
>> -    }
>> +      if (t.len == 0)
>> +    continue;
>>           /* Handle only the features here which are really used in the
>>        IFUNC-resolvers.  All others are ignored as the values are only used
>> @@ -85,85 +67,65 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>>         bool reset_features = false;
>>         unsigned long int hwcap_mask = 0UL;
>>         unsigned long long stfle_bits0_mask = 0ULL;
>> +      bool disable = t.disable;
>>   -      if ((*feature == 'z' || *feature == 'a'))
>> +      if (tunable_str_comma_strcmp_cte (&t, "zEC12")
>> +      || tunable_str_comma_strcmp_cte (&t, "arch10"))
>> +    {
>> +      reset_features = true;
>> +      disable = true;
>> +      hwcap_mask = HWCAP_S390_VXRS | HWCAP_S390_VXRS_EXT
>> +        | HWCAP_S390_VXRS_EXT2;
>> +      stfle_bits0_mask = S390_STFLE_MASK_ARCH13_MIE3;
>> +    }
>> +      else if (tunable_str_comma_strcmp_cte (&t, "z13")
>> +           || tunable_str_comma_strcmp_cte (&t, "arch11"))
>> +    {
>> +      reset_features = true;
>> +      disable = true;
>> +      hwcap_mask = HWCAP_S390_VXRS_EXT | HWCAP_S390_VXRS_EXT2;
>> +      stfle_bits0_mask = S390_STFLE_MASK_ARCH13_MIE3;
>> +    }
>> +      else if (tunable_str_comma_strcmp_cte (&t, "z14")
>> +           || tunable_str_comma_strcmp_cte (&t, "arch12"))
>> +    {
>> +      reset_features = true;
>> +      disable = true;
>> +      hwcap_mask = HWCAP_S390_VXRS_EXT2;
>> +      stfle_bits0_mask = S390_STFLE_MASK_ARCH13_MIE3;
>> +    }
>> +      else if (tunable_str_comma_strcmp_cte (&t, "z15")
>> +           || tunable_str_comma_strcmp_cte (&t, "z16")
>> +           || tunable_str_comma_strcmp_cte (&t, "arch13")
>> +           || tunable_str_comma_strcmp_cte (&t, "arch14"))
>>       {
>> -      if ((feature_len == 5 && *feature == 'z'
>> -           && memcmp (feature, "zEC12", 5) == 0)
>> -          || (feature_len == 6 && *feature == 'a'
>> -          && memcmp (feature, "arch10", 6) == 0))
>> -        {
>> -          reset_features = true;
>> -          disable = true;
>> -          hwcap_mask = HWCAP_S390_VXRS | HWCAP_S390_VXRS_EXT
>> -        | HWCAP_S390_VXRS_EXT2;
>> -          stfle_bits0_mask = S390_STFLE_MASK_ARCH13_MIE3;
>> -        }
>> -      else if ((feature_len == 3 && *feature == 'z'
>> -            && memcmp (feature, "z13", 3) == 0)
>> -           || (feature_len == 6 && *feature == 'a'
>> -               && memcmp (feature, "arch11", 6) == 0))
>> -        {
>> -          reset_features = true;
>> -          disable = true;
>> -          hwcap_mask = HWCAP_S390_VXRS_EXT | HWCAP_S390_VXRS_EXT2;
>> -          stfle_bits0_mask = S390_STFLE_MASK_ARCH13_MIE3;
>> -        }
>> -      else if ((feature_len == 3 && *feature == 'z'
>> -            && memcmp (feature, "z14", 3) == 0)
>> -           || (feature_len == 6 && *feature == 'a'
>> -               && memcmp (feature, "arch12", 6) == 0))
>> -        {
>> -          reset_features = true;
>> -          disable = true;
>> -          hwcap_mask = HWCAP_S390_VXRS_EXT2;
>> -          stfle_bits0_mask = S390_STFLE_MASK_ARCH13_MIE3;
>> -        }
>> -      else if ((feature_len == 3 && *feature == 'z'
>> -            && (memcmp (feature, "z15", 3) == 0
>> -            || memcmp (feature, "z16", 3) == 0))
>> -           || (feature_len == 6
>> -               && (memcmp (feature, "arch13", 6) == 0
>> -               || memcmp (feature, "arch14", 6) == 0)))
>> -        {
>> -          /* For z15 or newer we don't have to disable something,
>> -         but we have to reset to the original values.  */
>> -          reset_features = true;
>> -        }
>> +      /* For z15 or newer we don't have to disable something, but we have
>> +         to reset to the original values.  */
>> +      reset_features = true;
>>       }
>> -      else if (*feature == 'H')
>> +      else if (tunable_str_comma_strcmp_cte (&t, "HWCAP_S390_VXRS"))
>>       {
>> -      if (feature_len == 15
>> -          && memcmp (feature, "HWCAP_S390_VXRS", 15) == 0)
>> -        {
>> -          hwcap_mask = HWCAP_S390_VXRS;
>> -          if (disable)
>> -        hwcap_mask |= HWCAP_S390_VXRS_EXT | HWCAP_S390_VXRS_EXT2;
>> -        }
>> -      else if (feature_len == 19
>> -           && memcmp (feature, "HWCAP_S390_VXRS_EXT", 19) == 0)
>> -        {
>> -          hwcap_mask = HWCAP_S390_VXRS_EXT;
>> -          if (disable)
>> -        hwcap_mask |= HWCAP_S390_VXRS_EXT2;
>> -          else
>> -        hwcap_mask |= HWCAP_S390_VXRS;
>> -        }
>> -      else if (feature_len == 20
>> -           && memcmp (feature, "HWCAP_S390_VXRS_EXT2", 20) == 0)
>> -        {
>> -          hwcap_mask = HWCAP_S390_VXRS_EXT2;
>> -          if (!disable)
>> -        hwcap_mask |= HWCAP_S390_VXRS | HWCAP_S390_VXRS_EXT;
>> -        }
>> +      hwcap_mask = HWCAP_S390_VXRS;
>> +      if (t.disable)
>> +        hwcap_mask |= HWCAP_S390_VXRS_EXT | HWCAP_S390_VXRS_EXT2;
>>       }
>> -      else if (*feature == 'S')
>> +      else if (tunable_str_comma_strcmp_cte (&t, "HWCAP_S390_VXRS_EXT"))
>>       {
>> -      if (feature_len == 10
>> -          && memcmp (feature, "STFLE_MIE3", 10) == 0)
>> -        {
>> -          stfle_bits0_mask = S390_STFLE_MASK_ARCH13_MIE3;
>> -        }
>> +      hwcap_mask = HWCAP_S390_VXRS_EXT;
>> +      if (t.disable)
>> +        hwcap_mask |= HWCAP_S390_VXRS_EXT2;
>> +      else
>> +        hwcap_mask |= HWCAP_S390_VXRS;
>> +    }
>> +      else if (tunable_str_comma_strcmp_cte (&t, "HWCAP_S390_VXRS_EXT2"))
>> +    {
>> +      hwcap_mask = HWCAP_S390_VXRS_EXT2;
>> +      if (!t.disable)
>> +        hwcap_mask |= HWCAP_S390_VXRS | HWCAP_S390_VXRS_EXT;
>> +    }
>> +      else if (tunable_str_comma_strcmp_cte (&t, "STFLE_MIE3"))
>> +    {
> 
> Redundant braces.

Ack.

> 
>> +      stfle_bits0_mask = S390_STFLE_MASK_ARCH13_MIE3;
>>       }
>>           /* Perform the actions determined above.  */
>> @@ -187,14 +149,7 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>>         else
>>           cpu_features_curr.stfle_bits[0] |= stfle_bits0_mask;
>>       }
>> -
>> -      /* Jump over current token ... */
>> -      token += token_len;
>> -
>> -      /* ... and skip token separator for next round.  */
>> -      if (*token == ',') token++;
>>       }
>> -  while (*token != '\0');
>>       /* Copy back the features after checking that no unsupported features were
>>        enabled by user.  */
>> diff --git a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
>> index 233d5b2407..9b76cb89c7 100644
>> --- a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
>> +++ b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
>> @@ -16,10 +16,12 @@
>>      License along with the GNU C Library; if not, see
>>      <https://www.gnu.org/licenses/>.  */
>>   +#include <array_length.h>
>>   #include <cpu-features.h>
>>   #include <sys/auxv.h>
>>   #include <elf/dl-hwcaps.h>
>>   #include <sys/prctl.h>
>> +#include <dl-tunables-parse.h>
>>     #define DCZID_DZP_MASK (1 << 4)
>>   #define DCZID_BS_MASK (0xf)
>> @@ -33,28 +35,32 @@
>>   struct cpu_list
>>   {
>>     const char *name;
>> +  size_t len;
>>     uint64_t midr;
>>   };
>>   -static struct cpu_list cpu_list[] = {
>> -      {"falkor",     0x510FC000},
>> -      {"thunderxt88",     0x430F0A10},
>> -      {"thunderx2t99",   0x431F0AF0},
>> -      {"thunderx2t99p1", 0x420F5160},
>> -      {"phecda",     0x680F0000},
>> -      {"ares",         0x411FD0C0},
>> -      {"emag",         0x503F0001},
>> -      {"kunpeng920",      0x481FD010},
>> -      {"a64fx",         0x460F0010},
>> -      {"generic",      0x0}
>> +static const struct cpu_list cpu_list[] =
>> +{
>> +#define CPU_LIST_ENTRY(__str, __num) { __str, sizeof (__str) - 1, __num }
>> +  CPU_LIST_ENTRY ("falkor",         0x510FC000),
>> +  CPU_LIST_ENTRY ("thunderxt88",    0x430F0A10),
>> +  CPU_LIST_ENTRY ("thunderx2t99",   0x431F0AF0),
>> +  CPU_LIST_ENTRY ("thunderx2t99p1", 0x420F5160),
>> +  CPU_LIST_ENTRY ("phecda",         0x680F0000),
>> +  CPU_LIST_ENTRY ("ares",           0x411FD0C0),
>> +  CPU_LIST_ENTRY ("emag",           0x503F0001),
>> +  CPU_LIST_ENTRY ("kunpeng920",     0x481FD010),
>> +  CPU_LIST_ENTRY ("a64fx",          0x460F0010),
>> +  CPU_LIST_ENTRY ("generic",        0x0),
>>   };
>>     static uint64_t
>> -get_midr_from_mcpu (const char *mcpu)
>> +get_midr_from_mcpu (const struct tunable_str_t *mcpu)
>>   {
>> -  for (int i = 0; i < sizeof (cpu_list) / sizeof (struct cpu_list); i++)
>> -    if (strcmp (mcpu, cpu_list[i].name) == 0)
>> +  for (int i = 0; i < array_length (cpu_list); i++) {
>> +    if (tunable_strcmp (mcpu, cpu_list[i].name, cpu_list[i].len))
>>         return cpu_list[i].midr;
>> +  }
> 
> Redundant braces.
> 

Ack.

>>       return UINT64_MAX;
>>   }
>> @@ -65,7 +71,9 @@ init_cpu_features (struct cpu_features *cpu_features)
>>     register uint64_t midr = UINT64_MAX;
>>       /* Get the tunable override.  */
>> -  const char *mcpu = TUNABLE_GET (glibc, cpu, name, const char *, NULL);
>> +  const struct tunable_str_t *mcpu = TUNABLE_GET (glibc, cpu, name,
>> +                          struct tunable_str_t *,
>> +                          NULL);
>>     if (mcpu != NULL)
>>       midr = get_midr_from_mcpu (mcpu);
>>   diff --git a/sysdeps/unix/sysv/linux/powerpc/cpu-features.c b/sysdeps/unix/sysv/linux/powerpc/cpu-features.c
>> index 7c6e20e702..390b3fd11a 100644
>> --- a/sysdeps/unix/sysv/linux/powerpc/cpu-features.c
>> +++ b/sysdeps/unix/sysv/linux/powerpc/cpu-features.c
>> @@ -20,6 +20,7 @@
>>   #include <stdint.h>
>>   #include <cpu-features.h>
>>   #include <elf/dl-tunables.h>
>> +#include <dl-tunables-parse.h>
>>   #include <unistd.h>
>>   #include <string.h>
>>   @@ -43,41 +44,26 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>>     struct cpu_features *cpu_features = &GLRO(dl_powerpc_cpu_features);
>>     unsigned long int tcbv_hwcap = cpu_features->hwcap;
>>     unsigned long int tcbv_hwcap2 = cpu_features->hwcap2;
>> -  const char *token = valp->strval;
>> -  do
>> +
>> +  struct tunable_str_comma_state_t ts;
>> +  tunable_str_comma_init (&ts, valp);
>> +
>> +  struct tunable_str_comma_t t;
>> +  while (tunable_str_comma_next (&ts, &t))
>>       {
>> -      const char *token_end, *feature;
>> -      bool disable;
>> -      size_t token_len, i, feature_len, offset = 0;
>> -      /* Find token separator or end of string.  */
>> -      for (token_end = token; *token_end != ','; token_end++)
>> -    if (*token_end == '\0')
>> -      break;
>> +      if (t.len == 0)
>> +    continue;
>>   -      /* Determine feature.  */
>> -      token_len = token_end - token;
>> -      if (*token == '-')
>> -    {
>> -      disable = true;
>> -      feature = token + 1;
>> -      feature_len = token_len - 1;
>> -    }
>> -      else
>> -    {
>> -      disable = false;
>> -      feature = token;
>> -      feature_len = token_len;
>> -    }
>> -      for (i = 0; i < array_length (hwcap_tunables); ++i)
>> +      size_t offset = 0;
>> +      for (int i = 0; i < array_length (hwcap_tunables); ++i)
>>       {
>>         const char *hwcap_name = hwcap_names + offset;
>>         size_t hwcap_name_len = strlen (hwcap_name);
>>         /* Check the tunable name on the supported list.  */
>> -      if (hwcap_name_len == feature_len
>> -          && memcmp (feature, hwcap_name, feature_len) == 0)
>> +      if (tunable_str_comma_strcmp (&t, hwcap_name, hwcap_name_len))
>>           {
>>             /* Update the hwcap and hwcap2 bits.  */
>> -          if (disable)
>> +          if (t.disable)
>>           {
>>             /* Id is 1 for hwcap2 tunable.  */
>>             if (hwcap_tunables[i].id)
>> @@ -98,12 +84,7 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>>           }
>>         offset += hwcap_name_len + 1;
>>       }
>> -    token += token_len;
>> -    /* ... and skip token separator for next round.  */
>> -    if (*token == ',')
>> -      token++;
>>       }
>> -  while (*token != '\0');
>>   }
>>     static inline void
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/tst-hwcap-tunables.c b/sysdeps/unix/sysv/linux/powerpc/tst-hwcap-tunables.c
>> index 2631016a3a..049164f841 100644
>> --- a/sysdeps/unix/sysv/linux/powerpc/tst-hwcap-tunables.c
>> +++ b/sysdeps/unix/sysv/linux/powerpc/tst-hwcap-tunables.c
>> @@ -110,7 +110,11 @@ do_test (int argc, char *argv[])
>>       run_test ("-arch_2_06", "__memcpy_power7");
>>         if (hwcap & PPC_FEATURE_ARCH_2_05)
>>       run_test ("-arch_2_06,-arch_2_05","__memcpy_power6");
>> -      run_test ("-arch_2_06,-arch_2_05,-power5+,-power5,-power4", "__memcpy_power4");
>> +      run_test ("-arch_2_06,-arch_2_05,-power5+,-power5,-power4",
>> +        "__memcpy_power4");
>> +      /* Also run with valid, but empty settings.  */
>> +      run_test (",-,-arch_2_06,-arch_2_05,-power5+,-power5,,-power4,-",
>> +        "__memcpy_power4");
>>       }
>>     else
>>       {
>> diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile
>> index 917c26f116..a64e5f002a 100644
>> --- a/sysdeps/x86/Makefile
>> +++ b/sysdeps/x86/Makefile
>> @@ -12,7 +12,8 @@ CFLAGS-get-cpuid-feature-leaf.o += $(no-stack-protector)
>>     tests += tst-get-cpu-features tst-get-cpu-features-static \
>>        tst-cpu-features-cpuinfo tst-cpu-features-cpuinfo-static \
>> -     tst-cpu-features-supports tst-cpu-features-supports-static
>> +     tst-cpu-features-supports tst-cpu-features-supports-static \
>> +     tst-hwcap-tunables
>>   tests-static += tst-get-cpu-features-static \
>>           tst-cpu-features-cpuinfo-static \
>>           tst-cpu-features-supports-static
>> @@ -65,6 +66,7 @@ $(objpfx)tst-isa-level-1.out: $(objpfx)tst-isa-level-mod-1-baseline.so \
>>   endif
>>   tst-ifunc-isa-2-ENV = GLIBC_TUNABLES=glibc.cpu.hwcaps=-SSE4_2,-AVX,-AVX2,-AVX512F
>>   tst-ifunc-isa-2-static-ENV = $(tst-ifunc-isa-2-ENV)
>> +tst-hwcap-tunables-ARGS = -- $(host-test-program-cmd)
>>   endif
>>     ifeq ($(subdir),math)
>> diff --git a/sysdeps/x86/cpu-tunables.c b/sysdeps/x86/cpu-tunables.c
>> index 5697885226..0608209768 100644
>> --- a/sysdeps/x86/cpu-tunables.c
>> +++ b/sysdeps/x86/cpu-tunables.c
>> @@ -24,11 +24,12 @@
>>   #include <string.h>
>>   #include <cpu-features.h>
>>   #include <ldsodefs.h>
>> +#include <dl-tunables-parse.h>
>>   #include <dl-symbol-redir-ifunc.h>
>>     #define CHECK_GLIBC_IFUNC_CPU_OFF(f, cpu_features, name, len)        \
>>     _Static_assert (sizeof (#name) - 1 == len, #name " != " #len);    \
>> -  if (memcmp (f, #name, len) == 0)                    \
>> +  if (tunable_str_comma_strcmp_cte (&f, #name))                \
>>       {                                    \
>>         CPU_FEATURE_UNSET (cpu_features, name)                \
>>         break;                                \
>> @@ -38,7 +39,7 @@
>>      which isn't available.  */
>>   #define CHECK_GLIBC_IFUNC_PREFERRED_OFF(f, cpu_features, name, len)    \
>>     _Static_assert (sizeof (#name) - 1 == len, #name " != " #len);    \
>> -  if (memcmp (f, #name, len) == 0)                    \
>> +  if (tunable_str_comma_strcmp_cte (&f, #name) == 0)            \
>>       {                                    \
>>         cpu_features->preferred[index_arch_##name]            \
>>       &= ~bit_arch_##name;                        \
>> @@ -46,12 +47,11 @@
>>       }
>>     /* Enable/disable a preferred feature NAME.  */
>> -#define CHECK_GLIBC_IFUNC_PREFERRED_BOTH(f, cpu_features, name,    \
>> -                      disable, len)            \
>> +#define CHECK_GLIBC_IFUNC_PREFERRED_BOTH(f, cpu_features, name,    len)    \
> 
> Spurious tab.

Ack.

> 
>>     _Static_assert (sizeof (#name) - 1 == len, #name " != " #len);    \
>> -  if (memcmp (f, #name, len) == 0)                    \
>> +  if (tunable_str_comma_strcmp_cte (&f, #name))                \
>>       {                                    \
>> -      if (disable)                            \
>> +      if (f.disable)                            \
>>       cpu_features->preferred[index_arch_##name] &= ~bit_arch_##name;    \
>>         else                                \
>>       cpu_features->preferred[index_arch_##name] |= bit_arch_##name;    \
>> @@ -61,11 +61,11 @@
>>   /* Enable/disable a preferred feature NAME.  Enable a preferred feature
>>      only if the feature NEED is usable.  */
>>   #define CHECK_GLIBC_IFUNC_PREFERRED_NEED_BOTH(f, cpu_features, name,    \
>> -                           need, disable, len)    \
>> +                          need, len)        \
>>     _Static_assert (sizeof (#name) - 1 == len, #name " != " #len);    \
>> -  if (memcmp (f, #name, len) == 0)                    \
>> +  if (tunable_str_comma_strcmp_cte (&f, #name))                \
>>       {                                    \
>> -      if (disable)                            \
>> +      if (f.disable)                            \
>>       cpu_features->preferred[index_arch_##name] &= ~bit_arch_##name;    \
>>         else if (CPU_FEATURE_USABLE_P (cpu_features, need))        \
>>       cpu_features->preferred[index_arch_##name] |= bit_arch_##name;    \
>> @@ -93,38 +93,20 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>>        NOTE: the IFUNC selection may change over time.  Please check all
>>        multiarch implementations when experimenting.  */
>>   -  const char *p = valp->strval, *c;
>>     struct cpu_features *cpu_features = &GLRO(dl_x86_cpu_features);
>> -  size_t len;
>>   -  do
>> -    {
>> -      const char *n;
>> -      bool disable;
>> -      size_t nl;
>> -
>> -      for (c = p; *c != ','; c++)
>> -    if (*c == '\0')
>> -      break;
>> +  struct tunable_str_comma_state_t ts;
>> +  tunable_str_comma_init (&ts, valp);
>>   -      len = c - p;
>> -      disable = *p == '-';
>> -      if (disable)
>> -    {
>> -      n = p + 1;
>> -      nl = len - 1;
>> -    }
>> -      else
>> -    {
>> -      n = p;
>> -      nl = len;
>> -    }
>> -      switch (nl)
>> +  struct tunable_str_comma_t n;
>> +  while (tunable_str_comma_next (&ts, &n))
>> +    {
>> +      switch (n.len)
>>       {
>>       default:
>>         break;
>>       case 3:
>> -      if (disable)
>> +      if (n.disable)
>>           {
>>             CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, AVX, 3);
>>             CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, CX8, 3);
>> @@ -135,7 +117,7 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>>           }
>>         break;
>>       case 4:
>> -      if (disable)
>> +      if (n.disable)
>>           {
>>             CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, AVX2, 4);
>>             CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, BMI1, 4);
>> @@ -149,7 +131,7 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>>           }
>>         break;
>>       case 5:
>> -      if (disable)
>> +      if (n.disable)
>>           {
>>             CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, LZCNT, 5);
>>             CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, MOVBE, 5);
>> @@ -159,12 +141,12 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>>           }
>>         break;
>>       case 6:
>> -      if (disable)
>> +      if (n.disable)
>>           {
>>             CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, POPCNT, 6);
>>             CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, SSE4_1, 6);
>>             CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, SSE4_2, 6);
>> -          if (memcmp (n, "XSAVEC", 6) == 0)
>> +          if (memcmp (n.str, "XSAVEC", 6) == 0)
> 
> Why does this use the bare memcmp and not the tunables_strcmp?

If I recall correctly, I did tried it and it resulted in worse codegen.  The
tunable_str_comma_strcmp also checks the size, which on the x86 code is not
required and for some reason compiler can not eliminate.

> 
>>           {
>>             /* Update xsave_state_size to XSAVE state size.  */
>>             cpu_features->xsave_state_size
>> @@ -174,14 +156,14 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>>           }
>>         break;
>>       case 7:
>> -      if (disable)
>> +      if (n.disable)
>>           {
>>             CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, AVX512F, 7);
>>             CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, OSXSAVE, 7);
>>           }
>>         break;
>>       case 8:
>> -      if (disable)
>> +      if (n.disable)
>>           {
>>             CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, AVX512CD, 8);
>>             CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, AVX512BW, 8);
>> @@ -190,86 +172,72 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>>             CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, AVX512PF, 8);
>>             CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, AVX512VL, 8);
>>           }
>> -      CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features, Slow_BSF,
>> -                        disable, 8);
>> +      CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features, Slow_BSF, 8);
>>         break;
>>       case 11:
>>           {
>> -          CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features,
>> -                        Prefer_ERMS,
>> -                        disable, 11);
>> -          CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features,
>> -                        Prefer_FSRM,
>> -                        disable, 11);
>> +          CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features, Prefer_ERMS,
>> +                        11);
>> +          CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features, Prefer_FSRM,
>> +                        11);
>>             CHECK_GLIBC_IFUNC_PREFERRED_NEED_BOTH (n, cpu_features,
>>                                Slow_SSE4_2,
>>                                SSE4_2,
>> -                             disable, 11);
>> +                             11);
>>           }
>>         break;
>>       case 15:
>>           {
>>             CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features,
>> -                        Fast_Rep_String,
>> -                        disable, 15);
>> +                        Fast_Rep_String, 15);
>>           }
>>         break;
>>       case 16:
>>           {
>>             CHECK_GLIBC_IFUNC_PREFERRED_NEED_BOTH
>> -        (n, cpu_features, Prefer_No_AVX512, AVX512F,
>> -         disable, 16);
>> +        (n, cpu_features, Prefer_No_AVX512, AVX512F, 16);
>>           }
>>         break;
>>       case 18:
>>           {
>>             CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features,
>> -                        Fast_Copy_Backward,
>> -                        disable, 18);
>> +                        Fast_Copy_Backward, 18);
>>           }
>>         break;
>>       case 19:
>>           {
>>             CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features,
>> -                        Fast_Unaligned_Load,
>> -                        disable, 19);
>> +                        Fast_Unaligned_Load, 19);
>>             CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features,
>> -                        Fast_Unaligned_Copy,
>> -                        disable, 19);
>> +                        Fast_Unaligned_Copy, 19);
>>           }
>>         break;
>>       case 20:
>>           {
>>             CHECK_GLIBC_IFUNC_PREFERRED_NEED_BOTH
>> -        (n, cpu_features, Prefer_No_VZEROUPPER, AVX, disable,
>> -         20);
>> +        (n, cpu_features, Prefer_No_VZEROUPPER, AVX, 20);
>>           }
>>         break;
>>       case 23:
>>           {
>>             CHECK_GLIBC_IFUNC_PREFERRED_NEED_BOTH
>> -        (n, cpu_features, AVX_Fast_Unaligned_Load, AVX,
>> -         disable, 23);
>> +        (n, cpu_features, AVX_Fast_Unaligned_Load, AVX, 23);
>>           }
>>         break;
>>       case 24:
>>           {
>>             CHECK_GLIBC_IFUNC_PREFERRED_NEED_BOTH
>> -        (n, cpu_features, MathVec_Prefer_No_AVX512, AVX512F,
>> -         disable, 24);
>> +        (n, cpu_features, MathVec_Prefer_No_AVX512, AVX512F, 24);
>>           }
>>         break;
>>       case 26:
>>           {
>>             CHECK_GLIBC_IFUNC_PREFERRED_NEED_BOTH
>> -        (n, cpu_features, Prefer_PMINUB_for_stringop, SSE2,
>> -         disable, 26);
>> +        (n, cpu_features, Prefer_PMINUB_for_stringop, SSE2, 26);
>>           }
>>         break;
>>       }
>> -      p += len + 1;
>>       }
>> -  while (*c != '\0');
>>   }
>>     #if CET_ENABLED
>> @@ -277,11 +245,11 @@ attribute_hidden
>>   void
>>   TUNABLE_CALLBACK (set_x86_ibt) (tunable_val_t *valp)
>>   {
>> -  if (memcmp (valp->strval, "on", sizeof ("on")) == 0)
>> +  if (tunable_strcmp_cte (valp, "on"))
>>       GL(dl_x86_feature_control).ibt = cet_always_on;
>> -  else if (memcmp (valp->strval, "off", sizeof ("off")) == 0)
>> +  else if (tunable_strcmp_cte (valp, "off"))
>>       GL(dl_x86_feature_control).ibt = cet_always_off;
>> -  else if (memcmp (valp->strval, "permissive", sizeof ("permissive")) == 0)
>> +  else if (tunable_strcmp_cte (valp, "permissive"))
>>       GL(dl_x86_feature_control).ibt = cet_permissive;
>>   }
>>   @@ -289,11 +257,11 @@ attribute_hidden
>>   void
>>   TUNABLE_CALLBACK (set_x86_shstk) (tunable_val_t *valp)
>>   {
>> -  if (memcmp (valp->strval, "on", sizeof ("on")) == 0)
>> +  if (tunable_strcmp_cte (valp, "on"))
>>       GL(dl_x86_feature_control).shstk = cet_always_on;
>> -  else if (memcmp (valp->strval, "off", sizeof ("off")) == 0)
>> +  else if (tunable_strcmp_cte (valp, "off"))
>>       GL(dl_x86_feature_control).shstk = cet_always_off;
>> -  else if (memcmp (valp->strval, "permissive", sizeof ("permissive")) == 0)
>> +  else if (tunable_strcmp_cte (valp, "permissive"))
>>       GL(dl_x86_feature_control).shstk = cet_permissive;
>>   }
>>   #endif
>> diff --git a/sysdeps/x86/tst-hwcap-tunables.c b/sysdeps/x86/tst-hwcap-tunables.c
>> new file mode 100644
>> index 0000000000..0d0a45fa93
>> --- /dev/null
>> +++ b/sysdeps/x86/tst-hwcap-tunables.c
>> @@ -0,0 +1,148 @@
>> +/* Tests for powerpc GLIBC_TUNABLES=glibc.cpu.hwcaps filter.
> 
> s/powerpc/x86/

Ack.

> 
>> +   Copyright (C) 2023 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <http://www.gnu.org/licenses/>.  */
>> +
>> +#include <array_length.h>
>> +#include <getopt.h>
>> +#include <ifunc-impl-list.h>
>> +#include <spawn.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <intprops.h>
>> +#include <support/check.h>
>> +#include <support/support.h>
>> +#include <support/xunistd.h>
>> +#include <support/capture_subprocess.h>
>> +
>> +/* Nonzero if the program gets called via `exec'.  */
>> +#define CMDLINE_OPTIONS \
>> +  { "restart", no_argument, &restart, 1 },
>> +static int restart;
>> +
>> +/* Disable everything.  */
>> +static const char *test_1[] =
>> +{
>> +  "__memcpy_avx512_no_vzeroupper",
>> +  "__memcpy_avx512_unaligned",
>> +  "__memcpy_avx512_unaligned_erms",
>> +  "__memcpy_evex_unaligned",
>> +  "__memcpy_evex_unaligned_erms",
>> +  "__memcpy_avx_unaligned",
>> +  "__memcpy_avx_unaligned_erms",
>> +  "__memcpy_avx_unaligned_rtm",
>> +  "__memcpy_avx_unaligned_erms_rtm",
>> +  "__memcpy_ssse3",
>> +};
>> +
>> +static const struct test_t
>> +{
>> +  const char *env;
>> +  const char *const *funcs;
>> +  size_t nfuncs;
>> +} tests[] =
>> +{
>> +  {
>> +    /* Disable everything.  */
>> +    "-Prefer_ERMS,-Prefer_FSRM,-AVX,-AVX2,-AVX_Usable,-AVX2_Usable,"
>> +    "-AVX512F_Usable,-SSE4_1,-SSE4_2,-SSSE3,-Fast_Unaligned_Load,-ERMS,"
>> +    "-AVX_Fast_Unaligned_Load",
>> +    test_1,
>> +    array_length (test_1)
>> +  },
>> +  {
>> +    /* Same as before, but with some empty suboptions.  */
>> +    ",-,-Prefer_ERMS,-Prefer_FSRM,-AVX,-AVX2,-AVX_Usable,-AVX2_Usable,"
>> +    "-AVX512F_Usable,-SSE4_1,-SSE4_2,,-SSSE3,-Fast_Unaligned_Load,,-,-ERMS,"
>> +    "-AVX_Fast_Unaligned_Load,-,",
>> +    test_1,
>> +    array_length (test_1)
>> +  }
>> +};
>> +
>> +/* Called on process re-execution.  */
>> +_Noreturn static void
>> +handle_restart (int ntest)
>> +{
>> +  struct libc_ifunc_impl impls[32];
>> +  int cnt = __libc_ifunc_impl_list ("memcpy", impls, array_length (impls));
>> +  if (cnt == 0)
>> +    _exit (EXIT_SUCCESS);
>> +  TEST_VERIFY_EXIT (cnt >= 1);
>> +  for (int i = 0; i < cnt; i++)
>> +    {
>> +      for (int f = 0; f < tests[ntest].nfuncs; f++)
>> +    {
>> +      if (strcmp (impls[i].name, tests[ntest].funcs[f]) == 0)
>> +        TEST_COMPARE (impls[i].usable, false);
>> +    }
>> +    }
>> +
>> +  _exit (EXIT_SUCCESS);
>> +}
>> +
>> +static int
>> +do_test (int argc, char *argv[])
>> +{
>> +  /* We must have either:
>> +     - One our fource parameters left if called initially:
>> +       + path to ld.so         optional
>> +       + "--library-path"      optional
>> +       + the library path      optional
>> +       + the application name
>> +       + the test to check  */
>> +
>> +  TEST_VERIFY_EXIT (argc == 2 || argc == 5);
>> +
>> +  if (restart)
>> +    handle_restart (atoi (argv[1]));
>> +
>> +  char nteststr[INT_BUFSIZE_BOUND (int)];
>> +
>> +  char *spargv[10];
>> +  {
>> +    int i = 0;
>> +    for (; i < argc - 1; i++)
>> +      spargv[i] = argv[i + 1];
>> +    spargv[i++] = (char *) "--direct";
>> +    spargv[i++] = (char *) "--restart";
>> +    spargv[i++] = nteststr;
>> +    spargv[i] = NULL;
>> +  }
>> +
>> +  for (int i = 0; i < array_length (tests); i++)
>> +    {
>> +      snprintf (nteststr, sizeof nteststr, "%d", i);
>> +
>> +      printf ("[%d] Spawned test for %s\n", i, tests[i].env);
>> +      char *tunable = xasprintf ("glibc.cpu.hwcaps=%s", tests[i].env);
>> +      setenv ("GLIBC_TUNABLES", tunable, 1);
>> +
>> +      struct support_capture_subprocess result
>> +    = support_capture_subprogram (spargv[0], spargv);
>> +      support_capture_subprocess_check (&result, "tst-tunables", 0,
>> +                    sc_allow_stderr);
>> +      support_capture_subprocess_free (&result);
>> +
>> +      free (tunable);
>> +    }
>> +
>> +  return 0;
>> +}
>> +
>> +#define TEST_FUNCTION_ARGV do_test
>> +#include <support/test-driver.c>