| Message ID | PAWPR08MB8982C83A163970393E530A2683472@PAWPR08MB8982.eurprd08.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | AArch64: Use bitfield in cpu_features struct (BZ #32279) | expand |
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
* 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
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
* 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
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 --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; };