linux-kernel - RE: [PATCH bpf-next v4 01/11] bpf: x86: Factor out emission of ModR/M for *(reg + off)

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <5fce98d7d860e_5a9620833@john-XPS-13-9370.notmuch>
Date:   Mon, 07 Dec 2020 13:04:23 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Brendan Jackman <jackmanb@...gle.com>, bpf@...r.kernel.org
Cc:     Alexei Starovoitov <ast@...nel.org>, Yonghong Song <yhs@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        KP Singh <kpsingh@...omium.org>,
        Florent Revest <revest@...omium.org>,
        linux-kernel@...r.kernel.org, Jann Horn <jannh@...gle.com>,
        Brendan Jackman <jackmanb@...gle.com>
Subject: RE: [PATCH bpf-next v4 01/11] bpf: x86: Factor out emission of ModR/M
 for *(reg + off)

Brendan Jackman wrote:
> The case for JITing atomics is about to get more complicated. Let's
> factor out some common code to make the review and result more
> readable.
> 
> NB the atomics code doesn't yet use the new helper - a subsequent
> patch will add its use as a side-effect of other changes.
> 
> Signed-off-by: Brendan Jackman <jackmanb@...gle.com>
> ---

Small nit on style preference below.

Acked-by: John Fastabend <john.fastabend@...il.com>

[...]

>  
> @@ -1240,11 +1250,7 @@ st:			if (is_imm8(insn->off))
>  			goto xadd;
>  		case BPF_STX | BPF_XADD | BPF_DW:
>  			EMIT3(0xF0, add_2mod(0x48, dst_reg, src_reg), 0x01);
> -xadd:			if (is_imm8(insn->off))
> -				EMIT2(add_2reg(0x40, dst_reg, src_reg), insn->off);
> -			else
> -				EMIT1_off32(add_2reg(0x80, dst_reg, src_reg),
> -					    insn->off);
> +xadd:			emit_modrm_dstoff(&prog, dst_reg, src_reg, insn->off);

I at least prefer the xadd on its own line above the emit_*(). That seems
more consistent with the rest of the code in this file. The only other
example like this is st:.