On 11/21/22 9:48 PM, Alexei Starovoitov wrote:
On 11/21/22 9:05 AM, Yonghong Song wrote:
+ if (env->cur_state->active_rcu_lock) {
+ if (bpf_lsm_sleepable_func_proto(func_id) ||
+ bpf_tracing_sleepable_func_proto(func_id)) {
+ verbose(env, "sleepable helper %s#%din rcu_read_lock
region\n",
+ func_id_name(func_id), func_id);
+ return -EINVAL;
+ }
+
Even after patch 2 refactoring the above bit is still quite fragile.
Ex: bpf_d_path is not included, but it should be.
How about we add 'bool might_sleep' to bpf_func_proto and mark existing
5 functions with it and refactor patch 2 differently.
We won't be doing prog->aux->sleepable ? in bpf_tracing_func_proto()
anymore.
Those cbs will be returning func_proto-s,
but the verifier later will check might_sleep flag.
Ya, bpf_func_proto->might_sleep indeed better. I could do that.
The only problem is bpf_d_path.
static bool bpf_d_path_allowed(const struct bpf_prog *prog)
{
if (prog->type == BPF_PROG_TYPE_TRACING &&
prog->expected_attach_type == BPF_TRACE_ITER)
return true;
if (prog->type == BPF_PROG_TYPE_LSM)
return bpf_lsm_is_sleepable_hook(prog->aux->attach_btf_id);
return btf_id_set_contains(&btf_allowlist_d_path,
prog->aux->attach_btf_id);
}
If I understand correctly, bpf_d_path helper doesn't mean
the helper itself will be sleepable. For example, bpf_d_path can only
appear in sleepable programs if program type is BPF_PROG_TYPE_LSM,
from 6f100640ca5b ("bpf: Expose bpf_d_path helper to sleepable LSM
hooks") it looks like the reason is those sleepable lsm programs
provide better context so bpf_d_path won't have potential lock
or other issues. So essentially, bpf_d_path helper itself
won't be a helper causing the prog to sleep, right? If this is
the case, we only assign might_sleepable to the other 4 helpers.
