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

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

On 22/11/23 09:23, Siddhesh Poyarekar wrote:
> On 2023-11-21 13:12, Adhemerval Zanella Netto wrote:
>>>> -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:
> 
> Sure, but the strval here should not be the entire envvar, just the value portion of it.  Granted that _dl_strtoul will bail out on the first non-numeric character, so maybe it's not

For the GLIBC_TUNABLES itself, parse_tunables will pass the correct length
for each tunable.  For the alias case, get_next_env will pass the value
length (which I fixed on v4 btw). So I think there is no need to copy the
value on a temporary value, parsing can be done in-place since kernel ensures
the string will be null-terminated.

> 
>>>> -      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.
> 
> Why isn't it required? Couldn't n.str be smaller than 6 chars?

No because on the start of the loop the switch checks for 'n.len'.