[PATCH bpf-next v2] bpf, x86: Add support for signed arena loads

[PATCH bpf-next v2] bpf, x86: Add support for signed arena loads

[Kumar Kartikeya Dwivedi]

Currently, signed load instructions into arena memory are unsupported.
The compiler is free to generate these, and on GCC-14 we see a
corresponding error when it happens. The hurdle in supporting them is
deciding which unused opcode to use to mark them for the JIT's own
consumption. After much thinking, it appears 0xc0 / BPF_NOSPEC can be
combined with load instructions to identify signed arena loads. Use
this to recognize and JIT them appropriately, and remove the verifier
side limitation on the program if the JIT supports them.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
Changelog:
v1 -> v2
v1: https://lore.kernel.org/bpf/20250509194956.1635207-1-memxor@gmail.com

 * Use bpf_jit_supports_insn. (Alexei)
---
 arch/arm64/net/bpf_jit_comp.c |  5 +++++
 arch/s390/net/bpf_jit_comp.c  |  5 +++++
 arch/x86/net/bpf_jit_comp.c   | 35 ++++++++++++++++++++++++++++++++++-
 include/linux/filter.h        |  3 +++
 kernel/bpf/verifier.c         | 10 +++++++---
 5 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 70d7c89d3ac9..19524694151d 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -2753,6 +2753,11 @@ bool bpf_jit_supports_insn(struct bpf_insn *insn, bool in_arena)
 		if (!bpf_atomic_is_load_store(insn) &&
 		    !cpus_have_cap(ARM64_HAS_LSE_ATOMICS))
 			return false;
+		break;
+	case BPF_LDX | BPF_MEMSX | BPF_B:
+	case BPF_LDX | BPF_MEMSX | BPF_H:
+	case BPF_LDX | BPF_MEMSX | BPF_W:
+		return false;
 	}
 	return true;
 }
diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index 0776dfde2dba..cdcd33646f71 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -2928,6 +2928,11 @@ bool bpf_jit_supports_insn(struct bpf_insn *insn, bool in_arena)
 	case BPF_STX | BPF_ATOMIC | BPF_DW:
 		if (bpf_atomic_is_load_store(insn))
 			return false;
+		break;
+	case BPF_LDX | BPF_MEMSX | BPF_B:
+	case BPF_LDX | BPF_MEMSX | BPF_H:
+	case BPF_LDX | BPF_MEMSX | BPF_W:
+		return false;
 	}
 	return true;
 }
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 9e5fe2ba858f..79662b5a1e08 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1146,11 +1146,38 @@ static void emit_ldx_index(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, u32 i
 	*pprog = prog;
 }

+static void emit_ldsx_index(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, u32 index_reg, int off)
+{
+	u8 *prog = *pprog;
+
+	switch (size) {
+	case BPF_B:
+		/* movsx rax, byte ptr [rax + r12 + off] */
+		EMIT3(add_3mod(0x40, src_reg, dst_reg, index_reg), 0x0F, 0xBE);
+		break;
+	case BPF_H:
+		/* movsx rax, word ptr [rax + r12 + off] */
+		EMIT3(add_3mod(0x40, src_reg, dst_reg, index_reg), 0x0F, 0xBF);
+		break;
+	case BPF_W:
+		/* movsx rax, dword ptr [rax + r12 + off] */
+		EMIT2(add_3mod(0x40, src_reg, dst_reg, index_reg), 0x63);
+		break;
+	}
+	emit_insn_suffix_SIB(&prog, src_reg, dst_reg, index_reg, off);
+	*pprog = prog;
+}
+
 static void emit_ldx_r12(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, int off)
 {
 	emit_ldx_index(pprog, size, dst_reg, src_reg, X86_REG_R12, off);
 }

+static void emit_ldsx_r12(u8 **prog, u32 size, u32 dst_reg, u32 src_reg, int off)
+{
+	emit_ldsx_index(prog, size, dst_reg, src_reg, X86_REG_R12, off);
+}
+
 /* STX: *(u8*)(dst_reg + off) = src_reg */
 static void emit_stx(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, int off)
 {
@@ -2010,13 +2037,19 @@ st:			if (is_imm8(insn->off))
 		case BPF_LDX | BPF_PROBE_MEM32 | BPF_H:
 		case BPF_LDX | BPF_PROBE_MEM32 | BPF_W:
 		case BPF_LDX | BPF_PROBE_MEM32 | BPF_DW:
+		case BPF_LDX | BPF_PROBE_MEM32SX | BPF_B:
+		case BPF_LDX | BPF_PROBE_MEM32SX | BPF_H:
+		case BPF_LDX | BPF_PROBE_MEM32SX | BPF_W:
 		case BPF_STX | BPF_PROBE_MEM32 | BPF_B:
 		case BPF_STX | BPF_PROBE_MEM32 | BPF_H:
 		case BPF_STX | BPF_PROBE_MEM32 | BPF_W:
 		case BPF_STX | BPF_PROBE_MEM32 | BPF_DW:
 			start_of_ldx = prog;
 			if (BPF_CLASS(insn->code) == BPF_LDX)
-				emit_ldx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
+				if (BPF_MODE(insn->code) == BPF_PROBE_MEM32SX)
+					emit_ldsx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
+				else
+					emit_ldx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
 			else
 				emit_stx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
 populate_extable:
diff --git a/include/linux/filter.h b/include/linux/filter.h
index f5cf4d35d83e..aab2f1ec4542 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -78,6 +78,9 @@ struct ctl_table_header;
 /* unused opcode to mark special atomic instruction */
 #define BPF_PROBE_ATOMIC 0xe0

+/* unused opcode to mark special ldsx instruction. Same as BPF_NOSPEC */
+#define BPF_PROBE_MEM32SX 0xc0
+
 /* unused opcode to mark call to interpreter with arguments */
 #define BPF_CALL_ARGS	0xe0

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 28f5a7899bd6..917bada34c2f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20957,10 +20957,14 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 			continue;
 		case PTR_TO_ARENA:
 			if (BPF_MODE(insn->code) == BPF_MEMSX) {
-				verbose(env, "sign extending loads from arena are not supported yet\n");
-				return -EOPNOTSUPP;
+				if (!bpf_jit_supports_insn(insn, true)) {
+					verbose(env, "sign extending loads from arena are not supported yet\n");
+					return -EOPNOTSUPP;
+				}
+				insn->code = BPF_CLASS(insn->code) | BPF_PROBE_MEM32SX | BPF_SIZE(insn->code);
+			} else {
+				insn->code = BPF_CLASS(insn->code) | BPF_PROBE_MEM32 | BPF_SIZE(insn->code);
 			}
-			insn->code = BPF_CLASS(insn->code) | BPF_PROBE_MEM32 | BPF_SIZE(insn->code);
 			env->prog->aux->num_exentries++;
 			continue;
 		default:
--
2.47.1

Re: [PATCH bpf-next v2] bpf, x86: Add support for signed arena loads

[Alexei Starovoitov]

On Wed, May 14, 2025 at 10:54 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>  static void emit_stx(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, int off)
>  {
> @@ -2010,13 +2037,19 @@ st:                     if (is_imm8(insn->off))
>                 case BPF_LDX | BPF_PROBE_MEM32 | BPF_H:
>                 case BPF_LDX | BPF_PROBE_MEM32 | BPF_W:
>                 case BPF_LDX | BPF_PROBE_MEM32 | BPF_DW:
> +               case BPF_LDX | BPF_PROBE_MEM32SX | BPF_B:
> +               case BPF_LDX | BPF_PROBE_MEM32SX | BPF_H:
> +               case BPF_LDX | BPF_PROBE_MEM32SX | BPF_W:
>                 case BPF_STX | BPF_PROBE_MEM32 | BPF_B:
>                 case BPF_STX | BPF_PROBE_MEM32 | BPF_H:
>                 case BPF_STX | BPF_PROBE_MEM32 | BPF_W:
>                 case BPF_STX | BPF_PROBE_MEM32 | BPF_DW:
>                         start_of_ldx = prog;
>                         if (BPF_CLASS(insn->code) == BPF_LDX)
> -                               emit_ldx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
> +                               if (BPF_MODE(insn->code) == BPF_PROBE_MEM32SX)
> +                                       emit_ldsx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
> +                               else
> +                                       emit_ldx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
>                         else
>                                 emit_stx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
>  populate_extable:

Luckily I didn't trust CI and decided to test it manually:

./test_progs-cpuv4 -t arena_spin
[   68.977751] mem32 extable bug
[   68.984388] mem32 extable bug
[   69.182864] mem32 extable bug
[   69.190027] mem32 extable bug
[   69.408629] mem32 extable bug
[   69.415651] mem32 extable bug
libbpf: prog 'prog': BPF program load failed: -EINVAL
libbpf: prog 'prog': -- BEGIN PROG LOAD LOG --
Func#1 ('arena_spin_lock_slowpath') is safe for any args that match
its prototype
calling kernel functions are not allowed in non-JITed programs
processed 408 insns (limit 1000000) max_states_per_insn 1 total_states
42 peak_states 42 mark_read 7
-- END PROG LOAD LOG --

The verifier error is wrong.
The prog failed to JIT, but jit_subprog didn't return EFAULT
and the verifier tried to guess the error with:
        if (has_kfunc_call) {
                verbose(env, "calling kernel functions are not allowed
in non-JITed programs\n");
                return -EINVAL;
        }

and guessed it wrong,
but that is a separate issue.

The patch needs this fix:

index 70152200cc8c..a66c288dd812 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -21188,6 +21188,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
                        if (BPF_CLASS(insn->code) == BPF_LDX &&
                            (BPF_MODE(insn->code) == BPF_PROBE_MEM ||
                             BPF_MODE(insn->code) == BPF_PROBE_MEM32 ||
+                            BPF_MODE(insn->code) == BPF_PROBE_MEM32SX ||
                             BPF_MODE(insn->code) == BPF_PROBE_MEMSX))
                                num_exentries++;
                        if ((BPF_CLASS(insn->code) == BPF_STX ||


Before I tested it I thought we can apply this patch without
a new selftest, but that would have been a mistake.
We would have landed a half working sign extending loads :(

Please respin with the selftest.

pw-bot: cr

Re: [PATCH bpf-next v2] bpf, x86: Add support for signed arena loads

[Kumar Kartikeya Dwivedi]

On Wed, 14 May 2025 at 15:25, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, May 14, 2025 at 10:54 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >  static void emit_stx(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, int off)
> >  {
> > @@ -2010,13 +2037,19 @@ st:                     if (is_imm8(insn->off))
> >                 case BPF_LDX | BPF_PROBE_MEM32 | BPF_H:
> >                 case BPF_LDX | BPF_PROBE_MEM32 | BPF_W:
> >                 case BPF_LDX | BPF_PROBE_MEM32 | BPF_DW:
> > +               case BPF_LDX | BPF_PROBE_MEM32SX | BPF_B:
> > +               case BPF_LDX | BPF_PROBE_MEM32SX | BPF_H:
> > +               case BPF_LDX | BPF_PROBE_MEM32SX | BPF_W:
> >                 case BPF_STX | BPF_PROBE_MEM32 | BPF_B:
> >                 case BPF_STX | BPF_PROBE_MEM32 | BPF_H:
> >                 case BPF_STX | BPF_PROBE_MEM32 | BPF_W:
> >                 case BPF_STX | BPF_PROBE_MEM32 | BPF_DW:
> >                         start_of_ldx = prog;
> >                         if (BPF_CLASS(insn->code) == BPF_LDX)
> > -                               emit_ldx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
> > +                               if (BPF_MODE(insn->code) == BPF_PROBE_MEM32SX)
> > +                                       emit_ldsx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
> > +                               else
> > +                                       emit_ldx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
> >                         else
> >                                 emit_stx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
> >  populate_extable:
>
> Luckily I didn't trust CI and decided to test it manually:
>
> ./test_progs-cpuv4 -t arena_spin
> [   68.977751] mem32 extable bug
> [   68.984388] mem32 extable bug
> [   69.182864] mem32 extable bug
> [   69.190027] mem32 extable bug
> [   69.408629] mem32 extable bug
> [   69.415651] mem32 extable bug
> libbpf: prog 'prog': BPF program load failed: -EINVAL
> libbpf: prog 'prog': -- BEGIN PROG LOAD LOG --
> Func#1 ('arena_spin_lock_slowpath') is safe for any args that match
> its prototype
> calling kernel functions are not allowed in non-JITed programs
> processed 408 insns (limit 1000000) max_states_per_insn 1 total_states
> 42 peak_states 42 mark_read 7
> -- END PROG LOAD LOG --
>
> The verifier error is wrong.
> The prog failed to JIT, but jit_subprog didn't return EFAULT
> and the verifier tried to guess the error with:
>         if (has_kfunc_call) {
>                 verbose(env, "calling kernel functions are not allowed
> in non-JITed programs\n");
>                 return -EINVAL;
>         }
>
> and guessed it wrong,
> but that is a separate issue.
>
> The patch needs this fix:
>
> index 70152200cc8c..a66c288dd812 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -21188,6 +21188,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
>                         if (BPF_CLASS(insn->code) == BPF_LDX &&
>                             (BPF_MODE(insn->code) == BPF_PROBE_MEM ||
>                              BPF_MODE(insn->code) == BPF_PROBE_MEM32 ||
> +                            BPF_MODE(insn->code) == BPF_PROBE_MEM32SX ||
>                              BPF_MODE(insn->code) == BPF_PROBE_MEMSX))
>                                 num_exentries++;
>                         if ((BPF_CLASS(insn->code) == BPF_STX ||
>
>
> Before I tested it I thought we can apply this patch without
> a new selftest, but that would have been a mistake.
> We would have landed a half working sign extending loads :(
>
> Please respin with the selftest.

Hmm, weird.
I tested with an asm volatile sign extending load in arena_list before
sending out and didn't hit it.
That should have hit the extable too. It did fail on revert and
succeeded on applying the change.
But I'll add an explicit test.

>
> pw-bot: cr

Re: [PATCH bpf-next v2] bpf, x86: Add support for signed arena loads

[Eduard Zingerman]

Kumar Kartikeya Dwivedi <memxor@gmail.com> writes:

[...]

> +static void emit_ldsx_index(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, u32 index_reg, int off)
> +{
> +	u8 *prog = *pprog;
> +
> +	switch (size) {
> +	case BPF_B:
> +		/* movsx rax, byte ptr [rax + r12 + off] */
> +		EMIT3(add_3mod(0x40, src_reg, dst_reg, index_reg), 0x0F, 0xBE);
> +		break;
> +	case BPF_H:
> +		/* movsx rax, word ptr [rax + r12 + off] */
> +		EMIT3(add_3mod(0x40, src_reg, dst_reg, index_reg), 0x0F, 0xBF);
> +		break;
> +	case BPF_W:
> +		/* movsx rax, dword ptr [rax + r12 + off] */
> +		EMIT2(add_3mod(0x40, src_reg, dst_reg, index_reg), 0x63);
> +		break;
> +	}
> +	emit_insn_suffix_SIB(&prog, src_reg, dst_reg, index_reg, off);
> +	*pprog = prog;
> +}
> +

I tried the following test to see what disassembly looks like:

  SEC("syscall")
  __success
  __arch_x86_64
  __jited("movslq	0x10(%rax,%r12), %r14d")
  __jited("movswl	0x18(%rax,%r12), %r14d")
  __jited("movsbl	0x20(%rax,%r12), %r14d")
  __jited("movslq	0x10(%rdi,%r12), %r15d")
  __jited("movswl	0x18(%rdi,%r12), %r15d")
  __jited("movsbl	0x20(%rdi,%r12), %r15d")
  __naked void arena_ldsx_disasm(void *ctx)
  {
  	asm volatile (
  	"r1 = %[arena] ll;"
  	"r2 = 0;"
  	"r3 = 1;"
  	"r4 = %[numa_no_node];"
  	"r5 = 0;"
  	"call %[bpf_arena_alloc_pages];"
  	"r0 = addr_space_cast(r0, 0x0, 0x1);"
  	"r1 = r0;"
  	"r8 = *(s32 *)(r0 + 16);"
  	"r8 = *(s16 *)(r0 + 24);"
  	"r8 = *(s8  *)(r0 + 32);"
  	"r9 = *(s32 *)(r1 + 16);"
  	"r9 = *(s16 *)(r1 + 24);"
  	"r9 = *(s8  *)(r1 + 32);"
  	"r0 = 0;"
  	"exit;"
  	:: __imm(bpf_arena_alloc_pages),
  	   __imm_addr(arena),
  	   __imm_const(numa_no_node, NUMA_NO_NODE)
  	:  __clobber_all
  	);
  }

Disassembly shows instructions       movslq, movswl, movsbl.
While I'd expect that these shold be movslq, movswq, movsbq.
(Sign extend dword/word/byte to quad word).
These actually have different encodings, e.g.:

  46 0f be 74 20 20             movsbl  0x20(%rax,%r12), %r14d
  4e 0f be 74 20 20             movsbq  0x20(%rax,%r12), %r14

However, I can't conjure a test that shows a difference when loading and
sotring values. Is this just an instruction set oddity?

[...]