[PATCH bpf-next] s390/bpf: Fully order atomic "add", "and", "or" and "xor"

[PATCH bpf-next] s390/bpf: Fully order atomic "add", "and", "or" and "xor"

[Ilya Leoshkevich]

BPF_ATOMIC_OP() macro documentation states that "BPF_ADD | BPF_FETCH"
should be the same as atomic_fetch_add(), which is currently not the
case on s390x: the synchronization instruction "bcr 14,0" is missing.

This should not be a problem in practice, because s390x is allowed to
reorder only stores with subsequent fetches from different addresses.
Still, just to be on the safe side, and also for consistency, emit the
synchronization instruction.

Note that it's not required to do this for BPF_XCHG and BPF_CMPXCHG,
because COMPARE AND SWAP performs serialization itself.

Fixes: ba3b86b9cef0 ("s390/bpf: Implement new atomic ops")
Reported-by: Puranjay Mohan <puranjay12@gmail.com>
Closes: https://lore.kernel.org/bpf/mb61p34qvq3wf.fsf@kernel.org/
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/net/bpf_jit_comp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index fa2f824e3b06..a0dfb3f665ab 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -1427,6 +1427,8 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 	EMIT6_DISP_LH(0xeb000000, is32 ? (op32) : (op64),		\
 		      (insn->imm & BPF_FETCH) ? src_reg : REG_W0,	\
 		      src_reg, dst_reg, off);				\
+	/* bcr 14,0 - see atomic_fetch_{add,and,or,xor}() */		\
+	_EMIT2(0x07e0);							\
 	if (is32 && (insn->imm & BPF_FETCH))				\
 		EMIT_ZERO(src_reg);					\
 } while (0)
-- 
2.45.0

Re: [PATCH bpf-next] s390/bpf: Fully order atomic "add", "and", "or" and "xor"

[Puranjay Mohan]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> BPF_ATOMIC_OP() macro documentation states that "BPF_ADD | BPF_FETCH"
> should be the same as atomic_fetch_add(), which is currently not the
> case on s390x: the synchronization instruction "bcr 14,0" is missing.
>
> This should not be a problem in practice, because s390x is allowed to
> reorder only stores with subsequent fetches from different addresses.
> Still, just to be on the safe side, and also for consistency, emit the
> synchronization instruction.
>
> Note that it's not required to do this for BPF_XCHG and BPF_CMPXCHG,
> because COMPARE AND SWAP performs serialization itself.
>
> Fixes: ba3b86b9cef0 ("s390/bpf: Implement new atomic ops")
> Reported-by: Puranjay Mohan <puranjay12@gmail.com>
> Closes: https://lore.kernel.org/bpf/mb61p34qvq3wf.fsf@kernel.org/
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  arch/s390/net/bpf_jit_comp.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
> index fa2f824e3b06..a0dfb3f665ab 100644
> --- a/arch/s390/net/bpf_jit_comp.c
> +++ b/arch/s390/net/bpf_jit_comp.c
> @@ -1427,6 +1427,8 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
>  	EMIT6_DISP_LH(0xeb000000, is32 ? (op32) : (op64),		\
>  		      (insn->imm & BPF_FETCH) ? src_reg : REG_W0,	\
>  		      src_reg, dst_reg, off);				\
> +	/* bcr 14,0 - see atomic_fetch_{add,and,or,xor}() */		\
> +	_EMIT2(0x07e0);							\

Shouldn't this be:

        /* bcr 14,0 - see atomic_fetch_{add,and,or,xor}() */ 
        if (insn->imm & BPF_FETCH)
                _EMIT2(0x07e0); 

The barrier is only needed for the BPF_FETCH varient and I am assuming
that the barrier has some overhead. So we should not emit it for all
operations.


Thanks,
Puranjay