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=unavailable 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 C5A3CC433E2 for ; Mon, 14 Sep 2020 05:05:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 724B7208DB for ; Mon, 14 Sep 2020 05:05:04 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="KmOcYLCN" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726033AbgINFFB (ORCPT ); Mon, 14 Sep 2020 01:05:01 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35382 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725983AbgINFE4 (ORCPT ); Mon, 14 Sep 2020 01:04:56 -0400 Received: from mail-ed1-x543.google.com (mail-ed1-x543.google.com [IPv6:2a00:1450:4864:20::543]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DA94EC06178A for ; Sun, 13 Sep 2020 22:04:55 -0700 (PDT) Received: by mail-ed1-x543.google.com with SMTP id c8so16226223edv.5 for ; Sun, 13 Sep 2020 22:04:55 -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=Qw29AdXtCYZRa8G3I3RQgCCnOcHR3ivBh4i+1lH4G8A=; b=KmOcYLCNEj8MDt2SlHWzXxvKI5SNE6/iVdmXJePY2lFlJcMbJFQ5uAqQF5dppIdFN6 6MoWXQGDSpynqFq837oenEx5Wk2hEzAuDwMgHfcPbEBFDV9K7S3pBx1liT33N2SCYM4H agoYwSqNxOM3ekeAtL76nES5g5I45mZKPQ9dnFvhbYbyuqxO2Nxm41DRbQ0HhHF1spwP t0mpCRAajJwTWjR1FaJ6EQzwdUrVmfXHU40IZ7TMTG4d3nqMRrlwA7YFhYuKepa0f0eh sKbAzW23vIfeIFGefD5Blgv/wsF+u+3AMizvwZWwPaDOxQDB2Ni6xTL5X4QjFt2J0SCT Yy2w== 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=Qw29AdXtCYZRa8G3I3RQgCCnOcHR3ivBh4i+1lH4G8A=; b=S1IRxk+bKCbWvZxN51853taBOGFz1MJHOOyGOwDmvj1LzumpgBEB0gX1oYaH9RsENj vBDweH+USAXAG06XrTP80Es3s6xGekBklfMc9ojHVVIErl8p+6STMedTdD+W1/9DEeKd Wqx1XB5rCYixo7p25NGXAvF7IWilPAD0IhglFQyghDSyUfKrpIN0fbnhtYXfWGuVT54d 0iDabSI9lh4HlrjbyaQE3GGgMVwWHmNXkT/lBKfRC++4FNivf3D/fj5r7kuArxvS3BPL ACOjquZOphQ2NCjPgW5l0+fxV+6fzcftIKowVivDDRBKwTQX1gkrBiCX1/Yk8Nrd5O3f urUQ== X-Gm-Message-State: AOAM5325k1/qIUIvvt1DlPMgvjtTJFU45ny0O2/KXhkJVOMpopTpv+/H gQPvrXducQr9rwFQ+970uLhBzGG9j3DTghQQDaMM7Q== X-Google-Smtp-Source: ABdhPJylxkCeLNpusq7Kv7ORjJaYhNOus9fuu//uDY4QHsj0OCxPfeDDfiLhoChbm0j82d3PuxI5tlh4lc49+YOVygQ= X-Received: by 2002:aa7:dc18:: with SMTP id b24mr14613834edu.285.1600059893857; Sun, 13 Sep 2020 22:04:53 -0700 (PDT) MIME-Version: 1.0 References: <20200903223332.881541-1-haoluo@google.com> <20200903223332.881541-6-haoluo@google.com> In-Reply-To: From: Hao Luo Date: Sun, 13 Sep 2020 22:04:42 -0700 Message-ID: Subject: Re: [PATCH bpf-next v2 5/6] bpf: Introduce bpf_this_cpu_ptr() To: Andrii Nakryiko Cc: Networking , bpf , open list , "open list:KERNEL SELFTEST FRAMEWORK" , Shuah Khan , Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , Martin KaFai Lau , Song Liu , Yonghong Song , John Fastabend , KP Singh , Quentin Monnet , Steven Rostedt , Ingo Molnar , Andrey Ignatov , Jakub Sitnicki 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 Thanks for taking a look! On Fri, Sep 4, 2020 at 1:09 PM Andrii Nakryiko wrote: > > On Thu, Sep 3, 2020 at 3:35 PM Hao Luo wrote: > > > > Add bpf_this_cpu_ptr() to help access percpu var on this cpu. This > > helper always returns a valid pointer, therefore no need to check > > returned value for NULL. Also note that all programs run with > > preemption disabled, which means that the returned pointer is stable > > during all the execution of the program. > > > > Signed-off-by: Hao Luo > > --- > > looks good, few small things, but otherwise: > > Acked-by: Andrii Nakryiko > [...] > > > > /* eBPF function prototype used by verifier to allow BPF_CALLs from eBPF programs > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index d0ec94d5bdbf..e7ca91c697ed 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -3612,6 +3612,19 @@ union bpf_attr { > > * bpf_per_cpu_ptr() must check the returned value. > > * Return > > * A generic pointer pointing to the kernel percpu variable on *cpu*. > > + * > > + * void *bpf_this_cpu_ptr(const void *percpu_ptr) > > + * Description > > + * Take a pointer to a percpu ksym, *percpu_ptr*, and return a > > + * pointer to the percpu kernel variable on this cpu. See the > > + * description of 'ksym' in **bpf_per_cpu_ptr**\ (). > > + * > > + * bpf_this_cpu_ptr() has the same semantic as this_cpu_ptr() in > > + * the kernel. Different from **bpf_per_cpu_ptr**\ (), it would > > + * never return NULL. > > + * Return > > + * A generic pointer pointing to the kernel percpu variable on > > what's "a generic pointer"? is it as opposed to sk_buff pointer or something? > Ack. "A pointer" should be good enough. I wrote "generic pointer" because the per_cpu_ptr() in kernel code is a macro, whose returned value is a typed pointer, IIUC. But here we are missing the type. This is another difference between this helper and per_cpu_ptr(). But this may not matter. > > /* integer value in 'imm' field of BPF_CALL instruction selects which helper > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index a702600ff581..e070d2abc405 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -5016,8 +5016,10 @@ 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_MEM_OR_BTF_ID_OR_NULL) { > > + } else if (fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL || > > + fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID) { > > const struct btf_type *t; > > + bool not_null = fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID; > > nit: this is fine, but I'd inline it below > Ack. > > > > mark_reg_known_zero(env, regs, BPF_REG_0); > > t = btf_type_skip_modifiers(btf_vmlinux, meta.ret_btf_id, NULL); > > @@ -5034,10 +5036,12 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn > > tname, PTR_ERR(ret)); > > return -EINVAL; > > } > > - regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL; > > + regs[BPF_REG_0].type = not_null ? > > + PTR_TO_MEM : PTR_TO_MEM_OR_NULL; > > regs[BPF_REG_0].mem_size = tsize; > > } else { > > - regs[BPF_REG_0].type = PTR_TO_BTF_ID_OR_NULL; > > + regs[BPF_REG_0].type = not_null ? > > + PTR_TO_BTF_ID : PTR_TO_BTF_ID_OR_NULL; > > regs[BPF_REG_0].btf_id = meta.ret_btf_id; > > } > > } else if (fn->ret_type == RET_PTR_TO_BTF_ID_OR_NULL) { > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index d474c1530f87..466acf82a9c7 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -1160,6 +1160,18 @@ static const struct bpf_func_proto bpf_per_cpu_ptr_proto = { > > .arg2_type = ARG_ANYTHING, > > }; > > > > +BPF_CALL_1(bpf_this_cpu_ptr, const void *, percpu_ptr) > > +{ > > + return (u64)this_cpu_ptr(percpu_ptr); > > see previous comment, this might trigger unnecessary compilation > warnings on 32-bit arches > Ack. Will cast to "unsigned long". Thanks for catching this! > > +} > > + > > +static const struct bpf_func_proto bpf_this_cpu_ptr_proto = { > > + .func = bpf_this_cpu_ptr, > > + .gpl_only = false, > > + .ret_type = RET_PTR_TO_MEM_OR_BTF_ID, > > + .arg1_type = ARG_PTR_TO_PERCPU_BTF_ID, > > +}; > > + > > [...]