Re: [PATCH bpf-next v1 06/20] bpf: Defer bpf_task_wake() to irq_work when IRQs are disabled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] bpf: use-after-free in bpf_task_wake_defer() pinning
  task->usage
- [Medium] bpf: undrained per-CPU wake queue in bpf_task_wake_defer() on
  dying CPU
--

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

bpf: Defer bpf_task_wake() to irq_work when IRQs are disabled

This commit defers bpf_task_wake() calls made with IRQs disabled to a
per-CPU hard irq_work queue using a per-task bpf_wake_node list node.

> diff --git a/kernel/bpf/task_park.c b/kernel/bpf/task_park.c
> index 39c9f085f7a97..5a9b3124312ee 100644
> --- a/kernel/bpf/task_park.c
> +++ b/kernel/bpf/task_park.c
[ ... ]
> +static DEFINE_PER_CPU(struct wake_q_node *, bpf_task_wake_queue);
> +
> +static void bpf_task_wake_deferred(struct irq_work *work)
> +{
> +	struct wake_q_node *node = xchg(this_cpu_ptr(&bpf_task_wake_queue), NULL);
> +
> +	while (node) {
> +		struct task_struct *task = container_of(node, struct task_struct, bpf_wake_node);
> +
> +		node = node->next == WAKE_Q_TAIL ? NULL : node->next;
> +		/* The task may be queued again from here on. */
> +		WRITE_ONCE(task->bpf_wake_node.next, NULL);
> +		wake_up_state(task, TASK_INTERRUPTIBLE);
> +		put_task_struct(task);
> +	}
> +}
> +
> +static DEFINE_PER_CPU(struct irq_work, bpf_task_wake_work) =
> +	IRQ_WORK_INIT_HARD(bpf_task_wake_deferred);
> +
> +static void bpf_task_wake_defer(struct task_struct *task)
> +{
> +	struct wake_q_node *node = &task->bpf_wake_node, *head;
> +
> +	/*
> +	 * The claim, as in __wake_q_add(): NULL is free, anything else is
> +	 * queued. The xchg() that stored the pending word is a full barrier
> +	 * between that store and the claim, so a claim lost to another holder
> +	 * is still covered: the holder's wake follows its own reset of the
> +	 * node, which this claim did not observe, so the task it wakes
> +	 * observes the word.
> +	 */
> +	if (cmpxchg_relaxed(&node->next, NULL, WAKE_Q_TAIL))
> +		return;
> +	get_task_struct(task);

[Severity: High]
Can pinning task via get_task_struct() here cause a refcount addition on
zero and a use-after-free in bpf_task_wake_deferred()?

bpf_task_wake() is registered with KF_RCU, so a tracing or LSM program can
pass a task_struct whose usage count has already reached zero in
__put_task_struct():

__put_task_struct(tsk)
  security_task_free(tsk)
    <BPF program with IRQs disabled>
      bpf_task_wake(tsk, 0)
        bpf_task_wake_defer(tsk)
          get_task_struct(tsk)  <-- refcount_inc on 0
          irq_work_queue(...)
  free_task(tsk)
<irq_work>
  bpf_task_wake_deferred()
    WRITE_ONCE(tsk->bpf_wake_node.next, NULL)  <-- use-after-free
    wake_up_state(tsk, TASK_INTERRUPTIBLE)

A nascent task in copy_process() before rcu_users is initialized is also
freed by delayed_free_task() on fork failure without consulting
tsk->usage.  Pinning task with refcount_inc_not_zero(&task->rcu_users)
before claiming node->next (or releasing node->next on failure) and
dropping it with put_task_struct_rcu_user() in bpf_task_wake_deferred()
would avoid queuing tasks in either state.

> +	head = this_cpu_read(bpf_task_wake_queue);
> +	do {
> +		WRITE_ONCE(node->next, head ?: WAKE_Q_TAIL);
> +	} while (!try_cmpxchg(this_cpu_ptr(&bpf_task_wake_queue), &head, node));
> +	irq_work_queue(this_cpu_ptr(&bpf_task_wake_work));
> +}

[Severity: Medium]
Can a wakeup queued on an outgoing CPU after CPUHP_AP_SMPCFD_DYING stay
undrained while the CPU is offline?

During CPU hotplug teardown, __cpu_disable() disables local IPI delivery
and smpcfd_dying_cpu() runs irq_work_run() at CPUHP_AP_SMPCFD_DYING.
After that callback finishes, the migration stopper thread still parks on
the outgoing CPU and fires trace_sched_waking and trace_sched_switch with
IRQs disabled.  If a BPF program attached there calls bpf_task_wake(),
bpf_task_wake_defer() queues task onto the outgoing CPU's
bpf_task_wake_queue and raised_list, which smpcfd_dead_cpu() ->
irq_work_run_cpu() does not drain because it only flushes lazy_list.

The wakeup is lost and task->bpf_wake_node.next remains non-NULL while the
CPU stays offline, causing subsequent deferred bpf_task_wake() calls for
that task on online CPUs to fail the cmpxchg_relaxed() claim as well.

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