diff mbox series

selftests/vDSO: fix GNU hash table entry size for s390x

Message ID 20250213-selftests-vdso-s390-gnu-hash-v1-1-ace3bcc940a3@linutronix.de (mailing list archive)
State New
Headers show
Series selftests/vDSO: fix GNU hash table entry size for s390x | expand

Commit Message

Thomas Weißschuh Feb. 13, 2025, 9:41 a.m. UTC 
Commit 14be4e6f3522 ("selftests: vDSO: fix ELF hash table entry size for s390x")
changed the type of the ELF hash table entries to 64bit on s390x.
However the *GNU* hash tables entries are always 32bit.
The "bucket" pointer is shared between both hash algorithms.
On s390x the GNU algorithm assigns and dereferences this 64bit pointer as a
32bit pointer, leading to compiler warnings and runtime crashes.

Introduce a new dedicated "gnu_bucket" pointer which is used by the GNU hash.

Fixes: e0746bde6f82 ("selftests/vDSO: support DT_GNU_HASH")
Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
 tools/testing/selftests/vDSO/parse_vdso.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)


---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20250213-selftests-vdso-s390-gnu-hash-7206671abc85

Best regards,

Comments

Jens Remus Feb. 13, 2025, 12:47 p.m. UTC | #1 
On 13.02.2025 10:41, Thomas Weißschuh wrote:
> Commit 14be4e6f3522 ("selftests: vDSO: fix ELF hash table entry size for s390x")
> changed the type of the ELF hash table entries to 64bit on s390x.
> However the *GNU* hash tables entries are always 32bit.
> The "bucket" pointer is shared between both hash algorithms.
> On s390x the GNU algorithm assigns and dereferences this 64bit pointer as a
> 32bit pointer, leading to compiler warnings and runtime crashes.

Nit: The compiler complains about assignments between incompatible pointer
types (e.g. "Elf64_Xword *" and "Elf64_Word *").  The size of the pointers
themselves is not different, as it is usually defined by the architecture
regardless of the type of data pointed at.  The real issue is that the
32-bit GNU hash entries are erroneously accessed as if they were 64-bit
entries via "bucket" on s390x.

> Introduce a new dedicated "gnu_bucket" pointer which is used by the GNU hash.
> 
> Fixes: e0746bde6f82 ("selftests/vDSO: support DT_GNU_HASH")
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
>   tools/testing/selftests/vDSO/parse_vdso.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Jens Remus <jremus@linux.ibm.com>

Thanks for taking care!

Regards,
Jens
Thomas Weißschuh Feb. 13, 2025, 2:05 p.m. UTC | #2 
On Thu, Feb 13, 2025 at 01:47:26PM +0100, Jens Remus wrote:
> On 13.02.2025 10:41, Thomas Weißschuh wrote:
> > Commit 14be4e6f3522 ("selftests: vDSO: fix ELF hash table entry size for s390x")
> > changed the type of the ELF hash table entries to 64bit on s390x.
> > However the *GNU* hash tables entries are always 32bit.
> > The "bucket" pointer is shared between both hash algorithms.
> > On s390x the GNU algorithm assigns and dereferences this 64bit pointer as a
> > 32bit pointer, leading to compiler warnings and runtime crashes.
> 
> Nit: The compiler complains about assignments between incompatible pointer
> types (e.g. "Elf64_Xword *" and "Elf64_Word *").  The size of the pointers
> themselves is not different, as it is usually defined by the architecture
> regardless of the type of data pointed at.  The real issue is that the
> 32-bit GNU hash entries are erroneously accessed as if they were 64-bit
> entries via "bucket" on s390x.

Well, yes of course. It should have been
"pointer to 64bit as a pointer to 32bit"
I'll wait for some more feedback and roll it into v2.
Or I'd be happy if whoever picks it up fixes the wording.

> > Introduce a new dedicated "gnu_bucket" pointer which is used by the GNU hash.
> > 
> > Fixes: e0746bde6f82 ("selftests/vDSO: support DT_GNU_HASH")
> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> > ---
> >   tools/testing/selftests/vDSO/parse_vdso.c | 10 +++++-----
> >   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Reviewed-by: Jens Remus <jremus@linux.ibm.com>
> 
> Thanks for taking care!

You're welcome.

To be honest I'm a bit concerned that nobody noticed this before.
The test programs segfault instantly.
diff mbox series

Patch

diff --git a/tools/testing/selftests/vDSO/parse_vdso.c b/tools/testing/selftests/vDSO/parse_vdso.c
index 2fe5e983cb22f1ed066d0310a54f6aef2ed77ed8..f89d052c730eb43eea28d69ca27b56e897503e16 100644
--- a/tools/testing/selftests/vDSO/parse_vdso.c
+++ b/tools/testing/selftests/vDSO/parse_vdso.c
@@ -53,7 +53,7 @@  static struct vdso_info
 	/* Symbol table */
 	ELF(Sym) *symtab;
 	const char *symstrings;
-	ELF(Word) *gnu_hash;
+	ELF(Word) *gnu_hash, *gnu_bucket;
 	ELF_HASH_ENTRY *bucket, *chain;
 	ELF_HASH_ENTRY nbucket, nchain;
 
@@ -185,8 +185,8 @@  void vdso_init_from_sysinfo_ehdr(uintptr_t base)
 		/* The bucket array is located after the header (4 uint32) and the bloom
 		 * filter (size_t array of gnu_hash[2] elements).
 		 */
-		vdso_info.bucket = vdso_info.gnu_hash + 4 +
-				   sizeof(size_t) / 4 * vdso_info.gnu_hash[2];
+		vdso_info.gnu_bucket = vdso_info.gnu_hash + 4 +
+				       sizeof(size_t) / 4 * vdso_info.gnu_hash[2];
 	} else {
 		vdso_info.nbucket = hash[0];
 		vdso_info.nchain = hash[1];
@@ -268,11 +268,11 @@  void *vdso_sym(const char *version, const char *name)
 	if (vdso_info.gnu_hash) {
 		uint32_t h1 = gnu_hash(name), h2, *hashval;
 
-		i = vdso_info.bucket[h1 % vdso_info.nbucket];
+		i = vdso_info.gnu_bucket[h1 % vdso_info.nbucket];
 		if (i == 0)
 			return 0;
 		h1 |= 1;
-		hashval = vdso_info.bucket + vdso_info.nbucket +
+		hashval = vdso_info.gnu_bucket + vdso_info.nbucket +
 			  (i - vdso_info.gnu_hash[1]);
 		for (;; i++) {
 			ELF(Sym) *sym = &vdso_info.symtab[i];