On Mon, 14 Sep 2020 at 20:43, Martin KaFai Lau <kafai@xxxxxx> wrote: [...] > > For other ARG_PTR_TO_SOCK_COMMON helpers, they are not available to > the tracing prog type. Hence, they are fine to accept PTR_TO_BTF_ID > as ARG_PTR_TO_SOCK_COMMON since the only way for non tracing prog to > get a PTR_TO_BTF_ID is from casting helpers bpf_skc_to_* and > the NULL check on return value must be done first. If these > ARG_PTR_TO_* helpers were ever made available to tracing prog, > it might be better off to have another func_proto taking > ARG_PTR_TO_BTF_ID instead. I think such special cases increase the maintenance burden going forward, I'd prefer it if we had one set of rules that applies to all program types. > For the verifier, I think the PTR_TO_BTF_ID should only be accepted > as ARG_TO_* for non tracing program. That means the bpf_skc_to_* > proto has to be duplicated to take ARG_PTR_TO_SOCK_COMMON. I think > that may be cleaner going forward. Then the verifier does not need > to worry about how to deal with what btf_id can be taken as fullsock > ARG_PTR_TO_SOCKET. The helper taking ARG_PTR_TO_BTF_ID will decide > where it could be called from and see how it wants to treat > "struct sock *sk". So basically, we allow function prototypes to be specialised according to context type? > For example, the sk_storage_get_btf_proto > is taking &btf_sock_ids[BTF_SOCK_TYPE_SOCK] and is only used from > the LSM context that is holding a fullsock. That is a tempting simplification, but it makes future extensions of LSM context harder. Also, how could we enforce that we only have fullsocks in LSM? By looking at the list of helpers? I worry that this is very brittle. > > The same goes for the sock_map iter, how about the map_update > and map_lookup use a ARG_PTR_TO_BTF_ID and PTR_TO_BTF_ID instead? > For other prog types, they can keep using ARG_PTR_TO_SOCKET and > PTR_TO_SOCKET. Yeah, I've thought about that approach as well. The upside is that it's a much more limited change, and therefore I know that it is fairly safe to do. It relies on maps being able to override the "common" map_lookup_elem function proto, which is something we already do for sockmap and which could use some cleaning up. So if PTR_TO_BTF_ID aliasing with ARG_PTR_TO_SOCK_COMMON doesn't work out I'll propose this. The downside of specialised function protos is that we'll have to do it for every helper, context type, etc. To me it sounds like we want to use BTF to define context objects going forward as much as possible, so having a general solution to unify socket helpers across old-style and BTF contexts seems really useful to me. If we introduce ARG_PTR_TO_SOCK_COMMON_OR_NULL we can do this in a gradual way, by changing helpers that we want to be cross-compatible to take the new arg type and adding a few NULL and fullsock checks here and there. -- Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com
