Re: [PATCH] bpf: add KF_SPIN_LOCK flag for kfuncs under bpf_spin_lock

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

On Tue Aug 4, 2026 at 12:09 PM CEST, Kaitao Cheng wrote:
> 在 2026/8/4 17:42, Kumar Kartikeya Dwivedi 写道:
>> On Tue Aug 4, 2026 at 11:37 AM CEST, Kaitao Cheng wrote:
>>> 在 2026/8/4 16:43, Kumar Kartikeya Dwivedi 写道:
>>>> On Tue Aug 4, 2026 at 10:32 AM CEST, Kaitao Cheng wrote:
>>>>> From: Kaitao Cheng <chengkaitao@kylinos.cn>
>>>>>
>>>>> Introduce the KF_SPIN_LOCK kfunc metadata flag in BTF so kfuncs may be
>>>>> explicitly marked as safe to call while holding bpf_spin_lock.
>>>>>
>>>>> Allow kfuncs defined in kernel modules to be marked with KF_SPIN_LOCK.
>>>>>
>>>>> Example: BTF_ID_FLAGS(func, $kfunc_name, KF_SPIN_LOCK)
>>>>>
>>>>> Signed-off-by: Kaitao Cheng <chengkaitao@kylinos.cn>
>>>>> ---
>>>>>  include/linux/btf.h   |  1 +
>>>>>  kernel/bpf/verifier.c | 20 +++++++++++++++-----
>>>>>  2 files changed, 16 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>>>>> index c09b7994de4e..380dc10b6750 100644
>>>>> --- a/include/linux/btf.h
>>>>> +++ b/include/linux/btf.h
>>>>> @@ -79,6 +79,7 @@
>>>>>  #define KF_ARENA_ARG1   (1 << 14) /* kfunc takes an arena pointer as its first argument */
>>>>>  #define KF_ARENA_ARG2   (1 << 15) /* kfunc takes an arena pointer as its second argument */
>>>>>  #define KF_IMPLICIT_ARGS (1 << 16) /* kfunc has implicit arguments supplied by the verifier */
>>>>> +#define KF_SPIN_LOCK    (1 << 17) /* kfunc is allowed inside bpf_spin_lock-ed region */
>>>>>
>>>>>  /*
>>>>>   * Tag marking a kernel function as a kfunc. This is meant to minimize the
>>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>>>> index 7aa47342dc65..9f3feba3c2fe 100644
>>>>> --- a/kernel/bpf/verifier.c
>>>>> +++ b/kernel/bpf/verifier.c
>>>>> @@ -11657,11 +11657,21 @@ static bool is_bpf_stream_kfunc(u32 btf_id)
>>>>>  	       btf_id == special_kfunc_list[KF_bpf_stream_print_stack];
>>>>>  }
>>>>>
>>>>> -static bool kfunc_spin_allowed(u32 btf_id)
>>>>> +static bool kfunc_spin_allowed(struct bpf_verifier_env *env, s32 func_id, s16 offset)
>>>>>  {
>>>>> -	return is_bpf_graph_api_kfunc(btf_id) || is_bpf_iter_num_api_kfunc(btf_id) ||
>>>>> -	       is_bpf_res_spin_lock_kfunc(btf_id) || is_bpf_arena_kfunc(btf_id) ||
>>>>> -	       is_bpf_stream_kfunc(btf_id);
>>>>> +	struct bpf_kfunc_meta kfunc;
>>>>> +	int err;
>>>>> +
>>>>> +	if (is_bpf_graph_api_kfunc(func_id) || is_bpf_iter_num_api_kfunc(func_id) ||
>>>>> +	    is_bpf_res_spin_lock_kfunc(func_id) || is_bpf_arena_kfunc(func_id) ||
>>>>> +	    is_bpf_stream_kfunc(func_id))
>>>>> +		return true;
>>>>> +
>>>>> +	err = fetch_kfunc_meta(env, func_id, offset, &kfunc);
>>>>> +	if (err || !kfunc.flags)
>>>>> +		return false;
>>>>> +
>>>>> +	return *kfunc.flags & KF_SPIN_LOCK;
>>>>>  }
>>>>>
>>>>>  static bool is_sync_callback_calling_kfunc(u32 btf_id)
>>>>> @@ -17289,7 +17299,7 @@ static int do_check_insn(struct bpf_verifier_env *env, bool *do_print_state)
>>>>>  				     insn->imm != BPF_FUNC_spin_unlock &&
>>>>>  				     insn->imm != BPF_FUNC_kptr_xchg) ||
>>>>>  				    (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
>>>>> -				     (insn->off != 0 || !kfunc_spin_allowed(insn->imm)))) {
>>>>> +				     !kfunc_spin_allowed(env, insn->imm, insn->off))) {
>>>>>  					verbose(env,
>>>>>  						"function calls are not allowed while holding a lock\n");
>>>>>  					return -EINVAL;
>>>>
>>>> Why not apply it to all of the existing kfuncs? Also, use [PATCH bpf-next] subject.
>>>
>>> I do not think all kfuncs are suitable for the KF_SPIN_LOCK context.
>>> Kfuncs marked with KF_SLEEPABLE are an obvious example. Developers
>>> should add KF_SPIN_LOCK with great care and ensure that the kfunc
>>> does not sleep and is safe to call in atomic context; otherwise, it
>>> could easily introduce deadlocks.
>>>
>>> However, the kfuncs explicitly allowed by kfunc_spin_allowed(), such
>>> as those covered by is_bpf_graph_api_kfunc, is_bpf_iter_num_api_kfunc,
>>> is_bpf_res_spin_lock_kfunc, is_bpf_arena_kfunc and is_bpf_stream_kfunc,
>>> can be marked with KF_SPIN_LOCK. We can then simplify kfunc_spin_allowed()
>>> to rely solely on the flag.
>>
>> Yes, of course. I meant those that we maintain as a list in the verifier.
>> KF_SPIN_LOCK is not a very optimal name. Should we do KF_ALLOWED_IN_LOCK_CS?
>> A bit verbose but better captures the intended meaning.
>
> The existing KF_* flag names are generally concise, so a much longer name
> feels somewhat out of place. I would also prefer the name to preserve the
> full spin_lock semantics, since "lock" can refer to many different lock
> types, such as mutex.
>
> Alternatively, following the naming of GFP_ATOMIC, could we call it KF_ATOMIC?

I think that is also confusing, every kfunc is allowed in atomic context in
general. E.g. bpf_map_update_elem() in spin lock critical section would be
problematic (due to embedded bpf_spin_lock in map value surfacing a deadlock
opportunity). KF_OK_IN_SPIN_LOCK_CS is another option, but slightly longer than
the previous proposal.

In any case, let's choose this or the former one, and not fret too much about
the naming. We can also choose a better name later on.