linux-kernel - Re: [PATCH v1 06/10] powerpc/bpf: Perform complete extra passes to update addresses

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <d53644df-7f02-00fb-f022-58ce2436a245@csgroup.eu>
Date:   Tue, 31 Jan 2023 09:52:16 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Nicholas Piggin <npiggin@...il.com>
CC:     Andrii Nakryiko <andrii@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Hao Luo <haoluo@...gle.com>,
        John Fastabend <john.fastabend@...il.com>,
        Jiri Olsa <jolsa@...nel.org>, KP Singh <kpsingh@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Stanislav Fomichev <sdf@...gle.com>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>
Subject: Re: [PATCH v1 06/10] powerpc/bpf: Perform complete extra passes to
 update addresses



Le 10/01/2023 à 09:44, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>>
>>
>> Le 13/12/2022 à 11:23, Naveen N. Rao a écrit :
>>> Christophe Leroy wrote:
>>>> BPF core calls the jit compiler again for an extra pass in order
>>>> to properly set subprog addresses.
>>>>
>>>> Unlike other architectures, powerpc only updates the addresses
>>>> during that extra pass. It means that holes must have been left
>>>> in the code in order to enable the maximum possible instruction
>>>> size.
>>>>
>>>> In order avoid waste of space, and waste of CPU time on powerpc
>>>> processors on which the NOP instruction is not 0-cycle, perform
>>>> two real additional passes.
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>
>>>> ---
>>>>  arch/powerpc/net/bpf_jit_comp.c | 85 ---------------------------------
>>>>  1 file changed, 85 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/net/bpf_jit_comp.c 
>>>> b/arch/powerpc/net/bpf_jit_comp.c
>>>> index 43e634126514..8833bf23f5aa 100644
>>>> --- a/arch/powerpc/net/bpf_jit_comp.c
>>>> +++ b/arch/powerpc/net/bpf_jit_comp.c
>>>> @@ -23,74 +23,6 @@ static void bpf_jit_fill_ill_insns(void *area, 
>>>> unsigned int size)
>>>>      memset32(area, BREAKPOINT_INSTRUCTION, size / 4);
>>>>  }
>>>>
>>>> -/* Fix updated addresses (for subprog calls, ldimm64, et al) during 
>>>> extra pass */
>>>> -static int bpf_jit_fixup_addresses(struct bpf_prog *fp, u32 *image,
>>>> -                   struct codegen_context *ctx, u32 *addrs)
>>>> -{
>>>> -    const struct bpf_insn *insn = fp->insnsi;
>>>> -    bool func_addr_fixed;
>>>> -    u64 func_addr;
>>>> -    u32 tmp_idx;
>>>> -    int i, j, ret;
>>>> -
>>>> -    for (i = 0; i < fp->len; i++) {
>>>> -        /*
>>>> -         * During the extra pass, only the branch target addresses for
>>>> -         * the subprog calls need to be fixed. All other instructions
>>>> -         * can left untouched.
>>>> -         *
>>>> -         * The JITed image length does not change because we already
>>>> -         * ensure that the JITed instruction sequence for these calls
>>>> -         * are of fixed length by padding them with NOPs.
>>>> -         */
>>>> -        if (insn[i].code == (BPF_JMP | BPF_CALL) &&
>>>> -            insn[i].src_reg == BPF_PSEUDO_CALL) {
>>>> -            ret = bpf_jit_get_func_addr(fp, &insn[i], true,
>>>> -                            &func_addr,
>>>> -                            &func_addr_fixed);
>>>
>>> I don't see you updating calls to bpf_jit_get_func_addr() in 
>>> bpf_jit_build_body() to set extra_pass to true. Afaics, that's 
>>> required to get the correct address to be branched to for subprogs.
>>>
>>
>> I don't understand what you mean.
> 
> I am referring to the third parameter we pass to bpf_jit_get_func_addr().
> 
> In bpf_jit_build_body(), we do:
> 
>          case BPF_JMP | BPF_CALL:
>              ctx->seen |= SEEN_FUNC;
> 
>              ret = bpf_jit_get_func_addr(fp, &insn[i], false,
>                              &func_addr, &func_addr_fixed);
> 
> 
> The third parameter (extra_pass) to bpf_jit_get_func_addr() is set to 
> false. In bpf_jit_get_func_addr(), we have:
> 
>      *func_addr_fixed = insn->src_reg != BPF_PSEUDO_CALL;
>      if (!*func_addr_fixed) {
>          /* Place-holder address till the last pass has collected
>           * all addresses for JITed subprograms in which case we
>           * can pick them up from prog->aux.
>           */
>          if (!extra_pass)
>              addr = NULL;
> 
> Before this patch series, in bpf_jit_fixup_addresses(), we were calling 
> bpf_jit_get_func_addr() with the third parameter set to true.

Ah right, I see.

I will send out v2 shortly.

Thanks
Christophe