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=-14.6 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,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=no 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 C54BDC433E6 for ; Mon, 20 Jul 2020 17:03:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A4621206E9 for ; Mon, 20 Jul 2020 17:03:22 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="D1kNZ97u" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2389134AbgGTRDV (ORCPT ); Mon, 20 Jul 2020 13:03:21 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51898 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729957AbgGTRDT (ORCPT ); Mon, 20 Jul 2020 13:03:19 -0400 Received: from mail-ej1-x644.google.com (mail-ej1-x644.google.com [IPv6:2a00:1450:4864:20::644]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6E37AC0619D4 for ; Mon, 20 Jul 2020 10:03:18 -0700 (PDT) Received: by mail-ej1-x644.google.com with SMTP id y10so18847089eje.1 for ; Mon, 20 Jul 2020 10:03:18 -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=/D99yVB3Oiz5Mh00scTWz5/c04ErKfScrbcBA6SkIPw=; b=D1kNZ97uQbS+psUY/wpbNhL+V3j7kvil0De+3fNHHIbbmYkbXypwfizcVy0gp1lzE8 7PRzu+A/DJqCiXG3TFpg0CU2LqyzqlAcLEyovIpxOpRNKdKE/EdUIu5z2kXMQr928Q39 FS4Ga5tFbkn8/CP6o4go59NrA+6hN8L6Jic2QioJZzyLlPcKr0PoP434tfoWNi2AjRQC JgNnc1qxestfxBJdBYM3QGUwlDRa96HrLWyP2xLc1v6hPUO+iB07p7nxvDyVMXF+3oaY jGpI+XBdZQzcP4bCwpYBZrLCv/cGRpWAH+ITrpaSHEORxOjlOgZTjHcyXWHex5YWmq9d 8I7A== 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=/D99yVB3Oiz5Mh00scTWz5/c04ErKfScrbcBA6SkIPw=; b=YYU2WYOIBRIrQqbZxgOk0QzWW6j8ew0OOUla3eSdfrbTUPGG1WNfEKOFLMt1JZ1mhU BifCJFXjup7y7YFg7bYxqJM2Pzl+Ckb9vaImmkkOAYZhjqfDprPUp1CmNxZqagnz74e+ KTK/BGWAntLcPrdsdFGAYpSSVxJKWuyDWi69wYszuSPjcMGqv70ct+HyI11STQXoYVUR cM1T2SfpE+KEb7iIP4mH4wv1yc0sidIbblPIBxeYSiwMOyhA+swcPR/Vea8keKZJXTLQ 0UZN+Fi8L7fkRD+qagmjibPqTXixcDNBOGqHM/8jlIhjTghQ3YjS8G0677uUw0ZWidRJ TGjQ== X-Gm-Message-State: AOAM532nsVwytftmD2U1UIlhaOiRx6lAvPAq4U5eG2zBdJKuHl7xSFdt Si2crNM3houVx/yiS112AZ3C2og2XXjimEttSDxpHg== X-Google-Smtp-Source: ABdhPJzW7jeqn8/HrRf1zvDOSBhWBELyOoysxnoaonrBp2ayMlaTV+GUKzwc1g6K05h7+lORk8nrO2ZkuRfId+MHg7g= X-Received: by 2002:a17:907:10d4:: with SMTP id rv20mr22660986ejb.413.1595264596856; Mon, 20 Jul 2020 10:03:16 -0700 (PDT) MIME-Version: 1.0 References: <20200715214312.2266839-1-haoluo@google.com> <20200715214312.2266839-2-haoluo@google.com> In-Reply-To: From: Hao Luo Date: Mon, 20 Jul 2020 10:03:05 -0700 Message-ID: Subject: Re: [RFC PATCH bpf-next 1/2] bpf: BTF support for __ksym externs To: Andrii Nakryiko Cc: Networking , bpf , open list , "open list:KERNEL SELFTEST FRAMEWORK" , Shuah Khan , Alexei Starovoitov , Andrii Nakryiko , John Fastabend , Daniel Borkmann , Martin KaFai Lau , Song Liu , Yonghong Song , KP Singh , Stanislav Fomichev , Quentin Monnet Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Andrii, Thanks for taking a look at this. You comments are clear, I will fix them in v2. > Also, in the next version, please split kernel part and libbpf part > into separate patches. > Got it. Will do. > I don't think that's the right approach. It can't be the best effort. > It's actually pretty clear when a user wants a BTF-based variable with > ability to do direct memory access vs __ksym address that we have > right now: variable type info. In your patch you are only looking up > variable by name, but it needs to be more elaborate logic: > > 1. if variable type is `extern void` -- do what we do today (no BTF required) > 2. if the variable type is anything but `extern void`, then find that > variable in BTF. If no BTF or variable is not found -- hard error with > detailed enough message about what we expected to find in kernel BTF. > 3. If such a variable is found in the kernel, then might be a good > idea to additionally check type compatibility (e.g., struct/union > should match struct/union, int should match int, typedefs should get > resolved to underlying type, etc). I don't think deep comparison of > structs is right, though, due to CO-RE, so just high-level > compatibility checks to prevent the most obvious mistakes. > Ack. > > > > Also note since we need to carry the ksym's address (64bits) as well as > > its btf_id (32bits), pseudo_btf_id uses ld_imm64's both imm and off > > fields. > > For BTF-enabled ksyms, libbpf doesn't need to provide symbol address, > kernel will find it and substitute it, so BTF ID is the only > parameter. Thus it can just go into the imm field (and simplify > ldimm64 validation logic a bit). > Ack. > > /* when bpf_call->src_reg == BPF_PSEUDO_CALL, bpf_call->imm == pc-relative > > * offset to another bpf function > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 3c1efc9d08fd..3c925957b9b6 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -7131,15 +7131,29 @@ static int check_ld_imm(struct bpf_verifier_env *env, struct bpf_insn *insn) > > verbose(env, "invalid BPF_LD_IMM insn\n"); > > return -EINVAL; > > } > > + err = check_reg_arg(env, insn->dst_reg, DST_OP); > > + if (err) > > + return err; > > + > > + /* > > + * BPF_PSEUDO_BTF_ID insn's off fields carry the ksym's btf_id, so its > > + * handling has to come before the reserved field check. > > + */ > > + if (insn->src_reg == BPF_PSEUDO_BTF_ID) { > > + u32 id = ((u32)(insn + 1)->off << 16) | (u32)insn->off; > > + const struct btf_type *t = btf_type_by_id(btf_vmlinux, id); > > + > > This is the kernel, we should be paranoid and assume the hackers want > to do bad things. So check t for NULL. Check that it's actually a > BTF_KIND_VAR. Check the name, find ksym addr, etc. > Ack.