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.
