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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 10 Oct 2021 19:30:29 +0200
From:   Johan Almbladh <johan.almbladh@...finetworks.com>
To:     Tiezhu Yang <yangtiezhu@...ngson.cn>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Paul Burton <paulburton@...nel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org,
        Xuefeng Li <lixuefeng@...ngson.cn>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>
Subject: Re: [PATCH bpf-next 2/2] bpf, mips: Modify check condition about tail
 call count

On Sat, Oct 9, 2021 at 12:58 PM Tiezhu Yang <yangtiezhu@...ngson.cn> wrote:
>
> In emit_tail_call() of bpf_jit_comp32.c, "blez t2" (t2 <= 0) is not
> consistent with the comment "t2 < 0", modify the check condition to
> keep consistency.
>
> Signed-off-by: Tiezhu Yang <yangtiezhu@...ngson.cn>
> ---
>  arch/mips/net/bpf_jit_comp32.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/net/bpf_jit_comp32.c b/arch/mips/net/bpf_jit_comp32.c
> index 9d7041a..b887c01 100644
> --- a/arch/mips/net/bpf_jit_comp32.c
> +++ b/arch/mips/net/bpf_jit_comp32.c
> @@ -1312,12 +1312,12 @@ static int emit_tail_call(struct jit_context *ctx)
>         emit(ctx, sltu, t1, ind, t1);            /* t1 = ind < t1            */
>         emit(ctx, beqz, t1, get_offset(ctx, 1)); /* PC += off(1) if t1 == 0  */
>                                                  /* (next insn delay slot)   */
> -       /* if (TCC-- <= 0) goto out */
> +       /* if (--TCC < 0) goto out */
>         emit(ctx, lw, t2, ctx->stack_size, MIPS_R_SP);  /* t2 = *(SP + size) */
>         emit_load_delay(ctx);                     /* Load delay slot         */
> -       emit(ctx, blez, t2, get_offset(ctx, 1));  /* PC += off(1) if t2 < 0  */
>         emit(ctx, addiu, t2, t2, -1);             /* t2-- (delay slot)       */
>         emit(ctx, sw, t2, ctx->stack_size, MIPS_R_SP);  /* *(SP + size) = t2 */
> +       emit(ctx, bltz, t2, get_offset(ctx, 1));  /* PC += off(1) if t2 < 0  */

If the comment is not consistent with the code, and the code is
correct, why did you change the code? Have you seen the JIT fail on
any of the tail call test cases?

The current code works as intended. The t2 register is decremented in
the branch delay slot of the blez. After your change, the the comment
still says "delay slot", but it is no longer in the delay slot of a
branch. Instead the next instruction emitted, not visible in the patch
context, fills the delay slot of the bltz. In this case it probably is
ok, but if that instruction is also a branch, the result would be
unpredictable.

I prefer to emit the delay slot instruction immediately after the
branch is emitted when possible. If a branch and its delay slot is
separated in the JIT logic, it makes the JIT more brittle IMO.

Please keep the original logic, but update the blez comment so it is
consistent with the code.



>
>         /* prog = ary->ptrs[ind] */
>         off = offsetof(struct bpf_array, ptrs);
> --
> 2.1.0
>

Powered by blists - more mailing lists