Re: [PATCH] ppc: Fix BPF JIT for ABIv2

On 2016/06/21 11:47AM, Thadeu Lima de Souza Cascardo wrote:

On Tue, Jun 21, 2016 at 09:15:48PM +1000, Michael Ellerman wrote:

quoted

On Tue, 2016-06-21 at 14:28 +0530, Naveen N. Rao wrote:

quoted

On 2016/06/20 03:56PM, Thadeu Lima de Souza Cascardo wrote:

quoted

On Sun, Jun 19, 2016 at 11:19:14PM +0530, Naveen N. Rao wrote:

quoted

On 2016/06/17 10:00AM, Thadeu Lima de Souza Cascardo wrote:

quoted

Hi, Michael and Naveen.

I noticed independently that there is a problem with BPF JIT and ABIv2, and
worked out the patch below before I noticed Naveen's patchset and the latest
changes in ppc tree for a better way to check for ABI versions.

However, since the issue described below affect mainline and stable kernels,
would you consider applying it before merging your two patchsets, so that we can
more easily backport the fix?

Hi Cascardo,
Given that this has been broken on ABIv2 since forever, I didn't bother 
fixing it. But, I can see why this would be a good thing to have for 
-stable and existing distros. However, while your patch below may fix 
the crash you're seeing on ppc64le, it is not sufficient -- you'll need 
changes in bpf_jit_asm.S as well.

Hi, Naveen.

Any tips on how to exercise possible issues there? Or what changes you think
would be sufficient?

The calling convention is different with ABIv2 and so we'll need changes 
in bpf_slow_path_common() and sk_negative_common().

How big would those changes be? Do we know?

I don't think it'd be that much -- I will take a stab at this today.

quoted

How come no one reported this was broken previously? This is the first I've
heard of it being broken.

I just heard of it less than two weeks ago, and only could investigate it last
week, when I realized mainline was also affected.

It looks like the little-endian support for classic JIT were done before the
conversion to ABIv2. And as JIT is disabled by default, no one seems to have
exercised it.

Yes, my thoughts too. I didn't previously think much about this as JIT 
wouldn't be enabled by default. It's interesting though that no one else 
reported this as an issue before.

quoted

quoted

However, rather than enabling classic JIT for ppc64le, are we better off 
just disabling it?

--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig

@@ -128,7 +128,7 @@ config PPC
        select IRQ_FORCED_THREADING
        select HAVE_RCU_TABLE_FREE if SMP
        select HAVE_SYSCALL_TRACEPOINTS
-       select HAVE_CBPF_JIT
+       select HAVE_CBPF_JIT if CPU_BIG_ENDIAN
        select HAVE_ARCH_JUMP_LABEL
        select ARCH_HAVE_NMI_SAFE_CMPXCHG
        select ARCH_HAS_GCOV_PROFILE_ALL

Michael,
Let me know your thoughts on whether you intend to take this patch or 
Cascardo's patch for -stable before the eBPF patches. I can redo my 
patches accordingly.

This patch sounds like the best option at the moment for something we can
backport. Unless the changes to fix it are minimal.

Right -- I will take a look today to see what changes would be needed.

- Naveen