Re: [PATCH 1/5] bpf: Allow non struct type for btf ctx access
From: Yonghong Song <hidden>
Date: 2020-01-07 17:55:43
Also in:
bpf
On 1/7/20 7:50 AM, Jiri Olsa wrote:
On Tue, Jan 07, 2020 at 01:13:23PM +0100, Jiri Olsa wrote:quoted
On Mon, Jan 06, 2020 at 09:36:17PM +0000, Yonghong Song wrote:quoted
On 12/29/19 6:37 AM, Jiri Olsa wrote:quoted
I'm not sure why the restriction was added, but I can't access pointers to POD types like const char * when probing vfs_read function. Removing the check and allow non struct type access in context. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- kernel/bpf/btf.c | 6 ------ 1 file changed, 6 deletions(-)diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index ed2075884724..ae90f60ac1b8 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c@@ -3712,12 +3712,6 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type, /* skip modifiers */ while (btf_type_is_modifier(t)) t = btf_type_by_id(btf, t->type); - if (!btf_type_is_struct(t)) { - bpf_log(log, - "func '%s' arg%d type %s is not a struct\n", - tname, arg, btf_kind_str[BTF_INFO_KIND(t->info)]); - return false; - }Hi, Jiri, the RFC looks great! Especially, you also referenced this will give great performance boost for bcc scripts. Could you provide more context on why the above change is needed? The function btf_ctx_access is used to check validity of accessing function parameters which are wrapped inside a structure, I am wondering what kinds of accesses you tried to address here.when I was transforming opensnoop.py to use this I got fail in there when I tried to access filename arg in do_sys_open but actualy it seems this should get recognized earlier by: if (btf_type_is_int(t)) /* accessing a scalar */ return true; I'm not sure why it did not pass for const char*, I'll checkit seems we don't check for pointer to scalar (just void), which is the case in my example 'const char *filename'
Thanks for clarification. See some comments below.
quoted hunk ↗ jump to hunk
I'll post this in v2 with other changes jirka ---diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index ed2075884724..650df4ed346e 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c@@ -3633,7 +3633,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;@@ -3695,6 +3695,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)) + /* This is a pointer scalar. + * It is the same as scalar from the verifier safety pov. + */ + return true;
This should work since:
- the int pointer will be treated as a scalar later on
- bpf_probe_read() will be used to read the contents
I am wondering whether we should add proper verifier support
to allow pointer to int ctx access. There, users do not need
to use bpf_probe_read() to dereference the pointer.
Discussed with Martin, maybe somewhere in check_ptr_to_btf_access(),
before btf_struct_access(), checking if it is a pointer to int/enum,
it should just allow and return SCALAR_VALUE.
If you do verifier changes, please ensure bpf_probe_read() is not
needed any more. In bcc, you need to hack to prevent rewriter to
re-introduce bpf_probe_read() :-).
+ /* this is a pointer to another type */ info->reg_type = PTR_TO_BTF_ID; info->btf_id = t->type;