[PATCH bpf-next 00/11] Implement cpuv4 support for s390x

[PATCH bpf-next 00/11] Implement cpuv4 support for s390x

[Ilya Leoshkevich]

Hi,

This series adds the cpuv4 support to the s390x eBPF JIT.
Patches 1-4 are preliminary bugfixes.
Patches 5-9 implement the new instructions.
Patches 10-11 enable the tests.

Best regards,
Ilya

Ilya Leoshkevich (11):
  bpf: Disable zero-extension for BPF_MEMSX
  net: netfilter: Adjust timeouts of non-confirmed CTs in
    bpf_ct_insert_entry()
  selftests/bpf: Unmount the cgroup2 work directory
  selftests/bpf: Add big-endian support to the ldsx test
  s390/bpf: Implement BPF_MOV | BPF_X with sign-extension
  s390/bpf: Implement BPF_MEMSX
  s390/bpf: Implement unconditional byte swap
  s390/bpf: Implement unconditional jump with 32-bit offset
  s390/bpf: Implement signed division
  selftests/bpf: Enable the cpuv4 tests for s390x
  selftests/bpf: Trim DENYLIST.s390x

 arch/s390/net/bpf_jit_comp.c                  | 265 +++++++++++++-----
 kernel/bpf/verifier.c                         |   4 +-
 net/netfilter/nf_conntrack_bpf.c              |   2 +
 tools/testing/selftests/bpf/DENYLIST.s390x    |  25 --
 tools/testing/selftests/bpf/cgroup_helpers.c  |  33 ++-
 .../selftests/bpf/progs/test_ldsx_insn.c      |   9 +-
 .../selftests/bpf/progs/verifier_bswap.c      |   3 +-
 .../selftests/bpf/progs/verifier_gotol.c      |   3 +-
 .../selftests/bpf/progs/verifier_ldsx.c       | 149 ++++++----
 .../selftests/bpf/progs/verifier_movsx.c      |   3 +-
 .../selftests/bpf/progs/verifier_sdiv.c       |   3 +-
 11 files changed, 335 insertions(+), 164 deletions(-)

-- 
2.41.0

[PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX

[Ilya Leoshkevich]

On the architectures that use bpf_jit_needs_zext(), e.g., s390x, the
verifier incorrectly inserts a zero-extension after BPF_MEMSX, leading
to miscompilations like the one below:

      24:       89 1a ff fe 00 00 00 00 "r1 = *(s16 *)(r10 - 2);"       # zext_dst set
   0x3ff7fdb910e:       lgh     %r2,-2(%r13,%r0)                        # load halfword
   0x3ff7fdb9114:       llgfr   %r2,%r2                                 # wrong!
      25:       65 10 00 03 00 00 7f ff if r1 s> 32767 goto +3 <l0_1>   # check_cond_jmp_op()

Disable such zero-extensions. The JITs need to insert sign-extension
themselves, if necessary.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 kernel/bpf/verifier.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index bb78212fa5b2..097985a46edc 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3110,7 +3110,9 @@ static void mark_insn_zext(struct bpf_verifier_env *env,
 {
 	s32 def_idx = reg->subreg_def;
 
-	if (def_idx == DEF_NOT_SUBREG)
+	if (def_idx == DEF_NOT_SUBREG ||
+	    (BPF_CLASS(env->prog->insnsi[def_idx - 1].code) == BPF_LDX &&
+	     BPF_MODE(env->prog->insnsi[def_idx - 1].code) == BPF_MEMSX))
 		return;
 
 	env->insn_aux_data[def_idx - 1].zext_dst = true;
-- 
2.41.0

[PATCH bpf-next 02/11] net: netfilter: Adjust timeouts of non-confirmed CTs in bpf_ct_insert_entry()

[Ilya Leoshkevich]

bpf_nf testcase fails on s390x: bpf_skb_ct_lookup() cannot find the
entry that was added by bpf_ct_insert_entry() within the same BPF
function.

The reason is that this entry is deleted by nf_ct_gc_expired().

The CT timeout starts ticking after the CT confirmation; therefore
nf_conn.timeout is initially set to the timeout value, and
__nf_conntrack_confirm() sets it to the deadline value.
bpf_ct_insert_entry() sets IPS_CONFIRMED_BIT, but does not adjust the
timeout, making its value meaningless and causing false positives.

Fix the problem by making bpf_ct_insert_entry() adjust the timeout,
like __nf_conntrack_confirm().

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 net/netfilter/nf_conntrack_bpf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
index c7a6114091ae..b21799d468d2 100644
--- a/net/netfilter/nf_conntrack_bpf.c
+++ b/net/netfilter/nf_conntrack_bpf.c
@@ -381,6 +381,8 @@ __bpf_kfunc struct nf_conn *bpf_ct_insert_entry(struct nf_conn___init *nfct_i)
 	struct nf_conn *nfct = (struct nf_conn *)nfct_i;
 	int err;
 
+	if (!nf_ct_is_confirmed(nfct))
+		nfct->timeout += nfct_time_stamp;
 	nfct->status |= IPS_CONFIRMED;
 	err = nf_conntrack_hash_check_insert(nfct);
 	if (err < 0) {
-- 
2.41.0

[PATCH bpf-next 03/11] selftests/bpf: Unmount the cgroup2 work directory

[Ilya Leoshkevich]

test_progs -t bind_perm,bpf_obj_pinning/mounted-str-rel fails when
the selftests directory is mounted under /mnt, which is a reasonable
thing to do when sharing the selftests residing on the host with a
virtual machine, e.g., using 9p.

The reason is that cgroup2 is mounted at /mnt and not unmounted,
causing subsequent tests that need to access the selftests directory
to fail.

Fix by unmounting it. The kernel maintains a mount stack, so this
reveals what was mounted there before. Introduce cgroup_workdir_mounted
in order to maintain idempotency. Make it thread-local in order to
support test_progs -j.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/cgroup_helpers.c | 33 +++++++++++++++-----
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/bpf/cgroup_helpers.c b/tools/testing/selftests/bpf/cgroup_helpers.c
index 2caee8423ee0..24ba56d42f2d 100644
--- a/tools/testing/selftests/bpf/cgroup_helpers.c
+++ b/tools/testing/selftests/bpf/cgroup_helpers.c
@@ -49,6 +49,10 @@
 	snprintf(buf, sizeof(buf), "%s%s", NETCLS_MOUNT_PATH,	\
 		 CGROUP_WORK_DIR)
 
+static __thread bool cgroup_workdir_mounted;
+
+static void __cleanup_cgroup_environment(void);
+
 static int __enable_controllers(const char *cgroup_path, const char *controllers)
 {
 	char path[PATH_MAX + 1];
@@ -209,9 +213,10 @@ int setup_cgroup_environment(void)
 		log_err("mount cgroup2");
 		return 1;
 	}
+	cgroup_workdir_mounted = true;
 
 	/* Cleanup existing failed runs, now that the environment is setup */
-	cleanup_cgroup_environment();
+	__cleanup_cgroup_environment();
 
 	if (mkdir(cgroup_workdir, 0777) && errno != EEXIST) {
 		log_err("mkdir cgroup work dir");
@@ -305,11 +310,26 @@ int join_parent_cgroup(const char *relative_path)
 	return join_cgroup_from_top(cgroup_path);
 }
 
+/**
+ * __cleanup_cgroup_environment() - Delete temporary cgroups
+ *
+ * This is a helper for cleanup_cgroup_environment() that is responsible for
+ * deletion of all temporary cgroups that have been created during the test.
+ */
+static void __cleanup_cgroup_environment(void)
+{
+	char cgroup_workdir[PATH_MAX + 1];
+
+	format_cgroup_path(cgroup_workdir, "");
+	join_cgroup_from_top(CGROUP_MOUNT_PATH);
+	nftw(cgroup_workdir, nftwfunc, WALK_FD_LIMIT, FTW_DEPTH | FTW_MOUNT);
+}
+
 /**
  * cleanup_cgroup_environment() - Cleanup Cgroup Testing Environment
  *
  * This is an idempotent function to delete all temporary cgroups that
- * have been created during the test, including the cgroup testing work
+ * have been created during the test and unmount the cgroup testing work
  * directory.
  *
  * At call time, it moves the calling process to the root cgroup, and then
@@ -320,11 +340,10 @@ int join_parent_cgroup(const char *relative_path)
  */
 void cleanup_cgroup_environment(void)
 {
-	char cgroup_workdir[PATH_MAX + 1];
-
-	format_cgroup_path(cgroup_workdir, "");
-	join_cgroup_from_top(CGROUP_MOUNT_PATH);
-	nftw(cgroup_workdir, nftwfunc, WALK_FD_LIMIT, FTW_DEPTH | FTW_MOUNT);
+	__cleanup_cgroup_environment();
+	if (cgroup_workdir_mounted && umount(CGROUP_MOUNT_PATH))
+		log_err("umount cgroup2");
+	cgroup_workdir_mounted = false;
 }
 
 /**
-- 
2.41.0

[PATCH bpf-next 04/11] selftests/bpf: Add big-endian support to the ldsx test

[Ilya Leoshkevich]

Prepare the ldsx test to run on big-endian systems by adding the
necessary endianness checks around narrow memory accesses.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 .../selftests/bpf/progs/test_ldsx_insn.c      |   6 +-
 .../selftests/bpf/progs/verifier_ldsx.c       | 146 ++++++++++--------
 2 files changed, 90 insertions(+), 62 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/test_ldsx_insn.c b/tools/testing/selftests/bpf/progs/test_ldsx_insn.c
index 67c14ba1e87b..3709e5eb7dd0 100644
--- a/tools/testing/selftests/bpf/progs/test_ldsx_insn.c
+++ b/tools/testing/selftests/bpf/progs/test_ldsx_insn.c
@@ -104,7 +104,11 @@ int _tc(volatile struct __sk_buff *skb)
 		      "%[tmp_mark] = r1"
 		      : [tmp_mark]"=r"(tmp_mark)
 		      : [ctx]"r"(skb),
-			[off_mark]"i"(offsetof(struct __sk_buff, mark))
+			[off_mark]"i"(offsetof(struct __sk_buff, mark)
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+			+ sizeof(skb->mark) - 1
+#endif
+			)
 		      : "r1");
 #else
 	tmp_mark = (char)skb->mark;
diff --git a/tools/testing/selftests/bpf/progs/verifier_ldsx.c b/tools/testing/selftests/bpf/progs/verifier_ldsx.c
index 0c638f45aaf1..803dc1d492a7 100644
--- a/tools/testing/selftests/bpf/progs/verifier_ldsx.c
+++ b/tools/testing/selftests/bpf/progs/verifier_ldsx.c
@@ -12,12 +12,16 @@ __description("LDSX, S8")
 __success __success_unpriv __retval(-2)
 __naked void ldsx_s8(void)
 {
-	asm volatile ("					\
-	r1 = 0x3fe;					\
-	*(u64 *)(r10 - 8) = r1;				\
-	r0 = *(s8 *)(r10 - 8);				\
-	exit;						\
-"	::: __clobber_all);
+	asm volatile (
+	"r1 = 0x3fe;"
+	"*(u64 *)(r10 - 8) = r1;"
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+	"r0 = *(s8 *)(r10 - 8);"
+#else
+	"r0 = *(s8 *)(r10 - 1);"
+#endif
+	"exit;"
+	::: __clobber_all);
 }
 
 SEC("socket")
@@ -25,12 +29,16 @@ __description("LDSX, S16")
 __success __success_unpriv __retval(-2)
 __naked void ldsx_s16(void)
 {
-	asm volatile ("					\
-	r1 = 0x3fffe;					\
-	*(u64 *)(r10 - 8) = r1;				\
-	r0 = *(s16 *)(r10 - 8);				\
-	exit;						\
-"	::: __clobber_all);
+	asm volatile (
+	"r1 = 0x3fffe;"
+	"*(u64 *)(r10 - 8) = r1;"
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+	"r0 = *(s16 *)(r10 - 8);"
+#else
+	"r0 = *(s16 *)(r10 - 2);"
+#endif
+	"exit;"
+	::: __clobber_all);
 }
 
 SEC("socket")
@@ -38,13 +46,17 @@ __description("LDSX, S32")
 __success __success_unpriv __retval(-1)
 __naked void ldsx_s32(void)
 {
-	asm volatile ("					\
-	r1 = 0xfffffffe;				\
-	*(u64 *)(r10 - 8) = r1;				\
-	r0 = *(s32 *)(r10 - 8);				\
-	r0 >>= 1;					\
-	exit;						\
-"	::: __clobber_all);
+	asm volatile (
+	"r1 = 0xfffffffe;"
+	"*(u64 *)(r10 - 8) = r1;"
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+	"r0 = *(s32 *)(r10 - 8);"
+#else
+	"r0 = *(s32 *)(r10 - 4);"
+#endif
+	"r0 >>= 1;"
+	"exit;"
+	::: __clobber_all);
 }
 
 SEC("socket")
@@ -53,20 +65,24 @@ __log_level(2) __success __retval(1)
 __msg("R1_w=scalar(smin=-128,smax=127)")
 __naked void ldsx_s8_range_priv(void)
 {
-	asm volatile ("					\
-	call %[bpf_get_prandom_u32];			\
-	*(u64 *)(r10 - 8) = r0;				\
-	r1 = *(s8 *)(r10 - 8);				\
-	/* r1 with s8 range */				\
-	if r1 s> 0x7f goto l0_%=;			\
-	if r1 s< -0x80 goto l0_%=;			\
-	r0 = 1;						\
-l1_%=:							\
-	exit;						\
-l0_%=:							\
-	r0 = 2;						\
-	goto l1_%=;					\
-"	:
+	asm volatile (
+	"call %[bpf_get_prandom_u32];"
+	"*(u64 *)(r10 - 8) = r0;"
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+	"r1 = *(s8 *)(r10 - 8);"
+#else
+	"r1 = *(s8 *)(r10 - 1);"
+#endif
+	/* r1 with s8 range */
+	"if r1 s> 0x7f goto l0_%=;"
+	"if r1 s< -0x80 goto l0_%=;"
+	"r0 = 1;"
+"l1_%=:"
+	"exit;"
+"l0_%=:"
+	"r0 = 2;"
+	"goto l1_%=;"
+	:
 	: __imm(bpf_get_prandom_u32)
 	: __clobber_all);
 }
@@ -76,20 +92,24 @@ __description("LDSX, S16 range checking")
 __success __success_unpriv __retval(1)
 __naked void ldsx_s16_range(void)
 {
-	asm volatile ("					\
-	call %[bpf_get_prandom_u32];			\
-	*(u64 *)(r10 - 8) = r0;				\
-	r1 = *(s16 *)(r10 - 8);				\
-	/* r1 with s16 range */				\
-	if r1 s> 0x7fff goto l0_%=;			\
-	if r1 s< -0x8000 goto l0_%=;			\
-	r0 = 1;						\
-l1_%=:							\
-	exit;						\
-l0_%=:							\
-	r0 = 2;						\
-	goto l1_%=;					\
-"	:
+	asm volatile (
+	"call %[bpf_get_prandom_u32];"
+	"*(u64 *)(r10 - 8) = r0;"
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+	"r1 = *(s16 *)(r10 - 8);"
+#else
+	"r1 = *(s16 *)(r10 - 2);"
+#endif
+	/* r1 with s16 range */
+	"if r1 s> 0x7fff goto l0_%=;"
+	"if r1 s< -0x8000 goto l0_%=;"
+	"r0 = 1;"
+"l1_%=:"
+	"exit;"
+"l0_%=:"
+	"r0 = 2;"
+	"goto l1_%=;"
+	:
 	: __imm(bpf_get_prandom_u32)
 	: __clobber_all);
 }
@@ -99,20 +119,24 @@ __description("LDSX, S32 range checking")
 __success __success_unpriv __retval(1)
 __naked void ldsx_s32_range(void)
 {
-	asm volatile ("					\
-	call %[bpf_get_prandom_u32];			\
-	*(u64 *)(r10 - 8) = r0;				\
-	r1 = *(s32 *)(r10 - 8);				\
-	/* r1 with s16 range */				\
-	if r1 s> 0x7fffFFFF goto l0_%=;			\
-	if r1 s< -0x80000000 goto l0_%=;		\
-	r0 = 1;						\
-l1_%=:							\
-	exit;						\
-l0_%=:							\
-	r0 = 2;						\
-	goto l1_%=;					\
-"	:
+	asm volatile (
+	"call %[bpf_get_prandom_u32];"
+	"*(u64 *)(r10 - 8) = r0;"
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+	"r1 = *(s32 *)(r10 - 8);"
+#else
+	"r1 = *(s32 *)(r10 - 4);"
+#endif
+	/* r1 with s16 range */
+	"if r1 s> 0x7fffFFFF goto l0_%=;"
+	"if r1 s< -0x80000000 goto l0_%=;"
+	"r0 = 1;"
+"l1_%=:"
+	"exit;"
+"l0_%=:"
+	"r0 = 2;"
+	"goto l1_%=;"
+	:
 	: __imm(bpf_get_prandom_u32)
 	: __clobber_all);
 }
-- 
2.41.0

[PATCH bpf-next 05/11] s390/bpf: Implement BPF_MOV | BPF_X with sign-extension

[Ilya Leoshkevich]

Implement the cpuv4 register-to-register move with sign extension. It
is distinguished from the normal moves by non-zero values in
insn->off, which determine the source size. s390x has instructions to
deal with all of them: lbr, lhr, lgbr, lghr and lgfr.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/net/bpf_jit_comp.c | 48 ++++++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index 5e9371fbf3d5..da6fb4669973 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -795,15 +795,47 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 	/*
 	 * BPF_MOV
 	 */
-	case BPF_ALU | BPF_MOV | BPF_X: /* dst = (u32) src */
-		/* llgfr %dst,%src */
-		EMIT4(0xb9160000, dst_reg, src_reg);
-		if (insn_is_zext(&insn[1]))
-			insn_count = 2;
+	case BPF_ALU | BPF_MOV | BPF_X:
+		switch (insn->off) {
+		case 0: /* DST = (u32) SRC */
+			/* llgfr %dst,%src */
+			EMIT4(0xb9160000, dst_reg, src_reg);
+			if (insn_is_zext(&insn[1]))
+				insn_count = 2;
+			break;
+		case 8: /* DST = (u32)(s8) SRC */
+			/* lbr %dst,%src */
+			EMIT4(0xb9260000, dst_reg, src_reg);
+			/* llgfr %dst,%dst */
+			EMIT4(0xb9160000, dst_reg, dst_reg);
+			break;
+		case 16: /* DST = (u32)(s16) SRC */
+			/* lhr %dst,%src */
+			EMIT4(0xb9270000, dst_reg, src_reg);
+			/* llgfr %dst,%dst */
+			EMIT4(0xb9160000, dst_reg, dst_reg);
+			break;
+		}
 		break;
-	case BPF_ALU64 | BPF_MOV | BPF_X: /* dst = src */
-		/* lgr %dst,%src */
-		EMIT4(0xb9040000, dst_reg, src_reg);
+	case BPF_ALU64 | BPF_MOV | BPF_X:
+		switch (insn->off) {
+		case 0: /* DST = SRC */
+			/* lgr %dst,%src */
+			EMIT4(0xb9040000, dst_reg, src_reg);
+			break;
+		case 8: /* DST = (s8) SRC */
+			/* lgbr %dst,%src */
+			EMIT4(0xb9060000, dst_reg, src_reg);
+			break;
+		case 16: /* DST = (s16) SRC */
+			/* lghr %dst,%src */
+			EMIT4(0xb9070000, dst_reg, src_reg);
+			break;
+		case 32: /* DST = (s32) SRC */
+			/* lgfr %dst,%src */
+			EMIT4(0xb9140000, dst_reg, src_reg);
+			break;
+		}
 		break;
 	case BPF_ALU | BPF_MOV | BPF_K: /* dst = (u32) imm */
 		/* llilf %dst,imm */
-- 
2.41.0

[PATCH bpf-next 06/11] s390/bpf: Implement BPF_MEMSX

[Ilya Leoshkevich]

Implement the cpuv4 load with sign-extension, which is encoded as
BPF_MEMSX (and, for internal uses cases only, BPF_PROBE_MEMSX).

This is the same as BPF_MEM and BPF_PROBE_MEM, but with sign
extension instead of zero extension, and s390x has the necessary
instructions: lgb, lgh and lgf.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/net/bpf_jit_comp.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index da6fb4669973..841c5f8ead15 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -670,15 +670,18 @@ static void bpf_jit_epilogue(struct bpf_jit *jit, u32 stack_depth)
 static int get_probe_mem_regno(const u8 *insn)
 {
 	/*
-	 * insn must point to llgc, llgh, llgf or lg, which have destination
-	 * register at the same position.
+	 * insn must point to llgc, llgh, llgf, lg, lgb, lgh or lgf, which have
+	 * destination register at the same position.
 	 */
-	if (insn[0] != 0xe3) /* common llgc, llgh, llgf and lg prefix */
+	if (insn[0] != 0xe3) /* common prefix */
 		return -1;
 	if (insn[5] != 0x90 && /* llgc */
 	    insn[5] != 0x91 && /* llgh */
 	    insn[5] != 0x16 && /* llgf */
-	    insn[5] != 0x04) /* lg */
+	    insn[5] != 0x04 && /* lg */
+	    insn[5] != 0x77 && /* lgb */
+	    insn[5] != 0x15 && /* lgh */
+	    insn[5] != 0x14) /* lgf */
 		return -1;
 	return insn[1] >> 4;
 }
@@ -788,7 +791,8 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 	int err;
 
 	if (BPF_CLASS(insn->code) == BPF_LDX &&
-	    BPF_MODE(insn->code) == BPF_PROBE_MEM)
+	    (BPF_MODE(insn->code) == BPF_PROBE_MEM ||
+	     BPF_MODE(insn->code) == BPF_PROBE_MEMSX))
 		probe_prg = jit->prg;
 
 	switch (insn->code) {
@@ -1406,6 +1410,12 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 		if (insn_is_zext(&insn[1]))
 			insn_count = 2;
 		break;
+	case BPF_LDX | BPF_MEMSX | BPF_B: /* dst = *(s8 *)(ul) (src + off) */
+	case BPF_LDX | BPF_PROBE_MEMSX | BPF_B:
+		/* lgb %dst,0(off,%src) */
+		EMIT6_DISP_LH(0xe3000000, 0x0077, dst_reg, src_reg, REG_0, off);
+		jit->seen |= SEEN_MEM;
+		break;
 	case BPF_LDX | BPF_MEM | BPF_H: /* dst = *(u16 *)(ul) (src + off) */
 	case BPF_LDX | BPF_PROBE_MEM | BPF_H:
 		/* llgh %dst,0(off,%src) */
@@ -1414,6 +1424,12 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 		if (insn_is_zext(&insn[1]))
 			insn_count = 2;
 		break;
+	case BPF_LDX | BPF_MEMSX | BPF_H: /* dst = *(s16 *)(ul) (src + off) */
+	case BPF_LDX | BPF_PROBE_MEMSX | BPF_H:
+		/* lgh %dst,0(off,%src) */
+		EMIT6_DISP_LH(0xe3000000, 0x0015, dst_reg, src_reg, REG_0, off);
+		jit->seen |= SEEN_MEM;
+		break;
 	case BPF_LDX | BPF_MEM | BPF_W: /* dst = *(u32 *)(ul) (src + off) */
 	case BPF_LDX | BPF_PROBE_MEM | BPF_W:
 		/* llgf %dst,off(%src) */
@@ -1422,6 +1438,12 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 		if (insn_is_zext(&insn[1]))
 			insn_count = 2;
 		break;
+	case BPF_LDX | BPF_MEMSX | BPF_W: /* dst = *(s32 *)(ul) (src + off) */
+	case BPF_LDX | BPF_PROBE_MEMSX | BPF_W:
+		/* lgf %dst,off(%src) */
+		jit->seen |= SEEN_MEM;
+		EMIT6_DISP_LH(0xe3000000, 0x0014, dst_reg, src_reg, REG_0, off);
+		break;
 	case BPF_LDX | BPF_MEM | BPF_DW: /* dst = *(u64 *)(ul) (src + off) */
 	case BPF_LDX | BPF_PROBE_MEM | BPF_DW:
 		/* lg %dst,0(off,%src) */
-- 
2.41.0

[PATCH bpf-next 07/11] s390/bpf: Implement unconditional byte swap

[Ilya Leoshkevich]

Implement the cpuv4 unconditional byte swap, which is encoded as
BPF_ALU64 | BPF_END | BPF_FROM_LE. Since s390x is big-endian, it's
the same as the existing BPF_ALU | BPF_END | BPF_FROM_LE.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/net/bpf_jit_comp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index 841c5f8ead15..4c4cf8c9fec6 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -1253,6 +1253,7 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 		}
 		break;
 	case BPF_ALU | BPF_END | BPF_FROM_LE:
+	case BPF_ALU64 | BPF_END | BPF_FROM_LE:
 		switch (imm) {
 		case 16: /* dst = (u16) cpu_to_le16(dst) */
 			/* lrvr %dst,%dst */
-- 
2.41.0

[PATCH bpf-next 08/11] s390/bpf: Implement unconditional jump with 32-bit offset

[Ilya Leoshkevich]

Implement the cpuv4 unconditional jump with 32-bit offset, which is
encoded as BPF_JMP32 | BPF_JA and stores the offset in the imm field.
Reuse the existing BPF_JMP | BPF_JA logic.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/net/bpf_jit_comp.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index 4c4cf8c9fec6..b331ab013cfd 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -779,6 +779,7 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 				 int i, bool extra_pass, u32 stack_depth)
 {
 	struct bpf_insn *insn = &fp->insnsi[i];
+	s16 branch_oc_off = insn->off;
 	u32 dst_reg = insn->dst_reg;
 	u32 src_reg = insn->src_reg;
 	int last, insn_count = 1;
@@ -1625,6 +1626,9 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 	 * instruction itself (loop) and for BPF with offset 0 we
 	 * branch to the instruction behind the branch.
 	 */
+	case BPF_JMP32 | BPF_JA: /* if (true) */
+		branch_oc_off = imm;
+		fallthrough;
 	case BPF_JMP | BPF_JA: /* if (true) */
 		mask = 0xf000; /* j */
 		goto branch_oc;
@@ -1793,14 +1797,16 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 		break;
 branch_oc:
 		if (!is_first_pass(jit) &&
-		    can_use_rel(jit, addrs[i + off + 1])) {
+		    can_use_rel(jit, addrs[i + branch_oc_off + 1])) {
 			/* brc mask,off */
 			EMIT4_PCREL_RIC(0xa7040000,
-					mask >> 12, addrs[i + off + 1]);
+					mask >> 12,
+					addrs[i + branch_oc_off + 1]);
 		} else {
 			/* brcl mask,off */
 			EMIT6_PCREL_RILC(0xc0040000,
-					 mask >> 12, addrs[i + off + 1]);
+					 mask >> 12,
+					 addrs[i + branch_oc_off + 1]);
 		}
 		break;
 	}
-- 
2.41.0

[PATCH bpf-next 09/11] s390/bpf: Implement signed division

[Ilya Leoshkevich]

Implement the cpuv4 signed division. It is encoded as unsigned
division, but with off field set to 1. s390x has the necessary
instructions: dsgfr, dsgf and dsgr.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/net/bpf_jit_comp.c | 172 +++++++++++++++++++++++++----------
 1 file changed, 125 insertions(+), 47 deletions(-)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index b331ab013cfd..cbbb82a63975 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -949,66 +949,115 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 	/*
 	 * BPF_DIV / BPF_MOD
 	 */
-	case BPF_ALU | BPF_DIV | BPF_X: /* dst = (u32) dst / (u32) src */
-	case BPF_ALU | BPF_MOD | BPF_X: /* dst = (u32) dst % (u32) src */
+	case BPF_ALU | BPF_DIV | BPF_X:
+	case BPF_ALU | BPF_MOD | BPF_X:
 	{
 		int rc_reg = BPF_OP(insn->code) == BPF_DIV ? REG_W1 : REG_W0;
 
-		/* lhi %w0,0 */
-		EMIT4_IMM(0xa7080000, REG_W0, 0);
-		/* lr %w1,%dst */
-		EMIT2(0x1800, REG_W1, dst_reg);
-		/* dlr %w0,%src */
-		EMIT4(0xb9970000, REG_W0, src_reg);
+		switch (off) {
+		case 0: /* dst = (u32) dst {/,%} (u32) src */
+			/* xr %w0,%w0 */
+			EMIT2(0x1700, REG_W0, REG_W0);
+			/* lr %w1,%dst */
+			EMIT2(0x1800, REG_W1, dst_reg);
+			/* dlr %w0,%src */
+			EMIT4(0xb9970000, REG_W0, src_reg);
+			break;
+		case 1: /* dst = (u32) ((s32) dst {/,%} (s32) src) */
+			/* lgfr %r1,%dst */
+			EMIT4(0xb9140000, REG_W1, dst_reg);
+			/* dsgfr %r0,%src */
+			EMIT4(0xb91d0000, REG_W0, src_reg);
+			break;
+		}
 		/* llgfr %dst,%rc */
 		EMIT4(0xb9160000, dst_reg, rc_reg);
 		if (insn_is_zext(&insn[1]))
 			insn_count = 2;
 		break;
 	}
-	case BPF_ALU64 | BPF_DIV | BPF_X: /* dst = dst / src */
-	case BPF_ALU64 | BPF_MOD | BPF_X: /* dst = dst % src */
+	case BPF_ALU64 | BPF_DIV | BPF_X:
+	case BPF_ALU64 | BPF_MOD | BPF_X:
 	{
 		int rc_reg = BPF_OP(insn->code) == BPF_DIV ? REG_W1 : REG_W0;
 
-		/* lghi %w0,0 */
-		EMIT4_IMM(0xa7090000, REG_W0, 0);
-		/* lgr %w1,%dst */
-		EMIT4(0xb9040000, REG_W1, dst_reg);
-		/* dlgr %w0,%dst */
-		EMIT4(0xb9870000, REG_W0, src_reg);
+		switch (off) {
+		case 0: /* dst = dst {/,%} src */
+			/* lghi %w0,0 */
+			EMIT4_IMM(0xa7090000, REG_W0, 0);
+			/* lgr %w1,%dst */
+			EMIT4(0xb9040000, REG_W1, dst_reg);
+			/* dlgr %w0,%src */
+			EMIT4(0xb9870000, REG_W0, src_reg);
+			break;
+		case 1: /* dst = (s64) dst {/,%} (s64) src */
+			/* lgr %w1,%dst */
+			EMIT4(0xb9040000, REG_W1, dst_reg);
+			/* dsgr %w0,%src */
+			EMIT4(0xb90d0000, REG_W0, src_reg);
+			break;
+		}
 		/* lgr %dst,%rc */
 		EMIT4(0xb9040000, dst_reg, rc_reg);
 		break;
 	}
-	case BPF_ALU | BPF_DIV | BPF_K: /* dst = (u32) dst / (u32) imm */
-	case BPF_ALU | BPF_MOD | BPF_K: /* dst = (u32) dst % (u32) imm */
+	case BPF_ALU | BPF_DIV | BPF_K:
+	case BPF_ALU | BPF_MOD | BPF_K:
 	{
 		int rc_reg = BPF_OP(insn->code) == BPF_DIV ? REG_W1 : REG_W0;
 
 		if (imm == 1) {
 			if (BPF_OP(insn->code) == BPF_MOD)
-				/* lhgi %dst,0 */
+				/* lghi %dst,0 */
 				EMIT4_IMM(0xa7090000, dst_reg, 0);
 			else
 				EMIT_ZERO(dst_reg);
 			break;
 		}
-		/* lhi %w0,0 */
-		EMIT4_IMM(0xa7080000, REG_W0, 0);
-		/* lr %w1,%dst */
-		EMIT2(0x1800, REG_W1, dst_reg);
 		if (!is_first_pass(jit) && can_use_ldisp_for_lit32(jit)) {
-			/* dl %w0,<d(imm)>(%l) */
-			EMIT6_DISP_LH(0xe3000000, 0x0097, REG_W0, REG_0, REG_L,
-				      EMIT_CONST_U32(imm));
+			switch (off) {
+			case 0: /* dst = (u32) dst {/,%} (u32) imm */
+				/* xr %w0,%w0 */
+				EMIT2(0x1700, REG_W0, REG_W0);
+				/* lr %w1,%dst */
+				EMIT2(0x1800, REG_W1, dst_reg);
+				/* dl %w0,<d(imm)>(%l) */
+				EMIT6_DISP_LH(0xe3000000, 0x0097, REG_W0, REG_0,
+					      REG_L, EMIT_CONST_U32(imm));
+				break;
+			case 1: /* dst = (s32) dst {/,%} (s32) imm */
+				/* lgfr %r1,%dst */
+				EMIT4(0xb9140000, REG_W1, dst_reg);
+				/* dsgf %r0,<d(imm)>(%l) */
+				EMIT6_DISP_LH(0xe3000000, 0x001d, REG_W0, REG_0,
+					      REG_L, EMIT_CONST_U32(imm));
+				break;
+			}
 		} else {
-			/* lgfrl %dst,imm */
-			EMIT6_PCREL_RILB(0xc40c0000, dst_reg,
-					 _EMIT_CONST_U32(imm));
-			jit->seen |= SEEN_LITERAL;
-			/* dlr %w0,%dst */
-			EMIT4(0xb9970000, REG_W0, dst_reg);
+			switch (off) {
+			case 0: /* dst = (u32) dst {/,%} (u32) imm */
+				/* xr %w0,%w0 */
+				EMIT2(0x1700, REG_W0, REG_W0);
+				/* lr %w1,%dst */
+				EMIT2(0x1800, REG_W1, dst_reg);
+				/* lrl %dst,imm */
+				EMIT6_PCREL_RILB(0xc40d0000, dst_reg,
+						 _EMIT_CONST_U32(imm));
+				jit->seen |= SEEN_LITERAL;
+				/* dlr %w0,%dst */
+				EMIT4(0xb9970000, REG_W0, dst_reg);
+				break;
+			case 1: /* dst = (s32) dst {/,%} (s32) imm */
+				/* lgfr %w1,%dst */
+				EMIT4(0xb9140000, REG_W1, dst_reg);
+				/* lgfrl %dst,imm */
+				EMIT6_PCREL_RILB(0xc40c0000, dst_reg,
+						 _EMIT_CONST_U32(imm));
+				jit->seen |= SEEN_LITERAL;
+				/* dsgr %w0,%dst */
+				EMIT4(0xb90d0000, REG_W0, dst_reg);
+				break;
+			}
 		}
 		/* llgfr %dst,%rc */
 		EMIT4(0xb9160000, dst_reg, rc_reg);
@@ -1016,8 +1065,8 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 			insn_count = 2;
 		break;
 	}
-	case BPF_ALU64 | BPF_DIV | BPF_K: /* dst = dst / imm */
-	case BPF_ALU64 | BPF_MOD | BPF_K: /* dst = dst % imm */
+	case BPF_ALU64 | BPF_DIV | BPF_K:
+	case BPF_ALU64 | BPF_MOD | BPF_K:
 	{
 		int rc_reg = BPF_OP(insn->code) == BPF_DIV ? REG_W1 : REG_W0;
 
@@ -1027,21 +1076,50 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 				EMIT4_IMM(0xa7090000, dst_reg, 0);
 			break;
 		}
-		/* lghi %w0,0 */
-		EMIT4_IMM(0xa7090000, REG_W0, 0);
-		/* lgr %w1,%dst */
-		EMIT4(0xb9040000, REG_W1, dst_reg);
 		if (!is_first_pass(jit) && can_use_ldisp_for_lit64(jit)) {
-			/* dlg %w0,<d(imm)>(%l) */
-			EMIT6_DISP_LH(0xe3000000, 0x0087, REG_W0, REG_0, REG_L,
-				      EMIT_CONST_U64(imm));
+			switch (off) {
+			case 0: /* dst = dst {/,%} imm */
+				/* lghi %w0,0 */
+				EMIT4_IMM(0xa7090000, REG_W0, 0);
+				/* lgr %w1,%dst */
+				EMIT4(0xb9040000, REG_W1, dst_reg);
+				/* dlg %w0,<d(imm)>(%l) */
+				EMIT6_DISP_LH(0xe3000000, 0x0087, REG_W0, REG_0,
+					      REG_L, EMIT_CONST_U64(imm));
+				break;
+			case 1: /* dst = (s64) dst {/,%} (s64) imm */
+				/* lgr %w1,%dst */
+				EMIT4(0xb9040000, REG_W1, dst_reg);
+				/* dsg %w0,<d(imm)>(%l) */
+				EMIT6_DISP_LH(0xe3000000, 0x000d, REG_W0, REG_0,
+					      REG_L, EMIT_CONST_U64(imm));
+				break;
+			}
 		} else {
-			/* lgrl %dst,imm */
-			EMIT6_PCREL_RILB(0xc4080000, dst_reg,
-					 _EMIT_CONST_U64(imm));
-			jit->seen |= SEEN_LITERAL;
-			/* dlgr %w0,%dst */
-			EMIT4(0xb9870000, REG_W0, dst_reg);
+			switch (off) {
+			case 0: /* dst = dst {/,%} imm */
+				/* lghi %w0,0 */
+				EMIT4_IMM(0xa7090000, REG_W0, 0);
+				/* lgr %w1,%dst */
+				EMIT4(0xb9040000, REG_W1, dst_reg);
+				/* lgrl %dst,imm */
+				EMIT6_PCREL_RILB(0xc4080000, dst_reg,
+						 _EMIT_CONST_U64(imm));
+				jit->seen |= SEEN_LITERAL;
+				/* dlgr %w0,%dst */
+				EMIT4(0xb9870000, REG_W0, dst_reg);
+				break;
+			case 1: /* dst = (s64) dst {/,%} (s64) imm */
+				/* lgr %w1,%dst */
+				EMIT4(0xb9040000, REG_W1, dst_reg);
+				/* lgrl %dst,imm */
+				EMIT6_PCREL_RILB(0xc4080000, dst_reg,
+						 _EMIT_CONST_U64(imm));
+				jit->seen |= SEEN_LITERAL;
+				/* dsgr %w0,%dst */
+				EMIT4(0xb90d0000, REG_W0, dst_reg);
+				break;
+			}
 		}
 		/* lgr %dst,%rc */
 		EMIT4(0xb9040000, dst_reg, rc_reg);
-- 
2.41.0

[PATCH bpf-next 10/11] selftests/bpf: Enable the cpuv4 tests for s390x

[Ilya Leoshkevich]

Now that all the cpuv4 support is in place, enable the tests.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/progs/test_ldsx_insn.c | 3 ++-
 tools/testing/selftests/bpf/progs/verifier_bswap.c | 3 ++-
 tools/testing/selftests/bpf/progs/verifier_gotol.c | 3 ++-
 tools/testing/selftests/bpf/progs/verifier_ldsx.c  | 3 ++-
 tools/testing/selftests/bpf/progs/verifier_movsx.c | 3 ++-
 tools/testing/selftests/bpf/progs/verifier_sdiv.c  | 3 ++-
 6 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/test_ldsx_insn.c b/tools/testing/selftests/bpf/progs/test_ldsx_insn.c
index 3709e5eb7dd0..3ddcb3777912 100644
--- a/tools/testing/selftests/bpf/progs/test_ldsx_insn.c
+++ b/tools/testing/selftests/bpf/progs/test_ldsx_insn.c
@@ -6,7 +6,8 @@
 #include <bpf/bpf_tracing.h>
 
 #if (defined(__TARGET_ARCH_arm64) || defined(__TARGET_ARCH_x86) || \
-     (defined(__TARGET_ARCH_riscv) && __riscv_xlen == 64)) && __clang_major__ >= 18
+     (defined(__TARGET_ARCH_riscv) && __riscv_xlen == 64) ||       \
+     defined(__TARGET_ARCH_s390)) && __clang_major__ >= 18
 const volatile int skip = 0;
 #else
 const volatile int skip = 1;
diff --git a/tools/testing/selftests/bpf/progs/verifier_bswap.c b/tools/testing/selftests/bpf/progs/verifier_bswap.c
index 8893094725f0..148b25868c34 100644
--- a/tools/testing/selftests/bpf/progs/verifier_bswap.c
+++ b/tools/testing/selftests/bpf/progs/verifier_bswap.c
@@ -5,7 +5,8 @@
 #include "bpf_misc.h"
 
 #if (defined(__TARGET_ARCH_arm64) || defined(__TARGET_ARCH_x86) || \
-     (defined(__TARGET_ARCH_riscv) && __riscv_xlen == 64)) && __clang_major__ >= 18
+     (defined(__TARGET_ARCH_riscv) && __riscv_xlen == 64) ||       \
+     defined(__TARGET_ARCH_s390)) && __clang_major__ >= 18
 
 SEC("socket")
 __description("BSWAP, 16")
diff --git a/tools/testing/selftests/bpf/progs/verifier_gotol.c b/tools/testing/selftests/bpf/progs/verifier_gotol.c
index 2dae5322a18e..46fe64121e50 100644
--- a/tools/testing/selftests/bpf/progs/verifier_gotol.c
+++ b/tools/testing/selftests/bpf/progs/verifier_gotol.c
@@ -5,7 +5,8 @@
 #include "bpf_misc.h"
 
 #if (defined(__TARGET_ARCH_arm64) || defined(__TARGET_ARCH_x86) || \
-     (defined(__TARGET_ARCH_riscv) && __riscv_xlen == 64)) && __clang_major__ >= 18
+     (defined(__TARGET_ARCH_riscv) && __riscv_xlen == 64) ||       \
+     defined(__TARGET_ARCH_s390)) && __clang_major__ >= 18
 
 SEC("socket")
 __description("gotol, small_imm")
diff --git a/tools/testing/selftests/bpf/progs/verifier_ldsx.c b/tools/testing/selftests/bpf/progs/verifier_ldsx.c
index 803dc1d492a7..c261f34876d6 100644
--- a/tools/testing/selftests/bpf/progs/verifier_ldsx.c
+++ b/tools/testing/selftests/bpf/progs/verifier_ldsx.c
@@ -5,7 +5,8 @@
 #include "bpf_misc.h"
 
 #if (defined(__TARGET_ARCH_arm64) || defined(__TARGET_ARCH_x86) || \
-     (defined(__TARGET_ARCH_riscv) && __riscv_xlen == 64)) && __clang_major__ >= 18
+     (defined(__TARGET_ARCH_riscv) && __riscv_xlen == 64) ||       \
+     defined(__TARGET_ARCH_s390)) && __clang_major__ >= 18
 
 SEC("socket")
 __description("LDSX, S8")
diff --git a/tools/testing/selftests/bpf/progs/verifier_movsx.c b/tools/testing/selftests/bpf/progs/verifier_movsx.c
index 3c8ac2c57b1b..4086147abdd7 100644
--- a/tools/testing/selftests/bpf/progs/verifier_movsx.c
+++ b/tools/testing/selftests/bpf/progs/verifier_movsx.c
@@ -5,7 +5,8 @@
 #include "bpf_misc.h"
 
 #if (defined(__TARGET_ARCH_arm64) || defined(__TARGET_ARCH_x86) || \
-     (defined(__TARGET_ARCH_riscv) && __riscv_xlen == 64)) && __clang_major__ >= 18
+     (defined(__TARGET_ARCH_riscv) && __riscv_xlen == 64) ||       \
+     defined(__TARGET_ARCH_s390)) && __clang_major__ >= 18
 
 SEC("socket")
 __description("MOV32SX, S8")
diff --git a/tools/testing/selftests/bpf/progs/verifier_sdiv.c b/tools/testing/selftests/bpf/progs/verifier_sdiv.c
index 0990f8825675..d52dcdbf6e7e 100644
--- a/tools/testing/selftests/bpf/progs/verifier_sdiv.c
+++ b/tools/testing/selftests/bpf/progs/verifier_sdiv.c
@@ -5,7 +5,8 @@
 #include "bpf_misc.h"
 
 #if (defined(__TARGET_ARCH_arm64) || defined(__TARGET_ARCH_x86) || \
-     (defined(__TARGET_ARCH_riscv) && __riscv_xlen == 64)) && __clang_major__ >= 18
+     (defined(__TARGET_ARCH_riscv) && __riscv_xlen == 64) ||       \
+     defined(__TARGET_ARCH_s390)) && __clang_major__ >= 18
 
 SEC("socket")
 __description("SDIV32, non-zero imm divisor, check 1")
-- 
2.41.0

[PATCH bpf-next 11/11] selftests/bpf: Trim DENYLIST.s390x

[Ilya Leoshkevich]

Enable all selftests, except the 2 that have to do with the userspace
unwinding, in the s390x CI.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/DENYLIST.s390x | 25 ----------------------
 1 file changed, 25 deletions(-)

diff --git a/tools/testing/selftests/bpf/DENYLIST.s390x b/tools/testing/selftests/bpf/DENYLIST.s390x
index 5061d9e24c16..a17baf8c6fd7 100644
--- a/tools/testing/selftests/bpf/DENYLIST.s390x
+++ b/tools/testing/selftests/bpf/DENYLIST.s390x
@@ -1,29 +1,4 @@
 # TEMPORARY
 # Alphabetical order
-bloom_filter_map                         # failed to find kernel BTF type ID of '__x64_sys_getpgid': -3                (?)
-bpf_cookie                               # failed to open_and_load program: -524 (trampoline)
-bpf_loop                                 # attaches to __x64_sys_nanosleep
-cgrp_local_storage                       # prog_attach unexpected error: -524                                          (trampoline)
-dynptr/test_dynptr_skb_data
-dynptr/test_skb_readonly
-fexit_sleep                              # fexit_skel_load fexit skeleton failed                                       (trampoline)
 get_stack_raw_tp                         # user_stack corrupted user stack                                             (no backchain userspace)
-iters/testmod_seq*                       # s390x doesn't support kfuncs in modules yet
-kprobe_multi_bench_attach                # bpf_program__attach_kprobe_multi_opts unexpected error: -95
-kprobe_multi_test                        # relies on fentry
-ksyms_btf/weak_ksyms*                    # test_ksyms_weak__open_and_load unexpected error: -22                        (kfunc)
-ksyms_module                             # test_ksyms_module__open_and_load unexpected error: -9                       (?)
-ksyms_module_libbpf                      # JIT does not support calling kernel function                                (kfunc)
-ksyms_module_lskel                       # test_ksyms_module_lskel__open_and_load unexpected error: -9                 (?)
-module_attach                            # skel_attach skeleton attach failed: -524                                    (trampoline)
-ringbuf                                  # skel_load skeleton load failed                                              (?)
 stacktrace_build_id                      # compare_map_keys stackid_hmap vs. stackmap err -2 errno 2                   (?)
-test_lsm                                 # attach unexpected error: -524                                               (trampoline)
-trace_printk                             # trace_printk__load unexpected error: -2 (errno 2)                           (?)
-trace_vprintk                            # trace_vprintk__open_and_load unexpected error: -9                           (?)
-unpriv_bpf_disabled                      # fentry
-user_ringbuf                             # failed to find kernel BTF type ID of '__s390x_sys_prctl': -3                (?)
-verif_stats                              # trace_vprintk__open_and_load unexpected error: -9                           (?)
-xdp_bonding                              # failed to auto-attach program 'trace_on_entry': -524                        (trampoline)
-xdp_metadata                             # JIT does not support calling kernel function                                (kfunc)
-test_task_under_cgroup                   # JIT does not support calling kernel function                                (kfunc)
-- 
2.41.0

Re: [PATCH bpf-next 02/11] net: netfilter: Adjust timeouts of non-confirmed CTs in bpf_ct_insert_entry()

[Daniel Borkmann]

[ +Florian ]

On 8/30/23 3:07 AM, Ilya Leoshkevich wrote:
> bpf_nf testcase fails on s390x: bpf_skb_ct_lookup() cannot find the
> entry that was added by bpf_ct_insert_entry() within the same BPF
> function.
> 
> The reason is that this entry is deleted by nf_ct_gc_expired().
> 
> The CT timeout starts ticking after the CT confirmation; therefore
> nf_conn.timeout is initially set to the timeout value, and
> __nf_conntrack_confirm() sets it to the deadline value.
> bpf_ct_insert_entry() sets IPS_CONFIRMED_BIT, but does not adjust the
> timeout, making its value meaningless and causing false positives.
> 
> Fix the problem by making bpf_ct_insert_entry() adjust the timeout,
> like __nf_conntrack_confirm().
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

Should we route this fix via bpf tree instead? Also, could you reply with
a Fixes tag?

> ---
>   net/netfilter/nf_conntrack_bpf.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
> index c7a6114091ae..b21799d468d2 100644
> --- a/net/netfilter/nf_conntrack_bpf.c
> +++ b/net/netfilter/nf_conntrack_bpf.c
> @@ -381,6 +381,8 @@ __bpf_kfunc struct nf_conn *bpf_ct_insert_entry(struct nf_conn___init *nfct_i)
>   	struct nf_conn *nfct = (struct nf_conn *)nfct_i;
>   	int err;
>   
> +	if (!nf_ct_is_confirmed(nfct))
> +		nfct->timeout += nfct_time_stamp;
>   	nfct->status |= IPS_CONFIRMED;
>   	err = nf_conntrack_hash_check_insert(nfct);
>   	if (err < 0) {
> 

Thanks,
Daniel

Re: [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX

[Yonghong Song]

On 8/29/23 9:07 PM, Ilya Leoshkevich wrote:
> On the architectures that use bpf_jit_needs_zext(), e.g., s390x, the
> verifier incorrectly inserts a zero-extension after BPF_MEMSX, leading
> to miscompilations like the one below:
> 
>        24:       89 1a ff fe 00 00 00 00 "r1 = *(s16 *)(r10 - 2);"       # zext_dst set
>     0x3ff7fdb910e:       lgh     %r2,-2(%r13,%r0)                        # load halfword
>     0x3ff7fdb9114:       llgfr   %r2,%r2                                 # wrong!
>        25:       65 10 00 03 00 00 7f ff if r1 s> 32767 goto +3 <l0_1>   # check_cond_jmp_op()
> 
> Disable such zero-extensions. The JITs need to insert sign-extension
> themselves, if necessary.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

Re: [PATCH bpf-next 10/11] selftests/bpf: Enable the cpuv4 tests for s390x

[Yonghong Song]

On 8/29/23 9:07 PM, Ilya Leoshkevich wrote:
> Now that all the cpuv4 support is in place, enable the tests.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

Re: [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX

[Puranjay Mohan]

Hi Ilya

On Wed, Aug 30, 2023 at 3:12 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On the architectures that use bpf_jit_needs_zext(), e.g., s390x, the
> verifier incorrectly inserts a zero-extension after BPF_MEMSX, leading
> to miscompilations like the one below:
>
>       24:       89 1a ff fe 00 00 00 00 "r1 = *(s16 *)(r10 - 2);"       # zext_dst set
>    0x3ff7fdb910e:       lgh     %r2,-2(%r13,%r0)                        # load halfword
>    0x3ff7fdb9114:       llgfr   %r2,%r2                                 # wrong!
>       25:       65 10 00 03 00 00 7f ff if r1 s> 32767 goto +3 <l0_1>   # check_cond_jmp_op()
>
> Disable such zero-extensions. The JITs need to insert sign-extension
> themselves, if necessary.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  kernel/bpf/verifier.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index bb78212fa5b2..097985a46edc 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3110,7 +3110,9 @@ static void mark_insn_zext(struct bpf_verifier_env *env,
>  {
>         s32 def_idx = reg->subreg_def;
>
> -       if (def_idx == DEF_NOT_SUBREG)

The problem here is that reg->subreg_def should be set as DEF_NOT_SUBREG for
registers that are used as destination registers of BPF_LDX |
BPF_MEMSX. I am seeing
the same problem on ARM32 and was going to send a patch today.

The problem is that is_reg64() returns false for destination registers
of BPF_LDX | BPF_MEMSX.
But BPF_LDX | BPF_MEMSX always loads a 64 bit value because of the
sign extension so
is_reg64() should return true.

I have written a patch that I will be sending as a reply to this.
Please let me know if that makes sense.

> +       if (def_idx == DEF_NOT_SUBREG ||
> +           (BPF_CLASS(env->prog->insnsi[def_idx - 1].code) == BPF_LDX &&
> +            BPF_MODE(env->prog->insnsi[def_idx - 1].code) == BPF_MEMSX))
>                 return;
>
>         env->insn_aux_data[def_idx - 1].zext_dst = true;
> --
> 2.41.0
>
>

Thanks,
Puranjay

Re: [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX

[Puranjay Mohan]

On Fri, Sep 01 2023, Puranjay Mohan wrote:

> The problem here is that reg->subreg_def should be set as DEF_NOT_SUBREG for
> registers that are used as destination registers of BPF_LDX |
> BPF_MEMSX. I am seeing
> the same problem on ARM32 and was going to send a patch today.
>
> The problem is that is_reg64() returns false for destination registers
> of BPF_LDX | BPF_MEMSX.
> But BPF_LDX | BPF_MEMSX always loads a 64 bit value because of the
> sign extension so
> is_reg64() should return true.
>
> I have written a patch that I will be sending as a reply to this.
> Please let me know if that makes sense.
>

The check_reg_arg() function will mark reg->subreg_def = DEF_NOT_SUBREG for destination
registers if is_reg64() returns true for these registers. My patch below make is_reg64()
return true for destination registers of BPF_LDX with mod = BPF_MEMSX. I feel this is the
correct way to fix this problem.

Here is my patch:

--- 8< ---
From cf1bf5282183cf721926ab14d968d3d4097b89b8 Mon Sep 17 00:00:00 2001
From: Puranjay Mohan <puranjay12@gmail.com>
Date: Fri, 1 Sep 2023 11:18:59 +0000
Subject: [PATCH bpf] bpf: verifier: mark destination of sign-extended load as
 64 bit

The verifier can emit instructions to zero-extend destination registers
when the register is being used to keep 32 bit values. This behaviour is
enabled only when the JIT sets bpf_jit_needs_zext() -> true. In the case
of a sign extended load instruction, the destination register always has a
64-bit value, therefore the verifier should not emit zero-extend
instructions for it.

Change is_reg64() to return true if the register under consideration is a
destination register of LDX instruction with mode = BPF_MEMSX.

Fixes: 1f9a1ea821ff ("bpf: Support new sign-extension load insns")
Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
---
 kernel/bpf/verifier.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index bb78212fa5b2..93f84b868ccc 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3029,7 +3029,7 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
 
 	if (class == BPF_LDX) {
 		if (t != SRC_OP)
-			return BPF_SIZE(code) == BPF_DW;
+			return (BPF_SIZE(code) == BPF_DW || BPF_MODE(code) == BPF_MEMSX);
 		/* LDX source must be ptr. */
 		return true;
 	}
-- 
2.39.2

Re: [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX

[Ilya Leoshkevich]

On Fri, 2023-09-01 at 16:19 +0200, Puranjay Mohan wrote:
> Hi Ilya
> 
> On Wed, Aug 30, 2023 at 3:12 AM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > On the architectures that use bpf_jit_needs_zext(), e.g., s390x,
> > the
> > verifier incorrectly inserts a zero-extension after BPF_MEMSX,
> > leading
> > to miscompilations like the one below:
> > 
> >       24:       89 1a ff fe 00 00 00 00 "r1 = *(s16 *)(r10 -
> > 2);"       # zext_dst set
> >    0x3ff7fdb910e:       lgh     %r2,-
> > 2(%r13,%r0)                        # load halfword
> >    0x3ff7fdb9114:       llgfr  
> > %r2,%r2                                 # wrong!
> >       25:       65 10 00 03 00 00 7f ff if r1 s> 32767 goto +3
> > <l0_1>   # check_cond_jmp_op()
> > 
> > Disable such zero-extensions. The JITs need to insert sign-
> > extension
> > themselves, if necessary.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  kernel/bpf/verifier.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index bb78212fa5b2..097985a46edc 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -3110,7 +3110,9 @@ static void mark_insn_zext(struct
> > bpf_verifier_env *env,
> >  {
> >         s32 def_idx = reg->subreg_def;
> > 
> > -       if (def_idx == DEF_NOT_SUBREG)
> 
> The problem here is that reg->subreg_def should be set as
> DEF_NOT_SUBREG for
> registers that are used as destination registers of BPF_LDX |
> BPF_MEMSX. I am seeing
> the same problem on ARM32 and was going to send a patch today.
> 
> The problem is that is_reg64() returns false for destination
> registers
> of BPF_LDX | BPF_MEMSX.
> But BPF_LDX | BPF_MEMSX always loads a 64 bit value because of the
> sign extension so
> is_reg64() should return true.
> 
> I have written a patch that I will be sending as a reply to this.
> Please let me know if that makes sense.
> 
> > +       if (def_idx == DEF_NOT_SUBREG ||
> > +           (BPF_CLASS(env->prog->insnsi[def_idx - 1].code) ==
> > BPF_LDX &&
> > +            BPF_MODE(env->prog->insnsi[def_idx - 1].code) ==
> > BPF_MEMSX))
> >                 return;
> > 
> >         env->insn_aux_data[def_idx - 1].zext_dst = true;
> > --
> > 2.41.0
> > 
> > 
> 
> Thanks,
> Puranjay

Hi,

I also considered doing this, and I think both approaches are
functionally equivalent and work in practice.

However, I can envision that, just like we have the zext_dst 
optimization today, we might want a sext_dst optimization in the
future. Therefore I think it's better to fix this by not setting
zext_dst instead of not setting subreg_def.

Best regards,
Ilya

Re: [PATCH bpf-next 02/11] net: netfilter: Adjust timeouts of non-confirmed CTs in bpf_ct_insert_entry()

[Ilya Leoshkevich]

On Thu, 2023-08-31 at 17:30 +0200, Daniel Borkmann wrote:
> [ +Florian ]
> 
> On 8/30/23 3:07 AM, Ilya Leoshkevich wrote:
> > bpf_nf testcase fails on s390x: bpf_skb_ct_lookup() cannot find the
> > entry that was added by bpf_ct_insert_entry() within the same BPF
> > function.
> > 
> > The reason is that this entry is deleted by nf_ct_gc_expired().
> > 
> > The CT timeout starts ticking after the CT confirmation; therefore
> > nf_conn.timeout is initially set to the timeout value, and
> > __nf_conntrack_confirm() sets it to the deadline value.
> > bpf_ct_insert_entry() sets IPS_CONFIRMED_BIT, but does not adjust
> > the
> > timeout, making its value meaningless and causing false positives.
> > 
> > Fix the problem by making bpf_ct_insert_entry() adjust the timeout,
> > like __nf_conntrack_confirm().
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> 
> Should we route this fix via bpf tree instead? Also, could you reply
> with
> a Fixes tag?

Yes, putting this into the bpf tree makes sense to me. Should I resend
with a different subject-prefix?

Fixes: 2cdaa3eefed8 ("netfilter: conntrack: restore IPS_CONFIRMED out
of nf_conntrack_hash_check_insert()")

[...]

Re: [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX

[Alexei Starovoitov]

On Fri, Sep 1, 2023 at 7:57 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
>
> On Fri, Sep 01 2023, Puranjay Mohan wrote:
>
> > The problem here is that reg->subreg_def should be set as DEF_NOT_SUBREG for
> > registers that are used as destination registers of BPF_LDX |
> > BPF_MEMSX. I am seeing
> > the same problem on ARM32 and was going to send a patch today.
> >
> > The problem is that is_reg64() returns false for destination registers
> > of BPF_LDX | BPF_MEMSX.
> > But BPF_LDX | BPF_MEMSX always loads a 64 bit value because of the
> > sign extension so
> > is_reg64() should return true.
> >
> > I have written a patch that I will be sending as a reply to this.
> > Please let me know if that makes sense.
> >
>
> The check_reg_arg() function will mark reg->subreg_def = DEF_NOT_SUBREG for destination
> registers if is_reg64() returns true for these registers. My patch below make is_reg64()
> return true for destination registers of BPF_LDX with mod = BPF_MEMSX. I feel this is the
> correct way to fix this problem.
>
> Here is my patch:
>
> --- 8< ---
> From cf1bf5282183cf721926ab14d968d3d4097b89b8 Mon Sep 17 00:00:00 2001
> From: Puranjay Mohan <puranjay12@gmail.com>
> Date: Fri, 1 Sep 2023 11:18:59 +0000
> Subject: [PATCH bpf] bpf: verifier: mark destination of sign-extended load as
>  64 bit
>
> The verifier can emit instructions to zero-extend destination registers
> when the register is being used to keep 32 bit values. This behaviour is
> enabled only when the JIT sets bpf_jit_needs_zext() -> true. In the case
> of a sign extended load instruction, the destination register always has a
> 64-bit value, therefore the verifier should not emit zero-extend
> instructions for it.
>
> Change is_reg64() to return true if the register under consideration is a
> destination register of LDX instruction with mode = BPF_MEMSX.
>
> Fixes: 1f9a1ea821ff ("bpf: Support new sign-extension load insns")
> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> ---
>  kernel/bpf/verifier.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index bb78212fa5b2..93f84b868ccc 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3029,7 +3029,7 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
>
>         if (class == BPF_LDX) {
>                 if (t != SRC_OP)
> -                       return BPF_SIZE(code) == BPF_DW;
> +                       return (BPF_SIZE(code) == BPF_DW || BPF_MODE(code) == BPF_MEMSX);

Looks like we have a bug here for normal LDX too.
This 'if' condition was inserting unnecessary zext for LDX.
It was harmless for LDX and broken for LDSX.
Both LDX and LDSX write all bits of 64-bit register.

I think the proper fix is to remove above two lines.
wdyt?

Re: [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX

[Puranjay Mohan]

On Wed, Sep 06 2023, Alexei Starovoitov wrote:

> On Fri, Sep 1, 2023 at 7:57 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
>>
>> On Fri, Sep 01 2023, Puranjay Mohan wrote:
>>
>> > The problem here is that reg->subreg_def should be set as DEF_NOT_SUBREG for
>> > registers that are used as destination registers of BPF_LDX |
>> > BPF_MEMSX. I am seeing
>> > the same problem on ARM32 and was going to send a patch today.
>> >
>> > The problem is that is_reg64() returns false for destination registers
>> > of BPF_LDX | BPF_MEMSX.
>> > But BPF_LDX | BPF_MEMSX always loads a 64 bit value because of the
>> > sign extension so
>> > is_reg64() should return true.
>> >
>> > I have written a patch that I will be sending as a reply to this.
>> > Please let me know if that makes sense.
>> >
>>
>> The check_reg_arg() function will mark reg->subreg_def = DEF_NOT_SUBREG for destination
>> registers if is_reg64() returns true for these registers. My patch below make is_reg64()
>> return true for destination registers of BPF_LDX with mod = BPF_MEMSX. I feel this is the
>> correct way to fix this problem.
>>
>> Here is my patch:
>>
>> --- 8< ---
>> From cf1bf5282183cf721926ab14d968d3d4097b89b8 Mon Sep 17 00:00:00 2001
>> From: Puranjay Mohan <puranjay12@gmail.com>
>> Date: Fri, 1 Sep 2023 11:18:59 +0000
>> Subject: [PATCH bpf] bpf: verifier: mark destination of sign-extended load as
>>  64 bit
>>
>> The verifier can emit instructions to zero-extend destination registers
>> when the register is being used to keep 32 bit values. This behaviour is
>> enabled only when the JIT sets bpf_jit_needs_zext() -> true. In the case
>> of a sign extended load instruction, the destination register always has a
>> 64-bit value, therefore the verifier should not emit zero-extend
>> instructions for it.
>>
>> Change is_reg64() to return true if the register under consideration is a
>> destination register of LDX instruction with mode = BPF_MEMSX.
>>
>> Fixes: 1f9a1ea821ff ("bpf: Support new sign-extension load insns")
>> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
>> ---
>>  kernel/bpf/verifier.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index bb78212fa5b2..93f84b868ccc 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -3029,7 +3029,7 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>
>>         if (class == BPF_LDX) {
>>                 if (t != SRC_OP)
>> -                       return BPF_SIZE(code) == BPF_DW;
>> +                       return (BPF_SIZE(code) == BPF_DW || BPF_MODE(code) == BPF_MEMSX);
>
> Looks like we have a bug here for normal LDX too.
> This 'if' condition was inserting unnecessary zext for LDX.
> It was harmless for LDX and broken for LDSX.
> Both LDX and LDSX write all bits of 64-bit register.
>
> I think the proper fix is to remove above two lines.
> wdyt?

For LDX this returns true only if it is with a BPF_DW, for others it returns false.
This means a zext is inserted for BPF_LDX | BPF_B/H/W.

This is not a bug because LDX writes 64 bits of the register only with BPF_DW.
With BPF_B/H/W It only writes the lower 32bits and needs zext for upper 32 bits.
On 32 bit architectures where a 64-bit BPF register is simulated with two 32-bit registers,
explicit zext is required for BPF_LDX | BPF_B/H/W.

So, we should not remove this.

Thanks,
Puranjay

Re: [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX

[Alexei Starovoitov]

On Thu, Sep 7, 2023 at 12:33 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
>
> On Wed, Sep 06 2023, Alexei Starovoitov wrote:
>
> > On Fri, Sep 1, 2023 at 7:57 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
> >>
> >> On Fri, Sep 01 2023, Puranjay Mohan wrote:
> >>
> >> > The problem here is that reg->subreg_def should be set as DEF_NOT_SUBREG for
> >> > registers that are used as destination registers of BPF_LDX |
> >> > BPF_MEMSX. I am seeing
> >> > the same problem on ARM32 and was going to send a patch today.
> >> >
> >> > The problem is that is_reg64() returns false for destination registers
> >> > of BPF_LDX | BPF_MEMSX.
> >> > But BPF_LDX | BPF_MEMSX always loads a 64 bit value because of the
> >> > sign extension so
> >> > is_reg64() should return true.
> >> >
> >> > I have written a patch that I will be sending as a reply to this.
> >> > Please let me know if that makes sense.
> >> >
> >>
> >> The check_reg_arg() function will mark reg->subreg_def = DEF_NOT_SUBREG for destination
> >> registers if is_reg64() returns true for these registers. My patch below make is_reg64()
> >> return true for destination registers of BPF_LDX with mod = BPF_MEMSX. I feel this is the
> >> correct way to fix this problem.
> >>
> >> Here is my patch:
> >>
> >> --- 8< ---
> >> From cf1bf5282183cf721926ab14d968d3d4097b89b8 Mon Sep 17 00:00:00 2001
> >> From: Puranjay Mohan <puranjay12@gmail.com>
> >> Date: Fri, 1 Sep 2023 11:18:59 +0000
> >> Subject: [PATCH bpf] bpf: verifier: mark destination of sign-extended load as
> >>  64 bit
> >>
> >> The verifier can emit instructions to zero-extend destination registers
> >> when the register is being used to keep 32 bit values. This behaviour is
> >> enabled only when the JIT sets bpf_jit_needs_zext() -> true. In the case
> >> of a sign extended load instruction, the destination register always has a
> >> 64-bit value, therefore the verifier should not emit zero-extend
> >> instructions for it.
> >>
> >> Change is_reg64() to return true if the register under consideration is a
> >> destination register of LDX instruction with mode = BPF_MEMSX.
> >>
> >> Fixes: 1f9a1ea821ff ("bpf: Support new sign-extension load insns")
> >> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> >> ---
> >>  kernel/bpf/verifier.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index bb78212fa5b2..93f84b868ccc 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -3029,7 +3029,7 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
> >>
> >>         if (class == BPF_LDX) {
> >>                 if (t != SRC_OP)
> >> -                       return BPF_SIZE(code) == BPF_DW;
> >> +                       return (BPF_SIZE(code) == BPF_DW || BPF_MODE(code) == BPF_MEMSX);
> >
> > Looks like we have a bug here for normal LDX too.
> > This 'if' condition was inserting unnecessary zext for LDX.
> > It was harmless for LDX and broken for LDSX.
> > Both LDX and LDSX write all bits of 64-bit register.
> >
> > I think the proper fix is to remove above two lines.
> > wdyt?
>
> For LDX this returns true only if it is with a BPF_DW, for others it returns false.
> This means a zext is inserted for BPF_LDX | BPF_B/H/W.
>
> This is not a bug because LDX writes 64 bits of the register only with BPF_DW.
> With BPF_B/H/W It only writes the lower 32bits and needs zext for upper 32 bits.

No. The interpreter writes all 64-bit for any LDX insn.
All JITs must do it as well.

> On 32 bit architectures where a 64-bit BPF register is simulated with two 32-bit registers,
> explicit zext is required for BPF_LDX | BPF_B/H/W.

zext JIT-aid done by the verifier has nothing to do with 32-bit architecture.
It's necessary on 64-bit as well when HW doesn't automatically zero out
upper 32-bit like it does on arm64 and x86-64

> So, we should not remove this.

I still think we should.

Re: [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX

[Puranjay Mohan]

On Thu, Sep 07 2023, Alexei Starovoitov wrote:

> On Thu, Sep 7, 2023 at 12:33 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
>>
>> On Wed, Sep 06 2023, Alexei Starovoitov wrote:
>>
>> > On Fri, Sep 1, 2023 at 7:57 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
>> >>
>> >> On Fri, Sep 01 2023, Puranjay Mohan wrote:
>> >>
>> >> > The problem here is that reg->subreg_def should be set as DEF_NOT_SUBREG for
>> >> > registers that are used as destination registers of BPF_LDX |
>> >> > BPF_MEMSX. I am seeing
>> >> > the same problem on ARM32 and was going to send a patch today.
>> >> >
>> >> > The problem is that is_reg64() returns false for destination registers
>> >> > of BPF_LDX | BPF_MEMSX.
>> >> > But BPF_LDX | BPF_MEMSX always loads a 64 bit value because of the
>> >> > sign extension so
>> >> > is_reg64() should return true.
>> >> >
>> >> > I have written a patch that I will be sending as a reply to this.
>> >> > Please let me know if that makes sense.
>> >> >
>> >>
>> >> The check_reg_arg() function will mark reg->subreg_def = DEF_NOT_SUBREG for destination
>> >> registers if is_reg64() returns true for these registers. My patch below make is_reg64()
>> >> return true for destination registers of BPF_LDX with mod = BPF_MEMSX. I feel this is the
>> >> correct way to fix this problem.
>> >>
>> >> Here is my patch:
>> >>
>> >> --- 8< ---
>> >> From cf1bf5282183cf721926ab14d968d3d4097b89b8 Mon Sep 17 00:00:00 2001
>> >> From: Puranjay Mohan <puranjay12@gmail.com>
>> >> Date: Fri, 1 Sep 2023 11:18:59 +0000
>> >> Subject: [PATCH bpf] bpf: verifier: mark destination of sign-extended load as
>> >>  64 bit
>> >>
>> >> The verifier can emit instructions to zero-extend destination registers
>> >> when the register is being used to keep 32 bit values. This behaviour is
>> >> enabled only when the JIT sets bpf_jit_needs_zext() -> true. In the case
>> >> of a sign extended load instruction, the destination register always has a
>> >> 64-bit value, therefore the verifier should not emit zero-extend
>> >> instructions for it.
>> >>
>> >> Change is_reg64() to return true if the register under consideration is a
>> >> destination register of LDX instruction with mode = BPF_MEMSX.
>> >>
>> >> Fixes: 1f9a1ea821ff ("bpf: Support new sign-extension load insns")
>> >> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
>> >> ---
>> >>  kernel/bpf/verifier.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> >> index bb78212fa5b2..93f84b868ccc 100644
>> >> --- a/kernel/bpf/verifier.c
>> >> +++ b/kernel/bpf/verifier.c
>> >> @@ -3029,7 +3029,7 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
>> >>
>> >>         if (class == BPF_LDX) {
>> >>                 if (t != SRC_OP)
>> >> -                       return BPF_SIZE(code) == BPF_DW;
>> >> +                       return (BPF_SIZE(code) == BPF_DW || BPF_MODE(code) == BPF_MEMSX);
>> >
>> > Looks like we have a bug here for normal LDX too.
>> > This 'if' condition was inserting unnecessary zext for LDX.
>> > It was harmless for LDX and broken for LDSX.
>> > Both LDX and LDSX write all bits of 64-bit register.
>> >
>> > I think the proper fix is to remove above two lines.
>> > wdyt?
>>
>> For LDX this returns true only if it is with a BPF_DW, for others it returns false.
>> This means a zext is inserted for BPF_LDX | BPF_B/H/W.
>>
>> This is not a bug because LDX writes 64 bits of the register only with BPF_DW.
>> With BPF_B/H/W It only writes the lower 32bits and needs zext for upper 32 bits.
>
> No. The interpreter writes all 64-bit for any LDX insn.
> All JITs must do it as well.
>
>> On 32 bit architectures where a 64-bit BPF register is simulated with two 32-bit registers,
>> explicit zext is required for BPF_LDX | BPF_B/H/W.
>
> zext JIT-aid done by the verifier has nothing to do with 32-bit architecture.
> It's necessary on 64-bit as well when HW doesn't automatically zero out
> upper 32-bit like it does on arm64 and x86-64

Yes, I agree that zext JIT-aid is required for all 32-bit architectures and some 64-bit architectures
that can't automatically zero out the upper 32-bits.
Basically any architecture that sets bpf_jit_needs_zext() -> true.

>> So, we should not remove this.
>
> I still think we should.

If we remove this then some JITs will not zero extend the upper 32-bits for BPF_LDX | BPF_B/H/W.

My understanding is that Verifier sets prog->aux->verifier_zext if it emits zext instructions. If the verifier
doesn't emit zext for LDX but sets prog->aux->verifier_zext that would cause wrong behavior for some JITs:

Example code from ARM32 jit doing BPF_LDX | BPF_MEM | BPF_B:

case BPF_B:
		/* Load a Byte */
		emit(ARM_LDRB_I(rd[1], rm, off), ctx);
		if (!ctx->prog->aux->verifier_zext)
			emit_a32_mov_i(rd[0], 0, ctx);
		break;

Here if ctx->prog->aux->verifier_zext is set by the verifier, and zext was not emitted for LDX, JIT will not zero
the upper 32-bits.

RISCV32, PowerPC32, x86-32 JITs have similar code paths. Only MIPS32 JIT zero-extends for LDX without checking  
prog->aux->verifier_zext.

So, if we want to stop emitting zext for LDX then we would need to modify all these JITs to always zext for LDX.

Let me know if my understanding has some gaps, also if we decide to remove it, I am happy to send patches for it
and fix the JITs that need modifications.

Thanks,
Puranjay

Re: [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX

[Alexei Starovoitov]

On Thu, Sep 7, 2023 at 9:39 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
>
> On Thu, Sep 07 2023, Alexei Starovoitov wrote:
>
> > On Thu, Sep 7, 2023 at 12:33 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
> >>
> >> On Wed, Sep 06 2023, Alexei Starovoitov wrote:
> >>
> >> > On Fri, Sep 1, 2023 at 7:57 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
> >> >>
> >> >> On Fri, Sep 01 2023, Puranjay Mohan wrote:
> >> >>
> >> >> > The problem here is that reg->subreg_def should be set as DEF_NOT_SUBREG for
> >> >> > registers that are used as destination registers of BPF_LDX |
> >> >> > BPF_MEMSX. I am seeing
> >> >> > the same problem on ARM32 and was going to send a patch today.
> >> >> >
> >> >> > The problem is that is_reg64() returns false for destination registers
> >> >> > of BPF_LDX | BPF_MEMSX.
> >> >> > But BPF_LDX | BPF_MEMSX always loads a 64 bit value because of the
> >> >> > sign extension so
> >> >> > is_reg64() should return true.
> >> >> >
> >> >> > I have written a patch that I will be sending as a reply to this.
> >> >> > Please let me know if that makes sense.
> >> >> >
> >> >>
> >> >> The check_reg_arg() function will mark reg->subreg_def = DEF_NOT_SUBREG for destination
> >> >> registers if is_reg64() returns true for these registers. My patch below make is_reg64()
> >> >> return true for destination registers of BPF_LDX with mod = BPF_MEMSX. I feel this is the
> >> >> correct way to fix this problem.
> >> >>
> >> >> Here is my patch:
> >> >>
> >> >> --- 8< ---
> >> >> From cf1bf5282183cf721926ab14d968d3d4097b89b8 Mon Sep 17 00:00:00 2001
> >> >> From: Puranjay Mohan <puranjay12@gmail.com>
> >> >> Date: Fri, 1 Sep 2023 11:18:59 +0000
> >> >> Subject: [PATCH bpf] bpf: verifier: mark destination of sign-extended load as
> >> >>  64 bit
> >> >>
> >> >> The verifier can emit instructions to zero-extend destination registers
> >> >> when the register is being used to keep 32 bit values. This behaviour is
> >> >> enabled only when the JIT sets bpf_jit_needs_zext() -> true. In the case
> >> >> of a sign extended load instruction, the destination register always has a
> >> >> 64-bit value, therefore the verifier should not emit zero-extend
> >> >> instructions for it.
> >> >>
> >> >> Change is_reg64() to return true if the register under consideration is a
> >> >> destination register of LDX instruction with mode = BPF_MEMSX.
> >> >>
> >> >> Fixes: 1f9a1ea821ff ("bpf: Support new sign-extension load insns")
> >> >> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> >> >> ---
> >> >>  kernel/bpf/verifier.c | 2 +-
> >> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> >> index bb78212fa5b2..93f84b868ccc 100644
> >> >> --- a/kernel/bpf/verifier.c
> >> >> +++ b/kernel/bpf/verifier.c
> >> >> @@ -3029,7 +3029,7 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
> >> >>
> >> >>         if (class == BPF_LDX) {
> >> >>                 if (t != SRC_OP)
> >> >> -                       return BPF_SIZE(code) == BPF_DW;
> >> >> +                       return (BPF_SIZE(code) == BPF_DW || BPF_MODE(code) == BPF_MEMSX);
> >> >
> >> > Looks like we have a bug here for normal LDX too.
> >> > This 'if' condition was inserting unnecessary zext for LDX.
> >> > It was harmless for LDX and broken for LDSX.
> >> > Both LDX and LDSX write all bits of 64-bit register.
> >> >
> >> > I think the proper fix is to remove above two lines.
> >> > wdyt?
> >>
> >> For LDX this returns true only if it is with a BPF_DW, for others it returns false.
> >> This means a zext is inserted for BPF_LDX | BPF_B/H/W.
> >>
> >> This is not a bug because LDX writes 64 bits of the register only with BPF_DW.
> >> With BPF_B/H/W It only writes the lower 32bits and needs zext for upper 32 bits.
> >
> > No. The interpreter writes all 64-bit for any LDX insn.
> > All JITs must do it as well.
> >
> >> On 32 bit architectures where a 64-bit BPF register is simulated with two 32-bit registers,
> >> explicit zext is required for BPF_LDX | BPF_B/H/W.
> >
> > zext JIT-aid done by the verifier has nothing to do with 32-bit architecture.
> > It's necessary on 64-bit as well when HW doesn't automatically zero out
> > upper 32-bit like it does on arm64 and x86-64
>
> Yes, I agree that zext JIT-aid is required for all 32-bit architectures and some 64-bit architectures
> that can't automatically zero out the upper 32-bits.
> Basically any architecture that sets bpf_jit_needs_zext() -> true.
>
> >> So, we should not remove this.
> >
> > I still think we should.
>
> If we remove this then some JITs will not zero extend the upper 32-bits for BPF_LDX | BPF_B/H/W.
>
> My understanding is that Verifier sets prog->aux->verifier_zext if it emits zext instructions. If the verifier
> doesn't emit zext for LDX but sets prog->aux->verifier_zext that would cause wrong behavior for some JITs:
>
> Example code from ARM32 jit doing BPF_LDX | BPF_MEM | BPF_B:
>
> case BPF_B:
>                 /* Load a Byte */
>                 emit(ARM_LDRB_I(rd[1], rm, off), ctx);
>                 if (!ctx->prog->aux->verifier_zext)
>                         emit_a32_mov_i(rd[0], 0, ctx);
>                 break;
>
> Here if ctx->prog->aux->verifier_zext is set by the verifier, and zext was not emitted for LDX, JIT will not zero
> the upper 32-bits.
>
> RISCV32, PowerPC32, x86-32 JITs have similar code paths. Only MIPS32 JIT zero-extends for LDX without checking
> prog->aux->verifier_zext.
>
> So, if we want to stop emitting zext for LDX then we would need to modify all these JITs to always zext for LDX.

I guess we never clearly defined what 'needs_zext' is supposed to be,
so it wouldn't be fair to call 32-bit JITs buggy.
But we better address this issue now.
This 32-bit zeroing after LDX hurts mips64, s390, ppc64, riscv64.
I believe all 4 JITs emit proper zero extension into 64-bit register
by using single cpu instruction,
but they also define bpf_jit_needs_zext() as true,
so extra BPF_ZEXT_REG() is added by the verifier
and it is a pure run-time overhead.

It's better to remove
if (t != SRC_OP)
    return BPF_SIZE(code) == BPF_DW;
from is_reg64() to avoid adding BPF_ZEXT_REG() insn
and fix 32-bit JITs at the same time.
RISCV32, PowerPC32, x86-32 JITs fixed in the first 3 patches
to always zero upper 32-bit after LDX and
then 4th patch to remove these two lines.

> Let me know if my understanding has some gaps, also if we decide to remove it, I am happy to send patches for it
> and fix the JITs that need modifications.

Thank you for working on it!

cc-ing JIT experts.

Re: [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX

[Puranjay Mohan]

On Fri, Sep 8, 2023 at 12:45 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Sep 7, 2023 at 9:39 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
> >
> > On Thu, Sep 07 2023, Alexei Starovoitov wrote:
> >
> > > On Thu, Sep 7, 2023 at 12:33 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
> > >>
> > >> On Wed, Sep 06 2023, Alexei Starovoitov wrote:
> > >>
> > >> > On Fri, Sep 1, 2023 at 7:57 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
> > >> >>
> > >> >> On Fri, Sep 01 2023, Puranjay Mohan wrote:
> > >> >>
> > >> >> > The problem here is that reg->subreg_def should be set as DEF_NOT_SUBREG for
> > >> >> > registers that are used as destination registers of BPF_LDX |
> > >> >> > BPF_MEMSX. I am seeing
> > >> >> > the same problem on ARM32 and was going to send a patch today.
> > >> >> >
> > >> >> > The problem is that is_reg64() returns false for destination registers
> > >> >> > of BPF_LDX | BPF_MEMSX.
> > >> >> > But BPF_LDX | BPF_MEMSX always loads a 64 bit value because of the
> > >> >> > sign extension so
> > >> >> > is_reg64() should return true.
> > >> >> >
> > >> >> > I have written a patch that I will be sending as a reply to this.
> > >> >> > Please let me know if that makes sense.
> > >> >> >
> > >> >>
> > >> >> The check_reg_arg() function will mark reg->subreg_def = DEF_NOT_SUBREG for destination
> > >> >> registers if is_reg64() returns true for these registers. My patch below make is_reg64()
> > >> >> return true for destination registers of BPF_LDX with mod = BPF_MEMSX. I feel this is the
> > >> >> correct way to fix this problem.
> > >> >>
> > >> >> Here is my patch:
> > >> >>
> > >> >> --- 8< ---
> > >> >> From cf1bf5282183cf721926ab14d968d3d4097b89b8 Mon Sep 17 00:00:00 2001
> > >> >> From: Puranjay Mohan <puranjay12@gmail.com>
> > >> >> Date: Fri, 1 Sep 2023 11:18:59 +0000
> > >> >> Subject: [PATCH bpf] bpf: verifier: mark destination of sign-extended load as
> > >> >>  64 bit
> > >> >>
> > >> >> The verifier can emit instructions to zero-extend destination registers
> > >> >> when the register is being used to keep 32 bit values. This behaviour is
> > >> >> enabled only when the JIT sets bpf_jit_needs_zext() -> true. In the case
> > >> >> of a sign extended load instruction, the destination register always has a
> > >> >> 64-bit value, therefore the verifier should not emit zero-extend
> > >> >> instructions for it.
> > >> >>
> > >> >> Change is_reg64() to return true if the register under consideration is a
> > >> >> destination register of LDX instruction with mode = BPF_MEMSX.
> > >> >>
> > >> >> Fixes: 1f9a1ea821ff ("bpf: Support new sign-extension load insns")
> > >> >> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> > >> >> ---
> > >> >>  kernel/bpf/verifier.c | 2 +-
> > >> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >> >>
> > >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > >> >> index bb78212fa5b2..93f84b868ccc 100644
> > >> >> --- a/kernel/bpf/verifier.c
> > >> >> +++ b/kernel/bpf/verifier.c
> > >> >> @@ -3029,7 +3029,7 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
> > >> >>
> > >> >>         if (class == BPF_LDX) {
> > >> >>                 if (t != SRC_OP)
> > >> >> -                       return BPF_SIZE(code) == BPF_DW;
> > >> >> +                       return (BPF_SIZE(code) == BPF_DW || BPF_MODE(code) == BPF_MEMSX);
> > >> >
> > >> > Looks like we have a bug here for normal LDX too.
> > >> > This 'if' condition was inserting unnecessary zext for LDX.
> > >> > It was harmless for LDX and broken for LDSX.
> > >> > Both LDX and LDSX write all bits of 64-bit register.
> > >> >
> > >> > I think the proper fix is to remove above two lines.
> > >> > wdyt?
> > >>
> > >> For LDX this returns true only if it is with a BPF_DW, for others it returns false.
> > >> This means a zext is inserted for BPF_LDX | BPF_B/H/W.
> > >>
> > >> This is not a bug because LDX writes 64 bits of the register only with BPF_DW.
> > >> With BPF_B/H/W It only writes the lower 32bits and needs zext for upper 32 bits.
> > >
> > > No. The interpreter writes all 64-bit for any LDX insn.
> > > All JITs must do it as well.
> > >
> > >> On 32 bit architectures where a 64-bit BPF register is simulated with two 32-bit registers,
> > >> explicit zext is required for BPF_LDX | BPF_B/H/W.
> > >
> > > zext JIT-aid done by the verifier has nothing to do with 32-bit architecture.
> > > It's necessary on 64-bit as well when HW doesn't automatically zero out
> > > upper 32-bit like it does on arm64 and x86-64
> >
> > Yes, I agree that zext JIT-aid is required for all 32-bit architectures and some 64-bit architectures
> > that can't automatically zero out the upper 32-bits.
> > Basically any architecture that sets bpf_jit_needs_zext() -> true.
> >
> > >> So, we should not remove this.
> > >
> > > I still think we should.
> >
> > If we remove this then some JITs will not zero extend the upper 32-bits for BPF_LDX | BPF_B/H/W.
> >
> > My understanding is that Verifier sets prog->aux->verifier_zext if it emits zext instructions. If the verifier
> > doesn't emit zext for LDX but sets prog->aux->verifier_zext that would cause wrong behavior for some JITs:
> >
> > Example code from ARM32 jit doing BPF_LDX | BPF_MEM | BPF_B:
> >
> > case BPF_B:
> >                 /* Load a Byte */
> >                 emit(ARM_LDRB_I(rd[1], rm, off), ctx);
> >                 if (!ctx->prog->aux->verifier_zext)
> >                         emit_a32_mov_i(rd[0], 0, ctx);
> >                 break;
> >
> > Here if ctx->prog->aux->verifier_zext is set by the verifier, and zext was not emitted for LDX, JIT will not zero
> > the upper 32-bits.
> >
> > RISCV32, PowerPC32, x86-32 JITs have similar code paths. Only MIPS32 JIT zero-extends for LDX without checking
> > prog->aux->verifier_zext.
> >
> > So, if we want to stop emitting zext for LDX then we would need to modify all these JITs to always zext for LDX.
>
> I guess we never clearly defined what 'needs_zext' is supposed to be,
> so it wouldn't be fair to call 32-bit JITs buggy.
> But we better address this issue now.
> This 32-bit zeroing after LDX hurts mips64, s390, ppc64, riscv64.
> I believe all 4 JITs emit proper zero extension into 64-bit register
> by using single cpu instruction,
> but they also define bpf_jit_needs_zext() as true,
> so extra BPF_ZEXT_REG() is added by the verifier
> and it is a pure run-time overhead.
>
> It's better to remove
> if (t != SRC_OP)
>     return BPF_SIZE(code) == BPF_DW;
> from is_reg64() to avoid adding BPF_ZEXT_REG() insn
> and fix 32-bit JITs at the same time.
> RISCV32, PowerPC32, x86-32 JITs fixed in the first 3 patches
> to always zero upper 32-bit after LDX and
> then 4th patch to remove these two lines.
>
> > Let me know if my understanding has some gaps, also if we decide to remove it, I am happy to send patches for it
> > and fix the JITs that need modifications.
>
> Thank you for working on it!
>
> cc-ing JIT experts.

Thanks for the detailed explanation. I agree with this approach.
I will be sending the patches for this soon.

Thanks,
Puranjay

Re: [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX

[Puranjay Mohan]

Hi Alexei,

[...]

> I guess we never clearly defined what 'needs_zext' is supposed to be,
> so it wouldn't be fair to call 32-bit JITs buggy.
> But we better address this issue now.
> This 32-bit zeroing after LDX hurts mips64, s390, ppc64, riscv64.
> I believe all 4 JITs emit proper zero extension into 64-bit register
> by using single cpu instruction,
> but they also define bpf_jit_needs_zext() as true,
> so extra BPF_ZEXT_REG() is added by the verifier
> and it is a pure run-time overhead.

I just realised that these zext instructions will not be a runtime
overhead because the JITs ignore them.
Like
s390 does:
case BPF_LDX | BPF_MEM | BPF_B: /* dst = *(u8 *)(ul) (src + off) */
case BPF_LDX | BPF_PROBE_MEM | BPF_B:
        /* llgc %dst,0(off,%src) */
        EMIT6_DISP_LH(0xe3000000, 0x0090, dst_reg, src_reg, REG_0, off);
        jit->seen |= SEEN_MEM;
        if (insn_is_zext(&insn[1]))
                insn_count = 2; /* this will skip the next zext instruction */
        break;

powerpc does after LDX:
if (size != BPF_DW && insn_is_zext(&insn[i + 1]))
        addrs[++i] = ctx->idx * 4;

> It's better to remove
> if (t != SRC_OP)
>     return BPF_SIZE(code) == BPF_DW;
> from is_reg64() to avoid adding BPF_ZEXT_REG() insn
> and fix 32-bit JITs at the same time.
> RISCV32, PowerPC32, x86-32 JITs fixed in the first 3 patches
> to always zero upper 32-bit after LDX and
> then 4th patch to remove these two lines.

I have sent the patches for above, although I think this optimization
is useful because
zero extension after LDX is only required when the loaded value is
later being used as
a 64-bit value. If it is not the case then the verifier will not emit
the zext and 32-bit JITs will emit
1 less instruction because they expect the verifier to do the zext for
them where required.

Link to patch series:
https://lore.kernel.org/bpf/20230912224654.6556-1-puranjay12@gmail.com/T/#t

Thanks,
Puranjay

Re: [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX

[Alexei Starovoitov]

On Tue, Sep 12, 2023 at 3:49 PM Puranjay Mohan <puranjay12@gmail.com> wrote:
>
> Hi Alexei,
>
> [...]
>
> > I guess we never clearly defined what 'needs_zext' is supposed to be,
> > so it wouldn't be fair to call 32-bit JITs buggy.
> > But we better address this issue now.
> > This 32-bit zeroing after LDX hurts mips64, s390, ppc64, riscv64.
> > I believe all 4 JITs emit proper zero extension into 64-bit register
> > by using single cpu instruction,
> > but they also define bpf_jit_needs_zext() as true,
> > so extra BPF_ZEXT_REG() is added by the verifier
> > and it is a pure run-time overhead.
>
> I just realised that these zext instructions will not be a runtime
> overhead because the JITs ignore them.
> Like
> s390 does:
> case BPF_LDX | BPF_MEM | BPF_B: /* dst = *(u8 *)(ul) (src + off) */
> case BPF_LDX | BPF_PROBE_MEM | BPF_B:
>         /* llgc %dst,0(off,%src) */
>         EMIT6_DISP_LH(0xe3000000, 0x0090, dst_reg, src_reg, REG_0, off);
>         jit->seen |= SEEN_MEM;
>         if (insn_is_zext(&insn[1]))
>                 insn_count = 2; /* this will skip the next zext instruction */
>         break;
>
> powerpc does after LDX:
> if (size != BPF_DW && insn_is_zext(&insn[i + 1]))
>         addrs[++i] = ctx->idx * 4;


I see. Indeed the 64-bit JITs ignore this special zext insn after LDX.

> > It's better to remove
> > if (t != SRC_OP)
> >     return BPF_SIZE(code) == BPF_DW;
> > from is_reg64() to avoid adding BPF_ZEXT_REG() insn
> > and fix 32-bit JITs at the same time.
> > RISCV32, PowerPC32, x86-32 JITs fixed in the first 3 patches
> > to always zero upper 32-bit after LDX and
> > then 4th patch to remove these two lines.
>
> I have sent the patches for above, although I think this optimization
> is useful because
> zero extension after LDX is only required when the loaded value is
> later being used as
> a 64-bit value. If it is not the case then the verifier will not emit
> the zext and 32-bit JITs will emit
> 1 less instruction because they expect the verifier to do the zext for
> them where required.

You're correct.
Ok. Let's keep zext for LDX as-is.

Re: [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX

[Puranjay Mohan]

On Wed, Sep 13, 2023 at 2:09 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Sep 12, 2023 at 3:49 PM Puranjay Mohan <puranjay12@gmail.com> wrote:
> >
> > Hi Alexei,
> >
> > [...]
> >
> > > I guess we never clearly defined what 'needs_zext' is supposed to be,
> > > so it wouldn't be fair to call 32-bit JITs buggy.
> > > But we better address this issue now.
> > > This 32-bit zeroing after LDX hurts mips64, s390, ppc64, riscv64.
> > > I believe all 4 JITs emit proper zero extension into 64-bit register
> > > by using single cpu instruction,
> > > but they also define bpf_jit_needs_zext() as true,
> > > so extra BPF_ZEXT_REG() is added by the verifier
> > > and it is a pure run-time overhead.
> >
> > I just realised that these zext instructions will not be a runtime
> > overhead because the JITs ignore them.
> > Like
> > s390 does:
> > case BPF_LDX | BPF_MEM | BPF_B: /* dst = *(u8 *)(ul) (src + off) */
> > case BPF_LDX | BPF_PROBE_MEM | BPF_B:
> >         /* llgc %dst,0(off,%src) */
> >         EMIT6_DISP_LH(0xe3000000, 0x0090, dst_reg, src_reg, REG_0, off);
> >         jit->seen |= SEEN_MEM;
> >         if (insn_is_zext(&insn[1]))
> >                 insn_count = 2; /* this will skip the next zext instruction */
> >         break;
> >
> > powerpc does after LDX:
> > if (size != BPF_DW && insn_is_zext(&insn[i + 1]))
> >         addrs[++i] = ctx->idx * 4;
>
>
> I see. Indeed the 64-bit JITs ignore this special zext insn after LDX.
>
> > > It's better to remove
> > > if (t != SRC_OP)
> > >     return BPF_SIZE(code) == BPF_DW;
> > > from is_reg64() to avoid adding BPF_ZEXT_REG() insn
> > > and fix 32-bit JITs at the same time.
> > > RISCV32, PowerPC32, x86-32 JITs fixed in the first 3 patches
> > > to always zero upper 32-bit after LDX and
> > > then 4th patch to remove these two lines.
> >
> > I have sent the patches for above, although I think this optimization
> > is useful because
> > zero extension after LDX is only required when the loaded value is
> > later being used as
> > a 64-bit value. If it is not the case then the verifier will not emit
> > the zext and 32-bit JITs will emit
> > 1 less instruction because they expect the verifier to do the zext for
> > them where required.
>
> You're correct.
> Ok. Let's keep zext for LDX as-is.

Yes,
let's do
        if (class == BPF_LDX) {
                if (t != SRC_OP)
-                       return BPF_SIZE(code) == BPF_DW;
+                       return (BPF_SIZE(code) == BPF_DW ||
BPF_MODE(code) == BPF_MEMSX);

Thanks,
Puranjay

Re: [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX

[Alexei Starovoitov]

On Tue, Sep 12, 2023 at 5:22 PM Puranjay Mohan <puranjay12@gmail.com> wrote:
>
> On Wed, Sep 13, 2023 at 2:09 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Sep 12, 2023 at 3:49 PM Puranjay Mohan <puranjay12@gmail.com> wrote:
> > >
> > > Hi Alexei,
> > >
> > > [...]
> > >
> > > > I guess we never clearly defined what 'needs_zext' is supposed to be,
> > > > so it wouldn't be fair to call 32-bit JITs buggy.
> > > > But we better address this issue now.
> > > > This 32-bit zeroing after LDX hurts mips64, s390, ppc64, riscv64.
> > > > I believe all 4 JITs emit proper zero extension into 64-bit register
> > > > by using single cpu instruction,
> > > > but they also define bpf_jit_needs_zext() as true,
> > > > so extra BPF_ZEXT_REG() is added by the verifier
> > > > and it is a pure run-time overhead.
> > >
> > > I just realised that these zext instructions will not be a runtime
> > > overhead because the JITs ignore them.
> > > Like
> > > s390 does:
> > > case BPF_LDX | BPF_MEM | BPF_B: /* dst = *(u8 *)(ul) (src + off) */
> > > case BPF_LDX | BPF_PROBE_MEM | BPF_B:
> > >         /* llgc %dst,0(off,%src) */
> > >         EMIT6_DISP_LH(0xe3000000, 0x0090, dst_reg, src_reg, REG_0, off);
> > >         jit->seen |= SEEN_MEM;
> > >         if (insn_is_zext(&insn[1]))
> > >                 insn_count = 2; /* this will skip the next zext instruction */
> > >         break;
> > >
> > > powerpc does after LDX:
> > > if (size != BPF_DW && insn_is_zext(&insn[i + 1]))
> > >         addrs[++i] = ctx->idx * 4;
> >
> >
> > I see. Indeed the 64-bit JITs ignore this special zext insn after LDX.
> >
> > > > It's better to remove
> > > > if (t != SRC_OP)
> > > >     return BPF_SIZE(code) == BPF_DW;
> > > > from is_reg64() to avoid adding BPF_ZEXT_REG() insn
> > > > and fix 32-bit JITs at the same time.
> > > > RISCV32, PowerPC32, x86-32 JITs fixed in the first 3 patches
> > > > to always zero upper 32-bit after LDX and
> > > > then 4th patch to remove these two lines.
> > >
> > > I have sent the patches for above, although I think this optimization
> > > is useful because
> > > zero extension after LDX is only required when the loaded value is
> > > later being used as
> > > a 64-bit value. If it is not the case then the verifier will not emit
> > > the zext and 32-bit JITs will emit
> > > 1 less instruction because they expect the verifier to do the zext for
> > > them where required.
> >
> > You're correct.
> > Ok. Let's keep zext for LDX as-is.
>
> Yes,
> let's do
>         if (class == BPF_LDX) {
>                 if (t != SRC_OP)
> -                       return BPF_SIZE(code) == BPF_DW;
> +                       return (BPF_SIZE(code) == BPF_DW ||
> BPF_MODE(code) == BPF_MEMSX);

Agree. imo that's a cleaner approach vs changing mark_insn_zext().

Re: [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX

[Christophe Leroy]

Le 13/09/2023 à 02:22, Puranjay Mohan a écrit :
> On Wed, Sep 13, 2023 at 2:09 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> On Tue, Sep 12, 2023 at 3:49 PM Puranjay Mohan <puranjay12@gmail.com> wrote:
>>>
>>> Hi Alexei,
>>>
>>> [...]
>>>
>>>> I guess we never clearly defined what 'needs_zext' is supposed to be,
>>>> so it wouldn't be fair to call 32-bit JITs buggy.
>>>> But we better address this issue now.
>>>> This 32-bit zeroing after LDX hurts mips64, s390, ppc64, riscv64.
>>>> I believe all 4 JITs emit proper zero extension into 64-bit register
>>>> by using single cpu instruction,
>>>> but they also define bpf_jit_needs_zext() as true,
>>>> so extra BPF_ZEXT_REG() is added by the verifier
>>>> and it is a pure run-time overhead.
>>>
>>> I just realised that these zext instructions will not be a runtime
>>> overhead because the JITs ignore them.
>>> Like
>>> s390 does:
>>> case BPF_LDX | BPF_MEM | BPF_B: /* dst = *(u8 *)(ul) (src + off) */
>>> case BPF_LDX | BPF_PROBE_MEM | BPF_B:
>>>          /* llgc %dst,0(off,%src) */
>>>          EMIT6_DISP_LH(0xe3000000, 0x0090, dst_reg, src_reg, REG_0, off);
>>>          jit->seen |= SEEN_MEM;
>>>          if (insn_is_zext(&insn[1]))
>>>                  insn_count = 2; /* this will skip the next zext instruction */
>>>          break;
>>>
>>> powerpc does after LDX:
>>> if (size != BPF_DW && insn_is_zext(&insn[i + 1]))
>>>          addrs[++i] = ctx->idx * 4;
>>
>>
>> I see. Indeed the 64-bit JITs ignore this special zext insn after LDX.
>>
>>>> It's better to remove
>>>> if (t != SRC_OP)
>>>>      return BPF_SIZE(code) == BPF_DW;
>>>> from is_reg64() to avoid adding BPF_ZEXT_REG() insn
>>>> and fix 32-bit JITs at the same time.
>>>> RISCV32, PowerPC32, x86-32 JITs fixed in the first 3 patches
>>>> to always zero upper 32-bit after LDX and
>>>> then 4th patch to remove these two lines.
>>>
>>> I have sent the patches for above, although I think this optimization
>>> is useful because
>>> zero extension after LDX is only required when the loaded value is
>>> later being used as
>>> a 64-bit value. If it is not the case then the verifier will not emit
>>> the zext and 32-bit JITs will emit
>>> 1 less instruction because they expect the verifier to do the zext for
>>> them where required.
>>
>> You're correct.
>> Ok. Let's keep zext for LDX as-is.
> 
> Yes,
> let's do
>          if (class == BPF_LDX) {
>                  if (t != SRC_OP)
> -                       return BPF_SIZE(code) == BPF_DW;
> +                       return (BPF_SIZE(code) == BPF_DW ||
> BPF_MODE(code) == BPF_MEMSX);

You don't need the parenthesis, just do

	return BPF_SIZE(code) == BPF_DW || BPF_MODE(code) == BPF_MEMSX;


Christophe

Re: [PATCH bpf-next 00/11] Implement cpuv4 support for s390x

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Wed, 30 Aug 2023 03:07:41 +0200 you wrote:
> Hi,
> 
> This series adds the cpuv4 support to the s390x eBPF JIT.
> Patches 1-4 are preliminary bugfixes.
> Patches 5-9 implement the new instructions.
> Patches 10-11 enable the tests.
> 
> [...]

Here is the summary with links:
  - [bpf-next,01/11] bpf: Disable zero-extension for BPF_MEMSX
    (no matching commit)
  - [bpf-next,02/11] net: netfilter: Adjust timeouts of non-confirmed CTs in bpf_ct_insert_entry()
    https://git.kernel.org/bpf/bpf/c/6bd5bcb18f94
  - [bpf-next,03/11] selftests/bpf: Unmount the cgroup2 work directory
    (no matching commit)
  - [bpf-next,04/11] selftests/bpf: Add big-endian support to the ldsx test
    (no matching commit)
  - [bpf-next,05/11] s390/bpf: Implement BPF_MOV | BPF_X with sign-extension
    (no matching commit)
  - [bpf-next,06/11] s390/bpf: Implement BPF_MEMSX
    (no matching commit)
  - [bpf-next,07/11] s390/bpf: Implement unconditional byte swap
    (no matching commit)
  - [bpf-next,08/11] s390/bpf: Implement unconditional jump with 32-bit offset
    (no matching commit)
  - [bpf-next,09/11] s390/bpf: Implement signed division
    (no matching commit)
  - [bpf-next,10/11] selftests/bpf: Enable the cpuv4 tests for s390x
    (no matching commit)
  - [bpf-next,11/11] selftests/bpf: Trim DENYLIST.s390x
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html