Re: [PATCH 1/2] powerpc/bpf: Fix detecting BPF atomic instructions

From: Jiri Olsa <jolsa@redhat.com>
To: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Cc: bpf@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
	Michael Ellerman <mpe@ellerman.id.au>,
	Brendan Jackman <jackmanb@google.com>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Daniel Borkmann <daniel@iogearbox.net>
Subject: Re: [PATCH 1/2] powerpc/bpf: Fix detecting BPF atomic instructions
Date: Fri, 2 Jul 2021 12:26:11 +0200	[thread overview]
Message-ID: <YN7pw2mAg35Yao6/@krava> (raw)
In-Reply-To: <4117b430ffaa8cd7af042496f87fd7539e4f17fd.1625145429.git.naveen.n.rao@linux.vnet.ibm.com>

On Thu, Jul 01, 2021 at 08:38:58PM +0530, Naveen N. Rao wrote:
> Commit 91c960b0056672 ("bpf: Rename BPF_XADD and prepare to encode other
> atomics in .imm") converted BPF_XADD to BPF_ATOMIC and added a way to
> distinguish instructions based on the immediate field. Existing JIT
> implementations were updated to check for the immediate field and to
> reject programs utilizing anything more than BPF_ADD (such as BPF_FETCH)
> in the immediate field.
> 
> However, the check added to powerpc64 JIT did not look at the correct
> BPF instruction. Due to this, such programs would be accepted and
> incorrectly JIT'ed resulting in soft lockups, as seen with the atomic
> bounds test. Fix this by looking at the correct immediate value.
> 
> Fixes: 91c960b0056672 ("bpf: Rename BPF_XADD and prepare to encode other atomics in .imm")
> Reported-by: Jiri Olsa <jolsa@redhat.com>
> Tested-by: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> Hi Jiri,
> FYI: I made a small change in this patch -- using 'imm' directly, rather 
> than insn[i].imm. I've still added your Tested-by since this shouldn't 
> impact the fix in any way.

yep, it works nicely

thanks
jirka

> 
> - Naveen
> 
> 
>  arch/powerpc/net/bpf_jit_comp64.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
> index 5cad5b5a7e9774..de8595880feec6 100644
> --- a/arch/powerpc/net/bpf_jit_comp64.c
> +++ b/arch/powerpc/net/bpf_jit_comp64.c
> @@ -667,7 +667,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
>  		 * BPF_STX ATOMIC (atomic ops)
>  		 */
>  		case BPF_STX | BPF_ATOMIC | BPF_W:
> -			if (insn->imm != BPF_ADD) {
> +			if (imm != BPF_ADD) {
>  				pr_err_ratelimited(
>  					"eBPF filter atomic op code %02x (@%d) unsupported\n",
>  					code, i);
> @@ -689,7 +689,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
>  			PPC_BCC_SHORT(COND_NE, tmp_idx);
>  			break;
>  		case BPF_STX | BPF_ATOMIC | BPF_DW:
> -			if (insn->imm != BPF_ADD) {
> +			if (imm != BPF_ADD) {
>  				pr_err_ratelimited(
>  					"eBPF filter atomic op code %02x (@%d) unsupported\n",
>  					code, i);
> -- 
> 2.31.1
>