From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-17.4 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2F67AC433DF for ; Sat, 22 Aug 2020 07:04:44 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id F13BA20724 for ; Sat, 22 Aug 2020 07:04:43 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="AtiHmTov" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726535AbgHVHEm (ORCPT ); Sat, 22 Aug 2020 03:04:42 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56538 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725864AbgHVHEl (ORCPT ); Sat, 22 Aug 2020 03:04:41 -0400 Received: from mail-ed1-x542.google.com (mail-ed1-x542.google.com [IPv6:2a00:1450:4864:20::542]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9F9F5C061573 for ; Sat, 22 Aug 2020 00:04:40 -0700 (PDT) Received: by mail-ed1-x542.google.com with SMTP id l63so368577edl.9 for ; Sat, 22 Aug 2020 00:04:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=fwfunudANjOoTZXdPkqCPqeCMIZCPPl9JuhA1bl5pBw=; b=AtiHmTovOFzL0yalRrqMCdj2asvK/1cxQIlCX37p5Ym1vHLaK5/uyoXDO5PV0Cynn4 dUM/RyqHAU8i3IVnpbxyAy5dQVFN3g1WwEgsnZVzWq2wHi8GnA8hZpnKmhaHNY1HPo7I NFWTu98Pw2ehMm3Mg9FVtUzRJ8xGKoxA8Cto7R84r8AZJDA4CVjbhQHT8pSBZ0sVyoEx P3xULHzA0ZOWuFJmDb5bDK2gJCPGMIn+ihEgGD4GwaiL75StGkLVdp9+E8TrJT4jSIF+ WslnbwaP1YImU7zqbBh5iEYHAe5d1z08fGQSgSaPSylBFep6M6IalNYNsDlgFB5hK0MK ncYA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=fwfunudANjOoTZXdPkqCPqeCMIZCPPl9JuhA1bl5pBw=; b=BBBBUcrpO40BgBqj4mTC3KxKoScJqg8UxsTimdhXSc7SnlqzO8lzxdTPywMvxbjtQx jSC2S3C3JowhsfGpLw0qA0ngXB6lKYY88TgQD8iWpXUfZ0CteMdyCsZzspp9tFFhaj48 JlOUkDcvFbjP36IuBTmPOEo9Avoa/D23+cyM0plc87XXYxkVYXp2aq5zyasouHr22B+1 3IxByFDlQt5+Xnco7pV/BQVbyJUPxnt6s6uBIBHidxWmJTVqaGtOKdlZ/EEVVywsX+g0 SN3eK9FU3RdWLwjKiLxhpdgaGyqIXCrqrhXInh6y1JzrSk9VXwNBG0TpGFb8++T+Nswj 0riw== X-Gm-Message-State: AOAM533aaLo7aICqmpEM+YAUcFBD2xby6RMKcR9W1UYWuRJEf8KeF2ki yuEkiXjW/D0f7oysNv2K6pR+Fr6iCDbc+hALKkBlhA== X-Google-Smtp-Source: ABdhPJxYY8CRHDb7qzuMzF83Z04DfNrYhMMudL5DibJuxlaN0+NROigEgLH3AQp37MMKbZJaEeSrr6V8kf9TiBhN0P4= X-Received: by 2002:a05:6402:30a5:: with SMTP id df5mr6082918edb.18.1598079878906; Sat, 22 Aug 2020 00:04:38 -0700 (PDT) MIME-Version: 1.0 References: <20200819224030.1615203-1-haoluo@google.com> <20200819224030.1615203-4-haoluo@google.com> In-Reply-To: From: Hao Luo Date: Sat, 22 Aug 2020 00:04:27 -0700 Message-ID: Subject: Re: [PATCH bpf-next v1 3/8] bpf: Introduce help function to validate ksym's type. To: Andrii Nakryiko Cc: Yonghong Song , Networking , bpf , open list , "open list:KERNEL SELFTEST FRAMEWORK" , Shuah Khan , Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , Martin KaFai Lau , Song Liu , John Fastabend , KP Singh , Quentin Monnet , Steven Rostedt , Ingo Molnar , Andrey Ignatov , Jakub Sitnicki Content-Type: text/plain; charset="UTF-8" Sender: bpf-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org Ah, I see bpf_core_types_are_compat() after sync'ing my local repo. It seems the perfect fit for my use case. I only found the btf_equal_xxx() defined in btf.c when posting these patches. I can test and use bpf_core_types_are_compat() in v2. Thanks for pointing it out and explaining the public APIs. Hao On Fri, Aug 21, 2020 at 7:43 PM Andrii Nakryiko wrote: > > On Fri, Aug 21, 2020 at 5:43 PM Hao Luo wrote: > > > > On Fri, Aug 21, 2020 at 2:50 PM Andrii Nakryiko > > wrote: > > > > > > On Thu, Aug 20, 2020 at 10:22 AM Yonghong Song wrote: > > > > > > > > > > > > > > > > On 8/19/20 3:40 PM, Hao Luo wrote: > > > > > For a ksym to be safely dereferenced and accessed, its type defined in > > > > > bpf program should basically match its type defined in kernel. Implement > > > > > a help function for a quick matching, which is used by libbpf when > > > > > resolving the kernel btf_id of a ksym. > > > > > > > > > > Signed-off-by: Hao Luo > > > > > --- > > [...] > > > > > +/* > > > > > + * Match a ksym's type defined in bpf programs against its type encoded in > > > > > + * kernel btf. > > > > > + */ > > > > > +bool btf_ksym_type_match(const struct btf *ba, __u32 id_a, > > > > > + const struct btf *bb, __u32 id_b) > > > > > +{ > > > > > > [...] > > > > > > > > + } > > > > > + } > > > > > > > > I am wondering whether this is too strict and how this can co-work with > > > > CO-RE. Forcing users to write almost identical structure definition to > > > > the underlying kernel will not be user friendly and may not work cross > > > > kernel versions even if the field user cares have not changed. > > > > > > > > Maybe we can relax the constraint here. You can look at existing > > > > libbpf CO-RE code. > > > > > > Right. Hao, can you just re-use bpf_core_types_are_compat() instead? > > > See if semantics makes sense, but I think it should. BPF CO-RE has > > > been permissive in terms of struct size and few other type aspects, > > > because it handles relocations so well. This approach allows to not > > > have to exactly match all possible variations of some struct > > > definition, which is a big problem with ever-changing kernel data > > > structures. > > > > > > > I have to say I hate myself writing another type comparison instead of > > reusing the existing one. The issue is that when bpf_core_types_compat > > compares names, it uses t1->name_off == t2->name_off. It is also used > > Huh? Are we talking about the same bpf_core_types_are_compat() (there > is no bpf_core_types_compat, I think it's a typo)? > bpf_core_types_are_compat() doesn't even compare any name, so I'm not > sure what you are talking about. Some of btf_dedup functions do string > comparisons using name_off directly, but that's a special and very > careful case, it's not relevant here. > > > > in bpf_equal_common(). In my case, because these types are from two > > different BTFs, their name_off are not expected to be the same, right? > > I didn't find a good solution to refactor before posting this patch. I > > bpf_core_types_are_compat() didn't land until this week, so you must > be confusing something. Please take another look. > > > think I can adapt bpf_core_type_compat() and pay more attention to > > CO-RE. > > > > > > > > > > > + break; > > > > > + } > > > > > > [...] > > > > > > > > + > > > > > struct btf_ext_sec_setup_param { > > > > > __u32 off; > > > > > __u32 len; > > > > > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h > > > > > index 91f0ad0e0325..5ef220e52485 100644 > > > > > --- a/tools/lib/bpf/btf.h > > > > > +++ b/tools/lib/bpf/btf.h > > > > > @@ -52,6 +52,8 @@ LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name, > > > > > __u32 expected_key_size, > > > > > __u32 expected_value_size, > > > > > __u32 *key_type_id, __u32 *value_type_id); > > > > > +LIBBPF_API bool btf_ksym_type_match(const struct btf *ba, __u32 id_a, > > > > > + const struct btf *bb, __u32 id_b); > > > > > > > > > > LIBBPF_API struct btf_ext *btf_ext__new(__u8 *data, __u32 size); > > > > > LIBBPF_API void btf_ext__free(struct btf_ext *btf_ext); > > > > > > > > The new API function should be added to libbpf.map. > > > > > > My question is why does this even have to be a public API? > > > > I can fix. Please pardon my ignorance, what is the difference between > > public and internal APIs? I wasn't sure, so used it improperly. > > public APIs are those that users of libbpf are supposed to use, > internal one is just for libbpf internal use. The former can't change, > the latter can be refactor as much as we need to. > > > > > Thanks, > > Hao