Merge branch 'bpf: Allow skb dynptr for tp_btf'

Philo Lu says:

====================
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, because
pointers passed by tracepoint are trusted (PTR_TRUSTED) while those of
fentry/fexit are not.

Another problem raises that NULL pointers could be passed to tracepoint,
such as trace_tcp_send_reset, and we need to recognize them. This is done
by add a "__nullable" suffix in the func_proto of the tracepoint,
discussed in [1].

2 Test cases are added, one for "__nullable" suffix, and the other for
using skb dynptr in tp_btf.

changelog
v2 -> v3 (Andrii Nakryiko):
 Patch 1:
  - Remove prog type check in prog_arg_maybe_null()
  - Add bpf_put_raw_tracepoint() after get()
  - Use kallsyms_lookup() instead of sprintf("%ps")
 Patch 2: Add separate test "tp_btf_nullable", and use full failure msg
v1 -> v2:
 - Add "__nullable" suffix support (Alexei Starovoitov)
 - Replace "struct __sk_buff*" with "void*" in test (Martin KaFai Lau)

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

Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>

include/trace/events/tcp.h

@@ -91,10 +91,10 @@ 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,
  const enum sk_rst_reason reason),
 
- TP_ARGS(sk, skb, reason),
+ TP_ARGS(sk, skb__nullable, reason),
 
  TP_STRUCT__entry(
  __field(const void *, skbaddr)
@@ -106,7 +106,7 @@ TRACE_EVENT(tcp_send_reset,
  ),
 
  TP_fast_assign(
- __entry->skbaddr = skb;
+ __entry->skbaddr = skb__nullable;
  __entry->skaddr = sk;
  /* Zero means unknown state. */
  __entry->state = sk ? sk->sk_state : 0;
@@ -118,13 +118,13 @@ TRACE_EVENT(tcp_send_reset,
  const struct inet_sock *inet = inet_sk(sk);
 
  TP_STORE_ADDR_PORTS(__entry, inet, sk);
- } else if (skb) {
- const struct tcphdr *th = (const struct tcphdr *)skb->data;
+ } else if (skb__nullable) {
+ const struct tcphdr *th = (const struct tcphdr *)skb__nullable->data;
  /*
  * We should reverse the 4-tuple of skb, so later
  * it can print the right flow direction of rst.
  */
- TP_STORE_ADDR_PORTS_SKB(skb, th, entry->daddr, entry->saddr);
+ TP_STORE_ADDR_PORTS_SKB(skb__nullable, th, entry->daddr, entry->saddr);
  }
  __entry->reason = reason;
  ),

kernel/bpf/btf.c

@@ -6523,6 +6523,9 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
  if (prog_args_trusted(prog))
  info->reg_type |= PTR_TRUSTED;
 
+ if (btf_param_match_suffix(btf, &args[arg], "__nullable"))
+ info->reg_type |= PTR_MAYBE_NULL;
+
  if (tgt_prog) {
  enum bpf_prog_type tgt_type;
 

kernel/bpf/verifier.c

@@ -28,6 +28,8 @@
 #include <linux/cpumask.h>
 #include <linux/bpf_mem_alloc.h>
 #include <net/xdp.h>
+#include <linux/trace_events.h>
+#include <linux/kallsyms.h>
 
 #include "disasm.h"
 
@@ -21154,11 +21156,13 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 {
  bool prog_extension = prog->type == BPF_PROG_TYPE_EXT;
  bool prog_tracing = prog->type == BPF_PROG_TYPE_TRACING;
+ char trace_symbol[KSYM_SYMBOL_LEN];
  const char prefix[] = "btf_trace_";
+ struct bpf_raw_event_map *btp;
  int ret = 0, subprog = -1, i;
  const struct btf_type *t;
  bool conservative = true;
- const char *tname;
+ const char *tname, *fname;
  struct btf *btf;
  long addr = 0;
  struct module *mod = NULL;
@@ -21289,10 +21293,34 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
  return -EINVAL;
  }
  tname += sizeof(prefix) - 1;
- t = btf_type_by_id(btf, t->type);
- if (!btf_type_is_ptr(t))
- /* should never happen in valid vmlinux build */
+
+ /* The func_proto of "btf_trace_##tname" is generated from typedef without argument
+ * names. Thus using bpf_raw_event_map to get argument names.
+ */
+ btp = bpf_get_raw_tracepoint(tname);
+ if (!btp)
  return -EINVAL;
+ fname = kallsyms_lookup((unsigned long)btp->bpf_func, NULL, NULL, NULL,
+ trace_symbol);
+ bpf_put_raw_tracepoint(btp);
+
+ if (fname)
+ ret = btf_find_by_name_kind(btf, fname, BTF_KIND_FUNC);
+
+ if (!fname || ret < 0) {
+ bpf_log(log, "Cannot find btf of tracepoint template, fall back to %s%s.\n",
+ prefix, tname);
+ t = btf_type_by_id(btf, t->type);
+ if (!btf_type_is_ptr(t))
+ /* should never happen in valid vmlinux build */
+ return -EINVAL;
+ } else {
+ t = btf_type_by_id(btf, ret);
+ if (!btf_type_is_func(t))
+ /* should never happen in valid vmlinux build */
+ return -EINVAL;
+ }
+
  t = btf_type_by_id(btf, t->type);
  if (!btf_type_is_func_proto(t))
  /* should never happen in valid vmlinux build */

net/core/filter.c

@@ -12063,7 +12063,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)
@@ -12112,6 +12112,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);

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h

@@ -34,6 +34,12 @@ DECLARE_TRACE(bpf_testmod_test_write_bare,
  TP_ARGS(task, ctx)
 );
 
+/* Used in bpf_testmod_test_read() to test __nullable suffix */
+DECLARE_TRACE(bpf_testmod_test_nullable_bare,
+ TP_PROTO(struct bpf_testmod_test_read_ctx *ctx__nullable),
+ TP_ARGS(ctx__nullable)
+);
+
 #undef BPF_TESTMOD_DECLARE_TRACE
 #ifdef DECLARE_TRACE_WRITABLE
 #define BPF_TESTMOD_DECLARE_TRACE(call, proto, args, size) \

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c

@@ -356,6 +356,8 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj,
  if (bpf_testmod_loop_test(101) > 100)
  trace_bpf_testmod_test_read(current, &ctx);
 
+ trace_bpf_testmod_test_nullable_bare(NULL);
+
  /* Magic number to enable writable tp */
  if (len == 64) {
  struct bpf_testmod_test_writable_ctx writable = {

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,14 +29,15 @@ 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)
 {
  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,37 @@ 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);
+ bpf_link__destroy(link);
+
+ if (!ASSERT_OK(err, "test_run"))
+ goto cleanup;
+
+ break;
+ }
  }
 
  ASSERT_EQ(skel->bss->err, 0, "err");

tools/testing/selftests/bpf/prog_tests/tp_btf_nullable.c

@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+#include "test_tp_btf_nullable.skel.h"
+
+void test_tp_btf_nullable(void)
+{
+ if (!env.has_testmod) {
+ test__skip();
+ return;
+ }
+
+ RUN_TESTS(test_tp_btf_nullable);
+}

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, void *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, void *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")

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, void *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;
+}

tools/testing/selftests/bpf/progs/test_tp_btf_nullable.c

@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "../bpf_testmod/bpf_testmod.h"
+#include "bpf_misc.h"
+
+SEC("tp_btf/bpf_testmod_test_nullable_bare")
+__failure __msg("R1 invalid mem access 'trusted_ptr_or_null_'")
+int BPF_PROG(handle_tp_btf_nullable_bare1, struct bpf_testmod_test_read_ctx *nullable_ctx)
+{
+ return nullable_ctx->len;
+}
+
+SEC("tp_btf/bpf_testmod_test_nullable_bare")
+int BPF_PROG(handle_tp_btf_nullable_bare2, struct bpf_testmod_test_read_ctx *nullable_ctx)
+{
+ if (nullable_ctx)
+ return nullable_ctx->len;
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";