Re: "Segmentation fault" of pahole

[Ihor Solodrai <ihor.solodrai@linux.dev>]

On 8/19/25 10:33 AM, Alan Maguire wrote:
> On 18/08/2025 21:52, Arnaldo Carvalho de Melo wrote:
>> On Mon, Aug 18, 2025 at 10:56:36AM -0700, Ihor Solodrai wrote:
>>>
>>> [...]
>>>
>>> Hi everyone.
>>>
>>> I was able to reproduce the error by feeding pahole a vmlinux with a
>>> debuglink [1], created with:
>>>
>>>      vmlinux=$(realpath ~/kernels/bpf-next/.tmp_vmlinux1)
>>>      objcopy --only-keep-debug $vmlinux vmlinux.debug
>>>      objcopy --strip-all --add-gnu-debuglink=vmlinux.debug $vmlinux
>>> vmlinux.stripped
>>>
>>> With that, I got the following valgrind output:
>>>
>>>      $ valgrind ./build/pahole --btf_features=default -J
>>> ./mbox/vmlinux.stripped
>>>      ==40680== Memcheck, a memory error detector
>>>      ==40680== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et
>>> al.
>>>      ==40680== Using Valgrind-3.25.1 and LibVEX; rerun with -h for copyright
>>> info
>>>      ==40680== Command: ./build/pahole --btf_features=default -J
>>> ./mbox/vmlinux.stripped
>>>      ==40680==
>>>      ==40680== Warning: set address range perms: large range [0x7c20000,
>>> 0x32e2d000) (defined)
>>>      ==40680== Thread 2:
>>>      ==40680== Invalid write of size 8
>>>      ==40680==    at 0x487D34D: __list_del (list.h:106)
>>>      ==40680==    by 0x487D384: list_del (list.h:118)
>>>      ==40680==    by 0x487D6DB: elf_functions__delete (btf_encoder.c:170)
>>>      ==40680==    by 0x487D77C: elf_functions__new (btf_encoder.c:201)
>>>      ==40680==    by 0x4880E2A: btf_encoder__elf_functions
>>> (btf_encoder.c:1485)
>>>      ==40680==    by 0x4883558: btf_encoder__new (btf_encoder.c:2450)
>>>      ==40680==    by 0x4078DD: pahole_stealer__btf_encode (pahole.c:3160)
>>>      ==40680==    by 0x407B0D: pahole_stealer (pahole.c:3221)
>>>      ==40680==    by 0x488D2F5: cus__steal_now (dwarf_loader.c:3266)
>>>      ==40680==    by 0x488DF74: dwarf_loader__worker_thread
>>> (dwarf_loader.c:3678)
>>>      ==40680==    by 0x4A8F723: start_thread (pthread_create.c:448)
>>>      ==40680==    by 0x4B13613: clone (clone.S:100)
>>>      ==40680==  Address 0x8 is not stack'd, malloc'd or (recently) free'd
>>>
>>> As far as I understand, in principle pahole could support search for a
>>> file linked via .gnu_debuglink, but that's a separate issue.
>>
>> Agreed.
>>   
>>> Please see a bugfix patch below.
>>>
>>> [1]
>>> https://manpages.debian.org/unstable/binutils-common/objcopy.1.en.html#add~3
>>>
>>>
>>>  From 6104783080709dad0726740615149951109f839e Mon Sep 17 00:00:00 2001
>>> From: Ihor Solodrai <ihor.solodrai@linux.dev>
>>> Date: Mon, 18 Aug 2025 10:30:16 -0700
>>> Subject: [PATCH] btf_encoder: fix elf_functions cleanup on error
>>>
>>> When elf_functions__new() errors out and jumps to
>>> elf_functions__delete(), pahole segfaults on attempt to list_del the
>>> elf_functions instance from a list, to which it was never added.
>>>
>>> Fix this by changing elf_functions__delete() to
>>> elf_functions__clear(), moving list_del and free calls out of it. Then
>>> clear and free on error, and remove from the list on normal cleanup in
>>> elf_functions_list__clear().
>>
>> I think we should still call it __delete() to have a counterpart to
>> __new() and just remove that removal from the list from the __delete().

Thanks for the review. Here is a v2:

 From f3d6b1eb33df182bed94e09d716de0f883816513 Mon Sep 17 00:00:00 2001
From: Ihor Solodrai <ihor.solodrai@linux.dev>
Date: Tue, 19 Aug 2025 12:05:38 -0700
Subject: [PATCH dwarves v2] btf_encoder: fix elf_functions cleanup on error

When elf_functions__new() errors out and jumps to
elf_functions__delete(), pahole segfaults on attempt to list_del() the
elf_functions instance from a list, to which it was never added.

Fix this by moving list_del() call out of
elf_functions__delete(). Remove from the list only on normal cleanup
in elf_functions_list__clear().

v1: 
https://lore.kernel.org/dwarves/979a1ac4-21d3-4384-8ce4-d10f41887088@linux.dev/

Closes: 
https://lore.kernel.org/dwarves/24bcc853-533c-42ab-bc37-0c13e0baa217@windriver.com/
Reported-by: Changqing Li <changqing.li@windriver.com>
Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
  btf_encoder.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 3f040fe..6300a43 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -168,7 +168,6 @@ static inline void elf_functions__delete(struct 
elf_functions *funcs)
  		free(funcs->entries[i].alias);
  	free(funcs->entries);
  	elf_symtab__delete(funcs->symtab);
-	list_del(&funcs->node);
  	free(funcs);
  }

@@ -210,6 +209,7 @@ static inline void elf_functions_list__clear(struct 
list_head *elf_functions_lis

  	list_for_each_safe(pos, tmp, elf_functions_list) {
  		funcs = list_entry(pos, struct elf_functions, node);
+		list_del(&funcs->node);
  		elf_functions__delete(funcs);
  	}
  }
-- 
2.50.1



>>
>> Apart from that, it looks to address a bug, so with the above changed:
>>
>> Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>>
> 
> Thanks for the fix Ihor!
> 
> Sorry to bikeshed this but how about using funcs->elf as a proxy for
> determining if we have elf function info to add to the list, so we could
> then fix elf_functions__delete() to guard the list_del():
> 
> 	if (funcs->elf)
> 		list_del(&funcs->node);
> 
> 
> we'd just then need to tweak
> 
> -	funcs->elf = elf;
>          err = elf_functions__collect(funcs);
> 	if (err < 0)
>                  goto out_delete;
> +	funcs->elf = elf;
> 
> Would that work?

Not for this bug, because we actually check for a NULL Elf earlier here:

static struct elf_functions *btf_encoder__elf_functions(struct 
btf_encoder *encoder)
{
	struct elf_functions *funcs = NULL;

	if (!encoder->cu || !encoder->cu->elf)    // <-- this
		return NULL;

	funcs = elf_functions__find(encoder->cu->elf, 
&encoder->elf_functions_list);
	if (!funcs) {
		funcs = elf_functions__new(encoder->cu->elf);
		if (funcs)
			list_add(&funcs->node, &encoder->elf_functions_list);
	}

	return funcs;
}

The condition triggering an error (at least in the case of debuglink
that I made up) is in elf_symtab__new():

struct elf_symtab *elf_symtab__new(const char *name, Elf *elf)
{
	size_t symtab_index;

	if (name == NULL)
		name = ".symtab";

	GElf_Shdr shdr;
	Elf_Scn *sec = elf_section_by_name(elf, &shdr, name, &symtab_index);

	if (sec == NULL)    // <--- this
		return NULL;
     ...


> 
>> [...]