netdev - Re: [PATCH 5/5] bpf ppc32: Add instructions for atomic

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: <025e9a60-46d9-bc3d-224e-1d92bc05f857@csgroup.eu>
Date:   Fri, 13 May 2022 07:50:01 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Hari Bathini <hbathini@...ux.ibm.com>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>
CC:     Michael Ellerman <mpe@...erman.id.au>,
        "Naveen N. Rao" <naveen.n.rao@...ux.ibm.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Jordan Niethe <jniethe5@...il.com>
Subject: Re: [PATCH 5/5] bpf ppc32: Add instructions for atomic_[cmp]xchg



Le 12/05/2022 à 09:45, Hari Bathini a écrit :
> This adds two atomic opcodes BPF_XCHG and BPF_CMPXCHG on ppc32, both
> of which include the BPF_FETCH flag.  The kernel's atomic_cmpxchg
> operation fundamentally has 3 operands, but we only have two register
> fields. Therefore the operand we compare against (the kernel's API
> calls it 'old') is hard-coded to be BPF_REG_R0. Also, kernel's
> atomic_cmpxchg returns the previous value at dst_reg + off. JIT the
> same for BPF too with return value put in BPF_REG_0.
> 
>    BPF_REG_R0 = atomic_cmpxchg(dst_reg + off, BPF_REG_R0, src_reg);


Ah, now we mix the xchg's with the bitwise operations. Ok I understand 
better that goto atomic_ops in the previous patch then. But it now 
becomes uneasy to read and follow.

I think it would be cleaner to separate completely the bitwise 
operations and this, even if it duplicates half a dozen of lines.

> 
> Signed-off-by: Hari Bathini <hbathini@...ux.ibm.com>
> ---
>   arch/powerpc/net/bpf_jit_comp32.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
> index 5604ae1b60ab..4690fd6e9e52 100644
> --- a/arch/powerpc/net/bpf_jit_comp32.c
> +++ b/arch/powerpc/net/bpf_jit_comp32.c
> @@ -829,6 +829,23 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
>   				/* we're done if this succeeded */
>   				PPC_BCC_SHORT(COND_NE, tmp_idx);
>   				break;
> +			case BPF_CMPXCHG:
> +				/* Compare with old value in BPF_REG_0 */
> +				EMIT(PPC_RAW_CMPW(bpf_to_ppc(BPF_REG_0), _R0));
> +				/* Don't set if different from old value */
> +				PPC_BCC_SHORT(COND_NE, (ctx->idx + 3) * 4);
> +				fallthrough;
> +			case BPF_XCHG:
> +				/* store new value */
> +				EMIT(PPC_RAW_STWCX(src_reg, tmp_reg, dst_reg));
> +				PPC_BCC_SHORT(COND_NE, tmp_idx);
> +				/*
> +				 * Return old value in src_reg for BPF_XCHG &
> +				 * BPF_REG_0 for BPF_CMPXCHG.
> +				 */
> +				EMIT(PPC_RAW_MR(imm == BPF_XCHG ? src_reg : bpf_to_ppc(BPF_REG_0),
> +						_R0));

If the line spreads into two lines, compact form is probably not worth 
it. Would be more readable as

	if (imm == BPF_XCHG)
		EMIT_PPC_RAW_MR(src_reg, _R0));
	else
		EMIT_PPC_RAW_MR(src_reg, bpf_to_ppc(BPF_REG_0)));


At the end, it's probably even more readable if you separate both cases 
completely:

	case BPF_CMPXCHG:
		/* Compare with old value in BPF_REG_0 */
		EMIT(PPC_RAW_CMPW(bpf_to_ppc(BPF_REG_0), _R0));
		/* Don't set if different from old value */
		PPC_BCC_SHORT(COND_NE, (ctx->idx + 3) * 4);
		/* store new value */
		EMIT(PPC_RAW_STWCX(src_reg, tmp_reg, dst_reg));
		PPC_BCC_SHORT(COND_NE, tmp_idx);
		/* Return old value in BPF_REG_0 */
		EMIT_PPC_RAW_MR(src_reg, bpf_to_ppc(BPF_REG_0)));
		break;
	case BPF_XCHG:
		/* store new value */
		EMIT(PPC_RAW_STWCX(src_reg, tmp_reg, dst_reg));
		PPC_BCC_SHORT(COND_NE, tmp_idx);
		/* Return old value in src_reg */
		EMIT_PPC_RAW_MR(src_reg, _R0));
		break;


> +				break;
>   			default:
>   				pr_err_ratelimited("eBPF filter atomic op code %02x (@%d) unsupported\n",
>   						   code, i);