Re: [PATCH bpf v1 1/2] bpf: Fix tracing of kfuncs with implicit args

["Kumar Kartikeya Dwivedi" <memxor@gmail.com>]

On Fri Jul 10, 2026 at 7:34 PM CEST, Ihor Solodrai wrote:
> On 7/10/26 10:23 AM, Kumar Kartikeya Dwivedi wrote:
>> On Fri Jul 10, 2026 at 7:08 PM CEST, Ihor Solodrai wrote:
>>> On 7/9/26 6:21 PM, sashiko-bot@kernel.org wrote:
>>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>>> - [High] Accessing `btf->kfunc_set_tab` without holding a module reference causes a use-after-free vulnerability.
>>>> --
>>>>
>>>> --- Patch [1]: [PATCH bpf v1 1/2] bpf: Fix tracing of kfuncs with implicit args ---
>>>> commit 13df91bf669b3a55c459c1663fbf726f9f52bc5c
>>>> Author: Ihor Solodrai <ihor.solodrai@linux.dev>
>>>>
>>>> bpf: Fix tracing of kfuncs with implicit args
>>>>
>>>> A kfunc marked with KF_IMPLICIT_ARGS flag takes implicit arguments
>>>> (such as bpf_prog_aux) that the verifier injects at load time.
>>>> resolve_btfids strips those from the kfunc's BTF-visible prototype and
>>>> keeps the real kernel ABI in a counterpart _impl prototype.
>>>>
>>>> Link: https://github.com/sched-ext/scx/issues/3687#issuecomment-4906694106
>>>>
>>>>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>>>>> index 64572f85edc83..20aeca6b4f95c 100644
>>>>> --- a/kernel/bpf/btf.c
>>>>> +++ b/kernel/bpf/btf.c
>>>> [ ... ]
>>>>> @@ -9114,6 +9114,26 @@ u32 *btf_kfunc_flags(const struct btf *btf, u32 kfunc_btf_id, const struct bpf_p
>>>>>  	return btf_kfunc_id_set_contains(btf, hook, kfunc_btf_id);
>>>>>  }
>>>>>
>>>>> +/*
>>>>> + * Return the union of a kfunc's flags across all hooks.
>>>>> + * Unlike btf_kfunc_flags(), not restricted to a calling
>>>>> + * program's hook. Used when attaching to a kfunc for tracing.
>>>>> + */
>>>>> +u32 btf_kfunc_accumulated_flags(const struct btf *btf, u32 kfunc_btf_id)
>>>>> +{
>>>>> +	enum btf_kfunc_hook hook;
>>>>> +	u32 *hook_flags;
>>>>> +	u32 flags = 0;
>>>>> +
>>>>> +	for (hook = 0; hook < BTF_KFUNC_HOOK_MAX; hook++) {
>>>>> +		hook_flags = btf_kfunc_id_set_contains(btf, hook, kfunc_btf_id);
>>>>> +		if (hook_flags)
>>>>> +			flags |= *hook_flags;
>>>>> +	}
>>>>> +
>>>>> +	return flags;
>>>>> +}
>>>>> +
>>>>>  u32 *btf_kfunc_is_modify_return(const struct btf *btf, u32 kfunc_btf_id,
>>>>>  				const struct bpf_prog *prog)
>>>>>  {
>>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>>>> index 6515d4d3c0032..fde11a2f6869a 100644
>>>>> --- a/kernel/bpf/verifier.c
>>>>> +++ b/kernel/bpf/verifier.c
>>>> [ ... ]
>>>>> @@ -19121,8 +19146,8 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>>>>>  		if (prog_extension &&
>>>>>  		    btf_check_type_match(log, prog, btf, t))
>>>>>  			return -EINVAL;
>>>>> -		t = btf_type_by_id(btf, t->type);
>>>>> -		if (!btf_type_is_func_proto(t))
>>>>> +		t = btf_attach_func_proto(log, btf, btf_id);
>>>>> +		if (!t || !btf_type_is_func_proto(t))
>>>>>  			return -EINVAL;
>>>>
>>
>> Also, why not do it for BPF_TRACE_ITER case as well? I just feel keeping it
>> consistent may outweigh figuring out whether it's necessary in that case or not,
>> etc.
>
>
> I mentioned this in the patch description:
>
>   The BPF_TRACE_ITER case distills a registered iterator's
>   prototype, and bpf_struct_ops_desc_init() distills the
>   function-pointer members of a struct_ops type. Neither is
>   a kfunc, and so can't have implicit arguments.
>

Yeah, I saw that, but abstracting that inside the function and not having to
think from the caller about those details would be better. That said, I don't
have any strong preference, it is obviously correct as it is.

>
>>
>> Please supply a cover letter in the next version too since you're sending
>> multiple patches.
>
> I used the first patch's message as a cover letter, since it's
> just a fix + selftest pair.
>
> The series could be split further into refactoring changes, but I thought
> it'll be easier to backport if the fix is in a single self-contained patch.
>
> I think we want the fix to get into 7.2, and backport it to 7.1.
>
> If it's ok to carry separate refactoring patches there, then I'll split.
> Kartikeya, wdyt?
>

You can still keep the patches as is, but just add a cover letter, it will not
affect backporting. The cover letter is just to summarize the series and the fix
in the merge commit. Also, my understanding is that with the Fixes: tag, it
should automatically be picked up for stable kernels.