Re: [PATCH bpf-next v2 1/4] bpf: Reject bpf_obj_drop() from tracing progs

["Alexei Starovoitov" <alexei.starovoitov@gmail.com>]

On Tue Jun 9, 2026 at 7:08 AM PDT, Kumar Kartikeya Dwivedi wrote:
> On Tue Jun 9, 2026 at 3:31 PM CEST, Mykyta Yatsenko wrote:
>>
>>
>> On 6/9/26 10:37 AM, Kumar Kartikeya Dwivedi wrote:
>>> From: Justin Suess <utilityemal77@gmail.com>
>>>
>>> bpf_obj_drop() runs bpf_obj_free_fields() synchronously for
>>> program-allocated objects. When such an object contains NMI unsafe
>>> fields, a non-iterator tracing program can reach that destruction from
>>> arbitrary instrumented context, including NMI.
>>>
>>> NMI is likely one instance of this problem, and other instances would
>>> include possible unsafe reentrancy. Deferring bpf_obj_drop() is not
>>> appealing either: it would add delayed-free machinery to a release
>>> operation that otherwise has straightforward synchronous ownership
>>> semantics.
>>>
>>> Reject bpf_obj_drop() and bpf_percpu_obj_drop() from tracing programs
>>> unless every field in the object's BTF record is explicitly NMI safe.
>>> Note that while bpf_rb_root and bpf_list_head would be NMI safe on their
>>> own to free, the objects recursively held by them may not be; be
>>> conservative and just mark them as not NMI safe for now.
>>>
>>> Use a whitelist for the NMI-safe field set instead of listing only known
>>> NMI unsafe fields. Locks, async fields, unreferenced kptrs, and
>>> refcounts are known to be NMI safe because their destruction is either a
>>> no-op, simple state reset, or async cancellation. Referenced kptrs,
>>> percpu referenced kptrs, uptrs, graph roots, graph nodes, and any future
>>> field type are rejected until audited for arbitrary tracing and NMI
>>> contexts. This is less susceptible to future changes in fields that were
>>> previously safe by exclusion, and to new fields being added without
>>> updating this check.
>>>
>>> Convert the existing recursive local-object drop success case to a
>>> syscall program in the same commit, since this verifier change makes the
>>> old tracing program form invalid. The test still exercises
>>> bpf_obj_drop() releasing a referenced task kptr from a safe program
>>> type.
>>>
>>> Fixes: ac9f06050a35 ("bpf: Introduce bpf_obj_drop")
>>> Signed-off-by: Justin Suess <utilityemal77@gmail.com>
>>> Co-developed-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>>> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>>> ---
>>>  include/linux/bpf.h                           | 29 +++++++++++++
>>>  kernel/bpf/verifier.c                         | 16 +++++++
>>>  .../selftests/bpf/prog_tests/task_kfunc.c     | 42 ++++++++++++++++++-
>>>  .../selftests/bpf/progs/task_kfunc_success.c  | 13 +++---
>>>  4 files changed, 92 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>> index 62bba7a4876f..0654d2ffadc1 100644
>>> --- a/include/linux/bpf.h
>>> +++ b/include/linux/bpf.h
>>> @@ -492,6 +492,35 @@ static inline bool btf_record_has_field(const struct btf_record *rec, enum btf_f
>>>  	return rec->field_mask & type;
>>>  }
>>>
>>> +static inline bool btf_field_is_nmi_safe(enum btf_field_type type)
>>> +{
>>> +	switch (type) {
>>> +	case BPF_SPIN_LOCK:
>>> +	case BPF_RES_SPIN_LOCK:
>>> +	case BPF_TIMER:
>>> +	case BPF_WORKQUEUE:
>>> +	case BPF_TASK_WORK:
>>> +	case BPF_KPTR_UNREF:
>>> +	case BPF_REFCOUNT:
>>> +		return true;
>>> +	default:
>>> +		return false;
>>> +	}
>>> +}
>>> +
>>> +static inline bool btf_record_has_nmi_unsafe_fields(const struct btf_record *rec)
>>> +{
>>> +	int i;
>>> +
>>> +	if (IS_ERR_OR_NULL(rec))
>>> +		return false;
>>> +	for (i = 0; i < rec->cnt; i++) {
>>> +		if (!btf_field_is_nmi_safe(rec->fields[i].type))
>>> +			return true;
>>> +	}
>>> +	return false;
>>> +}
>>> +
>>
>> Do we need this stuff in the header file? Why not put it into the verifier.c?
>
> No strong reason, I can move it to verifier.c too.
>
>>
>>>  static inline void bpf_obj_init(const struct btf_record *rec, void *obj)
>>>  {
>>>  	int i;
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index ed7ba0e6a9ce..3b43e2bd0f2a 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -205,6 +205,7 @@ static int release_reference_nomark(struct bpf_verifier_state *state, int id);
>>>  static int release_reference(struct bpf_verifier_env *env, int id);
>>>  static void invalidate_non_owning_refs(struct bpf_verifier_env *env);
>>>  static bool in_rbtree_lock_required_cb(struct bpf_verifier_env *env);
>>> +static bool is_tracing_prog_type(enum bpf_prog_type type);
>>>  static int ref_set_non_owning(struct bpf_verifier_env *env,
>>>  			      struct bpf_reg_state *reg);
>>>  static bool is_trusted_reg(struct bpf_verifier_env *env, const struct bpf_reg_state *reg);
>>> @@ -12881,6 +12882,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>>  			    int *insn_idx_p)
>>>  {
>>>  	bool sleepable, rcu_lock, rcu_unlock, preempt_disable, preempt_enable;
>>> +	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
>>>  	struct bpf_reg_state *regs = cur_regs(env);
>>>  	const char *func_name, *ptr_type_name;
>>>  	const struct btf_type *t, *ptr_type;
>>> @@ -12957,6 +12959,20 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>>  	if (err < 0)
>>>  		return err;
>>>
>>> +	if ((is_bpf_obj_drop_kfunc(meta.func_id) ||
>>> +	     is_bpf_percpu_obj_drop_kfunc(meta.func_id)) && (is_tracing_prog_type(prog_type) ||
>>
>> We have sleepable tracepoints, are we going to reject them as well? I wonder if there is a better way
>> to detect NMI context in verifier.
>>
>
> NMI is probably one concern, reentrancy is another, though admiteddly sleepable
> tracepoints are probably fine for this case (I didn't think too hard).

sleepable tracepoints are fine.
Could you adjust the check to allow it?

> That said, on the other bit about having better ways of detecting such things, I
> was wondering whether we should bite the bullet and decouple context information
> from prog type. We sort of use program types as a proxy for it, but we may have
> more clarity around expectations and safety checks if the context a program may
> run in is extracted from the type (+ more contextual information like attach BTF
> ID if available, otherwise falling back to the conservative set), and operations
> have their checks be done against the context. For helpers, kfuncs, except for
> specific special cases that require specific program hooks to function
> correctly, registration could move from program types to contexts it can be
> called in safely in general.
>
> This is something I've been wondering for a while, and it's a substantial
> change, but it appears to better fit the way we reason about safety now and use
> various program types in checks and kfunc registration in practice anyway.

Long term it's the right approach, but lets fix what we can for now.