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. [...] > @@ -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. -- Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com
