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.
