Re: [PATCH bpf-next 1/2] bpf: Initialize ftrace_managed in bpf_trampoline_get()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: dummy trampoline causes ftrace_managed text corruption
--

commit df0c3beb4928ce14ffbe4b44e2bc365c2a470352
Author: Leon Hwang <leon.hwang@linux.dev>

bpf: Initialize ftrace_managed in bpf_trampoline_get()

This patch removes the dynamic ftrace_location() check from
register_fentry() and instead initializes ftrace_managed in
bpf_trampoline_get() based on whether tr->ip is non-zero.

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 24edb59238f34..17f2a8133f611 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -475,18 +475,11 @@ static int register_fentry(struct bpf_trampoline *tr, struct bpf_tramp_image *im
>  			   void *data __maybe_unused)
>  {
>  	void *new_addr = im->image;
> -	void *ip = tr->func.addr;
> -	unsigned long faddr;
>  	int ret;
>  
> -	faddr = ftrace_location((unsigned long)ip);
> -	if (faddr) {
> +	if (tr->func.ftrace_managed) {
>  		if (!tr->fops)
>  			return -ENOTSUPP;
> -		tr->func.ftrace_managed = true;
> -	}
> -
> -	if (tr->func.ftrace_managed) {
>  		ret = direct_ops_add(tr, new_addr);
>  	} else {
>  		ret = bpf_trampoline_update_fentry(tr, 0, NULL, new_addr);

[Severity: High]
Because ftrace_managed is no longer evaluated dynamically here, is there a
risk of corrupting ftrace-managed text if tr->func.ftrace_managed was
incorrectly cached as false? 

If it evaluates to false, register_fentry() falls back to 
bpf_trampoline_update_fentry() which does raw text poking.

[ ... ]

> @@ -1286,6 +1279,7 @@ struct bpf_trampoline *bpf_trampoline_get(u64 key,
>  
>  	memcpy(&tr->func.model, &tgt_info->fmodel, sizeof(tgt_info->fmodel));
>  	tr->func.addr = (void *)tgt_info->tgt_addr;
> +	tr->func.ftrace_managed = tr->ip != 0;
>  out:
>  	trampoline_unlock(tr);
>  	return tr;

[Severity: High]
Can this assignment incorrectly mark a ftrace-managed target as non-managed
if a concurrent operation creates a dummy trampoline?

If bpf_trampoline_unlink_cgroup_shim() is called for a program that was
never successfully linked to a trampoline, it calls
bpf_trampoline_lookup(key, 0). Since the trampoline doesn't exist, a dummy
trampoline with tr->ip = 0 is temporarily created.

If a concurrent bpf_trampoline_get() targets the same hook during this
window, it finds the dummy trampoline. With this change, it will cache
tr->func.ftrace_managed as false because tr->ip is 0, regardless of the
actual target address.

When register_fentry() then runs, it bypasses direct ops and uses raw
bpf_arch_text_poke() on the address. Could this cause kernel text
corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016142014.12420-1-leon.hwang@linux.dev?part=1