Also, could you include a test to make sure sleepable programs cannot
call bpf_task_acquire? It seems to assume RCU read lock is held while
that may not be true. If already not possible, maybe a WARN_ON_ONCE
inside the helper to ensure future cases don't creep in.
I don't _think_ it's unsafe for a sleepable program to call
bpf_task_acquire(). My understanding is that the struct task_struct *
parameter to bpf_task_acquire() is not PTR_UNTRUSTED, so it's safe to
dereference directly in the kfunc. The implicit assumption here is that
the task was either passed to the BPF program (which is calling
bpf_task_acquire()) from the main kernel in something like a trace or
struct_ops callback, or it was a referenced kptr that was removed from a
map with bpf_kptr_xchg(), and is now owned by the BPF program. Given
that the ptr type is not PTR_UNTRUSTED, it seemed correct to assume that
the task was valid in bpf_task_acquire() regardless of whether we were
in an RCU read region or not, but please let me know if I'm wrong about
I don't think it's correct. You can just walk arbitrary structures and
obtain a normal PTR_TO_BTF_ID that looks seemingly ok to the verifier
but has no lifetime guarantees. It's a separate pre-existing problem
unrelated to your series . PTR_UNTRUSTED is not set for those cases
So the argument to your bpf_task_acquire may already be freed by then.
This issue would be exacerbated in sleepable BPF programs, where RCU
read lock is not held, so some cases of pointer walking where it may
be safe no longer holds.
I am planning to clean this up, but I'd still prefer if we don't allow
this helper in sleepable programs, yet. kptr_get is ok to allow.
Once you have explicit BPF RCU read sections, sleepable programs can
take it, do loads, and operate on the RCU pointer safely until they
invalidate it with the outermost bpf_rcu_read_unlock. It's needed for
local kptrs as well, not just this. I plan to post this very soon, so
we should be able to fix it up in the current cycle after landing your
__rcu tags in the kernel will automatically be understood by the
verifier and for the majority of the correctly annotated cases this
will work fine and not break tracing programs.
that. Other kfuncs I saw such as bpf_xdp_ct_lookup() assumed that the
parameter passed by the BPF program (which itself was passing on a
pointer given to it by the main kernel) is valid as well.
Yeah, but the CT API doesn't assume validity of random PTR_TO_BTF_ID,
it requires KF_TRUSTED_ARGS which forces them to have ref_obj_id != 0.