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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D822AC433EF for ; Tue, 10 May 2022 23:20:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229714AbiEJXUH (ORCPT ); Tue, 10 May 2022 19:20:07 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60204 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237166AbiEJXUD (ORCPT ); Tue, 10 May 2022 19:20:03 -0400 Received: from mail-il1-x12d.google.com (mail-il1-x12d.google.com [IPv6:2607:f8b0:4864:20::12d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9D531289BDB for ; Tue, 10 May 2022 16:19:53 -0700 (PDT) Received: by mail-il1-x12d.google.com with SMTP id j12so307996ila.12 for ; Tue, 10 May 2022 16:19:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=i6FmjKIwcCgjrTVNoBlmFquHutE6K153srrhtK/CB1k=; b=bIyvDlqkMNc01lw6uJpNaL+CE7fQWFkWQSZMVXEwax5uutRbU4WxmjgbwsRlgzpBzm 1y+h9JjKydf+N6P5oiaMn1nmUbIAMYPZCM25YmqYfH0tRpJnv2Lun80PF4y8lCTNAE6C S6k4ET63z5unqf5KeWlfTA9rTNGcz/EgaUNw3xsdHfPONl6XIY+xNJzYPc+noaqrWcwR vj8hB6DTeyg0Q+ULnvr0o+v9bWVhg88Bg/iSQl0+dZ8Bc94+jfEiO4NB97TWXMD0Q++r aG+2DzGPddSvd1B2x9MOJTI8tLjs1bn97dRkQ02Pq/749Bju3zUTB89Z2aVExuC/o3sa aAeA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=i6FmjKIwcCgjrTVNoBlmFquHutE6K153srrhtK/CB1k=; b=oHUstEzCqzHjrwrep1Wbu3b/iJIuPhrH6/p/xlEIRd0ze1FAuSf8tNE5cx8H3yYYlm dpOHLrczaZlYfcEECDlDGIutY0ezUU0ce1AyGAQ4KcZNMS8RS2ix431QKWmfRaTiMxBK ff+DFDsAS/c5GEQU1LxLNdCSIncTF4hseXqLWQPpJ13UDeVcR4YXs3yRq7i9B7PrDMG2 Pc9ftvuzmYGNYqmqJkuKaz88ZICDBE6p0mZGJ3v0E/iq7+3ppCJEcPFsbKMsyXHLWjX7 FlymabSMLONARK1BNEbkc7QwUMJ1/3KCvGkV96LfXtSeiyGl8e6qdNE4ikdhhPPfSISL bHEA== X-Gm-Message-State: AOAM533FZGrqBi05uozs/1KJDFYFG53Oi2aFqxoYoVBb5ROEQ0B7vedk 9rqgumxcWgD74m/6qpVeSVRaVyBgeTEYyQrka/rTpbh2 X-Google-Smtp-Source: ABdhPJztIryNZJ2O0WH7mtV1SEXah96A1fkRzheTltPcchp0KoCJqmxTDZzywp+a9szN/jjPidg0G9yNOo0NQokVJhQ= X-Received: by 2002:a05:6e02:11a3:b0:2cf:90f9:30e0 with SMTP id 3-20020a056e0211a300b002cf90f930e0mr6808874ilj.252.1652224792933; Tue, 10 May 2022 16:19:52 -0700 (PDT) MIME-Version: 1.0 References: <20220501190002.2576452-1-yhs@fb.com> <20220501190012.2577087-1-yhs@fb.com> <7246362c-8eca-027e-d43d-8d4955ad5bdd@fb.com> In-Reply-To: <7246362c-8eca-027e-d43d-8d4955ad5bdd@fb.com> From: Andrii Nakryiko Date: Tue, 10 May 2022 16:19:42 -0700 Message-ID: Subject: Re: [PATCH bpf-next 02/12] libbpf: Permit 64bit relocation value To: Yonghong Song Cc: bpf , Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , Kernel Team Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On Tue, May 10, 2022 at 3:14 PM Yonghong Song wrote: > > > > On 5/9/22 3:37 PM, Andrii Nakryiko wrote: > > On Sun, May 1, 2022 at 12:00 PM Yonghong Song wrote: > >> > >> Currently, the libbpf limits the relocation value to be 32bit > >> since all current relocations have such a limit. But with > >> BTF_KIND_ENUM64 support, the enum value could be 64bit. > >> So let us permit 64bit relocation value in libbpf. > >> > >> Signed-off-by: Yonghong Song > >> --- > >> tools/lib/bpf/relo_core.c | 24 ++++++++++++------------ > >> tools/lib/bpf/relo_core.h | 4 ++-- > >> 2 files changed, 14 insertions(+), 14 deletions(-) > >> > > > > [...] > > > >> @@ -929,7 +929,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, > >> int insn_idx, const struct bpf_core_relo *relo, > >> int relo_idx, const struct bpf_core_relo_res *res) > >> { > >> - __u32 orig_val, new_val; > >> + __u64 orig_val, new_val; > >> __u8 class; > >> > >> class = BPF_CLASS(insn->code); > >> @@ -954,14 +954,14 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, > >> if (BPF_SRC(insn->code) != BPF_K) > >> return -EINVAL; > >> if (res->validate && insn->imm != orig_val) { > >> - pr_warn("prog '%s': relo #%d: unexpected insn #%d (ALU/ALU64) value: got %u, exp %u -> %u\n", > >> + pr_warn("prog '%s': relo #%d: unexpected insn #%d (ALU/ALU64) value: got %u, exp %llu -> %llu\n", > >> prog_name, relo_idx, > >> insn_idx, insn->imm, orig_val, new_val); > > > > %llu is not valid formatter for __u64 on all architectures, please add > > explicit (unsigned long long) cast > > Okay, will do. > > > > > but also in general for non-ldimm64 instructions we need to check that > > new value fits in 32 bits > > The real 64-bit value can only be retrieved for ldimm64 insn, so I > suppose it should be fine here. But let me double check. So, technically (I don't think that happens in practice, though), you can have ALU operation with a local 32-bit enum with some reasonable value, which in the kernel is actually ENUM64 with huge value. > > > > > [...] > > > >> @@ -1026,7 +1026,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, > >> > >> imm = insn[0].imm + ((__u64)insn[1].imm << 32); > >> if (res->validate && imm != orig_val) { > >> - pr_warn("prog '%s': relo #%d: unexpected insn #%d (LDIMM64) value: got %llu, exp %u -> %u\n", > >> + pr_warn("prog '%s': relo #%d: unexpected insn #%d (LDIMM64) value: got %llu, exp %llu -> %llu\n", > >> prog_name, relo_idx, > >> insn_idx, (unsigned long long)imm, > >> orig_val, new_val); > >> @@ -1035,7 +1035,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, > >> > >> insn[0].imm = new_val; > >> insn[1].imm = 0; /* currently only 32-bit values are supported */ > > > > as Dave mentioned, not anymore, so this should take higher 32-bit of new_val > > Will do. > > > > > > >> - pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %u\n", > >> + pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %llu\n", > >> prog_name, relo_idx, insn_idx, > >> (unsigned long long)imm, new_val); > >> break; > >> @@ -1261,7 +1261,7 @@ int bpf_core_calc_relo_insn(const char *prog_name, > >> * decision and value, otherwise it's dangerous to > >> * proceed due to ambiguity > >> */ > >> - pr_warn("prog '%s': relo #%d: relocation decision ambiguity: %s %u != %s %u\n", > >> + pr_warn("prog '%s': relo #%d: relocation decision ambiguity: %s %llu != %s %llu\n", > >> prog_name, relo_idx, > >> cand_res.poison ? "failure" : "success", cand_res.new_val, > >> targ_res->poison ? "failure" : "success", targ_res->new_val); > [...]