[PATCH bpf-next 0/2] bpf: Allow skb dynptr for tp_btf

[PATCH bpf-next 0/2] bpf: Allow skb dynptr for tp_btf

[Philo Lu]

This makes bpf_dynptr_from_skb usable for tp_btf, so that we can easily
parse skb in tracepoints. This has been discussed in [0], and Martin
suggested to use dynptr (instead of helpers like bpf_skb_load_bytes).

For safety, skb dynptr shouldn't be used in fentry/fexit. This is achieved
by add KF_TRUSTED_ARGS flag in bpf_dynptr_from_skb defination. IIUC, the
flag can be added to current defination directly, because skb is always
passed from ctx. But I'm not definitely sure about this. Please tell me if
there is any problem. Thanks in advance.

Selftests are expanded with skb dynptr used in tp_btf. They also make sure
that skb dynptr cannot be used in fentry/fexit.

[0]
https://lore.kernel.org/all/20240205121038.41344-1-lulie@linux.alibaba.com/T/

Philo Lu (2):
  bpf: Allow bpf_dynptr_from_skb() for tp_btf
  selftests/bpf: Expand skb dynptr selftests for tp_btf

 net/core/filter.c                             |  3 +-
 .../testing/selftests/bpf/prog_tests/dynptr.c | 36 +++++++++++++++++--
 .../testing/selftests/bpf/progs/dynptr_fail.c | 25 +++++++++++++
 .../selftests/bpf/progs/dynptr_success.c      | 23 ++++++++++++
 4 files changed, 84 insertions(+), 3 deletions(-)

--
2.32.0.3.g01195cf9f

[PATCH bpf-next 1/2] bpf: Allow bpf_dynptr_from_skb() for tp_btf

[Philo Lu]

Making tp_btf able to use bpf_dynptr_from_skb(), which is useful for skb
parsing, especially for non-linear paged skb data. This is achieved by
adding KF_TRUSTED_ARGS flag to bpf_dynptr_from_skb and registering it
for TRACING progs. With KF_TRUSTED_ARGS, args from fentry/fexit are
excluded, so that unsafe progs like fexit/__kfree_skb are not allowed.

We also need the skb dynptr to be read-only in tp_btf. Because
may_access_direct_pkt_data() returns false by default when checking
bpf_dynptr_from_skb, there is no need to add BPF_PROG_TYPE_TRACING to it
explicitly.

Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
---
 net/core/filter.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 786d792ac816..399492970b8c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -11990,7 +11990,7 @@ int bpf_dynptr_from_skb_rdonly(struct sk_buff *skb, u64 flags,
 }
 
 BTF_KFUNCS_START(bpf_kfunc_check_set_skb)
-BTF_ID_FLAGS(func, bpf_dynptr_from_skb)
+BTF_ID_FLAGS(func, bpf_dynptr_from_skb, KF_TRUSTED_ARGS)
 BTF_KFUNCS_END(bpf_kfunc_check_set_skb)
 
 BTF_KFUNCS_START(bpf_kfunc_check_set_xdp)
@@ -12039,6 +12039,7 @@ static int __init bpf_kfunc_init(void)
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LWT_XMIT, &bpf_kfunc_set_skb);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LWT_SEG6LOCAL, &bpf_kfunc_set_skb);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_NETFILTER, &bpf_kfunc_set_skb);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_kfunc_set_skb);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &bpf_kfunc_set_xdp);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
 					       &bpf_kfunc_set_sock_addr);
-- 
2.32.0.3.g01195cf9f

[PATCH bpf-next 2/2] selftests/bpf: Expand skb dynptr selftests for tp_btf

[Philo Lu]

Add 3 test cases for skb dynptr used in tp_btf:
- test_dynptr_skb_tp_btf: use skb dynptr in tp_btf and make sure it is
  read-only.
- skb_invalid_ctx_fentry/skb_invalid_ctx_fexit: bpf_dynptr_from_skb
  should fail in fentry/fexit.

In test_dynptr_skb_tp_btf, to trigger the tracepoint in kfree_skb,
test_pkt_access is used for its test_run, as in kfree_skb.c. Because the
test process is different from others, a new setup type is defined,
i.e., SETUP_SKB_PROG_TP.

The result is like:
$ ./test_progs -t 'dynptr/test_dynptr_skb_tp_btf'
  #77/14   dynptr/test_dynptr_skb_tp_btf:OK
  #77      dynptr:OK
  #120     kfunc_dynptr_param:OK
  Summary: 2/1 PASSED, 0 SKIPPED, 0 FAILED

$ ./test_progs -t 'dynptr/skb_invalid_ctx_f'
  #77/83   dynptr/skb_invalid_ctx_fentry:OK
  #77/84   dynptr/skb_invalid_ctx_fexit:OK
  #77      dynptr:OK
  #120     kfunc_dynptr_param:OK
  Summary: 2/2 PASSED, 0 SKIPPED, 0 FAILED

Also fix two coding style nits (change spaces to tabs).

Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
---
 .../testing/selftests/bpf/prog_tests/dynptr.c | 36 +++++++++++++++++--
 .../testing/selftests/bpf/progs/dynptr_fail.c | 25 +++++++++++++
 .../selftests/bpf/progs/dynptr_success.c      | 23 ++++++++++++
 3 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/testing/selftests/bpf/prog_tests/dynptr.c
index 7cfac53c0d58..ba40be8b1c4e 100644
--- a/tools/testing/selftests/bpf/prog_tests/dynptr.c
+++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
@@ -9,6 +9,7 @@
 enum test_setup_type {
 	SETUP_SYSCALL_SLEEP,
 	SETUP_SKB_PROG,
+	SETUP_SKB_PROG_TP,
 };
 
 static struct {
@@ -28,6 +29,7 @@ static struct {
 	{"test_dynptr_clone", SETUP_SKB_PROG},
 	{"test_dynptr_skb_no_buff", SETUP_SKB_PROG},
 	{"test_dynptr_skb_strcmp", SETUP_SKB_PROG},
+	{"test_dynptr_skb_tp_btf", SETUP_SKB_PROG_TP},
 };
 
 static void verify_success(const char *prog_name, enum test_setup_type setup_type)
@@ -35,7 +37,7 @@ static void verify_success(const char *prog_name, enum test_setup_type setup_typ
 	struct dynptr_success *skel;
 	struct bpf_program *prog;
 	struct bpf_link *link;
-       int err;
+	int err;
 
 	skel = dynptr_success__open();
 	if (!ASSERT_OK_PTR(skel, "dynptr_success__open"))
@@ -47,7 +49,7 @@ static void verify_success(const char *prog_name, enum test_setup_type setup_typ
 	if (!ASSERT_OK_PTR(prog, "bpf_object__find_program_by_name"))
 		goto cleanup;
 
-       bpf_program__set_autoload(prog, true);
+	bpf_program__set_autoload(prog, true);
 
 	err = dynptr_success__load(skel);
 	if (!ASSERT_OK(err, "dynptr_success__load"))
@@ -87,6 +89,36 @@ static void verify_success(const char *prog_name, enum test_setup_type setup_typ
 
 		break;
 	}
+	case SETUP_SKB_PROG_TP:
+	{
+		struct __sk_buff skb = {};
+		struct bpf_object *obj;
+		int aux_prog_fd;
+
+		/* Just use its test_run to trigger kfree_skb tracepoint */
+		err = bpf_prog_test_load("./test_pkt_access.bpf.o", BPF_PROG_TYPE_SCHED_CLS,
+					 &obj, &aux_prog_fd);
+		if (!ASSERT_OK(err, "prog_load sched cls"))
+			goto cleanup;
+
+		LIBBPF_OPTS(bpf_test_run_opts, topts,
+			    .data_in = &pkt_v4,
+			    .data_size_in = sizeof(pkt_v4),
+			    .ctx_in = &skb,
+			    .ctx_size_in = sizeof(skb),
+		);
+
+		link = bpf_program__attach(prog);
+		if (!ASSERT_OK_PTR(link, "bpf_program__attach"))
+			goto cleanup;
+
+		err = bpf_prog_test_run_opts(aux_prog_fd, &topts);
+
+		if (!ASSERT_OK(err, "test_run"))
+			goto cleanup;
+
+		break;
+	}
 	}
 
 	ASSERT_EQ(skel->bss->err, 0, "err");
diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
index 7ce7e827d5f0..c438d1c3cac5 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
@@ -6,6 +6,7 @@
 #include <stdbool.h>
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
 #include <linux/if_ether.h>
 #include "bpf_misc.h"
 #include "bpf_kfuncs.h"
@@ -1254,6 +1255,30 @@ int skb_invalid_ctx(void *ctx)
 	return 0;
 }
 
+SEC("fentry/skb_tx_error")
+__failure __msg("must be referenced or trusted")
+int BPF_PROG(skb_invalid_ctx_fentry, struct __sk_buff *skb)
+{
+	struct bpf_dynptr ptr;
+
+	/* this should fail */
+	bpf_dynptr_from_skb(skb, 0, &ptr);
+
+	return 0;
+}
+
+SEC("fexit/skb_tx_error")
+__failure __msg("must be referenced or trusted")
+int BPF_PROG(skb_invalid_ctx_fexit, struct __sk_buff *skb)
+{
+	struct bpf_dynptr ptr;
+
+	/* this should fail */
+	bpf_dynptr_from_skb(skb, 0, &ptr);
+
+	return 0;
+}
+
 /* Reject writes to dynptr slot for uninit arg */
 SEC("?raw_tp")
 __failure __msg("potential write to dynptr at off=-16")
diff --git a/tools/testing/selftests/bpf/progs/dynptr_success.c b/tools/testing/selftests/bpf/progs/dynptr_success.c
index 5985920d162e..8faafab97c0e 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_success.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_success.c
@@ -5,6 +5,7 @@
 #include <stdbool.h>
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
 #include "bpf_misc.h"
 #include "bpf_kfuncs.h"
 #include "errno.h"
@@ -544,3 +545,25 @@ int test_dynptr_skb_strcmp(struct __sk_buff *skb)
 
 	return 1;
 }
+
+SEC("tp_btf/kfree_skb")
+int BPF_PROG(test_dynptr_skb_tp_btf, struct __sk_buff *skb, void *location)
+{
+	__u8 write_data[2] = {1, 2};
+	struct bpf_dynptr ptr;
+	int ret;
+
+	if (bpf_dynptr_from_skb(skb, 0, &ptr)) {
+		err = 1;
+		return 1;
+	}
+
+	/* since tp_btf skbs are read only, writes should fail */
+	ret = bpf_dynptr_write(&ptr, 0, write_data, sizeof(write_data), 0);
+	if (ret != -EINVAL) {
+		err = 2;
+		return 1;
+	}
+
+	return 1;
+}
-- 
2.32.0.3.g01195cf9f

Re: [PATCH bpf-next 1/2] bpf: Allow bpf_dynptr_from_skb() for tp_btf

[Martin KaFai Lau]

On 4/30/24 5:18 AM, Philo Lu wrote:
> Making tp_btf able to use bpf_dynptr_from_skb(), which is useful for skb
> parsing, especially for non-linear paged skb data. This is achieved by
> adding KF_TRUSTED_ARGS flag to bpf_dynptr_from_skb and registering it
> for TRACING progs. With KF_TRUSTED_ARGS, args from fentry/fexit are
> excluded, so that unsafe progs like fexit/__kfree_skb are not allowed.
> 
> We also need the skb dynptr to be read-only in tp_btf. Because
> may_access_direct_pkt_data() returns false by default when checking
> bpf_dynptr_from_skb, there is no need to add BPF_PROG_TYPE_TRACING to it
> explicitly.
> 
> Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
> ---
>   net/core/filter.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 786d792ac816..399492970b8c 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -11990,7 +11990,7 @@ int bpf_dynptr_from_skb_rdonly(struct sk_buff *skb, u64 flags,
>   }
>   
>   BTF_KFUNCS_START(bpf_kfunc_check_set_skb)
> -BTF_ID_FLAGS(func, bpf_dynptr_from_skb)
> +BTF_ID_FLAGS(func, bpf_dynptr_from_skb, KF_TRUSTED_ARGS)

I can see the usefulness of having the same way parsing the header as the 
tc-bpf. However, it implicitly means the skb->data and skb_shinfo are trusted 
also. afaik, it should be as long as skb is not NULL.

 From looking at include/trace/events, there is case that skb is NULL. e.g. 
tcp_send_reset. It is not something new though, e.g. using skb->sk in the tp_btf 
could be bad already. This should be addressed before allowing more kfunc/helper.

I would like to hear how others think about it.

pw-bot: cr

>   BTF_KFUNCS_END(bpf_kfunc_check_set_skb)
>   
>   BTF_KFUNCS_START(bpf_kfunc_check_set_xdp)
> @@ -12039,6 +12039,7 @@ static int __init bpf_kfunc_init(void)
>   	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LWT_XMIT, &bpf_kfunc_set_skb);
>   	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LWT_SEG6LOCAL, &bpf_kfunc_set_skb);
>   	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_NETFILTER, &bpf_kfunc_set_skb);
> +	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_kfunc_set_skb);
>   	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &bpf_kfunc_set_xdp);
>   	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
>   					       &bpf_kfunc_set_sock_addr);

Re: [PATCH bpf-next 2/2] selftests/bpf: Expand skb dynptr selftests for tp_btf

[Martin KaFai Lau]

On 4/30/24 5:18 AM, Philo Lu wrote:
> Add 3 test cases for skb dynptr used in tp_btf:
> - test_dynptr_skb_tp_btf: use skb dynptr in tp_btf and make sure it is
>    read-only.
> - skb_invalid_ctx_fentry/skb_invalid_ctx_fexit: bpf_dynptr_from_skb
>    should fail in fentry/fexit.
> 
> In test_dynptr_skb_tp_btf, to trigger the tracepoint in kfree_skb,
> test_pkt_access is used for its test_run, as in kfree_skb.c. Because the
> test process is different from others, a new setup type is defined,
> i.e., SETUP_SKB_PROG_TP.
> 
> The result is like:
> $ ./test_progs -t 'dynptr/test_dynptr_skb_tp_btf'
>    #77/14   dynptr/test_dynptr_skb_tp_btf:OK
>    #77      dynptr:OK
>    #120     kfunc_dynptr_param:OK
>    Summary: 2/1 PASSED, 0 SKIPPED, 0 FAILED
> 
> $ ./test_progs -t 'dynptr/skb_invalid_ctx_f'
>    #77/83   dynptr/skb_invalid_ctx_fentry:OK
>    #77/84   dynptr/skb_invalid_ctx_fexit:OK
>    #77      dynptr:OK
>    #120     kfunc_dynptr_param:OK
>    Summary: 2/2 PASSED, 0 SKIPPED, 0 FAILED
> 
> Also fix two coding style nits (change spaces to tabs).
> 
> Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
> ---
>   .../testing/selftests/bpf/prog_tests/dynptr.c | 36 +++++++++++++++++--
>   .../testing/selftests/bpf/progs/dynptr_fail.c | 25 +++++++++++++
>   .../selftests/bpf/progs/dynptr_success.c      | 23 ++++++++++++
>   3 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/testing/selftests/bpf/prog_tests/dynptr.c
> index 7cfac53c0d58..ba40be8b1c4e 100644
> --- a/tools/testing/selftests/bpf/prog_tests/dynptr.c
> +++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
> @@ -9,6 +9,7 @@
>   enum test_setup_type {
>   	SETUP_SYSCALL_SLEEP,
>   	SETUP_SKB_PROG,
> +	SETUP_SKB_PROG_TP,
>   };
>   
>   static struct {
> @@ -28,6 +29,7 @@ static struct {
>   	{"test_dynptr_clone", SETUP_SKB_PROG},
>   	{"test_dynptr_skb_no_buff", SETUP_SKB_PROG},
>   	{"test_dynptr_skb_strcmp", SETUP_SKB_PROG},
> +	{"test_dynptr_skb_tp_btf", SETUP_SKB_PROG_TP},
>   };
>   
>   static void verify_success(const char *prog_name, enum test_setup_type setup_type)
> @@ -35,7 +37,7 @@ static void verify_success(const char *prog_name, enum test_setup_type setup_typ
>   	struct dynptr_success *skel;
>   	struct bpf_program *prog;
>   	struct bpf_link *link;
> -       int err;
> +	int err;
>   
>   	skel = dynptr_success__open();
>   	if (!ASSERT_OK_PTR(skel, "dynptr_success__open"))
> @@ -47,7 +49,7 @@ static void verify_success(const char *prog_name, enum test_setup_type setup_typ
>   	if (!ASSERT_OK_PTR(prog, "bpf_object__find_program_by_name"))
>   		goto cleanup;
>   
> -       bpf_program__set_autoload(prog, true);
> +	bpf_program__set_autoload(prog, true);
>   
>   	err = dynptr_success__load(skel);
>   	if (!ASSERT_OK(err, "dynptr_success__load"))
> @@ -87,6 +89,36 @@ static void verify_success(const char *prog_name, enum test_setup_type setup_typ
>   
>   		break;
>   	}
> +	case SETUP_SKB_PROG_TP:
> +	{
> +		struct __sk_buff skb = {};
> +		struct bpf_object *obj;
> +		int aux_prog_fd;
> +
> +		/* Just use its test_run to trigger kfree_skb tracepoint */
> +		err = bpf_prog_test_load("./test_pkt_access.bpf.o", BPF_PROG_TYPE_SCHED_CLS,
> +					 &obj, &aux_prog_fd);
> +		if (!ASSERT_OK(err, "prog_load sched cls"))
> +			goto cleanup;
> +
> +		LIBBPF_OPTS(bpf_test_run_opts, topts,
> +			    .data_in = &pkt_v4,
> +			    .data_size_in = sizeof(pkt_v4),
> +			    .ctx_in = &skb,
> +			    .ctx_size_in = sizeof(skb),
> +		);
> +
> +		link = bpf_program__attach(prog);
> +		if (!ASSERT_OK_PTR(link, "bpf_program__attach"))
> +			goto cleanup;
> +
> +		err = bpf_prog_test_run_opts(aux_prog_fd, &topts);
> +
> +		if (!ASSERT_OK(err, "test_run"))
> +			goto cleanup;
> +
> +		break;
> +	}
>   	}
>   
>   	ASSERT_EQ(skel->bss->err, 0, "err");
> diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> index 7ce7e827d5f0..c438d1c3cac5 100644
> --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
> +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> @@ -6,6 +6,7 @@
>   #include <stdbool.h>
>   #include <linux/bpf.h>
>   #include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
>   #include <linux/if_ether.h>
>   #include "bpf_misc.h"
>   #include "bpf_kfuncs.h"
> @@ -1254,6 +1255,30 @@ int skb_invalid_ctx(void *ctx)
>   	return 0;
>   }
>   
> +SEC("fentry/skb_tx_error")
> +__failure __msg("must be referenced or trusted")
> +int BPF_PROG(skb_invalid_ctx_fentry, struct __sk_buff *skb)
> +{
> +	struct bpf_dynptr ptr;
> +
> +	/* this should fail */
> +	bpf_dynptr_from_skb(skb, 0, &ptr);
> +
> +	return 0;
> +}
> +
> +SEC("fexit/skb_tx_error")
> +__failure __msg("must be referenced or trusted")
> +int BPF_PROG(skb_invalid_ctx_fexit, struct __sk_buff *skb)
> +{
> +	struct bpf_dynptr ptr;
> +
> +	/* this should fail */
> +	bpf_dynptr_from_skb(skb, 0, &ptr);
> +
> +	return 0;
> +}
> +
>   /* Reject writes to dynptr slot for uninit arg */
>   SEC("?raw_tp")
>   __failure __msg("potential write to dynptr at off=-16")
> diff --git a/tools/testing/selftests/bpf/progs/dynptr_success.c b/tools/testing/selftests/bpf/progs/dynptr_success.c
> index 5985920d162e..8faafab97c0e 100644
> --- a/tools/testing/selftests/bpf/progs/dynptr_success.c
> +++ b/tools/testing/selftests/bpf/progs/dynptr_success.c
> @@ -5,6 +5,7 @@
>   #include <stdbool.h>
>   #include <linux/bpf.h>
>   #include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
>   #include "bpf_misc.h"
>   #include "bpf_kfuncs.h"
>   #include "errno.h"
> @@ -544,3 +545,25 @@ int test_dynptr_skb_strcmp(struct __sk_buff *skb)
>   
>   	return 1;
>   }
> +
> +SEC("tp_btf/kfree_skb")
> +int BPF_PROG(test_dynptr_skb_tp_btf, struct __sk_buff *skb, void *location)

struct __sk_buff is the incorrect type. This happens to work but will be a 
surprise for people trying to read something (e.g. skb->len). The same goes for 
the ones in dynptr_fail.c.

> +{
> +	__u8 write_data[2] = {1, 2};
> +	struct bpf_dynptr ptr;
> +	int ret;
> +
> +	if (bpf_dynptr_from_skb(skb, 0, &ptr)) {
> +		err = 1;
> +		return 1;
> +	}
> +
> +	/* since tp_btf skbs are read only, writes should fail */
> +	ret = bpf_dynptr_write(&ptr, 0, write_data, sizeof(write_data), 0);
> +	if (ret != -EINVAL) {
> +		err = 2;
> +		return 1;
> +	}
> +
> +	return 1;
> +}

Re: [PATCH bpf-next 1/2] bpf: Allow bpf_dynptr_from_skb() for tp_btf

[Alexei Starovoitov]

On Mon, May 6, 2024 at 2:39 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 4/30/24 5:18 AM, Philo Lu wrote:
> > Making tp_btf able to use bpf_dynptr_from_skb(), which is useful for skb
> > parsing, especially for non-linear paged skb data. This is achieved by
> > adding KF_TRUSTED_ARGS flag to bpf_dynptr_from_skb and registering it
> > for TRACING progs. With KF_TRUSTED_ARGS, args from fentry/fexit are
> > excluded, so that unsafe progs like fexit/__kfree_skb are not allowed.
> >
> > We also need the skb dynptr to be read-only in tp_btf. Because
> > may_access_direct_pkt_data() returns false by default when checking
> > bpf_dynptr_from_skb, there is no need to add BPF_PROG_TYPE_TRACING to it
> > explicitly.
> >
> > Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
> > ---
> >   net/core/filter.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 786d792ac816..399492970b8c 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -11990,7 +11990,7 @@ int bpf_dynptr_from_skb_rdonly(struct sk_buff *skb, u64 flags,
> >   }
> >
> >   BTF_KFUNCS_START(bpf_kfunc_check_set_skb)
> > -BTF_ID_FLAGS(func, bpf_dynptr_from_skb)
> > +BTF_ID_FLAGS(func, bpf_dynptr_from_skb, KF_TRUSTED_ARGS)
>
> I can see the usefulness of having the same way parsing the header as the
> tc-bpf. However, it implicitly means the skb->data and skb_shinfo are trusted
> also. afaik, it should be as long as skb is not NULL.
>
>  From looking at include/trace/events, there is case that skb is NULL. e.g.
> tcp_send_reset. It is not something new though, e.g. using skb->sk in the tp_btf
> could be bad already. This should be addressed before allowing more kfunc/helper.

Good catch.
We need to fix this part first:
        if (prog_args_trusted(prog))
                info->reg_type |= PTR_TRUSTED;

Brute force fix by adding PTR_MAYBE_NULL is probably overkill.
I suspect passing NULL into tracepoint is more of an exception than the rule.
Maybe we can use kfunc's "__" suffix approach for tracepoint args?
[43947] FUNC_PROTO '(anon)' ret_type_id=0 vlen=4
        '__data' type_id=10
        'sk' type_id=3434
        'skb' type_id=2386
        'reason' type_id=39860
[43948] FUNC '__bpf_trace_tcp_send_reset' type_id=43947 linkage=static

Then do:
diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
index 49b5ee091cf6..325e8a31729a 100644
--- a/include/trace/events/tcp.h
+++ b/include/trace/events/tcp.h
@@ -91,7 +91,7 @@ DEFINE_RST_REASON(FN, FN)
 TRACE_EVENT(tcp_send_reset,

        TP_PROTO(const struct sock *sk,
-                const struct sk_buff *skb,
+                const struct sk_buff *skb__nullable,

and detect it in btf_ctx_access().

Re: [PATCH bpf-next 2/2] selftests/bpf: Expand skb dynptr selftests for tp_btf

[Philo Lu]

On 2024/5/7 05:43, Martin KaFai Lau wrote:
> On 4/30/24 5:18 AM, Philo Lu wrote:
>> Add 3 test cases for skb dynptr used in tp_btf:
[...]
>> +
>> +SEC("tp_btf/kfree_skb")
>> +int BPF_PROG(test_dynptr_skb_tp_btf, struct __sk_buff *skb, void 
>> *location)
> 
> struct __sk_buff is the incorrect type. This happens to work but will be 
> a surprise for people trying to read something (e.g. skb->len). The same 
> goes for the ones in dynptr_fail.c.
> 

What do you think if I replace "struct __sk_buff" with "void"? The diffs 
are appended below.

Because we are not to read anything in these cases, I think using void* 
is enough to avoid confusion. On the other hand, to use "struct sk_buff" 
here, we have to introduce the definition, and tune codes as the input 
type of bpf_dynptr_from_skb() is defined as struct __sk_buff in 
"bpf_kfuncs.h".

Thanks.

-----------------
diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c 
b/tools/testing/selftests/bpf/progs/dynptr_fail.c
index c438d1c3cac56..42dbf8715c6a8 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
@@ -1257,7 +1257,7 @@ int skb_invalid_ctx(void *ctx)

  SEC("fentry/skb_tx_error")
  __failure __msg("must be referenced or trusted")
-int BPF_PROG(skb_invalid_ctx_fentry, struct __sk_buff *skb)
+int BPF_PROG(skb_invalid_ctx_fentry, void *skb)
  {
         struct bpf_dynptr ptr;

@@ -1269,7 +1269,7 @@ int BPF_PROG(skb_invalid_ctx_fentry, struct 
__sk_buff *skb)

  SEC("fexit/skb_tx_error")
  __failure __msg("must be referenced or trusted")
-int BPF_PROG(skb_invalid_ctx_fexit, struct __sk_buff *skb)
+int BPF_PROG(skb_invalid_ctx_fexit, void *skb)
  {
         struct bpf_dynptr ptr;

diff --git a/tools/testing/selftests/bpf/progs/dynptr_success.c 
b/tools/testing/selftests/bpf/progs/dynptr_success.c
index 8faafab97c0ec..bfcc85686cf04 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_success.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_success.c
@@ -547,7 +547,7 @@ int test_dynptr_skb_strcmp(struct __sk_buff *skb)
  }

  SEC("tp_btf/kfree_skb")
-int BPF_PROG(test_dynptr_skb_tp_btf, struct __sk_buff *skb, void *location)
+int BPF_PROG(test_dynptr_skb_tp_btf, void *skb, void *location)
  {
         __u8 write_data[2] = {1, 2};
         struct bpf_dynptr ptr;

Re: [PATCH bpf-next 2/2] selftests/bpf: Expand skb dynptr selftests for tp_btf

[Martin KaFai Lau]

On 5/6/24 8:15 PM, Philo Lu wrote:
> What do you think if I replace "struct __sk_buff" with "void"? The diffs are 
> appended below.
> 
> Because we are not to read anything in these cases, I think using void* is 
> enough to avoid confusion. On the other hand, to use "struct sk_buff" here, we 
> have to introduce the definition, and tune codes as the input type of 

The sk_buff definition is in vmlinux.h. However, the dynptr_fail.c does not use 
vmlinux.h now, so it may need some more changes.

Yep, I think "void *skb" here is fine.

> bpf_dynptr_from_skb() is defined as struct __sk_buff in "bpf_kfuncs.h".

Re: [PATCH bpf-next 1/2] bpf: Allow bpf_dynptr_from_skb() for tp_btf

[Martin KaFai Lau]

On 5/6/24 4:29 PM, Alexei Starovoitov wrote:
> On Mon, May 6, 2024 at 2:39 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 4/30/24 5:18 AM, Philo Lu wrote:
>>> Making tp_btf able to use bpf_dynptr_from_skb(), which is useful for skb
>>> parsing, especially for non-linear paged skb data. This is achieved by
>>> adding KF_TRUSTED_ARGS flag to bpf_dynptr_from_skb and registering it
>>> for TRACING progs. With KF_TRUSTED_ARGS, args from fentry/fexit are
>>> excluded, so that unsafe progs like fexit/__kfree_skb are not allowed.
>>>
>>> We also need the skb dynptr to be read-only in tp_btf. Because
>>> may_access_direct_pkt_data() returns false by default when checking
>>> bpf_dynptr_from_skb, there is no need to add BPF_PROG_TYPE_TRACING to it
>>> explicitly.
>>>
>>> Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
>>> ---
>>>    net/core/filter.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index 786d792ac816..399492970b8c 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -11990,7 +11990,7 @@ int bpf_dynptr_from_skb_rdonly(struct sk_buff *skb, u64 flags,
>>>    }
>>>
>>>    BTF_KFUNCS_START(bpf_kfunc_check_set_skb)
>>> -BTF_ID_FLAGS(func, bpf_dynptr_from_skb)
>>> +BTF_ID_FLAGS(func, bpf_dynptr_from_skb, KF_TRUSTED_ARGS)
>>
>> I can see the usefulness of having the same way parsing the header as the
>> tc-bpf. However, it implicitly means the skb->data and skb_shinfo are trusted
>> also. afaik, it should be as long as skb is not NULL.
>>
>>   From looking at include/trace/events, there is case that skb is NULL. e.g.
>> tcp_send_reset. It is not something new though, e.g. using skb->sk in the tp_btf
>> could be bad already. This should be addressed before allowing more kfunc/helper.
> 
> Good catch.
> We need to fix this part first:
>          if (prog_args_trusted(prog))
>                  info->reg_type |= PTR_TRUSTED;
> 
> Brute force fix by adding PTR_MAYBE_NULL is probably overkill.
> I suspect passing NULL into tracepoint is more of an exception than the rule.
> Maybe we can use kfunc's "__" suffix approach for tracepoint args?
> [43947] FUNC_PROTO '(anon)' ret_type_id=0 vlen=4
>          '__data' type_id=10
>          'sk' type_id=3434
>          'skb' type_id=2386
>          'reason' type_id=39860
> [43948] FUNC '__bpf_trace_tcp_send_reset' type_id=43947 linkage=static
> 
> Then do:
> diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
> index 49b5ee091cf6..325e8a31729a 100644
> --- a/include/trace/events/tcp.h
> +++ b/include/trace/events/tcp.h
> @@ -91,7 +91,7 @@ DEFINE_RST_REASON(FN, FN)
>   TRACE_EVENT(tcp_send_reset,
> 
>          TP_PROTO(const struct sock *sk,
> -                const struct sk_buff *skb,
> +                const struct sk_buff *skb__nullable,
> 
> and detect it in btf_ctx_access().

+1. It is a neat solution. Thanks for the suggestion.

Philo, can you give it a try to fix this in the next re-spin?

Re: [PATCH bpf-next 1/2] bpf: Allow bpf_dynptr_from_skb() for tp_btf

[Philo Lu]

On 2024/5/9 08:24, Martin KaFai Lau wrote:
> On 5/6/24 4:29 PM, Alexei Starovoitov wrote:
>> On Mon, May 6, 2024 at 2:39 PM Martin KaFai Lau <martin.lau@linux.dev> 
>> wrote:
>>>
>>> On 4/30/24 5:18 AM, Philo Lu wrote:
>>>> Making tp_btf able to use bpf_dynptr_from_skb(), which is useful for 
>>>> skb
>>>> parsing, especially for non-linear paged skb data. This is achieved by
>>>> adding KF_TRUSTED_ARGS flag to bpf_dynptr_from_skb and registering it
>>>> for TRACING progs. With KF_TRUSTED_ARGS, args from fentry/fexit are
>>>> excluded, so that unsafe progs like fexit/__kfree_skb are not allowed.
>>>>
>>>> We also need the skb dynptr to be read-only in tp_btf. Because
>>>> may_access_direct_pkt_data() returns false by default when checking
>>>> bpf_dynptr_from_skb, there is no need to add BPF_PROG_TYPE_TRACING 
>>>> to it
>>>> explicitly.
>>>>
>>>> Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
>>>> ---
>>>>    net/core/filter.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>> index 786d792ac816..399492970b8c 100644
>>>> --- a/net/core/filter.c
>>>> +++ b/net/core/filter.c
>>>> @@ -11990,7 +11990,7 @@ int bpf_dynptr_from_skb_rdonly(struct 
>>>> sk_buff *skb, u64 flags,
>>>>    }
>>>>
>>>>    BTF_KFUNCS_START(bpf_kfunc_check_set_skb)
>>>> -BTF_ID_FLAGS(func, bpf_dynptr_from_skb)
>>>> +BTF_ID_FLAGS(func, bpf_dynptr_from_skb, KF_TRUSTED_ARGS)
>>>
>>> I can see the usefulness of having the same way parsing the header as 
>>> the
>>> tc-bpf. However, it implicitly means the skb->data and skb_shinfo are 
>>> trusted
>>> also. afaik, it should be as long as skb is not NULL.
>>>
>>>   From looking at include/trace/events, there is case that skb is 
>>> NULL. e.g.
>>> tcp_send_reset. It is not something new though, e.g. using skb->sk in 
>>> the tp_btf
>>> could be bad already. This should be addressed before allowing more 
>>> kfunc/helper.
>>
>> Good catch.
>> We need to fix this part first:
>>          if (prog_args_trusted(prog))
>>                  info->reg_type |= PTR_TRUSTED;
>>
>> Brute force fix by adding PTR_MAYBE_NULL is probably overkill.
>> I suspect passing NULL into tracepoint is more of an exception than 
>> the rule.
>> Maybe we can use kfunc's "__" suffix approach for tracepoint args?
>> [43947] FUNC_PROTO '(anon)' ret_type_id=0 vlen=4
>>          '__data' type_id=10
>>          'sk' type_id=3434
>>          'skb' type_id=2386
>>          'reason' type_id=39860
>> [43948] FUNC '__bpf_trace_tcp_send_reset' type_id=43947 linkage=static
>>
>> Then do:
>> diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
>> index 49b5ee091cf6..325e8a31729a 100644
>> --- a/include/trace/events/tcp.h
>> +++ b/include/trace/events/tcp.h
>> @@ -91,7 +91,7 @@ DEFINE_RST_REASON(FN, FN)
>>   TRACE_EVENT(tcp_send_reset,
>>
>>          TP_PROTO(const struct sock *sk,
>> -                const struct sk_buff *skb,
>> +                const struct sk_buff *skb__nullable,
>>
>> and detect it in btf_ctx_access().
> 
> +1. It is a neat solution. Thanks for the suggestion.
> 
> Philo, can you give it a try to fix this in the next re-spin?

Glad to do that. I'll try to implement a "__nullable" suffix for tracepoint.

Thanks.