On 10/22/24 3:59 PM, Alexei Starovoitov wrote:
On Tue, Oct 22, 2024 at 3:41 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
On 10/22/24 2:57 PM, Alexei Starovoitov wrote:
On Tue, Oct 22, 2024 at 2:43 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
To handle a subprog may be used in more than one
subtree (subprog 0 tree or async tree), I need to
add a 'visited' field to bpf_subprog_info.
I think this should work.
This is getting quite complicated.
But looks like we have even bigger problem:
SEC("lsm/...")
int BPF_PROG(...)
{
volatile char buf[..];
buf[..] =
}
If I understand correctly, lsm/... corresponds to BPF_PROG_TYPE_LSM prog type.
The current implementation only supports the following plus struct_ops programs.
+ switch (env->prog->type) {
+ case BPF_PROG_TYPE_KPROBE:
+ case BPF_PROG_TYPE_TRACEPOINT:
+ case BPF_PROG_TYPE_PERF_EVENT:
+ case BPF_PROG_TYPE_RAW_TRACEPOINT:
+ return true;
+ case BPF_PROG_TYPE_TRACING:
+ if (env->prog->expected_attach_type != BPF_TRACE_ITER)
+ return true;
+ fallthrough;
+ default:
+ return false;
+ }
I do agree that lsm programs will have issues if using private stack
since preemptible is possible and we don't have recursion check for
them (which is right in order to provide correct functionality).
static inline bool bpf_prog_check_recur(const struct bpf_prog *prog)
{
switch (resolve_prog_type(prog)) {
case BPF_PROG_TYPE_TRACING:
return prog->expected_attach_type != BPF_TRACE_ITER;
case BPF_PROG_TYPE_STRUCT_OPS:
case BPF_PROG_TYPE_LSM:
return false;
default:
return true;
}
}
LSM prog is an example. The same issue is with struct_ops progs.
But struct_ops sched-ext progs is main motivation for adding
priv stack.
sched-ext will signal to bpf that it needs priv stack and
we would have to add "recursion no more than 1" check
and there is a chance (like above LSM prog demonstrates)
that struct_ops will be hitting this recursion check
and the prog will not be run.
The miss count will increment, of course, but the whole
priv stack feature for struct_ops becomes unreliable.
Hence the patches become questionable.
Why add a feature when the main user will struggle to use it.
Indeed, this is a known issue we kind of already aware of.
The recursion check (regardless it is one or four) may cause
prog no run if actual recursion level is beyond what recursion
check is doing.
I guess we indeed need to go back to drawing board again,
starting from struct_ops which is the main motivation of this
idea.
