On Mon, Sep 14, 2020 at 10:26:18AM +0100, Lorenz Bauer wrote: > On Sat, 12 Sep 2020 at 05:59, Martin KaFai Lau <kafai@xxxxxx> wrote: > > > > There is a constant need to add more fields into the bpf_tcp_sock > > for the bpf programs running at tc, sock_ops...etc. > > > > A current workaround could be to use bpf_probe_read_kernel(). However, > > other than making another helper call for reading each field and missing > > CO-RE, it is also not as intuitive to use as directly reading > > "tp->lsndtime" for example. While already having perfmon cap to do > > bpf_probe_read_kernel(), it will be much easier if the bpf prog can > > directly read from the tcp_sock. > > > > This patch tries to do that by using the existing casting-helpers > > bpf_skc_to_*() whose func_proto returns a btf_id. For example, the > > func_proto of bpf_skc_to_tcp_sock returns the btf_id of the > > kernel "struct tcp_sock". > > > > [ One approach is to make a separate copy of the bpf_skc_to_* > > func_proto and use ARG_PTR_TO_SOCK_COMMON instead of ARG_PTR_TO_BTF_ID. > > More on this later (1). ] > > > > This patch modifies the existing bpf_skc_to_* func_proto to take > > ARG_PTR_TO_SOCK_COMMON instead of taking > > "ARG_PTR_TO_BTF_ID + &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]". > > That will allow tc, sock_ops,...etc to call these casting helpers > > because they already hold the PTR_TO_SOCK_COMMON (or its > > equivalent). For example: > > > > sk = sock_ops->sk; > > if (!sk) > > return; > > tp = bpf_skc_to_tcp_sock(sk); > > if (!tp) > > return; > > /* Read tp as a PTR_TO_BTF_ID */ > > lsndtime = tp->lsndtime; > > > > To ensure the current bpf prog passing a PTR_TO_BTF_ID to > > bpf_skc_to_*() still works as is, the verifier is modified such that > > ARG_PTR_TO_SOCK_COMMON can accept a reg with reg->type == PTR_TO_BTF_ID > > and reg->btf_id is btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON] > > > > To do that, an idea is borrowed from one of the Lorenz's patch: > > https://lore.kernel.org/bpf/20200904112401.667645-12-lmb@xxxxxxxxxxxxxx/ . > > It adds PTR_TO_BTF_ID as one of the acceptable reg->type for > > ARG_PTR_TO_SOCK_COMMON and also specifies what btf_id it can take. > > By doing this, the bpf_skc_to_* will work as before and can still > > take PTR_TO_BTF_ID as the arg. e.g. The bpf tcp iter will work > > as is. > > > > This will also make other existing helper taking ARG_PTR_TO_SOCK_COMMON > > works with the pointer obtained from bpf_skc_to_*(). For example: > > Unfortunately, I think that we need to introduce a new > ARG_PTR_TO_SOCK_COMMON_OR_NULL for this to work. This is because > dereferencing a "tracing" pointer can yield NULL at runtime due to a > page fault, so the helper has to deal with this. Other than that I > think this is a really nice approach: we can gradually move helpers to > PTR_TO_SOCK_COMMON_OR_NULL and in doing so make them compatible with > BTF pointers. Good point on the sk could be NULL. I think the existing bpf_skc_to_*() helper is missing the "!sk" check regardless of this patch. 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. 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". 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. 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. > > [...] > > > @@ -4014,7 +4022,17 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 arg, > > > > found: > > if (type == PTR_TO_BTF_ID) { > > - u32 *expected_btf_id = fn->arg_btf_id[arg]; > > + u32 *expected_btf_id; > > + > > + if (arg_type == ARG_PTR_TO_BTF_ID) { > > + expected_btf_id = fn->arg_btf_id[arg]; > > Personal preference, but what do you think about moving this to after > the assignment of compatible? > > btf_id = compatible->btf_id; > if (arg_type == ARG_PTR_TO_BTF_ID) > btf_id = fn->fn->arg_btf_id[arg]; > > That makes it clearer that we have to special case ARG_PTR_TO_BTF_ID since > it doesn't make sense to use compatible->btf_id in that case. Sure. I don't mind moving this assignment out of the else. However, I think resorting to "compatible" btf_id is the exception instead.
