Re: [PATCH bpf-next 2/4] bpf: add bpf_thread_wq kthread-backed workqueue with cgroup placement

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The verifier fails to validate KF_ARG_PTR_TO_THREAD_WQ pointers for type correctness and exact struct offset.
- [High] bpf_thread_wq_cancel_and_free() calls sleepable functions but is invoked synchronously from atomic contexts during map element updates.
- [High] bpf_thread_wq_start() acquires a raw spinlock but is exposed to NMI context tracing programs, leading to hard deadlocks.
--

--- Patch [2]: [PATCH bpf-next 2/4] bpf: add bpf_thread_wq kthread-backed workqueue with cgroup placement ---
commit 287b8c7d2c6aff4fceb91479e0723ac87382505d
Author: Hui Zhu <zhuhui@kylinos.cn>

bpf: add bpf_thread_wq kthread-backed workqueue with cgroup placement

Introduce bpf_thread_wq, a new BPF embedded map field similar to
bpf_wq but backed by a dedicated kthread_worker instead of a system
workqueue. The worker kthread can be attached to a specific cgroup at
init time so BPF-deferred callbacks run under the resource limits of
the target cgroup.

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
[ ... ]
> @@ -502,6 +511,7 @@ static inline bool btf_field_is_nmi_safe(enum btf_field_type type)
>  	case BPF_TASK_WORK:
>  	case BPF_KPTR_UNREF:
>  	case BPF_REFCOUNT:
> +	case BPF_THREAD_WQ:
>  		return true;
>  	default:
>  		return false;

[Severity: High]
Will this allow NMI tracing programs to deadlock the system?

By returning true here, the verifier permits bpf_thread_wq in NMI contexts.
However, bpf_thread_wq_start() invokes kthread_queue_work(), which acquires a
raw spinlock. If an NMI interrupts a CPU that already holds this lock, the
NMI handler will spin forever waiting for it.

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
[ ... ]
> @@ -839,6 +848,9 @@ void bpf_obj_free_fields(const struct btf_record *rec, void *obj)
>  		case BPF_TASK_WORK:
>  			bpf_task_work_cancel_and_free(field_ptr);
>  			break;
> +		case BPF_THREAD_WQ:
> +			bpf_thread_wq_cancel_and_free(field_ptr);
> +			break;
>  		case BPF_KPTR_UNREF:
>  			WRITE_ONCE(*(u64 *)field_ptr, 0);
>  			break;

[Severity: High]
Can this trigger a scheduling while atomic regression?

When a BPF program updates an existing map element, the BPF subsystem invokes
bpf_obj_free_fields() to tear down old special fields in-place. This update
path can run in atomic contexts, such as under rcu_read_lock() or with
preemption disabled.

Since bpf_thread_wq_cancel_and_free() explicitly calls might_sleep() and the
blocking kthread_cancel_work_sync(), calling it from this atomic context seems
like it would result in a scheduling bug or deadlock.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -12192,6 +12219,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_call_arg_me
>  		case KF_ARG_PTR_TO_WORKQUEUE:
>  		case KF_ARG_PTR_TO_TIMER:
>  		case KF_ARG_PTR_TO_TASK_WORK:
> +		case KF_ARG_PTR_TO_THREAD_WQ:
>  		case KF_ARG_PTR_TO_IRQ_FLAG:
>  		case KF_ARG_PTR_TO_RES_SPIN_LOCK:
>  			break;

[Severity: Critical]
Does this code leave the thread workqueue pointer unvalidated?

This adds the argument type to the initial bypass switch statement in
check_kfunc_args(), but there does not appear to be a corresponding addition
in the subsequent switch statement that validates the pointer's map origin
and offset.

For example, KF_ARG_PTR_TO_TASK_WORK handles this via:

check_kfunc_args() {
    ...
    case KF_ARG_PTR_TO_TASK_WORK:
        if (reg->type != PTR_TO_MAP_VALUE) { ... }
        ret = check_map_field_pointer(env, reg, argno, BPF_TASK_WORK, ...);
    ...
}

Without a similar check for KF_ARG_PTR_TO_THREAD_WQ, it looks like a crafted
pointer could bypass offset validation, allowing the kfuncs to cast and
dereference invalid memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786086076.git.zhuhui@kylinos.cn?part=2