On Mon, Jun 22, 2020 at 5:38 PM Yonghong Song <yhs@xxxxxx> wrote:
>
> The helper is used in tracing programs to cast a socket
> pointer to a tcp6_sock pointer.
> The return value could be NULL if the casting is illegal.
>
> A new helper return type RET_PTR_TO_BTF_ID_OR_NULL is added
> so the verifier is able to deduce proper return types for the helper.
>
> Different from the previous BTF_ID based helpers,
> the bpf_skc_to_tcp6_sock() argument can be several possible
> btf_ids. More specifically, all possible socket data structures
> with sock_common appearing in the first in the memory layout.
> This patch only added socket types related to tcp and udp.
>
> All possible argument btf_id and return value btf_id
> for helper bpf_skc_to_tcp6_sock() are pre-calculcated and
> cached. In the future, it is even possible to precompute
> these btf_id's at kernel build time.
>
> Acked-by: Martin KaFai Lau <kafai@xxxxxx>
> Signed-off-by: Yonghong Song <yhs@xxxxxx>
> ---
Looks good to me as is, but see a few suggestions, they will probably
save me time at some point as well :)
Acked-by: Andrii Nakryiko <andriin@xxxxxx>
> include/linux/bpf.h | 12 +++++
> include/uapi/linux/bpf.h | 9 +++-
> kernel/bpf/btf.c | 1 +
> kernel/bpf/verifier.c | 43 +++++++++++++-----
> kernel/trace/bpf_trace.c | 2 +
> net/core/filter.c | 80 ++++++++++++++++++++++++++++++++++
> scripts/bpf_helpers_doc.py | 2 +
> tools/include/uapi/linux/bpf.h | 9 +++-
> 8 files changed, 146 insertions(+), 12 deletions(-)
>
[...]
> @@ -4815,6 +4826,18 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
> regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
> regs[BPF_REG_0].id = ++env->id_gen;
> regs[BPF_REG_0].mem_size = meta.mem_size;
> + } else if (fn->ret_type == RET_PTR_TO_BTF_ID_OR_NULL) {
> + int ret_btf_id;
> +
> + mark_reg_known_zero(env, regs, BPF_REG_0);
> + regs[BPF_REG_0].type = PTR_TO_BTF_ID_OR_NULL;
> + ret_btf_id = *fn->ret_btf_id;
might be a good idea to check fb->ret_btf_id for NULL and print a
warning + return -EFAULT. It's not supposed to happen on properly
configured kernel, but during development this will save a bunch of
time and frustration for next person trying to add something with
RET_PTR_TO_BTF_ID_OR_NULL.
> + if (ret_btf_id == 0) {
This also has to be struct/union (after typedef/mods stripping, of
course). Or are there other cases?
> + verbose(env, "invalid return type %d of func %s#%d\n",
> + fn->ret_type, func_id_name(func_id), func_id);
> + return -EINVAL;
> + }
> + regs[BPF_REG_0].btf_id = ret_btf_id;
> } else {
> verbose(env, "unknown return type %d of func %s#%d\n",
> fn->ret_type, func_id_name(func_id), func_id);
[...]
> +void init_btf_sock_ids(struct btf *btf)
> +{
> + int i, btf_id;
> +
> + for (i = 0; i < MAX_BTF_SOCK_TYPE; i++) {
> + btf_id = btf_find_by_name_kind(btf, bpf_sock_types[i],
> + BTF_KIND_STRUCT);
> + if (btf_id > 0)
> + btf_sock_ids[i] = btf_id;
> + }
> +}
This will hopefully go away with Jiri's work on static BTF IDs, right?
So looking forward to that :)
> +
> +static bool check_arg_btf_id(u32 btf_id, u32 arg)
> +{
> + int i;
> +
> + /* only one argument, no need to check arg */
> + for (i = 0; i < MAX_BTF_SOCK_TYPE; i++)
> + if (btf_sock_ids[i] == btf_id)
> + return true;
> + return false;
> +}
> +
[...]
