Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200715214312.2266839-1-haoluo@google.com> <20200715214312.2266839-2-haoluo@google.com>
 <CAEf4BzZ5A+uMPFEmgom+0x+jju3JgTLXuuy=QB_dm2Skf--5Dg@mail.gmail.com>
In-Reply-To: <CAEf4BzZ5A+uMPFEmgom+0x+jju3JgTLXuuy=QB_dm2Skf--5Dg@mail.gmail.com>
From:   Hao Luo <haoluo@google.com>
Date:   Mon, 20 Jul 2020 10:03:05 -0700
Message-ID: <CA+khW7h1HBmV5LdALswF2d2q9cD_EU1CfbBEogdHszbHLnTVAQ@mail.gmail.com>
Subject: Re: [RFC PATCH bpf-next 1/2] bpf: BTF support for __ksym externs
To:     Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc:     Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>, Shuah Khan <shuah@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Andrii Nakryiko <andriin@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        KP Singh <kpsingh@chromium.org>,
        Stanislav Fomichev <sdf@google.com>,
        Quentin Monnet <quentin@isovalent.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.