| Message ID | 20200904112401.667645-1-lmb@cloudflare.com |
|---|---|
| Headers | show |
| Series | RFC: Make check_func_arg table driven | expand |
On Fri, Sep 04, 2020 at 12:23:50PM +0100, Lorenz Bauer wrote: > This is what happened when I got sidetracked from my work on sockmap > bpf_iter support [1]. For that I wanted to allow passing a BTF pointer > to functions expecting a PTR_TO_SOCKET. At first it wasn't at all > obvious to me how to add this to check_func_arg, so I started refactoring > the function bit by bit. This RFC series is the result of that. > > Note: this series is based on top of sockmap iterator, hence the RFC status. > > Currently, check_func_arg has this pretty gnarly if statement that > compares the valid arg_type with the actualy reg_type. Sprinkled > in-between are checks for register_is_null, to short circuit these > tests if we're dealing with a nullable arg_type. There is also some > code for later bounds / access checking hidden away in there. > > This series of patches refactors the function into something like this: > > if (reg_is_null && arg_type_is_nullable) > skip type checking > > do type checking, including BTF validation > > do bounds / access checking > > The type checking is now table driven, which makes it easy to extend > the acceptable types. Maybe more importantly, using a table makes it > easy to provide more helpful verifier output (see the last patch). > > I realise there are quite a few patches here. The most interesting > ones are #5 where I introduce a btf_id_set for each helper arg, > #10 where I simplify the nullable type checking and finally #11 > where I add the table of compatible types. > > There are some more simplifications that we could do that could get > rid of resolve_map_arg_type, but the series is already too long. > > Martin: you said that you're working on extending PTR_TO_SOCK_COMMON, > would this series help you with that? I skimmed through the set. Patch 5 to 11 are useful. It is a nice refactoring and clean up. Thanks for the work. I like the idea of moving out the logic after "if (!type_is_sk_pointer(type)) goto err_type;" and moving the null register check to the beginning. I don't think this set should depend on the sockmap iter set. I think the sockmap iter patches should depend on this set instead. For example, the changes in patch 1 of the sockmap iter patchset that moves out the "btf_struct_ids_match()" logic after the "if (!type_is_sk_pointer(type)) goto err_type;" should belong to this set.
On Wed, 9 Sep 2020 at 07:17, Martin KaFai Lau <kafai@fb.com> wrote: > > I skimmed through the set. Patch 5 to 11 are useful. It is a nice refactoring > and clean up. Thanks for the work. I like the idea of moving out the logic > after "if (!type_is_sk_pointer(type)) goto err_type;" and moving the null > register check to the beginning. Thanks! > I don't think this set should depend on the sockmap iter set. > I think the sockmap iter patches should depend on this set instead. > For example, the changes in patch 1 of the sockmap iter patchset that > moves out the "btf_struct_ids_match()" logic after the > "if (!type_is_sk_pointer(type)) goto err_type;" should belong to this set. That's what I did in the beginning. However this is a much bigger change to land than sockmap iter, and I anticipate that review will take longer. Which means it will delay the patchset that I actually need. Hence my preference to do it this way round, with the minimal changes necessary in sockmap iter.
This is what happened when I got sidetracked from my work on sockmap bpf_iter support [1]. For that I wanted to allow passing a BTF pointer to functions expecting a PTR_TO_SOCKET. At first it wasn't at all obvious to me how to add this to check_func_arg, so I started refactoring the function bit by bit. This RFC series is the result of that. Note: this series is based on top of sockmap iterator, hence the RFC status. Currently, check_func_arg has this pretty gnarly if statement that compares the valid arg_type with the actualy reg_type. Sprinkled in-between are checks for register_is_null, to short circuit these tests if we're dealing with a nullable arg_type. There is also some code for later bounds / access checking hidden away in there. This series of patches refactors the function into something like this: if (reg_is_null && arg_type_is_nullable) skip type checking do type checking, including BTF validation do bounds / access checking The type checking is now table driven, which makes it easy to extend the acceptable types. Maybe more importantly, using a table makes it easy to provide more helpful verifier output (see the last patch). I realise there are quite a few patches here. The most interesting ones are #5 where I introduce a btf_id_set for each helper arg, #10 where I simplify the nullable type checking and finally #11 where I add the table of compatible types. There are some more simplifications that we could do that could get rid of resolve_map_arg_type, but the series is already too long. Martin: you said that you're working on extending PTR_TO_SOCK_COMMON, would this series help you with that? 1: https://lore.kernel.org/bpf/20200904095904.612390-1-lmb@cloudflare.com/T/#t Lorenz Bauer (11): btf: Fix BTF_SET_START_GLOBAL macro btf: add a global set of valid BTF socket ids btf: make btf_set_contains take a const pointer bpf: check scalar or invalid register in check_helper_mem_access bpf: allow specifying a set of BTF IDs for helper arguments bpf: make reference tracking in check_func_arg generic bpf: always check access to PTR_TO_CTX regardless of arg_type bpf: set meta->raw_mode for pointers to memory closer to it's use bpf: check ARG_PTR_TO_SPINLOCK register type in check_func_arg bpf: hoist type checking for nullable arg types bpf: use a table to drive helper arg type checks include/linux/bpf.h | 25 ++- include/linux/btf_ids.h | 7 +- kernel/bpf/bpf_inode_storage.c | 8 +- kernel/bpf/btf.c | 24 +-- kernel/bpf/stackmap.c | 5 +- kernel/bpf/verifier.c | 355 ++++++++++++++++++--------------- kernel/trace/bpf_trace.c | 15 +- net/core/bpf_sk_storage.c | 10 +- net/core/filter.c | 38 ++-- net/ipv4/bpf_tcp_ca.c | 24 +-- tools/include/linux/btf_ids.h | 7 +- 11 files changed, 269 insertions(+), 249 deletions(-)