| Message ID | 20240206220441.38311-2-alexei.starovoitov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Introduce BPF arena. | expand |
On Tue, Feb 6, 2024 at 2:04 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > From: Alexei Starovoitov <ast@kernel.org> > > Recognize return of 'void *' from kfunc as returning unknown scalar. > > Signed-off-by: Alexei Starovoitov <ast@kernel.org> > --- > kernel/bpf/verifier.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index ddaf09db1175..d9c2dbb3939f 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -12353,6 +12353,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > meta.func_name); > return -EFAULT; > } > + } else if (btf_type_is_void(ptr_type)) { > + /* kfunc returning 'void *' is equivalent to returning scalar */ > + mark_reg_unknown(env, regs, BPF_REG_0); Acked-by: Andrii Nakryiko <andrii@kernel.org> I think we should do a similar extension when passing `void *` into global funcs. It's best to treat it as SCALAR instead of rejecting it because we can't calculate the size. Currently users in practice just have to define it as `uintptr_t` and then cast (or create static wrappers doing the casting). Anyways, my point is that it makes sense to treat `void *` as non-pointer. > } else if (!__btf_type_is_struct(ptr_type)) { > if (!meta.r0_size) { > __u32 sz; > -- > 2.34.1 >
On Thu, Feb 8, 2024 at 11:40 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Feb 6, 2024 at 2:04 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > From: Alexei Starovoitov <ast@kernel.org> > > > > Recognize return of 'void *' from kfunc as returning unknown scalar. > > > > Signed-off-by: Alexei Starovoitov <ast@kernel.org> > > --- > > kernel/bpf/verifier.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index ddaf09db1175..d9c2dbb3939f 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -12353,6 +12353,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > > meta.func_name); > > return -EFAULT; > > } > > + } else if (btf_type_is_void(ptr_type)) { > > + /* kfunc returning 'void *' is equivalent to returning scalar */ > > + mark_reg_unknown(env, regs, BPF_REG_0); > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > I think we should do a similar extension when passing `void *` into > global funcs. It's best to treat it as SCALAR instead of rejecting it > because we can't calculate the size. Currently users in practice just > have to define it as `uintptr_t` and then cast (or create static > wrappers doing the casting). Anyways, my point is that it makes sense > to treat `void *` as non-pointer. Makes sense. Will add it to my todo list. On that note I've been thinking how to get rid of __arg_arena that I'm adding in this series. How about the following algorithm? do_check_main() sees that scalar or ptr_to_arena is passed into global subprog that has BTF 'struct foo *' and today would require ptr_to_mem. Instead of rejecting the prog the verifier would override (only once and in one direction) that arg of that global func from ptr_to_mem into scalar. And will proceed as usual. do_check_common() of that global subprog will pick up scalar for that arg, since args are cached. And verification will proceed successfully without special __arg_arena .
On Tue, Feb 06, 2024 at 02:04:26PM -0800, Alexei Starovoitov wrote: > From: Alexei Starovoitov <ast@kernel.org> > > Recognize return of 'void *' from kfunc as returning unknown scalar. > > Signed-off-by: Alexei Starovoitov <ast@kernel.org> Acked-by: David Vernet <void@manifault.com>
On Thu, Feb 8, 2024 at 4:09 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Feb 8, 2024 at 11:40 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Feb 6, 2024 at 2:04 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > From: Alexei Starovoitov <ast@kernel.org> > > > > > > Recognize return of 'void *' from kfunc as returning unknown scalar. > > > > > > Signed-off-by: Alexei Starovoitov <ast@kernel.org> > > > --- > > > kernel/bpf/verifier.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > index ddaf09db1175..d9c2dbb3939f 100644 > > > --- a/kernel/bpf/verifier.c > > > +++ b/kernel/bpf/verifier.c > > > @@ -12353,6 +12353,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > > > meta.func_name); > > > return -EFAULT; > > > } > > > + } else if (btf_type_is_void(ptr_type)) { > > > + /* kfunc returning 'void *' is equivalent to returning scalar */ > > > + mark_reg_unknown(env, regs, BPF_REG_0); > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > > I think we should do a similar extension when passing `void *` into > > global funcs. It's best to treat it as SCALAR instead of rejecting it > > because we can't calculate the size. Currently users in practice just > > have to define it as `uintptr_t` and then cast (or create static > > wrappers doing the casting). Anyways, my point is that it makes sense > > to treat `void *` as non-pointer. > > Makes sense. Will add it to my todo list. > > On that note I've been thinking how to get rid of __arg_arena > that I'm adding in this series. > > How about the following algorithm? > do_check_main() sees that scalar or ptr_to_arena is passed > into global subprog that has BTF 'struct foo *' > and today would require ptr_to_mem. > Instead of rejecting the prog the verifier would override > (only once and in one direction) > that arg of that global func from ptr_to_mem into scalar. > And will proceed as usual. > do_check_common() of that global subprog will pick up scalar > for that arg, since args are cached. > And verification will proceed successfully without special __arg_arena > . Can we pass PTR_TO_MEM (e.g., map value pointer) to something that is expecting PTR_TO_ARENA? Because there are few problems with the above algorithm, I think. First, this check won't be just in do_check_main(), the same global function can be called from another function. And second, what if you have the first few calls that pass PTR_TO_MEM. Verifier sees that, allows it, assumes global func will take PTR_TO_MEM. Then we get to a call that passes PTR_TO_ARENA or scalar, we change the argument expectation to be __arg_arena-like and subsequent checks will assume arena stuff. But the first few calls already assumed correctness based on PTR_TO_MEM. In short, it seems like this introduces more subtleness and potentially unexpected interactions. I don't really see explicit __arg_arena as a bad thing, I find that explicit annotations for "special things" help in practice as they bring specialness into attention. And also allow people to ask/google more specific questions.
On Fri, Feb 9, 2024 at 11:09 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Feb 8, 2024 at 4:09 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Thu, Feb 8, 2024 at 11:40 AM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Tue, Feb 6, 2024 at 2:04 PM Alexei Starovoitov > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > From: Alexei Starovoitov <ast@kernel.org> > > > > > > > > Recognize return of 'void *' from kfunc as returning unknown scalar. > > > > > > > > Signed-off-by: Alexei Starovoitov <ast@kernel.org> > > > > --- > > > > kernel/bpf/verifier.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > > index ddaf09db1175..d9c2dbb3939f 100644 > > > > --- a/kernel/bpf/verifier.c > > > > +++ b/kernel/bpf/verifier.c > > > > @@ -12353,6 +12353,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > > > > meta.func_name); > > > > return -EFAULT; > > > > } > > > > + } else if (btf_type_is_void(ptr_type)) { > > > > + /* kfunc returning 'void *' is equivalent to returning scalar */ > > > > + mark_reg_unknown(env, regs, BPF_REG_0); > > > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > > > > I think we should do a similar extension when passing `void *` into > > > global funcs. It's best to treat it as SCALAR instead of rejecting it > > > because we can't calculate the size. Currently users in practice just > > > have to define it as `uintptr_t` and then cast (or create static > > > wrappers doing the casting). Anyways, my point is that it makes sense > > > to treat `void *` as non-pointer. > > > > Makes sense. Will add it to my todo list. > > > > On that note I've been thinking how to get rid of __arg_arena > > that I'm adding in this series. > > > > How about the following algorithm? > > do_check_main() sees that scalar or ptr_to_arena is passed > > into global subprog that has BTF 'struct foo *' > > and today would require ptr_to_mem. > > Instead of rejecting the prog the verifier would override > > (only once and in one direction) > > that arg of that global func from ptr_to_mem into scalar. > > And will proceed as usual. > > do_check_common() of that global subprog will pick up scalar > > for that arg, since args are cached. > > And verification will proceed successfully without special __arg_arena > > . > > Can we pass PTR_TO_MEM (e.g., map value pointer) to something that is > expecting PTR_TO_ARENA? Because there are few problems with the above > algorithm, I think. The patch 10 allows only ptr_to_arena and scalar to be passed in, but passing ptr_to_mem is safe too. It won't crash the kernel, but it won't do what user might expect. Hence it's disabled. > First, this check won't be just in do_check_main(), the same global > function can be called from another function. that shouldn't matter. > And second, what if you have the first few calls that pass PTR_TO_MEM. > Verifier sees that, allows it, assumes global func will take > PTR_TO_MEM. Then we get to a call that passes PTR_TO_ARENA or scalar, > we change the argument expectation to be __arg_arena-like and > subsequent checks will assume arena stuff. But the first few calls > already assumed correctness based on PTR_TO_MEM. I think that would the issue only if we call that global func with ptr_to_mem and then went at processed the body of it with ptr_to_mem and later discovered another call site that passes scalar. Such bug can be accounted for. > In short, it seems like this introduces more subtleness and > potentially unexpected interactions. I don't really see explicit > __arg_arena as a bad thing, I find that explicit annotations for > "special things" help in practice as they bring specialness into > attention. And also allow people to ask/google more specific > questions. In general I agree that __arg is a useful indication. I've been writing arena enabled bpf progs and found this __arg_arena quite annoying to add when converting static to global func. I know that globals are verified differently, but it feels we can do better with arena pointers. Anyway I'll proceed with the existing __arg_arean approach and put this "smart" detection of arena pointers on back burner for now.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index ddaf09db1175..d9c2dbb3939f 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -12353,6 +12353,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, meta.func_name); return -EFAULT; } + } else if (btf_type_is_void(ptr_type)) { + /* kfunc returning 'void *' is equivalent to returning scalar */ + mark_reg_unknown(env, regs, BPF_REG_0); } else if (!__btf_type_is_struct(ptr_type)) { if (!meta.r0_size) { __u32 sz;