[PATCH bpf-next 0/2] bpf: Fix an issue in verifing allow_ptr_leaks

[PATCH bpf-next 0/2] bpf: Fix an issue in verifing allow_ptr_leaks

[Yafang Shao]

Patch #1: An issue found in our local 6.1 kernel.
          This issue also exists in bpf-next.
Patch #2: Selftess for #1 

Yafang Shao (2):
  bpf: Fix issue in verifying allow_ptr_leaks
  selftests/bpf: Add selftest for allow_ptr_leaks

 kernel/bpf/verifier.c                           | 17 ++++++------
 tools/testing/selftests/bpf/prog_tests/tc_bpf.c | 36 ++++++++++++++++++++++++-
 tools/testing/selftests/bpf/progs/test_tc_bpf.c | 14 ++++++++++
 3 files changed, 58 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[PATCH bpf-next 1/2] bpf: Fix issue in verifying allow_ptr_leaks

[Yafang Shao]

After we converted the capabilities of our networking-bpf program from
cap_sys_admin to cap_net_admin+cap_bpf, our networking-bpf program
failed to start. Because it failed the bpf verifier, and the error log
is "R3 pointer comparison prohibited".

A simple reproducer as follows,

SEC("cls-ingress")
int ingress(struct __sk_buff *skb)
{
	struct iphdr *iph = (void *)(long)skb->data + sizeof(struct ethhdr);

	if ((long)(iph + 1) > (long)skb->data_end)
		return TC_ACT_STOLEN;
	return TC_ACT_OK;
}

Per discussion with Yonghong and Alexei [1], comparison of two packet
pointers is not a pointer leak. This patch fixes it.

Our local kernel is 6.1.y and we expect this fix to be backported to
6.1.y, so stable is CCed.

[1]. https://lore.kernel.org/bpf/CAADnVQ+Nmspr7Si+pxWn8zkE7hX-7s93ugwC+94aXSy4uQ9vBg@mail.gmail.com/

Suggested-by: Yonghong Song <yonghong.song@linux.dev>
Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: stable@vger.kernel.org
---
 kernel/bpf/verifier.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 4ccca1f..b6b60cd 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -14047,6 +14047,12 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
 		return -EINVAL;
 	}
 
+	/* check src2 operand */
+	err = check_reg_arg(env, insn->dst_reg, SRC_OP);
+	if (err)
+		return err;
+
+	dst_reg = &regs[insn->dst_reg];
 	if (BPF_SRC(insn->code) == BPF_X) {
 		if (insn->imm != 0) {
 			verbose(env, "BPF_JMP/JMP32 uses reserved fields\n");
@@ -14058,12 +14064,13 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
 		if (err)
 			return err;
 
-		if (is_pointer_value(env, insn->src_reg)) {
+		src_reg = &regs[insn->src_reg];
+		if (!(reg_is_pkt_pointer_any(dst_reg) && reg_is_pkt_pointer_any(src_reg)) &&
+		    is_pointer_value(env, insn->src_reg)) {
 			verbose(env, "R%d pointer comparison prohibited\n",
 				insn->src_reg);
 			return -EACCES;
 		}
-		src_reg = &regs[insn->src_reg];
 	} else {
 		if (insn->src_reg != BPF_REG_0) {
 			verbose(env, "BPF_JMP/JMP32 uses reserved fields\n");
@@ -14071,12 +14078,6 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
 		}
 	}
 
-	/* check src2 operand */
-	err = check_reg_arg(env, insn->dst_reg, SRC_OP);
-	if (err)
-		return err;
-
-	dst_reg = &regs[insn->dst_reg];
 	is_jmp32 = BPF_CLASS(insn->code) == BPF_JMP32;
 
 	if (BPF_SRC(insn->code) == BPF_K) {
-- 
1.8.3.1

Re: [PATCH bpf-next 1/2] bpf: Fix issue in verifying allow_ptr_leaks

[Eduard Zingerman]

On Fri, 2023-08-18 at 08:39 +0000, Yafang Shao wrote:
> After we converted the capabilities of our networking-bpf program from
> cap_sys_admin to cap_net_admin+cap_bpf, our networking-bpf program
> failed to start. Because it failed the bpf verifier, and the error log
> is "R3 pointer comparison prohibited".
> 
> A simple reproducer as follows,
> 
> SEC("cls-ingress")
> int ingress(struct __sk_buff *skb)
> {
> 	struct iphdr *iph = (void *)(long)skb->data + sizeof(struct ethhdr);
> 
> 	if ((long)(iph + 1) > (long)skb->data_end)
> 		return TC_ACT_STOLEN;
> 	return TC_ACT_OK;
> }
> 
> Per discussion with Yonghong and Alexei [1], comparison of two packet
> pointers is not a pointer leak. This patch fixes it.
> 
> Our local kernel is 6.1.y and we expect this fix to be backported to
> 6.1.y, so stable is CCed.
> 
> [1]. https://lore.kernel.org/bpf/CAADnVQ+Nmspr7Si+pxWn8zkE7hX-7s93ugwC+94aXSy4uQ9vBg@mail.gmail.com/
> 
> Suggested-by: Yonghong Song <yonghong.song@linux.dev>
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: stable@vger.kernel.org
> ---
>  kernel/bpf/verifier.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 4ccca1f..b6b60cd 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -14047,6 +14047,12 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
>  		return -EINVAL;
>  	}
>  
> +	/* check src2 operand */
> +	err = check_reg_arg(env, insn->dst_reg, SRC_OP);
> +	if (err)
> +		return err;
> +
> +	dst_reg = &regs[insn->dst_reg];
>  	if (BPF_SRC(insn->code) == BPF_X) {
>  		if (insn->imm != 0) {
>  			verbose(env, "BPF_JMP/JMP32 uses reserved fields\n");
> @@ -14058,12 +14064,13 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
>  		if (err)
>  			return err;
>  
> -		if (is_pointer_value(env, insn->src_reg)) {
> +		src_reg = &regs[insn->src_reg];
> +		if (!(reg_is_pkt_pointer_any(dst_reg) && reg_is_pkt_pointer_any(src_reg)) &&
> +		    is_pointer_value(env, insn->src_reg)) {
>  			verbose(env, "R%d pointer comparison prohibited\n",
>  				insn->src_reg);
>  			return -EACCES;
>  		}
> -		src_reg = &regs[insn->src_reg];

I tested this change and it seem to work as intended. Was worried a
bit that there are three places in this function where such checks are
applied:
1. upon entry for BPF_X case (this one): checks if dst_reg/src_reg are
   pointers to packet or packet end or packet meta;
2. when attempting to predict branch: prediction would be triggered
   only when dst/src is packet/packet_end (or vice-versa);
3. when prediction failed and both branches have to be visited
   (`try_match_pkt_pointers`): dst/src have to be packet/packet_end or
   meta/packet-start (or vice versa).
   
Check (1) is more permissive than (2) or (3) but either (2) or (3)
would be applied before exit, so there is no contradiction.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

>  	} else {
>  		if (insn->src_reg != BPF_REG_0) {
>  			verbose(env, "BPF_JMP/JMP32 uses reserved fields\n");
> @@ -14071,12 +14078,6 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
>  		}
>  	}
>  
> -	/* check src2 operand */
> -	err = check_reg_arg(env, insn->dst_reg, SRC_OP);
> -	if (err)
> -		return err;
> -
> -	dst_reg = &regs[insn->dst_reg];
>  	is_jmp32 = BPF_CLASS(insn->code) == BPF_JMP32;
>  
>  	if (BPF_SRC(insn->code) == BPF_K) {

[PATCH bpf-next 2/2] selftests/bpf: Add selftest for allow_ptr_leaks

[Yafang Shao]

- Without prev commit

  $ tools/testing/selftests/bpf/test_progs --name=tc_bpf
  #232/1   tc_bpf/tc_bpf_root:OK
  test_tc_bpf_non_root:PASS:set_cap_bpf_cap_net_admin 0 nsec
  test_tc_bpf_non_root:PASS:disable_cap_sys_admin 0 nsec
  0: R1=ctx(off=0,imm=0) R10=fp0
  ; if ((long)(iph + 1) > (long)skb->data_end)
  0: (61) r2 = *(u32 *)(r1 +80)         ; R1=ctx(off=0,imm=0) R2_w=pkt_end(off=0,imm=0)
  ; struct iphdr *iph = (void *)(long)skb->data + sizeof(struct ethhdr);
  1: (61) r1 = *(u32 *)(r1 +76)         ; R1_w=pkt(off=0,r=0,imm=0)
  ; if ((long)(iph + 1) > (long)skb->data_end)
  2: (07) r1 += 34                      ; R1_w=pkt(off=34,r=0,imm=0)
  3: (b4) w0 = 1                        ; R0_w=1
  4: (2d) if r1 > r2 goto pc+1
  R2 pointer comparison prohibited
  processed 5 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
  test_tc_bpf_non_root:FAIL:test_tc_bpf__open_and_load unexpected error: -13
  #233/2   tc_bpf_non_root:FAIL

- With prev commit

  $ tools/testing/selftests/bpf/test_progs --name=tc_bpf
  #232/1   tc_bpf/tc_bpf_root:OK
  #232/2   tc_bpf/tc_bpf_non_root:OK
  #232     tc_bpf:OK
  Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/tc_bpf.c | 36 ++++++++++++++++++++++++-
 tools/testing/selftests/bpf/progs/test_tc_bpf.c | 14 ++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/tc_bpf.c b/tools/testing/selftests/bpf/prog_tests/tc_bpf.c
index e873766..48b5553 100644
--- a/tools/testing/selftests/bpf/prog_tests/tc_bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/tc_bpf.c
@@ -3,6 +3,7 @@
 #include <test_progs.h>
 #include <linux/pkt_cls.h>
 
+#include "cap_helpers.h"
 #include "test_tc_bpf.skel.h"
 
 #define LO_IFINDEX 1
@@ -327,7 +328,7 @@ static int test_tc_bpf_api(struct bpf_tc_hook *hook, int fd)
 	return 0;
 }
 
-void test_tc_bpf(void)
+void tc_bpf_root(void)
 {
 	DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = LO_IFINDEX,
 			    .attach_point = BPF_TC_INGRESS);
@@ -393,3 +394,36 @@ void test_tc_bpf(void)
 	}
 	test_tc_bpf__destroy(skel);
 }
+
+void tc_bpf_non_root(void)
+{
+	struct test_tc_bpf *skel = NULL;
+	__u64 caps = 0;
+	int ret;
+
+	/* In case CAP_BPF and CAP_PERFMON is not set */
+	ret = cap_enable_effective(1ULL << CAP_BPF | 1ULL << CAP_NET_ADMIN, &caps);
+	if (!ASSERT_OK(ret, "set_cap_bpf_cap_net_admin"))
+		return;
+	ret = cap_disable_effective(1ULL << CAP_SYS_ADMIN | 1ULL << CAP_PERFMON, NULL);
+	if (!ASSERT_OK(ret, "disable_cap_sys_admin"))
+		goto restore_cap;
+
+	skel = test_tc_bpf__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "test_tc_bpf__open_and_load"))
+		goto restore_cap;
+
+	test_tc_bpf__destroy(skel);
+
+restore_cap:
+	if (caps)
+		cap_enable_effective(caps, NULL);
+}
+
+void test_tc_bpf(void)
+{
+	if (test__start_subtest("tc_bpf_root"))
+		tc_bpf_root();
+	if (test__start_subtest("tc_bpf_non_root"))
+		tc_bpf_non_root();
+}
diff --git a/tools/testing/selftests/bpf/progs/test_tc_bpf.c b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
index d28ca8d..3e0f218 100644
--- a/tools/testing/selftests/bpf/progs/test_tc_bpf.c
+++ b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
@@ -1,5 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0
 
+#include <linux/pkt_cls.h>
+#include <linux/ip.h>
+#include <linux/if_ether.h>
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 
@@ -10,3 +13,14 @@ int cls(struct __sk_buff *skb)
 {
 	return 0;
 }
+
+/* Prog to verify tc-bpf without cap_sys_admin and cap_perfmon */
+SEC("tcx/ingress")
+int pkt_ptr(struct __sk_buff *skb)
+{
+	struct iphdr *iph = (void *)(long)skb->data + sizeof(struct ethhdr);
+
+	if ((long)(iph + 1) > (long)skb->data_end)
+		return TC_ACT_STOLEN;
+	return TC_ACT_OK;
+}
-- 
1.8.3.1

Re: [PATCH bpf-next 2/2] selftests/bpf: Add selftest for allow_ptr_leaks

[Alexei Starovoitov]

On Fri, Aug 18, 2023 at 1:39 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> - Without prev commit
>
>   $ tools/testing/selftests/bpf/test_progs --name=tc_bpf
>   #232/1   tc_bpf/tc_bpf_root:OK
>   test_tc_bpf_non_root:PASS:set_cap_bpf_cap_net_admin 0 nsec
>   test_tc_bpf_non_root:PASS:disable_cap_sys_admin 0 nsec
>   0: R1=ctx(off=0,imm=0) R10=fp0
>   ; if ((long)(iph + 1) > (long)skb->data_end)
>   0: (61) r2 = *(u32 *)(r1 +80)         ; R1=ctx(off=0,imm=0) R2_w=pkt_end(off=0,imm=0)
>   ; struct iphdr *iph = (void *)(long)skb->data + sizeof(struct ethhdr);
>   1: (61) r1 = *(u32 *)(r1 +76)         ; R1_w=pkt(off=0,r=0,imm=0)
>   ; if ((long)(iph + 1) > (long)skb->data_end)
>   2: (07) r1 += 34                      ; R1_w=pkt(off=34,r=0,imm=0)
>   3: (b4) w0 = 1                        ; R0_w=1
>   4: (2d) if r1 > r2 goto pc+1
>   R2 pointer comparison prohibited
>   processed 5 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>   test_tc_bpf_non_root:FAIL:test_tc_bpf__open_and_load unexpected error: -13
>   #233/2   tc_bpf_non_root:FAIL
>
> - With prev commit
>
>   $ tools/testing/selftests/bpf/test_progs --name=tc_bpf
>   #232/1   tc_bpf/tc_bpf_root:OK
>   #232/2   tc_bpf/tc_bpf_non_root:OK
>   #232     tc_bpf:OK
>   Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  tools/testing/selftests/bpf/prog_tests/tc_bpf.c | 36 ++++++++++++++++++++++++-
>  tools/testing/selftests/bpf/progs/test_tc_bpf.c | 14 ++++++++++
>  2 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/tc_bpf.c b/tools/testing/selftests/bpf/prog_tests/tc_bpf.c
> index e873766..48b5553 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tc_bpf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tc_bpf.c
> @@ -3,6 +3,7 @@
>  #include <test_progs.h>
>  #include <linux/pkt_cls.h>
>
> +#include "cap_helpers.h"
>  #include "test_tc_bpf.skel.h"
>
>  #define LO_IFINDEX 1
> @@ -327,7 +328,7 @@ static int test_tc_bpf_api(struct bpf_tc_hook *hook, int fd)
>         return 0;
>  }
>
> -void test_tc_bpf(void)
> +void tc_bpf_root(void)
>  {
>         DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = LO_IFINDEX,
>                             .attach_point = BPF_TC_INGRESS);
> @@ -393,3 +394,36 @@ void test_tc_bpf(void)
>         }
>         test_tc_bpf__destroy(skel);
>  }
> +
> +void tc_bpf_non_root(void)
> +{
> +       struct test_tc_bpf *skel = NULL;
> +       __u64 caps = 0;
> +       int ret;
> +
> +       /* In case CAP_BPF and CAP_PERFMON is not set */
> +       ret = cap_enable_effective(1ULL << CAP_BPF | 1ULL << CAP_NET_ADMIN, &caps);
> +       if (!ASSERT_OK(ret, "set_cap_bpf_cap_net_admin"))
> +               return;
> +       ret = cap_disable_effective(1ULL << CAP_SYS_ADMIN | 1ULL << CAP_PERFMON, NULL);
> +       if (!ASSERT_OK(ret, "disable_cap_sys_admin"))
> +               goto restore_cap;
> +
> +       skel = test_tc_bpf__open_and_load();
> +       if (!ASSERT_OK_PTR(skel, "test_tc_bpf__open_and_load"))
> +               goto restore_cap;
> +
> +       test_tc_bpf__destroy(skel);
> +
> +restore_cap:
> +       if (caps)
> +               cap_enable_effective(caps, NULL);
> +}
> +
> +void test_tc_bpf(void)
> +{
> +       if (test__start_subtest("tc_bpf_root"))
> +               tc_bpf_root();
> +       if (test__start_subtest("tc_bpf_non_root"))
> +               tc_bpf_non_root();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_tc_bpf.c b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> index d28ca8d..3e0f218 100644
> --- a/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> +++ b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> @@ -1,5 +1,8 @@
>  // SPDX-License-Identifier: GPL-2.0
>
> +#include <linux/pkt_cls.h>
> +#include <linux/ip.h>
> +#include <linux/if_ether.h>

Due to above it fails to compile:

In file included from progs/test_tc_bpf.c:4:
In file included from /usr/include/linux/ip.h:21:
In file included from /usr/include/asm/byteorder.h:5:
In file included from /usr/include/linux/byteorder/little_endian.h:13:
/usr/include/linux/swab.h:136:8: error: unknown type name '__always_inline'
  136 | static __always_inline unsigned long __swab(const unsigned long y)
      |        ^

Re: [PATCH bpf-next 2/2] selftests/bpf: Add selftest for allow_ptr_leaks

[Yafang Shao]

On Tue, Aug 22, 2023 at 6:45 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Aug 18, 2023 at 1:39 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > - Without prev commit
> >
> >   $ tools/testing/selftests/bpf/test_progs --name=tc_bpf
> >   #232/1   tc_bpf/tc_bpf_root:OK
> >   test_tc_bpf_non_root:PASS:set_cap_bpf_cap_net_admin 0 nsec
> >   test_tc_bpf_non_root:PASS:disable_cap_sys_admin 0 nsec
> >   0: R1=ctx(off=0,imm=0) R10=fp0
> >   ; if ((long)(iph + 1) > (long)skb->data_end)
> >   0: (61) r2 = *(u32 *)(r1 +80)         ; R1=ctx(off=0,imm=0) R2_w=pkt_end(off=0,imm=0)
> >   ; struct iphdr *iph = (void *)(long)skb->data + sizeof(struct ethhdr);
> >   1: (61) r1 = *(u32 *)(r1 +76)         ; R1_w=pkt(off=0,r=0,imm=0)
> >   ; if ((long)(iph + 1) > (long)skb->data_end)
> >   2: (07) r1 += 34                      ; R1_w=pkt(off=34,r=0,imm=0)
> >   3: (b4) w0 = 1                        ; R0_w=1
> >   4: (2d) if r1 > r2 goto pc+1
> >   R2 pointer comparison prohibited
> >   processed 5 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
> >   test_tc_bpf_non_root:FAIL:test_tc_bpf__open_and_load unexpected error: -13
> >   #233/2   tc_bpf_non_root:FAIL
> >
> > - With prev commit
> >
> >   $ tools/testing/selftests/bpf/test_progs --name=tc_bpf
> >   #232/1   tc_bpf/tc_bpf_root:OK
> >   #232/2   tc_bpf/tc_bpf_non_root:OK
> >   #232     tc_bpf:OK
> >   Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  tools/testing/selftests/bpf/prog_tests/tc_bpf.c | 36 ++++++++++++++++++++++++-
> >  tools/testing/selftests/bpf/progs/test_tc_bpf.c | 14 ++++++++++
> >  2 files changed, 49 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/tc_bpf.c b/tools/testing/selftests/bpf/prog_tests/tc_bpf.c
> > index e873766..48b5553 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/tc_bpf.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/tc_bpf.c
> > @@ -3,6 +3,7 @@
> >  #include <test_progs.h>
> >  #include <linux/pkt_cls.h>
> >
> > +#include "cap_helpers.h"
> >  #include "test_tc_bpf.skel.h"
> >
> >  #define LO_IFINDEX 1
> > @@ -327,7 +328,7 @@ static int test_tc_bpf_api(struct bpf_tc_hook *hook, int fd)
> >         return 0;
> >  }
> >
> > -void test_tc_bpf(void)
> > +void tc_bpf_root(void)
> >  {
> >         DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = LO_IFINDEX,
> >                             .attach_point = BPF_TC_INGRESS);
> > @@ -393,3 +394,36 @@ void test_tc_bpf(void)
> >         }
> >         test_tc_bpf__destroy(skel);
> >  }
> > +
> > +void tc_bpf_non_root(void)
> > +{
> > +       struct test_tc_bpf *skel = NULL;
> > +       __u64 caps = 0;
> > +       int ret;
> > +
> > +       /* In case CAP_BPF and CAP_PERFMON is not set */
> > +       ret = cap_enable_effective(1ULL << CAP_BPF | 1ULL << CAP_NET_ADMIN, &caps);
> > +       if (!ASSERT_OK(ret, "set_cap_bpf_cap_net_admin"))
> > +               return;
> > +       ret = cap_disable_effective(1ULL << CAP_SYS_ADMIN | 1ULL << CAP_PERFMON, NULL);
> > +       if (!ASSERT_OK(ret, "disable_cap_sys_admin"))
> > +               goto restore_cap;
> > +
> > +       skel = test_tc_bpf__open_and_load();
> > +       if (!ASSERT_OK_PTR(skel, "test_tc_bpf__open_and_load"))
> > +               goto restore_cap;
> > +
> > +       test_tc_bpf__destroy(skel);
> > +
> > +restore_cap:
> > +       if (caps)
> > +               cap_enable_effective(caps, NULL);
> > +}
> > +
> > +void test_tc_bpf(void)
> > +{
> > +       if (test__start_subtest("tc_bpf_root"))
> > +               tc_bpf_root();
> > +       if (test__start_subtest("tc_bpf_non_root"))
> > +               tc_bpf_non_root();
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/test_tc_bpf.c b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > index d28ca8d..3e0f218 100644
> > --- a/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > +++ b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > @@ -1,5 +1,8 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >
> > +#include <linux/pkt_cls.h>
> > +#include <linux/ip.h>
> > +#include <linux/if_ether.h>
>
> Due to above it fails to compile:
>
> In file included from progs/test_tc_bpf.c:4:
> In file included from /usr/include/linux/ip.h:21:
> In file included from /usr/include/asm/byteorder.h:5:
> In file included from /usr/include/linux/byteorder/little_endian.h:13:
> /usr/include/linux/swab.h:136:8: error: unknown type name '__always_inline'
>   136 | static __always_inline unsigned long __swab(const unsigned long y)
>       |        ^

I can't find the above error log in the BPF CI log.
The BPF CI log just shows that it fails the test_map on s390 without
logs. Not sure why.

__always_inline is defined in bpf_helpers.h, so I think below
additional change could fix it.

--- a/tools/testing/selftests/bpf/progs/test_tc_bpf.c
+++ b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
@@ -1,10 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0

+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
 #include <linux/pkt_cls.h>
 #include <linux/ip.h>
 #include <linux/if_ether.h>
-#include <linux/bpf.h>
-#include <bpf/bpf_helpers.h>


-- 
Regards
Yafang

Re: [PATCH bpf-next 2/2] selftests/bpf: Add selftest for allow_ptr_leaks

[Alexei Starovoitov]

On Mon, Aug 21, 2023 at 7:43 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Tue, Aug 22, 2023 at 6:45 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Aug 18, 2023 at 1:39 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> > >
> > > - Without prev commit
> > >
> > >   $ tools/testing/selftests/bpf/test_progs --name=tc_bpf
> > >   #232/1   tc_bpf/tc_bpf_root:OK
> > >   test_tc_bpf_non_root:PASS:set_cap_bpf_cap_net_admin 0 nsec
> > >   test_tc_bpf_non_root:PASS:disable_cap_sys_admin 0 nsec
> > >   0: R1=ctx(off=0,imm=0) R10=fp0
> > >   ; if ((long)(iph + 1) > (long)skb->data_end)
> > >   0: (61) r2 = *(u32 *)(r1 +80)         ; R1=ctx(off=0,imm=0) R2_w=pkt_end(off=0,imm=0)
> > >   ; struct iphdr *iph = (void *)(long)skb->data + sizeof(struct ethhdr);
> > >   1: (61) r1 = *(u32 *)(r1 +76)         ; R1_w=pkt(off=0,r=0,imm=0)
> > >   ; if ((long)(iph + 1) > (long)skb->data_end)
> > >   2: (07) r1 += 34                      ; R1_w=pkt(off=34,r=0,imm=0)
> > >   3: (b4) w0 = 1                        ; R0_w=1
> > >   4: (2d) if r1 > r2 goto pc+1
> > >   R2 pointer comparison prohibited
> > >   processed 5 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
> > >   test_tc_bpf_non_root:FAIL:test_tc_bpf__open_and_load unexpected error: -13
> > >   #233/2   tc_bpf_non_root:FAIL
> > >
> > > - With prev commit
> > >
> > >   $ tools/testing/selftests/bpf/test_progs --name=tc_bpf
> > >   #232/1   tc_bpf/tc_bpf_root:OK
> > >   #232/2   tc_bpf/tc_bpf_non_root:OK
> > >   #232     tc_bpf:OK
> > >   Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED
> > >
> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > ---
> > >  tools/testing/selftests/bpf/prog_tests/tc_bpf.c | 36 ++++++++++++++++++++++++-
> > >  tools/testing/selftests/bpf/progs/test_tc_bpf.c | 14 ++++++++++
> > >  2 files changed, 49 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/tc_bpf.c b/tools/testing/selftests/bpf/prog_tests/tc_bpf.c
> > > index e873766..48b5553 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/tc_bpf.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/tc_bpf.c
> > > @@ -3,6 +3,7 @@
> > >  #include <test_progs.h>
> > >  #include <linux/pkt_cls.h>
> > >
> > > +#include "cap_helpers.h"
> > >  #include "test_tc_bpf.skel.h"
> > >
> > >  #define LO_IFINDEX 1
> > > @@ -327,7 +328,7 @@ static int test_tc_bpf_api(struct bpf_tc_hook *hook, int fd)
> > >         return 0;
> > >  }
> > >
> > > -void test_tc_bpf(void)
> > > +void tc_bpf_root(void)
> > >  {
> > >         DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = LO_IFINDEX,
> > >                             .attach_point = BPF_TC_INGRESS);
> > > @@ -393,3 +394,36 @@ void test_tc_bpf(void)
> > >         }
> > >         test_tc_bpf__destroy(skel);
> > >  }
> > > +
> > > +void tc_bpf_non_root(void)
> > > +{
> > > +       struct test_tc_bpf *skel = NULL;
> > > +       __u64 caps = 0;
> > > +       int ret;
> > > +
> > > +       /* In case CAP_BPF and CAP_PERFMON is not set */
> > > +       ret = cap_enable_effective(1ULL << CAP_BPF | 1ULL << CAP_NET_ADMIN, &caps);
> > > +       if (!ASSERT_OK(ret, "set_cap_bpf_cap_net_admin"))
> > > +               return;
> > > +       ret = cap_disable_effective(1ULL << CAP_SYS_ADMIN | 1ULL << CAP_PERFMON, NULL);
> > > +       if (!ASSERT_OK(ret, "disable_cap_sys_admin"))
> > > +               goto restore_cap;
> > > +
> > > +       skel = test_tc_bpf__open_and_load();
> > > +       if (!ASSERT_OK_PTR(skel, "test_tc_bpf__open_and_load"))
> > > +               goto restore_cap;
> > > +
> > > +       test_tc_bpf__destroy(skel);
> > > +
> > > +restore_cap:
> > > +       if (caps)
> > > +               cap_enable_effective(caps, NULL);
> > > +}
> > > +
> > > +void test_tc_bpf(void)
> > > +{
> > > +       if (test__start_subtest("tc_bpf_root"))
> > > +               tc_bpf_root();
> > > +       if (test__start_subtest("tc_bpf_non_root"))
> > > +               tc_bpf_non_root();
> > > +}
> > > diff --git a/tools/testing/selftests/bpf/progs/test_tc_bpf.c b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > > index d28ca8d..3e0f218 100644
> > > --- a/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > > +++ b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > > @@ -1,5 +1,8 @@
> > >  // SPDX-License-Identifier: GPL-2.0
> > >
> > > +#include <linux/pkt_cls.h>
> > > +#include <linux/ip.h>
> > > +#include <linux/if_ether.h>
> >
> > Due to above it fails to compile:
> >
> > In file included from progs/test_tc_bpf.c:4:
> > In file included from /usr/include/linux/ip.h:21:
> > In file included from /usr/include/asm/byteorder.h:5:
> > In file included from /usr/include/linux/byteorder/little_endian.h:13:
> > /usr/include/linux/swab.h:136:8: error: unknown type name '__always_inline'
> >   136 | static __always_inline unsigned long __swab(const unsigned long y)
> >       |        ^
>
> I can't find the above error log in the BPF CI log.
> The BPF CI log just shows that it fails the test_map on s390 without
> logs. Not sure why.

It's not in CI. I caught it by manual build.

> __always_inline is defined in bpf_helpers.h, so I think below
> additional change could fix it.
>
> --- a/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> +++ b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> @@ -1,10 +1,10 @@
>  // SPDX-License-Identifier: GPL-2.0
>
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
>  #include <linux/pkt_cls.h>
>  #include <linux/ip.h>
>  #include <linux/if_ether.h>
> -#include <linux/bpf.h>
> -#include <bpf/bpf_helpers.h>

Maybe, but I'd rather remove the includes and replace
TC_ACT_STOLEN/ACT_OK with zeros, since the return values are
irrelevant.

Re: [PATCH bpf-next 2/2] selftests/bpf: Add selftest for allow_ptr_leaks

[Yafang Shao]

On Tue, Aug 22, 2023 at 11:28 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Aug 21, 2023 at 7:43 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > On Tue, Aug 22, 2023 at 6:45 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Fri, Aug 18, 2023 at 1:39 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> > > >
> > > > - Without prev commit
> > > >
> > > >   $ tools/testing/selftests/bpf/test_progs --name=tc_bpf
> > > >   #232/1   tc_bpf/tc_bpf_root:OK
> > > >   test_tc_bpf_non_root:PASS:set_cap_bpf_cap_net_admin 0 nsec
> > > >   test_tc_bpf_non_root:PASS:disable_cap_sys_admin 0 nsec
> > > >   0: R1=ctx(off=0,imm=0) R10=fp0
> > > >   ; if ((long)(iph + 1) > (long)skb->data_end)
> > > >   0: (61) r2 = *(u32 *)(r1 +80)         ; R1=ctx(off=0,imm=0) R2_w=pkt_end(off=0,imm=0)
> > > >   ; struct iphdr *iph = (void *)(long)skb->data + sizeof(struct ethhdr);
> > > >   1: (61) r1 = *(u32 *)(r1 +76)         ; R1_w=pkt(off=0,r=0,imm=0)
> > > >   ; if ((long)(iph + 1) > (long)skb->data_end)
> > > >   2: (07) r1 += 34                      ; R1_w=pkt(off=34,r=0,imm=0)
> > > >   3: (b4) w0 = 1                        ; R0_w=1
> > > >   4: (2d) if r1 > r2 goto pc+1
> > > >   R2 pointer comparison prohibited
> > > >   processed 5 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
> > > >   test_tc_bpf_non_root:FAIL:test_tc_bpf__open_and_load unexpected error: -13
> > > >   #233/2   tc_bpf_non_root:FAIL
> > > >
> > > > - With prev commit
> > > >
> > > >   $ tools/testing/selftests/bpf/test_progs --name=tc_bpf
> > > >   #232/1   tc_bpf/tc_bpf_root:OK
> > > >   #232/2   tc_bpf/tc_bpf_non_root:OK
> > > >   #232     tc_bpf:OK
> > > >   Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED
> > > >
> > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > > ---
> > > >  tools/testing/selftests/bpf/prog_tests/tc_bpf.c | 36 ++++++++++++++++++++++++-
> > > >  tools/testing/selftests/bpf/progs/test_tc_bpf.c | 14 ++++++++++
> > > >  2 files changed, 49 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/tc_bpf.c b/tools/testing/selftests/bpf/prog_tests/tc_bpf.c
> > > > index e873766..48b5553 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/tc_bpf.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/tc_bpf.c
> > > > @@ -3,6 +3,7 @@
> > > >  #include <test_progs.h>
> > > >  #include <linux/pkt_cls.h>
> > > >
> > > > +#include "cap_helpers.h"
> > > >  #include "test_tc_bpf.skel.h"
> > > >
> > > >  #define LO_IFINDEX 1
> > > > @@ -327,7 +328,7 @@ static int test_tc_bpf_api(struct bpf_tc_hook *hook, int fd)
> > > >         return 0;
> > > >  }
> > > >
> > > > -void test_tc_bpf(void)
> > > > +void tc_bpf_root(void)
> > > >  {
> > > >         DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = LO_IFINDEX,
> > > >                             .attach_point = BPF_TC_INGRESS);
> > > > @@ -393,3 +394,36 @@ void test_tc_bpf(void)
> > > >         }
> > > >         test_tc_bpf__destroy(skel);
> > > >  }
> > > > +
> > > > +void tc_bpf_non_root(void)
> > > > +{
> > > > +       struct test_tc_bpf *skel = NULL;
> > > > +       __u64 caps = 0;
> > > > +       int ret;
> > > > +
> > > > +       /* In case CAP_BPF and CAP_PERFMON is not set */
> > > > +       ret = cap_enable_effective(1ULL << CAP_BPF | 1ULL << CAP_NET_ADMIN, &caps);
> > > > +       if (!ASSERT_OK(ret, "set_cap_bpf_cap_net_admin"))
> > > > +               return;
> > > > +       ret = cap_disable_effective(1ULL << CAP_SYS_ADMIN | 1ULL << CAP_PERFMON, NULL);
> > > > +       if (!ASSERT_OK(ret, "disable_cap_sys_admin"))
> > > > +               goto restore_cap;
> > > > +
> > > > +       skel = test_tc_bpf__open_and_load();
> > > > +       if (!ASSERT_OK_PTR(skel, "test_tc_bpf__open_and_load"))
> > > > +               goto restore_cap;
> > > > +
> > > > +       test_tc_bpf__destroy(skel);
> > > > +
> > > > +restore_cap:
> > > > +       if (caps)
> > > > +               cap_enable_effective(caps, NULL);
> > > > +}
> > > > +
> > > > +void test_tc_bpf(void)
> > > > +{
> > > > +       if (test__start_subtest("tc_bpf_root"))
> > > > +               tc_bpf_root();
> > > > +       if (test__start_subtest("tc_bpf_non_root"))
> > > > +               tc_bpf_non_root();
> > > > +}
> > > > diff --git a/tools/testing/selftests/bpf/progs/test_tc_bpf.c b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > > > index d28ca8d..3e0f218 100644
> > > > --- a/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > > > +++ b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > > > @@ -1,5 +1,8 @@
> > > >  // SPDX-License-Identifier: GPL-2.0
> > > >
> > > > +#include <linux/pkt_cls.h>
> > > > +#include <linux/ip.h>
> > > > +#include <linux/if_ether.h>
> > >
> > > Due to above it fails to compile:
> > >
> > > In file included from progs/test_tc_bpf.c:4:
> > > In file included from /usr/include/linux/ip.h:21:
> > > In file included from /usr/include/asm/byteorder.h:5:
> > > In file included from /usr/include/linux/byteorder/little_endian.h:13:
> > > /usr/include/linux/swab.h:136:8: error: unknown type name '__always_inline'
> > >   136 | static __always_inline unsigned long __swab(const unsigned long y)
> > >       |        ^
> >
> > I can't find the above error log in the BPF CI log.
> > The BPF CI log just shows that it fails the test_map on s390 without
> > logs. Not sure why.
>
> It's not in CI. I caught it by manual build.
>
> > __always_inline is defined in bpf_helpers.h, so I think below
> > additional change could fix it.
> >
> > --- a/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > +++ b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > @@ -1,10 +1,10 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf_helpers.h>
> >  #include <linux/pkt_cls.h>
> >  #include <linux/ip.h>
> >  #include <linux/if_ether.h>
> > -#include <linux/bpf.h>
> > -#include <bpf/bpf_helpers.h>
>
> Maybe, but I'd rather remove the includes and replace
> TC_ACT_STOLEN/ACT_OK with zeros, since the return values are
> irrelevant.

Good point. That could remove the "linux/pkt_cls.h".

-- 
Regards
Yafang

Re: [PATCH bpf-next 2/2] selftests/bpf: Add selftest for allow_ptr_leaks

[Eduard Zingerman]

On Mon, 2023-08-21 at 15:45 -0700, Alexei Starovoitov wrote:
[...]
> > diff --git a/tools/testing/selftests/bpf/progs/test_tc_bpf.c b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > index d28ca8d..3e0f218 100644
> > --- a/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > +++ b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > @@ -1,5 +1,8 @@
> >  // SPDX-License-Identifier: GPL-2.0
> > 
> > +#include <linux/pkt_cls.h>
> > +#include <linux/ip.h>
> > +#include <linux/if_ether.h>
> 
> Due to above it fails to compile:
> 
> In file included from progs/test_tc_bpf.c:4:
> In file included from /usr/include/linux/ip.h:21:
> In file included from /usr/include/asm/byteorder.h:5:
> In file included from /usr/include/linux/byteorder/little_endian.h:13:
> /usr/include/linux/swab.h:136:8: error: unknown type name '__always_inline'
>   136 | static __always_inline unsigned long __swab(const unsigned long y)
>       |        ^
> 

This is strange, I can compile it no problem. On my system:
/usr/include/linux/swab.h includes /usr/include/linux/stddef.h
which defines __always_inline.

What distro are you using?
I want to try it in chroot to see if we have any issues with test makefiles.

Re: [PATCH bpf-next 2/2] selftests/bpf: Add selftest for allow_ptr_leaks

[Alexei Starovoitov]

On Wed, Aug 23, 2023 at 2:45 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Mon, 2023-08-21 at 15:45 -0700, Alexei Starovoitov wrote:
> [...]
> > > diff --git a/tools/testing/selftests/bpf/progs/test_tc_bpf.c b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > > index d28ca8d..3e0f218 100644
> > > --- a/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > > +++ b/tools/testing/selftests/bpf/progs/test_tc_bpf.c
> > > @@ -1,5 +1,8 @@
> > >  // SPDX-License-Identifier: GPL-2.0
> > >
> > > +#include <linux/pkt_cls.h>
> > > +#include <linux/ip.h>
> > > +#include <linux/if_ether.h>
> >
> > Due to above it fails to compile:
> >
> > In file included from progs/test_tc_bpf.c:4:
> > In file included from /usr/include/linux/ip.h:21:
> > In file included from /usr/include/asm/byteorder.h:5:
> > In file included from /usr/include/linux/byteorder/little_endian.h:13:
> > /usr/include/linux/swab.h:136:8: error: unknown type name '__always_inline'
> >   136 | static __always_inline unsigned long __swab(const unsigned long y)
> >       |        ^
> >
>
> This is strange, I can compile it no problem. On my system:
> /usr/include/linux/swab.h includes /usr/include/linux/stddef.h
> which defines __always_inline.
>
> What distro are you using?

That was centos8.
dnf provides /usr/include/linux/swab.h
kernel-headers-6.4.3.x86_64 : Header files for the Linux kernel for use by glibc
Repo        : kernel
Matched from:
Filename    : /usr/include/linux/swab.h