Re: [RFC PATCH 03/10] bpf: Fail sleepable user copy helpers when current->mm is NULL

[bot+bpf-ci]

> bpf: Fail sleepable user copy helpers when current->mm is NULL

TL;DR: Commit message only: the security_file_release() example of a sleepable
program running from a kworker looks inaccurate, since such a program appears
not to be attachable there. The fix itself looks justified.

> bpf: Fail sleepable user copy helpers when current->mm is NULL
>
> Sleepable BPF programs can run on behalf of a kernel thread, or of a
> task that has already gone through exit_mm(). For example, bpf_wq
> callbacks always run from a kworker, bpf_task_work callbacks queued
> late enough run from exit_task_work(), and lsm.s or fentry.s programs
> may be attached to functions called during exit_files(),
> exit_task_work() or from kernel threads (e.g. security_file_release()
> from a kworker running delayed_fput()).
> In such contexts, bpf_copy_from_user(), bpf_copy_from_user_str() and
> bpf_copy_from_user{,_str}_dynptr() (when reading from the current task)
> call copy_from_user() or strncpy_from_user() without an mm. That access
> does not reliably fail: the user half of the address space belongs to
> whatever mm happens to be lazily loaded, if any, so the program may read
> another task's memory.
> As there is no user memory to read from in this case, fail with -EFAULT
> without attempting the access.
> Fixes: 07be4c4a3e7a ("bpf: Add bpf_copy_from_user() helper.")
> Fixes: 65ab5ac4df01 ("bpf: Add bpf_copy_from_user_str kfunc")
> Fixes: a498ee7576de ("bpf: Implement dynptr copy kfuncs")
> Signed-off-by: James Houghton <jthoughton@xxxxxxxxxx>

The commit message gives security_file_release() as an example of a place
where an lsm.s or fentry.s program can run from a kworker:

> (e.g. security_file_release() from a kworker running delayed_fput())

Is this example accurate for the tree being patched?

bpf_lsm_file_release is not in the sleepable_lsm_hooks set in
kernel/bpf/bpf_lsm.c, so btf_id_allow_sleepable() rejects a sleepable
BPF_PROG_TYPE_LSM program there.  bpf_lsm_file_free_security, the other hook
reached from __fput() through file_free(), is in untrusted_lsm_hooks rather
than the sleepable set.

For a BPF_PROG_TYPE_TRACING program, check_attach_sleepable() requires
within_error_injection_list(addr) (or a syscall prefix when
CONFIG_FUNCTION_ERROR_INJECTION is off), or an fmodret set with
KF_SLEEPABLE.  Neither security_file_release() nor bpf_lsm_file_release()
has ALLOW_ERROR_INJECTION, and the only fmodret sets are in
net/bpf/test_run.c, net/mptcp/bpf.c and bpf_testmod.

That suggests a program attached to security_file_release() is never
sleepable and cannot call bpf_copy_from_user() or the KF_SLEEPABLE copy
kfuncs.

The fix itself still looks justified, since bpf_wq callbacks run from a
kworker and is_async_cb_sleepable() is true for them.  Only the example
appears to be off.

Would an example that is actually reachable keep the changelog accurate?
For instance, bpf_lsm_task_alloc() when kthreadd forks a kernel thread
(current is kthreadd), or bpf_lsm_kernel_read_file() and bpf_lsm_file_open()
when a kworker loads firmware with kernel_read_file().  All three hooks are
in sleepable_lsm_hooks.


---
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/40410166379