Re: [PATCH bpf] bpf: Fail rhtab inserts and deletes with IRQs disabled

[bot+bpf-ci]

> bpf: Fail rhtab inserts and deletes with IRQs disabled

TL;DR: The irqs_disabled() check in rhtab delete and insert may return -EBUSY
for every IRQs-off caller, such as sched_switch or perf programs, not just
re-entry, breaking previously working users; the commit message's claim about
other hash maps is also questioned.

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c

[ ... ]

> @@ -3144,11 +3144,14 @@ static int rhtab_delete_elem(struct bpf_rhtab *rhtab, struct rhtab_elem *elem, v
>  	int err;
>  
>  	/*
> -	 * disable_instrumentation() mitigates the deadlock for programs running in NMI context.
> -	 * rhashtable locks bucket with local_irq_save(). Only NMI programs may reenter
> -	 * rhashtable code, bpf_disable_instrumentation() disables programs running in NMI, except
> -	 * raw tracepoints, which we don't have in rhashtable.
> +	 * rhashtable takes its bucket bit locks with IRQs disabled and can't
> +	 * detect re-entry. With IRQs already disabled this CPU may already
> +	 * hold one, e.g., in NMI or in a program attached inside the resize
> +	 * worker's locked section.
>  	 */
> +	if (irqs_disabled())
> +		return -EBUSY;
> +
>  	bpf_disable_instrumentation();

Does this irqs_disabled() check detect re-entry, or does it reject every
delete made from any context that runs with IRQs off?

Most of those contexts do not hold a bucket lock.  For example, a tp_btf
program on sched_switch runs after __schedule() has called
local_irq_disable() and taken the rq lock:

  __schedule()
    trace_sched_switch()
      bpf program
        bpf_map_delete_elem()
          rhtab_map_delete_elem()
            rhtab_delete_elem()
              irqs_disabled() -> -EBUSY

The same applies to sched_wakeup and irq_handler_entry tracepoints,
perf_event programs (which run from the PMU NMI or hrtimer hardirq),
fentry/kprobe on functions that run with IRQs off, sched_ext callbacks
under the rq lock, and code between bpf_local_irq_save() and
bpf_local_irq_restore().

The verifier has no program type restriction for BPF_MAP_TYPE_RHASH, and
these calls succeeded before this patch.  Would a guard that fires only on
actual re-entry, such as a per-CPU or per-map marker set around the
rhashtable locked sections, or a bounded trylock on the bucket lock, avoid
the deadlock without breaking these users?

The commit message says -EBUSY is returned "like the other hash maps do on
re-entry".  Is that accurate?  htab_lock_bucket() uses
raw_res_spin_lock_irqsave() and fails with -EDEADLK or -ETIMEDOUT only when
rqspinlock detects a deadlock or timeout, not merely because IRQs are
disabled.

Also, 46730ee6e884 ("rhashtable: Use irq work for shrinking") moved the
shrink kick to irq_work so that it may be called in NMI context.  With this
check, __rhashtable_remove_fast_one() can no longer reach that
irq_work_queue() call from NMI or with IRQs disabled via this map.

[ ... ]

> @@ -3261,6 +3264,10 @@ static long rhtab_map_update_elem(struct bpf_map *map, void *key, void *value, u
>  	if (atomic_read(&rhtab->ht.nelems) >= map->max_entries)
>  		return -E2BIG;
>  
> +	/* see rhtab_delete_elem() */
> +	if (irqs_disabled())
> +		return -EBUSY;
> +

The same question applies here: this makes every insert of a new key fail
with -EBUSY from any IRQs-off context, whether or not a bucket lock is held
by the current CPU.

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/41698986159