Re: [PATCH v10 1/4] kallsyms: embed source file:line info in kernel stack traces

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] kallsyms: gen_lineinfo excludes STT_NOTYPE and unsized symbols in sym_starts
- [Medium] kallsyms: collect_symbol_starts() includes SHN_ABS and mapping symbols
- [Medium] kallsyms: line-0 entries overwrite valid coverage in deduplicate()
- [Low] scripts: missing libelf in pkg-config query for gen_lineinfo in Makefile
--

commit 939ac5438a3c14ba3c356cc9994f3c7b50467285
Author: Sasha Levin <sashal@kernel.org>

kallsyms: embed source file:line info in kernel stack traces

This commit adds CONFIG_KALLSYMS_LINEINFO and a host tool
(scripts/gen_lineinfo) to extract DWARF .debug_line address-to-source-line
mappings into vmlinux .rodata so stack traces include (file:line)
annotations.

> diff --git a/scripts/Makefile b/scripts/Makefile
> index d7fbb7ca098b1..94b09cf7c1839 100644
> --- a/scripts/Makefile
> +++ b/scripts/Makefile
> @@ -4,6 +4,7 @@
>  # the kernel for the build process.
>  
>  hostprogs-always-y				+= kallsyms
> +hostprogs-always-$(CONFIG_KALLSYMS_LINEINFO)		+= gen_lineinfo
>  hostprogs-always-$(BUILD_C_RECORDMCOUNT)		+= recordmcount
>  hostprogs-always-$(CONFIG_BUILDTIME_TABLE_SORT)		+= sorttable
>  hostprogs-always-$(CONFIG_ASN1)				+= asn1_compiler
> @@ -40,6 +41,8 @@ HOSTCFLAGS_asn1_compiler.o = -I$(srctree)/include
>  HOSTCFLAGS_sign-file.o = $(shell $(HOSTPKG_CONFIG) --cflags libcrypto 2> /dev/null)
>  HOSTCFLAGS_sign-file.o += -I$(srctree)/tools/include/uapi/
>  HOSTLDLIBS_sign-file = $(shell $(HOSTPKG_CONFIG) --libs libcrypto 2> /dev/null || echo -lcrypto)
> +HOSTCFLAGS_gen_lineinfo.o = $(shell $(HOSTPKG_CONFIG) --cflags libdw 2> /dev/null)
> +HOSTLDLIBS_gen_lineinfo = $(shell $(HOSTPKG_CONFIG) --libs libdw 2> /dev/null || echo -ldw -lelf)

[Severity: Low]
Should HOSTCFLAGS_gen_lineinfo.o and HOSTLDLIBS_gen_lineinfo in
scripts/Makefile query pkg-config for both libdw and libelf?

scripts/gen_lineinfo.c directly includes <elf.h> and <gelf.h> and calls
libelf APIs in main(), collect_symbol_starts(), and find_vmlinux_sym()
(such as elf_version(), elf_begin(), elf_nextscn(), gelf_getshdr(),
elf_getdata(), gelf_getsym(), elf_strptr(), elf_errmsg(), elf_errno(), and
elf_end()).

In elfutils, libdw.pc lists libelf under Requires.private rather than
Requires. When pkg-config is installed on the host, running:

  $(HOSTPKG_CONFIG) --libs libdw

succeeds with exit status 0 and outputs only -ldw, which prevents the
"|| echo -ldw -lelf" fallback from running. On toolchains that default to
--no-copy-dt-needed-entries where libdw.so is a symlink to libdw.so.1
rather than a linker script, linking scripts/gen_lineinfo can fail with
undefined references to the libelf symbols.

Would querying "$(HOSTPKG_CONFIG) --cflags libdw libelf" and
"$(HOSTPKG_CONFIG) --libs libdw libelf" avoid this build failure?

> diff --git a/scripts/gen_lineinfo.c b/scripts/gen_lineinfo.c
> new file mode 100644
> index 0000000000000..7562ac81acca5
> --- /dev/null
> +++ b/scripts/gen_lineinfo.c
[ ... ]
> +static void collect_symbol_starts(Elf *elf, unsigned long long text_addr)
> +{
> +	Elf_Scn *scn = NULL;
> +	GElf_Shdr shdr;
> +
> +	while ((scn = elf_nextscn(elf, scn)) != NULL) {
> +		Elf_Data *data;
> +		size_t nsyms;
> +
> +		if (!gelf_getshdr(scn, &shdr))
> +			continue;
> +		if (shdr.sh_type != SHT_SYMTAB || !shdr.sh_entsize)
> +			continue;
> +
> +		data = elf_getdata(scn, NULL);
> +		if (!data)
> +			continue;
> +
> +		nsyms = shdr.sh_size / shdr.sh_entsize;
> +		for (size_t i = 0; i < nsyms; i++) {
> +			GElf_Sym sym;
> +			unsigned long long raw;
> +
> +			if (!gelf_getsym(data, i, &sym))
> +				continue;
> +			switch (GELF_ST_TYPE(sym.st_info)) {
> +			case STT_FUNC:
> +			case STT_NOTYPE:
> +				break;
> +			default:
> +				continue;
> +			}
> +			if (sym.st_value < text_addr)
> +				continue;
> +			if (text_end_addr && sym.st_value >= text_end_addr)
> +				continue;
> +
> +			raw = sym.st_value - text_addr;
> +			if (raw > UINT_MAX)
> +				continue;
> +
> +			if (num_sym_starts >= sym_starts_capacity) {
> +				sym_starts_capacity = sym_starts_capacity ?
> +					sym_starts_capacity * 2 : 16384;
> +				sym_starts = xrealloc(sym_starts,
> +						      sym_starts_capacity *
> +						      sizeof(*sym_starts));
> +			}
> +			if (num_text_starts >= text_starts_capacity) {
> +				text_starts_capacity = text_starts_capacity ?
> +					text_starts_capacity * 2 : 16384;
> +				text_starts = xrealloc(text_starts,
> +						       text_starts_capacity *
> +						       sizeof(*text_starts));
> +			}
> +			text_starts[num_text_starts].offset = (unsigned int)raw;
> +			text_starts[num_text_starts].size =
> +				sym.st_size > UINT_MAX ? UINT_MAX :
> +				(unsigned int)sym.st_size;
> +			num_text_starts++;
> +
> +			if (GELF_ST_TYPE(sym.st_info) != STT_FUNC)
> +				continue;
> +
> +			sym_starts[num_sym_starts].offset = (unsigned int)raw;
> +			sym_starts[num_sym_starts].size =
> +				sym.st_size > UINT_MAX ? UINT_MAX :
> +				(unsigned int)sym.st_size;
> +			num_sym_starts++;
> +		}
> +	}
> +
> +	sort_starts(sym_starts, &num_sym_starts);
> +	sort_starts(text_starts, &num_text_starts);
> +
> +	/*
> +	 * An unsized label reaches the next symbol, and a sequence beginning
> +	 * there cannot run past it.
> +	 */
> +	for (unsigned int i = 1; i < num_text_starts; i++) {
> +		if (!text_starts[i - 1].size)
> +			text_starts[i - 1].size = text_starts[i - 1].offset ?
> +						  text_starts[i].offset -
> +						  text_starts[i - 1].offset : 0;

[Severity: Medium]
Can unfiltered ELF symbols in collect_symbol_starts() in
scripts/gen_lineinfo.c corrupt the symbol extents in text_starts[]?

In collect_symbol_starts(), every STT_FUNC and STT_NOTYPE symbol in
SHT_SYMTAB whose st_value falls in [text_addr, text_end_addr) is added to
text_starts[] without checking sym.st_shndx or the symbol name:

  - SHN_ABS symbols (such as __kcfi_typeid_* hashes) whose values happen to
    fall in [text_addr, text_end_addr) are included.
  - Architecture mapping symbols ($x, $d on arm64/arm/riscv/loongarch) and
    assembler local labels (.L*) are included.

When the loop at the end of collect_symbol_starts() backfills sizes for
unsized entries:

	for (unsigned int i = 1; i < num_text_starts; i++) {
		if (!text_starts[i - 1].size)
			text_starts[i - 1].size = text_starts[i].offset -
						  text_starts[i - 1].offset;
	}

an internal .L* or $x label inside an unsized assembly routine truncates
that routine's extent at the first internal label, while an unsized mapping
or SHN_ABS symbol at a sequence boundary is given a non-zero size reaching
to the next symbol, which can cause starts_new_sequence() and
resolve_cu_row_groups() to misclassify DWARF sequence boundaries.

Additionally, why does collect_symbol_starts() exclude STT_NOTYPE and
unsized symbols from sym_starts[]?

At runtime, __sprint_symbol() -> kallsyms_lookup_buildid() resolves symbols
using the kallsyms table, which includes STT_NOTYPE code symbols and
unsized assembly labels (such as Clang .cold subfunctions, SYM_CODE_START
routines, SYM_INNER_LABEL entry points like entry_SYSCALL_64_after_hwframe,
and CONFIG_PROPELLER_CLANG basic-block symbols), and passes sym_start to
kallsyms_lookup_lineinfo():

	if (lineinfo_addrs[low] < min_offset)
		return false;

Because collect_symbol_starts() skips GELF_ST_TYPE(sym.st_info) != STT_FUNC
when populating sym_starts[] and never backfills zero sizes in
sym_starts[], synthesize_symbol_starts() skips those symbols (or rejects
st_size == 0 at "if (end <= start) continue;") and deduplicate() sets
at_symbol_start = false at their start offsets. Any frame inside those
symbols before a new file:line transition will then fail the min_offset
check in kallsyms_lookup_lineinfo() and lose its annotation.

[ ... ]
> +static void synthesize_symbol_starts(void)
> +{
> +	unsigned int base_entries = num_entries;
> +	unsigned int cursor = 0;
> +
> +	if (!base_entries || !num_sym_starts)
> +		return;
> +
> +	sort_unique(seq_ends, &num_seq_ends);
> +	qsort(asm_spans, num_asm_spans, sizeof(*asm_spans), compare_asm_spans);
> +
> +	for (unsigned int i = 0; i < num_sym_starts; i++) {
> +		unsigned int start = sym_starts[i].offset;
> +		unsigned int end = start + sym_starts[i].size;
> +
> +		while (cursor + 1 < base_entries &&
> +		       entries[cursor + 1].offset <= start)
> +			cursor++;
> +
> +		if (entries[cursor].offset > start)
> +			continue;	/* nothing covers this symbol */
> +		if (entries[cursor].offset == start)
> +			continue;	/* already has its own entry */
> +		if (seq_end_between(entries[cursor].offset, start))
> +			continue;	/* coverage stopped before here */
> +
> +		if (end <= start)
> +			continue;	/* size overflowed */
> +
> +		/*
> +		 * A row reaching over a symbol with none inside it is padding
> +		 * or a split-out fragment in C, carrying the line of whatever
> +		 * it came from.  In assembly it is one macro expanding to a
> +		 * function per invocation, which the row does describe.
> +		 */
> +		if (!in_asm_span(start) &&
> +		    (cursor + 1 >= base_entries ||
> +		     entries[cursor + 1].offset >= end))
> +			continue;
> +
> +		add_entry(start, entries[cursor].file_id, entries[cursor].line);
> +	}
> +}
> +
> +static void deduplicate(void)
> +{
> +	unsigned int sym_cursor = 0;
> +	unsigned int i, j;
> +
> +	if (num_entries < 2)
> +		return;
> +
> +	/* Sort by offset, then file_id, then line for stability */
> +	qsort(entries, num_entries, sizeof(*entries), compare_entries);
> +
> +	synthesize_symbol_starts();
> +	qsort(entries, num_entries, sizeof(*entries), compare_entries);
> +
> +	/*
> +	 * Remove duplicate entries:
> +	 * - Same offset: keep last.  Earlier rows at that address cover no
> +	 *   bytes, so the final one is what describes the instruction, and
> +	 *   it is what addr2line reports.  For an inlined call that is the
> +	 *   inlined body rather than the call site.
> +	 * - Same file:line as previous kept entry: redundant for binary
> +	 *   search -- any address between them resolves to the earlier one
> +	 *
> +	 * Entries sitting on a symbol start are exempt from the second rule:
> +	 * they are the only thing standing between that symbol and the
> +	 * kernel's boundary check.
> +	 */
> +	j = 0;
> +	for (i = 1; i < num_entries; i++) {
> +		bool at_symbol_start;
> +
> +		if (entries[i].offset == entries[j].offset) {
> +			/*
> +			 * Rows from different compile units can share an
> +			 * address, and coverage is their union: a real
> +			 * location beats another unit's no-coverage marker.
> +			 * Within one unit resolve_cu_row_groups() has already
> +			 * picked the row that describes the address.
> +			 */
> +			if (entries[i].line || !entries[j].line)
> +				entries[j] = entries[i];
> +			continue;
> +		}
> +
> +		while (sym_cursor < num_sym_starts &&
> +		       sym_starts[sym_cursor].offset < entries[i].offset)
> +			sym_cursor++;
> +		at_symbol_start = sym_cursor < num_sym_starts &&
> +				  sym_starts[sym_cursor].offset == entries[i].offset;
> +
> +		if (!at_symbol_start &&
> +		    entries[i].file_id == entries[j].file_id &&
> +		    entries[i].line == entries[j].line)
> +			continue;
> +
> +		j++;
> +		if (j != i)
> +			entries[j] = entries[i];
> +	}
> +	num_entries = j + 1;
> +}

[Severity: Medium]
Can line-0 end_sequence entries overwrite valid source coverage in
synthesize_symbol_starts() and deduplicate() in scripts/gen_lineinfo.c?

There appear to be three ways a line-0 entry can clobber a valid line entry
across compile units:

1. deduplicate() calls synthesize_symbol_starts() before collapsing
same-offset entries across compile units. When an earlier CU emits a valid
row (line > 0) at offset X and a later CU emits an end_sequence row
(line == 0) at the same offset X, the line-0 entry has a higher seq and
sorts last at offset X. In synthesize_symbol_starts(), the cursor loop:

	while (cursor + 1 < base_entries &&
	       entries[cursor + 1].offset <= start)
		cursor++;

advances to the last entry at offset X, picking the line-0 entry instead
of the valid line entry.

2. In process_dwarf(), every sequence end passed to record_seq_end() also
emits a line-0 entry into entries[] at that same offset. Because cursor in
synthesize_symbol_starts() stops at the largest entry offset <= start,
there can never be a sequence end strictly between entries[cursor].offset
and start without a corresponding entry in entries[] that would have
advanced cursor to it. Thus seq_end_between(entries[cursor].offset, start)
never triggers, and because synthesize_symbol_starts() does not check
whether entries[cursor].line == 0, a line-0 sequence-end entry at
entries[cursor].offset gets copied to start.

3. In deduplicate(), same-offset union resolution is interleaved with
adjacent (file_id, line) deduplication. Suppose entries[j] is at offset A
with (file_id=1, line=10), and at offset B > A (not a symbol start) CU1 has
(file_id=1, line=10) followed by CU2 having an end_sequence (file_id=0,
line=0) at the same offset B:

  - For CU1's entry at offset B, entries[i].offset != entries[j].offset,
    and since (file_id, line) matches entries[j], the loop hits continue
    without updating entries[j].offset to B.
  - For CU2's line-0 entry at offset B, entries[i].offset (B) !=
    entries[j].offset (A), so it misses the "if (entries[i].offset ==
    entries[j].offset)" union check and is appended as a new entry at
    offset B with line 0, cutting off valid coverage from offset B onward.

Would collapsing same-offset entries across CUs in a separate pass before
running synthesize_symbol_starts() and before collapsing adjacent
identical (file_id, line) entries (and checking entries[cursor].line != 0
in synthesize_symbol_starts()) avoid losing coverage here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102111455.919102-1-sashal@kernel.org?part=1