elf: Fix glibc-hwcaps priorities with cache flags mismatches [BZ #27046]
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
If lib->flags (in the cache) did not match GLRO (dl_correct_cache_id),
searching for further glibc-hwcaps entries did not happen, and it
was possible that the best glibc-hwcaps was not found. By accident,
this causes a test failure for elf/tst-glibc-hwcaps-prepend-cache
on armv7l.
This commit changes the cache lookup logic to continue searching
if (a) no match has been found, (b) a named glibc-hwcaps match
has been found(), or (c) non-glibc-hwcaps match has been found
and the entry flags and cache default flags do not match.
_DL_CACHE_DEFAULT_ID is used instead of GLRO (dl_correct_cache_id)
because the latter is only written once on i386 if loading
of libc.so.5 libraries is selected, so GLRO (dl_correct_cache_id)
should probably removed in a future change.
Tested on i686-linux-gnu, x86_64-linux-gnu, and on an armv7l system that
exposed the issue. Most of the diff is due to whitespace changes.
---
elf/dl-cache.c | 134 ++++++++++++++++++++++++++++-----------------------------
1 file changed, 65 insertions(+), 69 deletions(-)
Comments
The 06/22/2021 15:27, Florian Weimer via Libc-alpha wrote:
> If lib->flags (in the cache) did not match GLRO (dl_correct_cache_id),
> searching for further glibc-hwcaps entries did not happen, and it
> was possible that the best glibc-hwcaps was not found. By accident,
> this causes a test failure for elf/tst-glibc-hwcaps-prepend-cache
> on armv7l.
>
> This commit changes the cache lookup logic to continue searching
> if (a) no match has been found, (b) a named glibc-hwcaps match
> has been found(), or (c) non-glibc-hwcaps match has been found
> and the entry flags and cache default flags do not match.
>
> _DL_CACHE_DEFAULT_ID is used instead of GLRO (dl_correct_cache_id)
> because the latter is only written once on i386 if loading
> of libc.so.5 libraries is selected, so GLRO (dl_correct_cache_id)
> should probably removed in a future change.
>
> Tested on i686-linux-gnu, x86_64-linux-gnu, and on an armv7l system that
> exposed the issue. Most of the diff is due to whitespace changes.
Looks good, thanks.
Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
>
> ---
> elf/dl-cache.c | 134 ++++++++++++++++++++++++++++-----------------------------
> 1 file changed, 65 insertions(+), 69 deletions(-)
>
> diff --git a/elf/dl-cache.c b/elf/dl-cache.c
> index 32f3bef5ea..2b8da8650d 100644
> --- a/elf/dl-cache.c
> +++ b/elf/dl-cache.c
> @@ -269,81 +269,77 @@ search_cache (const char *string_table, uint32_t string_table_size,
> if (_dl_cache_check_flags (flags)
> && _dl_cache_verify_ptr (lib->value, string_table_size))
> {
> - if (best == NULL || flags == GLRO (dl_correct_cache_id))
> + /* Named/extension hwcaps get slightly different
> + treatment: We keep searching for a better
> + match. */
> + bool named_hwcap = false;
> +
> + if (entry_size >= sizeof (struct file_entry_new))
> {
> - /* Named/extension hwcaps get slightly different
> - treatment: We keep searching for a better
> - match. */
> - bool named_hwcap = false;
> + /* The entry is large enough to include
> + HWCAP data. Check it. */
> + struct file_entry_new *libnew
> + = (struct file_entry_new *) lib;
>
> - if (entry_size >= sizeof (struct file_entry_new))
> +#ifdef SHARED
> + named_hwcap = dl_cache_hwcap_extension (libnew);
> + if (named_hwcap
> + && !dl_cache_hwcap_isa_level_compatible (libnew))
> + continue;
> +#endif
> +
> + /* The entries with named/extension hwcaps have
> + been exhausted (they are listed before all
> + other entries). Return the best match
> + encountered so far if there is one. */
> + if (!named_hwcap && best != NULL)
> + break;
> +
> + if ((libnew->hwcap & hwcap_exclude) && !named_hwcap)
> + continue;
> + if (GLRO (dl_osversion)
> + && libnew->osversion > GLRO (dl_osversion))
> + continue;
> + if (_DL_PLATFORMS_COUNT
> + && (libnew->hwcap & _DL_HWCAP_PLATFORM) != 0
> + && ((libnew->hwcap & _DL_HWCAP_PLATFORM)
> + != platform))
> + continue;
> +
> +#ifdef SHARED
> + /* For named hwcaps, determine the priority and
> + see if beats what has been found so far. */
> + if (named_hwcap)
> {
> - /* The entry is large enough to include
> - HWCAP data. Check it. */
> - struct file_entry_new *libnew
> - = (struct file_entry_new *) lib;
> -
> -#ifdef SHARED
> - named_hwcap = dl_cache_hwcap_extension (libnew);
> - if (named_hwcap
> - && !dl_cache_hwcap_isa_level_compatible (libnew))
> + uint32_t entry_priority
> + = glibc_hwcaps_priority (libnew->hwcap);
> + if (entry_priority == 0)
> + /* Not usable at all. Skip. */
> + continue;
> + else if (best == NULL
> + || entry_priority < best_priority)
> + /* This entry is of higher priority
> + than the previous one, or it is the
> + first entry. */
> + best_priority = entry_priority;
> + else
> + /* An entry has already been found,
> + but it is a better match. */
> continue;
> -#endif
> -
> - /* The entries with named/extension hwcaps
> - have been exhausted. Return the best
> - match encountered so far if there is
> - one. */
> - if (!named_hwcap && best != NULL)
> - break;
> -
> - if ((libnew->hwcap & hwcap_exclude) && !named_hwcap)
> - continue;
> - if (GLRO (dl_osversion)
> - && libnew->osversion > GLRO (dl_osversion))
> - continue;
> - if (_DL_PLATFORMS_COUNT
> - && (libnew->hwcap & _DL_HWCAP_PLATFORM) != 0
> - && ((libnew->hwcap & _DL_HWCAP_PLATFORM)
> - != platform))
> - continue;
> -
> -#ifdef SHARED
> - /* For named hwcaps, determine the priority
> - and see if beats what has been found so
> - far. */
> - if (named_hwcap)
> - {
> - uint32_t entry_priority
> - = glibc_hwcaps_priority (libnew->hwcap);
> - if (entry_priority == 0)
> - /* Not usable at all. Skip. */
> - continue;
> - else if (best == NULL
> - || entry_priority < best_priority)
> - /* This entry is of higher priority
> - than the previous one, or it is the
> - first entry. */
> - best_priority = entry_priority;
> - else
> - /* An entry has already been found,
> - but it is a better match. */
> - continue;
> - }
> -#endif /* SHARED */
> }
> -
> - best = string_table + lib->value;
> -
> - if (flags == GLRO (dl_correct_cache_id)
> - && !named_hwcap)
> - /* We've found an exact match for the shared
> - object and no general `ELF' release. Stop
> - searching, but not if a named (extension)
> - hwcap is used. In this case, an entry with
> - a higher priority may come up later. */
> - break;
> +#endif /* SHARED */
> }
> +
> + best = string_table + lib->value;
> +
> + if (!named_hwcap && flags == _DL_CACHE_DEFAULT_ID)
> + /* With named hwcaps, we need to keep searching to
> + see if we find a better match. A better match
> + is also possible if the flags of the current
> + entry do not match the expected cache flags.
> + But if the flags match, no better entry will be
> + found. */
> + break;
> }
> }
> while (++middle <= right);
>
* Florian Weimer via Libc-alpha:
> If lib->flags (in the cache) did not match GLRO (dl_correct_cache_id),
> searching for further glibc-hwcaps entries did not happen, and it
> was possible that the best glibc-hwcaps was not found. By accident,
> this causes a test failure for elf/tst-glibc-hwcaps-prepend-cache
> on armv7l.
>
> This commit changes the cache lookup logic to continue searching
> if (a) no match has been found, (b) a named glibc-hwcaps match
> has been found(), or (c) non-glibc-hwcaps match has been found
> and the entry flags and cache default flags do not match.
>
> _DL_CACHE_DEFAULT_ID is used instead of GLRO (dl_correct_cache_id)
> because the latter is only written once on i386 if loading
> of libc.so.5 libraries is selected, so GLRO (dl_correct_cache_id)
> should probably removed in a future change.
>
> Tested on i686-linux-gnu, x86_64-linux-gnu, and on an armv7l system that
> exposed the issue. Most of the diff is due to whitespace changes.
Ping?
<https://sourceware.org/pipermail/libc-alpha/2021-June/127862.html>
Is there still interest in fixing this for 32-bit Arm?
Thanks,
Florian
* Florian Weimer:
> * Florian Weimer via Libc-alpha:
>
>> If lib->flags (in the cache) did not match GLRO (dl_correct_cache_id),
>> searching for further glibc-hwcaps entries did not happen, and it
>> was possible that the best glibc-hwcaps was not found. By accident,
>> this causes a test failure for elf/tst-glibc-hwcaps-prepend-cache
>> on armv7l.
>>
>> This commit changes the cache lookup logic to continue searching
>> if (a) no match has been found, (b) a named glibc-hwcaps match
>> has been found(), or (c) non-glibc-hwcaps match has been found
>> and the entry flags and cache default flags do not match.
>>
>> _DL_CACHE_DEFAULT_ID is used instead of GLRO (dl_correct_cache_id)
>> because the latter is only written once on i386 if loading
>> of libc.so.5 libraries is selected, so GLRO (dl_correct_cache_id)
>> should probably removed in a future change.
>>
>> Tested on i686-linux-gnu, x86_64-linux-gnu, and on an armv7l system that
>> exposed the issue. Most of the diff is due to whitespace changes.
>
> Ping?
>
> <https://sourceware.org/pipermail/libc-alpha/2021-June/127862.html>
>
> Is there still interest in fixing this for 32-bit Arm?
Eh, wait, this has already been merged. I will update the bug.
Thanks,
Florian
@@ -269,81 +269,77 @@ search_cache (const char *string_table, uint32_t string_table_size,
if (_dl_cache_check_flags (flags)
&& _dl_cache_verify_ptr (lib->value, string_table_size))
{
- if (best == NULL || flags == GLRO (dl_correct_cache_id))
+ /* Named/extension hwcaps get slightly different
+ treatment: We keep searching for a better
+ match. */
+ bool named_hwcap = false;
+
+ if (entry_size >= sizeof (struct file_entry_new))
{
- /* Named/extension hwcaps get slightly different
- treatment: We keep searching for a better
- match. */
- bool named_hwcap = false;
+ /* The entry is large enough to include
+ HWCAP data. Check it. */
+ struct file_entry_new *libnew
+ = (struct file_entry_new *) lib;
- if (entry_size >= sizeof (struct file_entry_new))
+#ifdef SHARED
+ named_hwcap = dl_cache_hwcap_extension (libnew);
+ if (named_hwcap
+ && !dl_cache_hwcap_isa_level_compatible (libnew))
+ continue;
+#endif
+
+ /* The entries with named/extension hwcaps have
+ been exhausted (they are listed before all
+ other entries). Return the best match
+ encountered so far if there is one. */
+ if (!named_hwcap && best != NULL)
+ break;
+
+ if ((libnew->hwcap & hwcap_exclude) && !named_hwcap)
+ continue;
+ if (GLRO (dl_osversion)
+ && libnew->osversion > GLRO (dl_osversion))
+ continue;
+ if (_DL_PLATFORMS_COUNT
+ && (libnew->hwcap & _DL_HWCAP_PLATFORM) != 0
+ && ((libnew->hwcap & _DL_HWCAP_PLATFORM)
+ != platform))
+ continue;
+
+#ifdef SHARED
+ /* For named hwcaps, determine the priority and
+ see if beats what has been found so far. */
+ if (named_hwcap)
{
- /* The entry is large enough to include
- HWCAP data. Check it. */
- struct file_entry_new *libnew
- = (struct file_entry_new *) lib;
-
-#ifdef SHARED
- named_hwcap = dl_cache_hwcap_extension (libnew);
- if (named_hwcap
- && !dl_cache_hwcap_isa_level_compatible (libnew))
+ uint32_t entry_priority
+ = glibc_hwcaps_priority (libnew->hwcap);
+ if (entry_priority == 0)
+ /* Not usable at all. Skip. */
+ continue;
+ else if (best == NULL
+ || entry_priority < best_priority)
+ /* This entry is of higher priority
+ than the previous one, or it is the
+ first entry. */
+ best_priority = entry_priority;
+ else
+ /* An entry has already been found,
+ but it is a better match. */
continue;
-#endif
-
- /* The entries with named/extension hwcaps
- have been exhausted. Return the best
- match encountered so far if there is
- one. */
- if (!named_hwcap && best != NULL)
- break;
-
- if ((libnew->hwcap & hwcap_exclude) && !named_hwcap)
- continue;
- if (GLRO (dl_osversion)
- && libnew->osversion > GLRO (dl_osversion))
- continue;
- if (_DL_PLATFORMS_COUNT
- && (libnew->hwcap & _DL_HWCAP_PLATFORM) != 0
- && ((libnew->hwcap & _DL_HWCAP_PLATFORM)
- != platform))
- continue;
-
-#ifdef SHARED
- /* For named hwcaps, determine the priority
- and see if beats what has been found so
- far. */
- if (named_hwcap)
- {
- uint32_t entry_priority
- = glibc_hwcaps_priority (libnew->hwcap);
- if (entry_priority == 0)
- /* Not usable at all. Skip. */
- continue;
- else if (best == NULL
- || entry_priority < best_priority)
- /* This entry is of higher priority
- than the previous one, or it is the
- first entry. */
- best_priority = entry_priority;
- else
- /* An entry has already been found,
- but it is a better match. */
- continue;
- }
-#endif /* SHARED */
}
-
- best = string_table + lib->value;
-
- if (flags == GLRO (dl_correct_cache_id)
- && !named_hwcap)
- /* We've found an exact match for the shared
- object and no general `ELF' release. Stop
- searching, but not if a named (extension)
- hwcap is used. In this case, an entry with
- a higher priority may come up later. */
- break;
+#endif /* SHARED */
}
+
+ best = string_table + lib->value;
+
+ if (!named_hwcap && flags == _DL_CACHE_DEFAULT_ID)
+ /* With named hwcaps, we need to keep searching to
+ see if we find a better match. A better match
+ is also possible if the flags of the current
+ entry do not match the expected cache flags.
+ But if the flags match, no better entry will be
+ found. */
+ break;
}
}
while (++middle <= right);