Re: [PATCH RFC 1/8] ftrace: Let ftrace_call_adjust() reject a patch site

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Permanent memory leak of ftrace page group when all tracepoints in a module are rejected.

Pre-existing issues:
- [High] Lockless readers can observe uninitialized ftrace pages due to a missing write memory barrier.
--

commit 47d688aa11de52022b8f09bf86665d475e816eb6
Author: Jose Fernandez (Anthropic) <jose.fernandez@linux.dev>
Subject: ftrace: Let ftrace_call_adjust() reject a patch site

This commit addresses an issue where ftrace_process_locs() does not skip a
patch site when ftrace_call_adjust() returns 0. It tests the adjusted address
and skips the site if it is NULL, preventing a bug_on() during ftrace_init_nop
on arm64.

> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index f9d80c7bd9f16..394f82230e0c3 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -7676,6 +7676,10 @@ static int ftrace_process_locs(struct module *mod,
>  		}
>  
>  		addr = ftrace_call_adjust(addr);
> +		if (!addr) {
> +			skipped++;
> +			continue;
> +		}

[Severity: Medium]
If all tracepoints in a module are rejected and skipped here, the allocated
start_pg retains an index of 0. When the module is later unloaded, does this
leave the empty page permanently in the global ftrace_pages list?

During module unload, ftrace_release_mod() checks if the first record's ip
is within the module:

kernel/trace/ftrace.c:ftrace_release_mod() {
    ...
	for (pg = ftrace_pages_start; pg; pg = pg->next) {
		rec = &pg->records[0];
		if (within_module(rec->ip, mod)) {
			/*
			 * pg->index could be zero if we only allocated
			 * a page but did not store any records
			 */
            ...
}

Since the record ip is 0, within_module(0, mod) fails, and the page group
would not be freed. Can this regression cause a memory leak of the ftrace
page group for each incompatible module load?

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this function,
I noticed that the new ftrace page is published to the lockless list without
a write memory barrier earlier in ftrace_process_locs():

kernel/trace/ftrace.c:ftrace_process_locs() {
    ...
		if (WARN_ON(ftrace_pages->next)) {
			/* Hmm, we have free pages? */
			while (ftrace_pages->next)
				ftrace_pages = ftrace_pages->next;
		}

		ftrace_pages->next = start_pg;
    ...
}

Because the cpu can reorder the kzalloc initialization of start_pg after its
publication to the ftrace_pages list, lockless readers like
ftrace_location_range() iterating the list under rcu_read_lock() might read
uninitialized data:

kernel/trace/ftrace.c:lookup_rec() {
    ...
	for (pg = ftrace_pages_start; pg; pg = pg->next) {
		if (pg->index == 0 ||
		    rec->ip < pg->records[0].ip ||
		    rec->ip > pg->records[pg->index - 1].ip)
			continue;
    ...
}

Could this lead to a panic or memory corruption due to an out-of-bounds
access or a garbage pointer dereference in bsearch() if the reader observes an
uninitialized pg->index?

>  
>  		end_offset = (pg->index+1) * sizeof(pg->records[0]);
>  		if (end_offset > PAGE_SIZE << pg->order) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-b4-arm64-callops-kcfi-v1-0-ce6687739b0c@linux.dev?part=1