On Tue, Apr 15, 2025 at 4:25 PM Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> wrote:
>
> From: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>
> Date: Tue, 15 Apr 2025 16:10:42 -0700
> > On Mon, Apr 14, 2025 at 2:22 PM Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> wrote:
> > >
> > > There are some failure paths in bpf_int_jit_compile() that are not
> > > worth triggering a warning in __bpf_prog_ret0_warn().
> > >
> > > For example, if we fail to allocate memory in bpf_int_jit_compile(),
> > > we should propagate -ENOMEM to userspace instead of attaching
> > > __bpf_prog_ret0_warn().
> > >
> > > Let's pass &err to bpf_int_jit_compile() to propagate errno.
> >
> > Is there any reason we are not just returning ERR_PTR() instead of the
> > approach in this patch? That seems more canonical within BPF
> > subsystem, if we need to return error for pointer-returning functions?
>
> From the comment in __bpf_prog_ret0_warn(), I thought BPF folks wanted
> to catch any JIT failure as a warning for syzbot etc, instead of returning
> an error to user. Even it can be done by adding WARN_ON() to the caller
> of bpf_int_jit_compile() though.
>
> That's why I only set error in the -ENOMEM paths in patch 2, which is
> not a BPF problem at least.
>
> With Alexei's suggestion, we can suppress that case, but as -ENOTSUPP.
>
> What do you think ?
I'm not sure I follow this completely. My only suggestion is the
*mechanics* of delivering error code. Instead of setting error by
pointer, we can just return it as ERR_PTR(). The callers of
bpf_int_jit_compile() (verifier and core.c) can still swallow any
errors, if necessary and revert to original pointer. E.g., in
verifier, instead of:
func[i] = bpf_int_jit_compile(func[i]);
if (!func[i]->jited) {
err = -ENOTSUPP;
goto out_free;
}
you'd do:
struct bpf_prog *jit_prog = bpf_int_jit_compile(func[i]);
if (IS_ERR(jit_prog) || !jit_prog->jited) {
err = -ENOTSUPP;
goto out_free;
}
func[i] = jit_prog;
Again, it's just weird to pass error as an extra out parameter, that's all.
But I don't feel strongly about this, was just trying to suggest a
cleaner alternative.
