Re: [PATCH bpf-next v1 12/29] bpf: Release resources in hidden slots on bpf_throw()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] bpf: broken IRQ state restoration for spinlocks in
  bpf_unwind_release_frame()
--

commit bc130ef20556bd747c8015574438bb24ed4f95b8
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Release resources in hidden slots on bpf_throw()

This commit adds runtime support in bpf_throw() to walk discarded BPF
stack frames and release held locks, IRQ flags, sections, and referenced
objects recorded in frame descriptors.

[ ... ]

> diff --git a/kernel/bpf/unwind.c b/kernel/bpf/unwind.c
> new file mode 100644
> index 0000000000000..3b488a4bd7592
> --- /dev/null
> +++ b/kernel/bpf/unwind.c

[ ... ]

> +static void bpf_unwind_release_frame(struct bpf_unwind_ctx *ctx, const struct bpf_prog *prog, u64 bp)
> +{
> +	const struct bpf_prog_aux *fn = bpf_prog_frame_aux(prog);
> +	const struct bpf_frame_desc_pool *pool = fn->frame_desc_pool;
> +	int passes = ctx->phase == BPF_UNWIND_PHASE_LOCKS ? 2 : 1, pass, s;
> +	const struct bpf_frame_desc *d;
> +	bool late;
> +	u64 fp;
> +	u32 i;
> +
> +	if (!fn->frame_desc)
> +		return;
> +	d = pool->desc[fn->frame_desc - 1];
> +	fp = bpf_unwind_fp(fn, bp);
> +	for (pass = 0; pass < passes; pass++) {
> +		for (i = 0; i < d->nent; i++) {
> +			const struct bpf_frame_desc_entry *ent = &d->ent[i];
> +			const struct bpf_resource *res = &pool->resources[ent->res];
> +			u64 *slot = (u64 *)(long)(fp + ent->off);
> +			void *handle = (void *)(long)*slot;
> +
> +			if (ent->flags & BPF_FRAME_ENT_COUNTER) {
> +				if (ctx->phase != BPF_UNWIND_PHASE_LOCKS || pass)
> +					continue;
> +				s = section_index(res->kind);
> +				ctx->section[s] = res;
> +				ctx->nr_sections[s] += (s64)*slot;
> +				continue;
> +			}
> +			if (!handle || release_phase(res) != ctx->phase)
> +				continue;
> +			late = res->kind == BPF_RESOURCE_IRQ_FLAG &&
> +			       !arch_irqs_disabled_flags(*(unsigned long *)handle);
> +			if (passes == 2 && late != (pass == 1))
> +				continue;
> +			bpf_unwind_release(res, handle);
> +		}
> +	}
> +}

[Severity: High]
Does releasing BPF_RESOURCE_SPIN_LOCK in pass 0 corrupt the CPU interrupt
state when a frame holds both a bpf_spin_lock and a nested
bpf_res_spin_lock_irqsave()?

bpf_spin_lock() saves interrupt flags in per-CPU irqsave_flags and
disables local interrupts, and bpf_spin_unlock() restores irqsave_flags:

kernel/bpf/helpers.c:__bpf_spin_unlock_irqrestore() {
	unsigned long flags;

	flags = __this_cpu_read(irqsave_flags);
	__bpf_spin_unlock(lock);
	local_irq_restore(flags);
}

bpf_res_spin_lock() and bpf_res_spin_lock_irqsave() are marked
KF_SPINLOCK_SAFE and can be acquired while a bpf_spin_lock is held.  When
a frame acquires bpf_spin_lock() with interrupts enabled and then calls
bpf_res_spin_lock_irqsave(), irqsave_flags holds the enabled state while
the stack slot for BPF_RESOURCE_IRQ_FLAG records interrupts as disabled.

Because only BPF_RESOURCE_IRQ_FLAG is checked for late, pass 0 runs
bpf_spin_unlock() first, re-enabling interrupts while the resilient
spinlock is still held, and then runs bpf_local_irq_restore() on the
BPF_RESOURCE_IRQ_FLAG slot, leaving local interrupts permanently disabled
through sleeping destructors in BPF_UNWIND_PHASE_OBJS and after the BPF
program returns.  In addition, releasing BPF_RESOURCE_RES_LOCK in pass 0
calls preempt_enable() in bpf_res_spin_unlock() before pass 1 restores
the interrupt state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794399387.git.memxor@gmail.com?part=12