diff mbox series

AArch64: Use bitfield in cpu_features struct (BZ #32279)

Message ID PAWPR08MB8982C83A163970393E530A2683472@PAWPR08MB8982.eurprd08.prod.outlook.com
State New
Headers
Series AArch64: Use bitfield in cpu_features struct (BZ #32279) |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed

Commit Message

Wilco Dijkstra Oct. 17, 2024, 11:59 a.m. UTC 
  To avoid increasing the size of the cpu_features struct, use bitfields for "sve" and 
"prefer_sve_ifuncs" fields.  Backporting this to 2.39 and older (but not 2.40) then
restores the original layout which is compatible with older dynamic linkers.
This avoids the issue reported in BZ 32279.

Passes regress, OK for backport?

---

Comments

Aurelien Jarno Oct. 18, 2024, 8:50 p.m. UTC | #1 
Hi,

On 2024-10-17 11:59, Wilco Dijkstra wrote:
> 
> To avoid increasing the size of the cpu_features struct, use bitfields for "sve" and 
> "prefer_sve_ifuncs" fields.  Backporting this to 2.39 and older (but not 2.40) then
> restores the original layout which is compatible with older dynamic linkers.
> This avoids the issue reported in BZ 32279.
> 
> Passes regress, OK for backport?
> 
> ---
> 
> diff --git a/sysdeps/aarch64/cpu-features.h b/sysdeps/aarch64/cpu-features.h
> index bc8d8422388f9cf0a06673af58b39c50dcaa3ee1..0a7a03c1c51e165384eb43d2085fc5c46e0aae21 100644
> --- a/sysdeps/aarch64/cpu-features.h
> +++ b/sysdeps/aarch64/cpu-features.h
> @@ -69,8 +69,8 @@ struct cpu_features
>    bool bti;
>    /* Currently, the GLIBC memory tagging tunable only defines 8 bits.  */
>    uint8_t mte_state;
> -  bool sve;
> -  bool prefer_sve_ifuncs;
> +  bool sve : 1;			/* Bitfield to avoid changing size of struct.  */
> +  bool prefer_sve_ifuncs : 1;	/* Bitfield to avoid changing size of struct.  */
>    bool mops;
>  };

Thanks for working on a patch, I conform it fixes the issue.

Tested-by: Aurelien Jarno <aurelien@aurel32.net>

Regards
Aurelien
Florian Weimer Oct. 21, 2024, 8:41 a.m. UTC | #2 
* Wilco Dijkstra:

> To avoid increasing the size of the cpu_features struct, use bitfields
> for "sve" and "prefer_sve_ifuncs" fields.  Backporting this to 2.39
> and older (but not 2.40) then restores the original layout which is
> compatible with older dynamic linkers.  This avoids the issue reported
> in BZ 32279.
>
> Passes regress, OK for backport?
>
> ---
>
> diff --git a/sysdeps/aarch64/cpu-features.h b/sysdeps/aarch64/cpu-features.h
> index bc8d8422388f9cf0a06673af58b39c50dcaa3ee1..0a7a03c1c51e165384eb43d2085fc5c46e0aae21 100644
> --- a/sysdeps/aarch64/cpu-features.h
> +++ b/sysdeps/aarch64/cpu-features.h
> @@ -69,8 +69,8 @@ struct cpu_features
>    bool bti;
>    /* Currently, the GLIBC memory tagging tunable only defines 8 bits.  */
>    uint8_t mte_state;
> -  bool sve;
> -  bool prefer_sve_ifuncs;
> +  bool sve : 1;			/* Bitfield to avoid changing size of struct.  */
> +  bool prefer_sve_ifuncs : 1;	/* Bitfield to avoid changing size of struct.  */
>    bool mops;
>  };

This changes ABI in the other direction once backported, with different
consequences.

It's probably better to shuffle things around a bit so that it's the
MOPS bit that, when set, produces a trap representation when interpreted
as bool.  Having sve and prefer_sve_ifuncs at the same time seems far
more likely at this point.

On the other hand, the previous ABI has been on the release branch for
month, so I'm not sure if we want to change it now.

Thanks,
Florian
Wilco Dijkstra Oct. 21, 2024, 11:43 a.m. UTC | #3 
Hi Florian,

> This changes ABI in the other direction once backported, with different
> consequences.

My guess is that few distros have picked it up so far, otherwise we might
have heard about this issue much earlier. Fixing it now prevents more distros
hitting the same issue. But yes, it also has the reverse risk for ones that did
pick it up already...

> It's probably better to shuffle things around a bit so that it's the
> MOPS bit that, when set, produces a trap representation when interpreted
> as bool.  Having sve and prefer_sve_ifuncs at the same time seems far
> more likely at this point.

I'm not sure I'm following. The mops and prefer_sve_ifuncs values would be
incorrect, however we crash much earlier because all fields in the dynamic linker
are shifted by 8 bytes. So I'm not sure whether we could ever get to the point of
resolving ifuncs...

I posted a separate patch to move this structure to the end of _rtld_global_ro
which avoids this issue in the future. And we should do more to decouple the
internal dynamic linker data structures from GLIBC as well as being clear what
is being shared and why (this structure was not marked in any way as changing
internal ABI, hence we've been changing it and backporting for many years...).

Cheers,
Wilco
Florian Weimer Oct. 21, 2024, 12:03 p.m. UTC | #4 
* Wilco Dijkstra:

>> It's probably better to shuffle things around a bit so that it's the
>> MOPS bit that, when set, produces a trap representation when interpreted
>> as bool.  Having sve and prefer_sve_ifuncs at the same time seems far
>> more likely at this point.
>
> I'm not sure I'm following. The mops and prefer_sve_ifuncs values
> would be incorrect, however we crash much earlier because all fields
> in the dynamic linker are shifted by 8 bytes. So I'm not sure whether
> we could ever get to the point of resolving ifuncs...

Hmm.  Maybe I'm wrong.  Do we even initialize these fields in the
dormant instance of ld.so?

Thanks,
Florian
Wilco Dijkstra Oct. 22, 2024, 5:42 p.m. UTC | #5 
Hi Florian,

>>> It's probably better to shuffle things around a bit so that it's the
>>> MOPS bit that, when set, produces a trap representation when interpreted
>>> as bool.  Having sve and prefer_sve_ifuncs at the same time seems far
>>> more likely at this point.
>>
>> I'm not sure I'm following. The mops and prefer_sve_ifuncs values
>> would be incorrect, however we crash much earlier because all fields
>> in the dynamic linker are shifted by 8 bytes. So I'm not sure whether
>> we could ever get to the point of resolving ifuncs...
>
> Hmm.  Maybe I'm wrong.  Do we even initialize these fields in the
> dormant instance of ld.so?

No if it uses the dormant ld.so, we crash early on.

The issue is caused by the __rtld_static_init in a statically linked binary writing to
_rtld_global_ro of the dynamically linked ld.so. Since _dl_dlfcn_hook is one of the
last fields in this struct, any change in layout means _dl_dlfcn_hook is incorrectly
initialized, which results in a crash. I can easily reproduce by adding or removing
a field before it.

So the goal here is to compress the cpu_features struct using bitfields so it is
back to the previous size, and thus _dl_dlfcn_hook is at the same offset as before.
It doesn't look feasible to detect this case since we don't know whether the struct
are mismatched.

Cheers,
Wilco
diff mbox series

Patch

diff --git a/sysdeps/aarch64/cpu-features.h b/sysdeps/aarch64/cpu-features.h
index bc8d8422388f9cf0a06673af58b39c50dcaa3ee1..0a7a03c1c51e165384eb43d2085fc5c46e0aae21 100644
--- a/sysdeps/aarch64/cpu-features.h
+++ b/sysdeps/aarch64/cpu-features.h
@@ -69,8 +69,8 @@  struct cpu_features
   bool bti;
   /* Currently, the GLIBC memory tagging tunable only defines 8 bits.  */
   uint8_t mte_state;
-  bool sve;
-  bool prefer_sve_ifuncs;
+  bool sve : 1;			/* Bitfield to avoid changing size of struct.  */
+  bool prefer_sve_ifuncs : 1;	/* Bitfield to avoid changing size of struct.  */
   bool mops;
 };