| Message ID | 20200121120512.758929-2-jolsa@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Add trampoline helpers | expand |
On Tue, Jan 21, 2020 at 4:05 AM Jiri Olsa <jolsa@kernel.org> wrote: > > When accessing the context we allow access to arguments with > scalar type and pointer to struct. But we omit pointer to scalar > type, which is the case for many functions and same case as > when accessing scalar. > > Adding the check if the pointer is to scalar type and allow it. > > Acked-by: John Fastabend <john.fastabend@gmail.com> > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > kernel/bpf/btf.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 832b5d7fd892..207ae554e0ce 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -3668,7 +3668,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type, > const struct bpf_prog *prog, > struct bpf_insn_access_aux *info) > { > - const struct btf_type *t = prog->aux->attach_func_proto; > + const struct btf_type *tp, *t = prog->aux->attach_func_proto; > struct bpf_prog *tgt_prog = prog->aux->linked_prog; > struct btf *btf = bpf_prog_get_target_btf(prog); > const char *tname = prog->aux->attach_func_name; > @@ -3730,6 +3730,17 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type, > */ > return true; > > + tp = btf_type_by_id(btf, t->type); > + /* skip modifiers */ > + while (btf_type_is_modifier(tp)) > + tp = btf_type_by_id(btf, tp->type); > + > + if (btf_type_is_int(tp) || btf_type_is_enum(tp)) > + /* This is a pointer scalar. > + * It is the same as scalar from the verifier safety pov. > + */ > + return true; The reason I didn't do it earlier is I was thinking to represent it as PTR_TO_BTF_ID as well, so that corresponding u8..u64 access into this memory would still be possible. I'm trying to analyze the situation that returning a scalar now and converting to PTR_TO_BTF_ID in the future will keep progs passing the verifier. Is it really the case? Could you give a specific example that needs this support? It will help me understand this backward compatibility concern. What prog is doing with that 'u32 *' that is seen as scalar ? It cannot dereference it. Use it as what?
On 1/21/20 5:51 PM, Alexei Starovoitov wrote: > On Tue, Jan 21, 2020 at 4:05 AM Jiri Olsa <jolsa@kernel.org> wrote: >> >> When accessing the context we allow access to arguments with >> scalar type and pointer to struct. But we omit pointer to scalar >> type, which is the case for many functions and same case as >> when accessing scalar. >> >> Adding the check if the pointer is to scalar type and allow it. >> >> Acked-by: John Fastabend <john.fastabend@gmail.com> >> Signed-off-by: Jiri Olsa <jolsa@kernel.org> >> --- >> kernel/bpf/btf.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c >> index 832b5d7fd892..207ae554e0ce 100644 >> --- a/kernel/bpf/btf.c >> +++ b/kernel/bpf/btf.c >> @@ -3668,7 +3668,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type, >> const struct bpf_prog *prog, >> struct bpf_insn_access_aux *info) >> { >> - const struct btf_type *t = prog->aux->attach_func_proto; >> + const struct btf_type *tp, *t = prog->aux->attach_func_proto; >> struct bpf_prog *tgt_prog = prog->aux->linked_prog; >> struct btf *btf = bpf_prog_get_target_btf(prog); >> const char *tname = prog->aux->attach_func_name; >> @@ -3730,6 +3730,17 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type, >> */ >> return true; >> >> + tp = btf_type_by_id(btf, t->type); >> + /* skip modifiers */ >> + while (btf_type_is_modifier(tp)) >> + tp = btf_type_by_id(btf, tp->type); >> + >> + if (btf_type_is_int(tp) || btf_type_is_enum(tp)) >> + /* This is a pointer scalar. >> + * It is the same as scalar from the verifier safety pov. >> + */ >> + return true; > > The reason I didn't do it earlier is I was thinking to represent it > as PTR_TO_BTF_ID as well, so that corresponding u8..u64 > access into this memory would still be possible. > I'm trying to analyze the situation that returning a scalar now > and converting to PTR_TO_BTF_ID in the future will keep progs > passing the verifier. Is it really the case? > Could you give a specific example that needs this support? > It will help me understand this backward compatibility concern. > What prog is doing with that 'u32 *' that is seen as scalar ? > It cannot dereference it. Use it as what? If this is from original bcc code, it will use bpf_probe_read for dereference. This is what I understand when I first reviewed this patch. But it will be good to get Jiri's confirmation.
On Wed, Jan 22, 2020 at 02:33:32AM +0000, Yonghong Song wrote: > > > On 1/21/20 5:51 PM, Alexei Starovoitov wrote: > > On Tue, Jan 21, 2020 at 4:05 AM Jiri Olsa <jolsa@kernel.org> wrote: > >> > >> When accessing the context we allow access to arguments with > >> scalar type and pointer to struct. But we omit pointer to scalar > >> type, which is the case for many functions and same case as > >> when accessing scalar. > >> > >> Adding the check if the pointer is to scalar type and allow it. > >> > >> Acked-by: John Fastabend <john.fastabend@gmail.com> > >> Signed-off-by: Jiri Olsa <jolsa@kernel.org> > >> --- > >> kernel/bpf/btf.c | 13 ++++++++++++- > >> 1 file changed, 12 insertions(+), 1 deletion(-) > >> > >> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > >> index 832b5d7fd892..207ae554e0ce 100644 > >> --- a/kernel/bpf/btf.c > >> +++ b/kernel/bpf/btf.c > >> @@ -3668,7 +3668,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type, > >> const struct bpf_prog *prog, > >> struct bpf_insn_access_aux *info) > >> { > >> - const struct btf_type *t = prog->aux->attach_func_proto; > >> + const struct btf_type *tp, *t = prog->aux->attach_func_proto; > >> struct bpf_prog *tgt_prog = prog->aux->linked_prog; > >> struct btf *btf = bpf_prog_get_target_btf(prog); > >> const char *tname = prog->aux->attach_func_name; > >> @@ -3730,6 +3730,17 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type, > >> */ > >> return true; > >> > >> + tp = btf_type_by_id(btf, t->type); > >> + /* skip modifiers */ > >> + while (btf_type_is_modifier(tp)) > >> + tp = btf_type_by_id(btf, tp->type); > >> + > >> + if (btf_type_is_int(tp) || btf_type_is_enum(tp)) > >> + /* This is a pointer scalar. > >> + * It is the same as scalar from the verifier safety pov. > >> + */ > >> + return true; > > > > The reason I didn't do it earlier is I was thinking to represent it > > as PTR_TO_BTF_ID as well, so that corresponding u8..u64 > > access into this memory would still be possible. > > I'm trying to analyze the situation that returning a scalar now > > and converting to PTR_TO_BTF_ID in the future will keep progs > > passing the verifier. Is it really the case? > > Could you give a specific example that needs this support? > > It will help me understand this backward compatibility concern. > > What prog is doing with that 'u32 *' that is seen as scalar ? > > It cannot dereference it. Use it as what? > > If this is from original bcc code, it will use bpf_probe_read for > dereference. This is what I understand when I first reviewed this patch. > But it will be good to get Jiri's confirmation. it blocked me from accessing 'filename' argument when I probed do_sys_open via trampoline in bcc, like: KRETFUNC_PROBE(do_sys_open) { const char *filename = (const char *) args[1]; AFAICS the current code does not allow for trampoline arguments being other pointers than to void or struct, the patch should detect that the argument is pointer to scalar type and let it pass jirka
On Wed, Jan 22, 2020 at 10:13:36AM +0100, Jiri Olsa wrote: > On Wed, Jan 22, 2020 at 02:33:32AM +0000, Yonghong Song wrote: > > > > > > On 1/21/20 5:51 PM, Alexei Starovoitov wrote: > > > On Tue, Jan 21, 2020 at 4:05 AM Jiri Olsa <jolsa@kernel.org> wrote: > > >> > > >> When accessing the context we allow access to arguments with > > >> scalar type and pointer to struct. But we omit pointer to scalar > > >> type, which is the case for many functions and same case as > > >> when accessing scalar. > > >> > > >> Adding the check if the pointer is to scalar type and allow it. > > >> > > >> Acked-by: John Fastabend <john.fastabend@gmail.com> > > >> Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > >> --- > > >> kernel/bpf/btf.c | 13 ++++++++++++- > > >> 1 file changed, 12 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > >> index 832b5d7fd892..207ae554e0ce 100644 > > >> --- a/kernel/bpf/btf.c > > >> +++ b/kernel/bpf/btf.c > > >> @@ -3668,7 +3668,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type, > > >> const struct bpf_prog *prog, > > >> struct bpf_insn_access_aux *info) > > >> { > > >> - const struct btf_type *t = prog->aux->attach_func_proto; > > >> + const struct btf_type *tp, *t = prog->aux->attach_func_proto; > > >> struct bpf_prog *tgt_prog = prog->aux->linked_prog; > > >> struct btf *btf = bpf_prog_get_target_btf(prog); > > >> const char *tname = prog->aux->attach_func_name; > > >> @@ -3730,6 +3730,17 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type, > > >> */ > > >> return true; > > >> > > >> + tp = btf_type_by_id(btf, t->type); > > >> + /* skip modifiers */ > > >> + while (btf_type_is_modifier(tp)) > > >> + tp = btf_type_by_id(btf, tp->type); > > >> + > > >> + if (btf_type_is_int(tp) || btf_type_is_enum(tp)) > > >> + /* This is a pointer scalar. > > >> + * It is the same as scalar from the verifier safety pov. > > >> + */ > > >> + return true; > > > > > > The reason I didn't do it earlier is I was thinking to represent it > > > as PTR_TO_BTF_ID as well, so that corresponding u8..u64 > > > access into this memory would still be possible. > > > I'm trying to analyze the situation that returning a scalar now > > > and converting to PTR_TO_BTF_ID in the future will keep progs > > > passing the verifier. Is it really the case? > > > Could you give a specific example that needs this support? > > > It will help me understand this backward compatibility concern. > > > What prog is doing with that 'u32 *' that is seen as scalar ? > > > It cannot dereference it. Use it as what? > > > > If this is from original bcc code, it will use bpf_probe_read for > > dereference. This is what I understand when I first reviewed this patch. > > But it will be good to get Jiri's confirmation. > > it blocked me from accessing 'filename' argument when I probed > do_sys_open via trampoline in bcc, like: > > KRETFUNC_PROBE(do_sys_open) > { > const char *filename = (const char *) args[1]; > > AFAICS the current code does not allow for trampoline arguments > being other pointers than to void or struct, the patch should > detect that the argument is pointer to scalar type and let it > pass Got it. I've looked up your bcc patches and I agree that there is no way to workaround. BTF type argument of that kernel function is 'const char *' and the verifier will enforce that if bpf program tries to cast it the verifier will still see 'const char *'. (It's done this way by design). How about we special case 'char *' in the verifier? Then my concern regarding future extensibility of 'int *' and 'long *' will go away. Compilers have a long history special casing 'char *'. In particular signed char because it's a pointer to null terminated string. I think it's still a special pointer from pointer aliasing point of view. I think the verifier can treat it as scalar here too. In the future the verifier will get smarter and will recognize it as PTR_TO_NULL_STRING while 'u8 *', 'u32 *' will be PTR_TO_BTF_ID. I think it will solve this particular issue. I like conservative approach to the verifier improvements: start with strict checking and relax it on case-by-case. Instead of accepting wide range of cases and cause potential compatibility issues.
On Wed, Jan 22, 2020 at 08:09:59AM -0800, Alexei Starovoitov wrote: SNIP > > > > It cannot dereference it. Use it as what? > > > > > > If this is from original bcc code, it will use bpf_probe_read for > > > dereference. This is what I understand when I first reviewed this patch. > > > But it will be good to get Jiri's confirmation. > > > > it blocked me from accessing 'filename' argument when I probed > > do_sys_open via trampoline in bcc, like: > > > > KRETFUNC_PROBE(do_sys_open) > > { > > const char *filename = (const char *) args[1]; > > > > AFAICS the current code does not allow for trampoline arguments > > being other pointers than to void or struct, the patch should > > detect that the argument is pointer to scalar type and let it > > pass > > Got it. I've looked up your bcc patches and I agree that there is no way to > workaround. BTF type argument of that kernel function is 'const char *' and the > verifier will enforce that if bpf program tries to cast it the verifier will > still see 'const char *'. (It's done this way by design). How about we special > case 'char *' in the verifier? Then my concern regarding future extensibility > of 'int *' and 'long *' will go away. > Compilers have a long history special casing 'char *'. In particular signed > char because it's a pointer to null terminated string. I think it's still a > special pointer from pointer aliasing point of view. I think the verifier can > treat it as scalar here too. In the future the verifier will get smarter and > will recognize it as PTR_TO_NULL_STRING while 'u8 *', 'u32 *' will be > PTR_TO_BTF_ID. I think it will solve this particular issue. I like conservative > approach to the verifier improvements: start with strict checking and relax it > on case-by-case. Instead of accepting wide range of cases and cause potential > compatibility issues. ok, so something like below? jirka --- diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 832b5d7fd892..dd678b8e00b7 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -3664,6 +3664,19 @@ struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog) } } +static bool is_string_ptr(struct btf *btf, const struct btf_type *t) +{ + /* t comes in already as a pointer */ + t = btf_type_by_id(btf, t->type); + + /* allow const */ + if (BTF_INFO_KIND(t->info) == BTF_KIND_CONST) + t = btf_type_by_id(btf, t->type); + + /* char, signed char, unsigned char */ + return btf_type_is_int(t) && t->size == 1; +} + bool btf_ctx_access(int off, int size, enum bpf_access_type type, const struct bpf_prog *prog, struct bpf_insn_access_aux *info) @@ -3730,6 +3743,9 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type, */ return true; + if (is_string_ptr(btf, t)) + return true; + /* this is a pointer to another type */ info->reg_type = PTR_TO_BTF_ID;
On Wed, Jan 22, 2020 at 1:18 PM Jiri Olsa <jolsa@redhat.com> wrote: > > On Wed, Jan 22, 2020 at 08:09:59AM -0800, Alexei Starovoitov wrote: > > SNIP > > > > > > It cannot dereference it. Use it as what? > > > > > > > > If this is from original bcc code, it will use bpf_probe_read for > > > > dereference. This is what I understand when I first reviewed this patch. > > > > But it will be good to get Jiri's confirmation. > > > > > > it blocked me from accessing 'filename' argument when I probed > > > do_sys_open via trampoline in bcc, like: > > > > > > KRETFUNC_PROBE(do_sys_open) > > > { > > > const char *filename = (const char *) args[1]; > > > > > > AFAICS the current code does not allow for trampoline arguments > > > being other pointers than to void or struct, the patch should > > > detect that the argument is pointer to scalar type and let it > > > pass > > > > Got it. I've looked up your bcc patches and I agree that there is no way to > > workaround. BTF type argument of that kernel function is 'const char *' and the > > verifier will enforce that if bpf program tries to cast it the verifier will > > still see 'const char *'. (It's done this way by design). How about we special > > case 'char *' in the verifier? Then my concern regarding future extensibility > > of 'int *' and 'long *' will go away. > > Compilers have a long history special casing 'char *'. In particular signed > > char because it's a pointer to null terminated string. I think it's still a > > special pointer from pointer aliasing point of view. I think the verifier can > > treat it as scalar here too. In the future the verifier will get smarter and > > will recognize it as PTR_TO_NULL_STRING while 'u8 *', 'u32 *' will be > > PTR_TO_BTF_ID. I think it will solve this particular issue. I like conservative > > approach to the verifier improvements: start with strict checking and relax it > > on case-by-case. Instead of accepting wide range of cases and cause potential > > compatibility issues. > > ok, so something like below? > > jirka > > > --- > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 832b5d7fd892..dd678b8e00b7 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -3664,6 +3664,19 @@ struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog) > } > } > > +static bool is_string_ptr(struct btf *btf, const struct btf_type *t) > +{ > + /* t comes in already as a pointer */ > + t = btf_type_by_id(btf, t->type); > + > + /* allow const */ > + if (BTF_INFO_KIND(t->info) == BTF_KIND_CONST) > + t = btf_type_by_id(btf, t->type); > + > + /* char, signed char, unsigned char */ > + return btf_type_is_int(t) && t->size == 1; > +} yep. looks like btf doesn't distinguish signedness for chars. So above is good.
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 832b5d7fd892..207ae554e0ce 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -3668,7 +3668,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type, const struct bpf_prog *prog, struct bpf_insn_access_aux *info) { - const struct btf_type *t = prog->aux->attach_func_proto; + const struct btf_type *tp, *t = prog->aux->attach_func_proto; struct bpf_prog *tgt_prog = prog->aux->linked_prog; struct btf *btf = bpf_prog_get_target_btf(prog); const char *tname = prog->aux->attach_func_name; @@ -3730,6 +3730,17 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type, */ return true; + tp = btf_type_by_id(btf, t->type); + /* skip modifiers */ + while (btf_type_is_modifier(tp)) + tp = btf_type_by_id(btf, tp->type); + + if (btf_type_is_int(tp) || btf_type_is_enum(tp)) + /* This is a pointer scalar. + * It is the same as scalar from the verifier safety pov. + */ + return true; + /* this is a pointer to another type */ info->reg_type = PTR_TO_BTF_ID;