| Message ID | 20200722211223.1055107-8-jolsa@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Add d_path helper | expand |
On Wed, Jul 22, 2020 at 2:13 PM Jiri Olsa <jolsa@kernel.org> wrote: > > Adding btf_struct_ids_match function to check if given address provided > by BTF object + offset is also address of another nested BTF object. > > This allows to pass an argument to helper, which is defined via parent > BTF object + offset, like for bpf_d_path (added in following changes): > > SEC("fentry/filp_close") > int BPF_PROG(prog_close, struct file *file, void *id) > { > ... > ret = bpf_d_path(&file->f_path, ... > > The first bpf_d_path argument is hold by verifier as BTF file object > plus offset of f_path member. > > The btf_struct_ids_match function will walk the struct file object and > check if there's nested struct path object on the given offset. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > include/linux/bpf.h | 2 ++ > kernel/bpf/btf.c | 29 +++++++++++++++++++++++++++++ > kernel/bpf/verifier.c | 18 ++++++++++++------ > 3 files changed, 43 insertions(+), 6 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index bae557ff2da8..c981e258fed3 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1306,6 +1306,8 @@ int btf_struct_access(struct bpf_verifier_log *log, > const struct btf_type *t, int off, int size, > enum bpf_access_type atype, > u32 *next_btf_id); > +bool btf_struct_ids_match(struct bpf_verifier_log *log, > + int off, u32 id, u32 mid); > int btf_resolve_helper_id(struct bpf_verifier_log *log, > const struct bpf_func_proto *fn, int); > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 1ab5fd5bf992..562d4453fad3 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -4140,6 +4140,35 @@ int btf_struct_access(struct bpf_verifier_log *log, > return -EINVAL; > } > > +bool btf_struct_ids_match(struct bpf_verifier_log *log, > + int off, u32 id, u32 mid) > +{ > + const struct btf_type *type; > + u32 nid; > + int err; > + mid and nid are terrible names, especially as an input argument name. mid == need_type_id? nid == cur_type_id or something along those lines? > + do { > + type = btf_type_by_id(btf_vmlinux, id); > + if (!type) > + return false; > + err = btf_struct_walk(log, type, off, 1, &nid); > + if (err < 0) > + return false; > + > + /* We found nested struct object. If it matches > + * the requested ID, we're done. Otherwise let's > + * continue the search with offset 0 in the new > + * type. > + */ > + if (err == walk_struct && mid == nid) > + return true; > + off = 0; > + id = nid; > + } while (err == walk_struct); This seems like a slightly more obvious control flow: again: ... if (err != walk_struct) return false; if (mid != nid) { off = 0; id = nid; goto again; } return true; > + > + return false; > +} > + > int btf_resolve_helper_id(struct bpf_verifier_log *log, > const struct bpf_func_proto *fn, int arg) > { [...]
On Tue, Jul 28, 2020 at 04:35:16PM -0700, Andrii Nakryiko wrote: SNIP > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index bae557ff2da8..c981e258fed3 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -1306,6 +1306,8 @@ int btf_struct_access(struct bpf_verifier_log *log, > > const struct btf_type *t, int off, int size, > > enum bpf_access_type atype, > > u32 *next_btf_id); > > +bool btf_struct_ids_match(struct bpf_verifier_log *log, > > + int off, u32 id, u32 mid); > > int btf_resolve_helper_id(struct bpf_verifier_log *log, > > const struct bpf_func_proto *fn, int); > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > index 1ab5fd5bf992..562d4453fad3 100644 > > --- a/kernel/bpf/btf.c > > +++ b/kernel/bpf/btf.c > > @@ -4140,6 +4140,35 @@ int btf_struct_access(struct bpf_verifier_log *log, > > return -EINVAL; > > } > > > > +bool btf_struct_ids_match(struct bpf_verifier_log *log, > > + int off, u32 id, u32 mid) > > +{ > > + const struct btf_type *type; > > + u32 nid; > > + int err; > > + > > mid and nid are terrible names, especially as an input argument name. > mid == need_type_id? nid == cur_type_id or something along those > lines? 'mid' was for matching id, 'nid' for nested id ;-) need_type_id/cur_type_id sound good > > > + do { > > + type = btf_type_by_id(btf_vmlinux, id); > > + if (!type) > > + return false; > > + err = btf_struct_walk(log, type, off, 1, &nid); > > + if (err < 0) > > + return false; > > + > > + /* We found nested struct object. If it matches > > + * the requested ID, we're done. Otherwise let's > > + * continue the search with offset 0 in the new > > + * type. > > + */ > > + if (err == walk_struct && mid == nid) > > + return true; > > + off = 0; > > + id = nid; > > + } while (err == walk_struct); > > This seems like a slightly more obvious control flow: > > again: > > ... > > if (err != walk_struct) > return false; ok, and perhaps use in here the switch(err) as in the previous patch? thanks, jirka > > if (mid != nid) { > off = 0; > id = nid; > goto again; > } > > return true; > > > + > > + return false; > > +} > > + > > int btf_resolve_helper_id(struct bpf_verifier_log *log, > > const struct bpf_func_proto *fn, int arg) > > { > > [...] >
On Wed, Jul 29, 2020 at 9:04 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Tue, Jul 28, 2020 at 04:35:16PM -0700, Andrii Nakryiko wrote: > > SNIP > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > index bae557ff2da8..c981e258fed3 100644 > > > --- a/include/linux/bpf.h > > > +++ b/include/linux/bpf.h > > > @@ -1306,6 +1306,8 @@ int btf_struct_access(struct bpf_verifier_log *log, > > > const struct btf_type *t, int off, int size, > > > enum bpf_access_type atype, > > > u32 *next_btf_id); > > > +bool btf_struct_ids_match(struct bpf_verifier_log *log, > > > + int off, u32 id, u32 mid); > > > int btf_resolve_helper_id(struct bpf_verifier_log *log, > > > const struct bpf_func_proto *fn, int); > > > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > > index 1ab5fd5bf992..562d4453fad3 100644 > > > --- a/kernel/bpf/btf.c > > > +++ b/kernel/bpf/btf.c > > > @@ -4140,6 +4140,35 @@ int btf_struct_access(struct bpf_verifier_log *log, > > > return -EINVAL; > > > } > > > > > > +bool btf_struct_ids_match(struct bpf_verifier_log *log, > > > + int off, u32 id, u32 mid) just realized that if id == mid and off == 0, btf_struct_ids_match() will return false. Right now verifier is careful to not call btf_struct_ids_match in such case, but I wonder if it's better to make that (common) case also work? > > > +{ > > > + const struct btf_type *type; > > > + u32 nid; > > > + int err; > > > + > > > > mid and nid are terrible names, especially as an input argument name. > > mid == need_type_id? nid == cur_type_id or something along those > > lines? > > 'mid' was for matching id, 'nid' for nested id ;-) > need_type_id/cur_type_id sound good nested I guessed, mid was a mystery to me :)) > > > > > > + do { > > > + type = btf_type_by_id(btf_vmlinux, id); > > > + if (!type) > > > + return false; > > > + err = btf_struct_walk(log, type, off, 1, &nid); > > > + if (err < 0) > > > + return false; > > > + > > > + /* We found nested struct object. If it matches > > > + * the requested ID, we're done. Otherwise let's > > > + * continue the search with offset 0 in the new > > > + * type. > > > + */ > > > + if (err == walk_struct && mid == nid) > > > + return true; > > > + off = 0; > > > + id = nid; > > > + } while (err == walk_struct); > > > > This seems like a slightly more obvious control flow: > > > > again: > > > > ... > > > > if (err != walk_struct) > > return false; > > ok, and perhaps use in here the switch(err) as in the previous patch? I think straightforward if is better than switch here, because anything but walk_struct is not what we expect. > > thanks, > jirka > > > > > if (mid != nid) { > > off = 0; > > id = nid; > > goto again; > > } > > > > return true; > > > > > + > > > + return false; > > > +} > > > + > > > int btf_resolve_helper_id(struct bpf_verifier_log *log, > > > const struct bpf_func_proto *fn, int arg) > > > { > > > > [...] > > >
On Wed, Jul 29, 2020 at 10:51:26AM -0700, Andrii Nakryiko wrote: > On Wed, Jul 29, 2020 at 9:04 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Tue, Jul 28, 2020 at 04:35:16PM -0700, Andrii Nakryiko wrote: > > > > SNIP > > > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > > index bae557ff2da8..c981e258fed3 100644 > > > > --- a/include/linux/bpf.h > > > > +++ b/include/linux/bpf.h > > > > @@ -1306,6 +1306,8 @@ int btf_struct_access(struct bpf_verifier_log *log, > > > > const struct btf_type *t, int off, int size, > > > > enum bpf_access_type atype, > > > > u32 *next_btf_id); > > > > +bool btf_struct_ids_match(struct bpf_verifier_log *log, > > > > + int off, u32 id, u32 mid); > > > > int btf_resolve_helper_id(struct bpf_verifier_log *log, > > > > const struct bpf_func_proto *fn, int); > > > > > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > > > index 1ab5fd5bf992..562d4453fad3 100644 > > > > --- a/kernel/bpf/btf.c > > > > +++ b/kernel/bpf/btf.c > > > > @@ -4140,6 +4140,35 @@ int btf_struct_access(struct bpf_verifier_log *log, > > > > return -EINVAL; > > > > } > > > > > > > > +bool btf_struct_ids_match(struct bpf_verifier_log *log, > > > > + int off, u32 id, u32 mid) > > just realized that if id == mid and off == 0, btf_struct_ids_match() > will return false. Right now verifier is careful to not call > btf_struct_ids_match in such case, but I wonder if it's better to make > that (common) case also work? right, also we should call btf_struct_ids_match when IDs are equal and off != 0, which we don't do now jirka
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index bae557ff2da8..c981e258fed3 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1306,6 +1306,8 @@ int btf_struct_access(struct bpf_verifier_log *log, const struct btf_type *t, int off, int size, enum bpf_access_type atype, u32 *next_btf_id); +bool btf_struct_ids_match(struct bpf_verifier_log *log, + int off, u32 id, u32 mid); int btf_resolve_helper_id(struct bpf_verifier_log *log, const struct bpf_func_proto *fn, int); diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 1ab5fd5bf992..562d4453fad3 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -4140,6 +4140,35 @@ int btf_struct_access(struct bpf_verifier_log *log, return -EINVAL; } +bool btf_struct_ids_match(struct bpf_verifier_log *log, + int off, u32 id, u32 mid) +{ + const struct btf_type *type; + u32 nid; + int err; + + do { + type = btf_type_by_id(btf_vmlinux, id); + if (!type) + return false; + err = btf_struct_walk(log, type, off, 1, &nid); + if (err < 0) + return false; + + /* We found nested struct object. If it matches + * the requested ID, we're done. Otherwise let's + * continue the search with offset 0 in the new + * type. + */ + if (err == walk_struct && mid == nid) + return true; + off = 0; + id = nid; + } while (err == walk_struct); + + return false; +} + int btf_resolve_helper_id(struct bpf_verifier_log *log, const struct bpf_func_proto *fn, int arg) { diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 9a6703bc3f36..39922fa07154 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -3887,16 +3887,21 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, goto err_type; } } else if (arg_type == ARG_PTR_TO_BTF_ID) { + bool ids_match = false; + expected_type = PTR_TO_BTF_ID; if (type != expected_type) goto err_type; if (!fn->check_btf_id) { if (reg->btf_id != meta->btf_id) { - verbose(env, "Helper has type %s got %s in R%d\n", - kernel_type_name(meta->btf_id), - kernel_type_name(reg->btf_id), regno); - - return -EACCES; + ids_match = btf_struct_ids_match(&env->log, reg->off, reg->btf_id, + meta->btf_id); + if (!ids_match) { + verbose(env, "Helper has type %s got %s in R%d\n", + kernel_type_name(meta->btf_id), + kernel_type_name(reg->btf_id), regno); + return -EACCES; + } } } else if (!fn->check_btf_id(reg->btf_id, arg)) { verbose(env, "Helper does not support %s in R%d\n", @@ -3904,7 +3909,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, return -EACCES; } - if (!tnum_is_const(reg->var_off) || reg->var_off.value || reg->off) { + if (!ids_match && + (!tnum_is_const(reg->var_off) || reg->var_off.value || reg->off)) { verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n", regno); return -EACCES;
Adding btf_struct_ids_match function to check if given address provided by BTF object + offset is also address of another nested BTF object. This allows to pass an argument to helper, which is defined via parent BTF object + offset, like for bpf_d_path (added in following changes): SEC("fentry/filp_close") int BPF_PROG(prog_close, struct file *file, void *id) { ... ret = bpf_d_path(&file->f_path, ... The first bpf_d_path argument is hold by verifier as BTF file object plus offset of f_path member. The btf_struct_ids_match function will walk the struct file object and check if there's nested struct path object on the given offset. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- include/linux/bpf.h | 2 ++ kernel/bpf/btf.c | 29 +++++++++++++++++++++++++++++ kernel/bpf/verifier.c | 18 ++++++++++++------ 3 files changed, 43 insertions(+), 6 deletions(-)