Re: [PATCH v10 2/4] kallsyms: extend lineinfo to loadable modules

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] kbuild: running module.lds twice in cmd_ld_ko_o breaks alloc_tags and arm64
- [Medium] scripts/gen_lineinfo: early return in deduplicate() leaves stale n_entries
- [Medium] scripts/gen_lineinfo: zero bias in resolve_covered_sections() aliases .text
- [Medium] scripts/gen_lineinfo: resolve_covered_sections() leaves zero bias gap
--

commit 576d6bd5b277b6e1ee712d143954da5c356b15a1
Author: Sasha Levin <sashal@kernel.org>

kallsyms: extend lineinfo to loadable modules

This commit adds CONFIG_KALLSYMS_LINEINFO_MODULES to embed source file and
line number lookup tables (.mod_lineinfo and .init.mod_lineinfo) into
loadable kernel modules via a two-stage partial link in Makefile.modfinal
and a new --module mode in scripts/gen_lineinfo.

> diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal
> index 01a37ec872b90..2f19dc184ffe7 100644
> --- a/scripts/Makefile.modfinal
> +++ b/scripts/Makefile.modfinal
> @@ -46,8 +46,47 @@ quiet_cmd_btf_ko = BTF [M] $@
>  		$(CONFIG_SHELL) $(srctree)/scripts/gen-btf.sh --btf_base $(objtree)/vmlinux $@; \
>  	fi;
>  
[ ... ]
> +ifdef CONFIG_KALLSYMS_LINEINFO_MODULES
> +lineinfo_ldflags_all = $(KBUILD_LDFLAGS_MODULE) $(LDFLAGS_MODULE)
> +lineinfo_ldflags_last = $(filter --be8 --build-id --build-id=%,		\
> +			       $(lineinfo_ldflags_all))
> +
> +      cmd_ld_ko_o =							\
> +	rm -f $@.lineinfo_pre $@.lineinfo.o;				\
> +	$(LD) -r $(KBUILD_LDFLAGS)					\
> +		$(filter-out $(lineinfo_ldflags_last),			\
> +			     $(lineinfo_ldflags_all))			\
> +		-T $(objtree)/scripts/module.lds			\
> +		-o $@.lineinfo_pre $(filter %.o, $^);			\
> +	$(CONFIG_SHELL) $(srctree)/scripts/gen-mod-lineinfo.sh		\
> +		$@.lineinfo_pre $@.lineinfo.o;				\
> +	$(LD) -r $(KBUILD_LDFLAGS) $(lineinfo_ldflags_last)		\
> +		-T $(objtree)/scripts/module.lds -o $@			\
> +		$$(test -f $@.lineinfo.o && echo $@.lineinfo.o)		\
> +		$@.lineinfo_pre;					\

[Severity: Critical]
Does passing -T $(objtree)/scripts/module.lds to both ld -r passes in
cmd_ld_ko_o break CONFIG_MEM_ALLOC_PROFILING and arm64 module loading?

Under CONFIG_MEM_ALLOC_PROFILING, scripts/module.lds.S expands
MOD_SEPARATE_CODETAG_SECTIONS() from include/asm-generic/codetag.lds.h into:

	.codetag.alloc_tags 0 : {
		. = ALIGN(8);
		__start_alloc_tags = .;
		KEEP(*(alloc_tags))
		__stop_alloc_tags = .;
	}

In the first ld -r pass, input alloc_tags sections from the module's .o files
are renamed into the output section .codetag.alloc_tags in $@.lineinfo_pre.
When the second ld -r pass runs with -T scripts/module.lds on
$@.lineinfo_pre, there are no longer any input sections named alloc_tags, so
KEEP(*(alloc_tags)) matches nothing and resets both __start_alloc_tags and
__stop_alloc_tags to offset 0 before orphan placement appends
.codetag.alloc_tags.

At module load time, move_module() allocates .codetag.alloc_tags with each
struct alloc_tag.counters initialized to NULL, while codetag_module_init() in
lib/codetag.c sees:

	/* Ignore empty ranges */
	if (range.start == range.stop)
		return 0;

and skips calling mm/alloc_tag.c:load_module() to allocate per-CPU counters.
The first instrumented allocation in the module then dereferences
tag->counters == NULL in alloc_tag_ref_set().

In addition, on arm64, arch/arm64/include/asm/module.lds.h defines:

	.text.ftrace_trampoline 0 : {
		BYTE(0)
	}

which creates a 1-byte writable (SHF_ALLOC | SHF_WRITE) section in
$@.lineinfo_pre during the first link. In the second link with
-T scripts/module.lds, the earlier rule in scripts/module.lds.S:

	.text 0 : {
		*(.text .text.[0-9a-zA-Z_]*)
	}

matches .text.ftrace_trampoline from $@.lineinfo_pre and merges it into
.text. That gives .text SHF_WRITE | SHF_EXECINSTR flags (and grows .text by 1
byte), causing module_enforce_rwx_sections() in kernel/module/strict_rwx.c to
reject every module load with -ENOEXEC when CONFIG_STRICT_MODULE_RWX is
enabled.

> diff --git a/scripts/gen_lineinfo.c b/scripts/gen_lineinfo.c
> index 7562ac81acca5..d9cc213e65735 100644
> --- a/scripts/gen_lineinfo.c
> +++ b/scripts/gen_lineinfo.c
[ ... ]
> @@ -635,12 +732,728 @@ static void collect_symbol_starts(Elf *elf, unsigned long long text_addr)
[ ... ]
> +static void resolve_covered_sections(Elf *elf,
> +				     struct covered_section *sections,
> +				     unsigned int num_sections)
> +{
> +	Elf_Scn *scn = NULL;
> +	GElf_Shdr shdr;
> +	size_t shstrndx;
> +	unsigned long long cursor = 0;

[Severity: Medium]
When cursor starts at 0 in resolve_covered_sections(), the first present
covered section in all_sections[] (normally .text) gets sections[0].bias = 0,
giving it the synthetic range [0, size).

In an ET_REL module object produced by ld -r -T scripts/module.lds, functions
in sections discarded by /DISCARD/ (such as .discard.*) or discarded COMDAT
groups have their .debug_line DW_LNE_set_address relocations zeroed or
pointed at STN_UNDEF (st_shndx == SHN_UNDEF, st_value == 0). In
apply_one_debug_reloc(), section_by_index() returns NULL for SHN_UNDEF, so no
bias is added and those discarded sequences remain at [0, seq_len).

Won't classify_address() in process_dwarf() then match those [0, seq_len)
addresses against sections[0] and merge discarded line entries into .text?

[ ... ]
> +	/* Pack present sections into non-overlapping bias ranges. */
> +	for (unsigned int i = 0; i < num_sections; i++) {
> +		if (!sections[i].present)
> +			continue;
> +		sections[i].bias = cursor;
> +		cursor += sections[i].size;
> +		cursor = (cursor + 15) & ~15ULL;	/* pad for separation */
> +	}
> +	for (unsigned int i = 0; i < num_extra_sections; i++) {
> +		extra_sections[i].bias = cursor;
> +		cursor += extra_sections[i].size;
> +		cursor = (cursor + 15) & ~15ULL;
> +	}
> +}

[Severity: Medium]
Does (cursor + 15) & ~15ULL in resolve_covered_sections() actually provide a
separation gap when a section's size is 0 or a multiple of 16 bytes?

Because text sections on architectures such as x86_64, arm64, riscv, s390,
and loongarch are commonly 16-byte aligned in size (or 0 bytes when empty,
since covered sections do not require shdr.sh_size != 0 to set present =
true), cursor + sections[i].size is already a multiple of 16. In that case,
(cursor + 15) & ~15ULL adds 0 bytes of padding and leaves
sections[next].bias == sections[i].bias + sections[i].size.

If an empty DWARF sequence starts at offset 0 of sections[next] (with an
end_sequence row at addr == sections[next].bias), process_dwarf() and
classify_seq_end() query classify_address() at addr - 1, which lands at
sections[i].bias + sections[i].size - 1 and misclassifies the row into
sections[i]. Likewise, any non-end_sequence address at offset ==
sections[i].size lands on sections[next].bias and is classified as offset 0
of sections[next].

[ ... ]
> @@ -1049,16 +1919,19 @@ static void synthesize_symbol_starts(void)
[ ... ]
> -static void deduplicate(void)
> +static void deduplicate(struct covered_section *sections,
> +			unsigned int num_sections)
>  {
>  	unsigned int sym_cursor = 0;
>  	unsigned int i, j;
>  
>  	if (num_entries < 2)
>  		return;

[Severity: Medium]
What happens in module mode if resolve_cu_row_groups() collapses a compile
unit's rows down to num_entries == 1 before deduplicate() is called?

In process_dwarf(), sections[sec_id].n_entries is incremented for every raw
DWARF line row before add_entry():

	loffset = (unsigned int)sec_off;
	sections[sec_id].n_entries++;

At the end of the compile unit, resolve_cu_row_groups() collapses rows sharing
the same (section_id, offset) and shrinks num_entries = j without adjusting
sections[sec_id].n_entries.

If num_entries is 1 when deduplicate(all_sections, ALL_SECTIONS) is called,
this early return skips both synthesize_symbol_starts() and the loop at the
end of deduplicate() that resets and recomputes sections[k].n_entries from
entries[0 .. num_entries - 1]:

	if (sections) {
		for (unsigned int k = 0; k < num_sections; k++)
			sections[k].n_entries = 0;
		for (i = 0; i < num_entries; i++)
			sections[entries[i].section_id].n_entries++;
	}

When emit_blob() -> emit_section_table() then iterates count =
all_sections[i].n_entries (>= 2) times, won't it read past num_entries into
stale entries[1 .. count - 1] that were discarded by resolve_cu_row_groups()?

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