[PATCH bpf-next v1 0/4] bpf: add __percpu tagging in vmlinux BTF

[PATCH bpf-next v1 0/4] bpf: add __percpu tagging in vmlinux BTF

[Hao Luo]

This patchset is very much similar to Yonghong's patchset on adding
__user tagging [1], where a "user" btf_type_tag was introduced to
describe __user memory pointers. Similar approach can be applied on
__percpu pointers. The __percpu attribute in kernel is used to identify
pointers that point to memory allocated in percpu region. Normally,
accessing __percpu memory requires using special functions like
per_cpu_ptr() etc. Directly accessing __percpu pointer is meaningless.

Currently vmlinux BTF does not have a way to differentiate a __percpu
pointer from a regular pointer. So BPF programs are allowed to load
__percpu memory directly, which is an incorrect behavior.

With the previous work that encodes __user information in BTF, a nice
framework has been set up to allow us to encode __percpu information in
BTF and let the verifier to reject programs that try to directly access
percpu pointer. Previously, there is a PTR_TO_PERCPU_BTF_ID reg type which
is used to represent those percpu static variables in the kernel. Pahole
is able to collect variables that are stored in ".data..percpu" section
in the kernel image and emit BTF information for those variables. The
bpf_per_cpu_ptr() and bpf_this_cpu_ptr() helper functions were added to
access these variables. Now with __percpu information, we can tag those
__percpu fields in a struct (such as cgroup->rstat_cpu) and allow the
pair of bpf percpu helpers to access them as well.

In addition to adding __percpu tagging, this patchset also fixes a
harmless bug in the previous patch that introduced __user. Patch 01/04
is for that. Patch 02/04 adds the new attribute "percpu". Patch 03/04
adds MEM_PERCPU tag for PTR_TO_BTF_ID and replaces PTR_TO_PERCPU_BTF_ID
with (BTF_ID | MEM_PERCPU). Patch 04/04 refactors the btf_tag test a bit
and adds tests for percpu tag.

Like [1], the minimal requirements for btf_type_tag is clang (>=
clang14) and pahole (>= 1.23).

[1] https://lore.kernel.org/bpf/20211220015110.3rqxk5qwub3pa2gh@ast-mbp.dhcp.thefacebook.com/t/

Hao Luo (4):
  bpf: Fix checking PTR_TO_BTF_ID in check_mem_access
  compiler_types: define __percpu as __attribute__((btf_type_tag("percpu")))
  bpf: Reject programs that try to load __percpu memory.
  selftests/bpf: Add a test for btf_type_tag "percpu"

 include/linux/bpf.h                           |  11 +-
 include/linux/compiler_types.h                |   7 +-
 kernel/bpf/btf.c                              |   8 +-
 kernel/bpf/verifier.c                         |  27 +--
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  17 ++
 .../selftests/bpf/prog_tests/btf_tag.c        | 164 ++++++++++++++----
 .../selftests/bpf/progs/btf_type_tag_percpu.c |  66 +++++++
 7 files changed, 256 insertions(+), 44 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/btf_type_tag_percpu.c

-- 
2.35.1.616.g0bdcbb4464-goog

[PATCH bpf-next v1 1/4] bpf: Fix checking PTR_TO_BTF_ID in check_mem_access

[Hao Luo]

With the introduction of MEM_USER in

 commit c6f1bfe89ac9 ("bpf: reject program if a __user tagged memory accessed in kernel way")

PTR_TO_BTF_ID can be combined with a MEM_USER tag. Therefore, most
likely, when we compare reg_type against PTR_TO_BTF_ID, we want to use
the reg's base_type. Previously the check in check_mem_access() wants
to say: if the reg is BTF_ID but not NULL, the execution flow falls
into the 'then' branch. But now a reg of (BTF_ID | MEM_USER), which
should go into the 'then' branch, goes into the 'else'.

The end results before and after this patch are the same: regs tagged
with MEM_USER get rejected, but not in a way we intended. So fix the
condition, the error message now is correct.

Before (log from commit 696c39011538):

  $ ./test_progs -v -n 22/3
  ...
  libbpf: prog 'test_user1': BPF program load failed: Permission denied
  libbpf: prog 'test_user1': -- BEGIN PROG LOAD LOG --
  R1 type=ctx expected=fp
  0: R1=ctx(id=0,off=0,imm=0) R10=fp0
  ; int BPF_PROG(test_user1, struct bpf_testmod_btf_type_tag_1 *arg)
  0: (79) r1 = *(u64 *)(r1 +0)
  func 'bpf_testmod_test_btf_type_tag_user_1' arg0 has btf_id 136561 type STRUCT 'bpf_testmod_btf_type_tag_1'
  1: R1_w=user_ptr_bpf_testmod_btf_type_tag_1(id=0,off=0,imm=0)
  ; g = arg->a;
  1: (61) r1 = *(u32 *)(r1 +0)
  R1 invalid mem access 'user_ptr_'

Now:

  libbpf: prog 'test_user1': BPF program load failed: Permission denied
  libbpf: prog 'test_user1': -- BEGIN PROG LOAD LOG --
  R1 type=ctx expected=fp
  0: R1=ctx(id=0,off=0,imm=0) R10=fp0
  ; int BPF_PROG(test_user1, struct bpf_testmod_btf_type_tag_1 *arg)
  0: (79) r1 = *(u64 *)(r1 +0)
  func 'bpf_testmod_test_btf_type_tag_user_1' arg0 has btf_id 104036 type STRUCT 'bpf_testmod_btf_type_tag_1'
  1: R1_w=user_ptr_bpf_testmod_btf_type_tag_1(id=0,ref_obj_id=0,off=0,imm=0)
  ; g = arg->a;
  1: (61) r1 = *(u32 *)(r1 +0)
  R1 is ptr_bpf_testmod_btf_type_tag_1 access user memory: off=0

Note the error message for the reason of rejection.

Fixes: c6f1bfe89ac9 ("bpf: reject program if a __user tagged memory accessed in kernel way")
Cc: Yonghong Song <yhs@fb.com>
Signed-off-by: Hao Luo <haoluo@google.com>
---
 kernel/bpf/verifier.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a57db4b2803c..d63b1f40e029 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4556,7 +4556,8 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
 		err = check_tp_buffer_access(env, reg, regno, off, size);
 		if (!err && t == BPF_READ && value_regno >= 0)
 			mark_reg_unknown(env, regs, value_regno);
-	} else if (reg->type == PTR_TO_BTF_ID) {
+	} else if (base_type(reg->type) == PTR_TO_BTF_ID &&
+		   !type_may_be_null(reg->type)) {
 		err = check_ptr_to_btf_access(env, regs, regno, off, size, t,
 					      value_regno);
 	} else if (reg->type == CONST_PTR_TO_MAP) {
-- 
2.35.1.616.g0bdcbb4464-goog

[PATCH bpf-next v1 2/4] compiler_types: define __percpu as __attribute__((btf_type_tag("percpu")))

[Hao Luo]

This is similar to commit 7472d5a642c9 ("compiler_types: define __user as
__attribute__((btf_type_tag("user")))"), where a type tag "user" was
introduced to identify the pointers that point to user memory. With that
change, the newest compile toolchain can encode __user information into
vmlinux BTF, which can be used by the BPF verifier to enforce safe
program behaviors.

Similarly, we have __percpu attribute, which is mainly used to indicate
memory is allocated in percpu region. The __percpu pointers in kernel
are supposed to be used together with functions like per_cpu_ptr() and
this_cpu_ptr(), which perform necessary calculation on the pointer's
base address. Without the btf_type_tag introduced in this patch,
__percpu pointers will be treated as regular memory pointers in vmlinux
BTF and BPF programs are allowed to directly dereference them, generating
incorrect behaviors. Now with "percpu" btf_type_tag, the BPF verifier is
able to differentiate __percpu pointers from regular pointers and forbids
unexpected behaviors like direct load.

The following is an example similar to the one given in commit
7472d5a642c9:

  [$ ~] cat test.c
  #define __percpu __attribute__((btf_type_tag("percpu")))
  int foo(int __percpu *arg) {
  	return *arg;
  }
  [$ ~] clang -O2 -g -c test.c
  [$ ~] pahole -JV test.o
  ...
  File test.o:
  [1] INT int size=4 nr_bits=32 encoding=SIGNED
  [2] TYPE_TAG percpu type_id=1
  [3] PTR (anon) type_id=2
  [4] FUNC_PROTO (anon) return=1 args=(3 arg)
  [5] FUNC foo type_id=4
  [$ ~]

for the function argument "int __percpu *arg", its type is described as
	PTR -> TYPE_TAG(percpu) -> INT
The kernel can use this information for bpf verification or other
use cases.

Like commit 7472d5a642c9, this feature requires clang (>= clang14) and
pahole (>= 1.23).

Cc: Yonghong Song <yhs@fb.com>
Signed-off-by: Hao Luo <haoluo@google.com>
---
 include/linux/compiler_types.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 3f31ff400432..223abf43679a 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -38,7 +38,12 @@ static inline void __chk_io_ptr(const volatile void __iomem *ptr) { }
 #  define __user
 # endif
 # define __iomem
-# define __percpu
+# if defined(CONFIG_DEBUG_INFO_BTF) && defined(CONFIG_PAHOLE_HAS_BTF_TAG) && \
+	__has_attribute(btf_type_tag)
+#  define __percpu	__attribute__((btf_type_tag("percpu")))
+# else
+#  define __percpu
+# endif
 # define __rcu
 # define __chk_user_ptr(x)	(void)0
 # define __chk_io_ptr(x)	(void)0
-- 
2.35.1.616.g0bdcbb4464-goog

[PATCH bpf-next v1 3/4] bpf: Reject programs that try to load __percpu memory.

[Hao Luo]

With the introduction of the btf_type_tag "percpu", we can add a
MEM_PERCPU to identify those pointers that point to percpu memory.
The ability of differetiating percpu pointers from regular memory
pointers have two benefits:

 1. It forbids unexpected use of percpu pointers, such as direct loads.
    In kernel, there are special functions used for accessing percpu
    memory. Directly loading percpu memory is meaningless. We already
    have BPF helpers like bpf_per_cpu_ptr() and bpf_this_cpu_ptr() that
    wrap the kernel percpu functions. So we can now convert percpu
    pointers into regular pointers in a safe way.

 2. Previously, bpf_per_cpu_ptr() and bpf_this_cpu_ptr() only work on
    PTR_TO_PERCPU_BTF_ID, a special reg_type which describes static
    percpu variables in kernel (we rely on pahole to encode them into
    vmlinux BTF). Now, since we can identify __percpu tagged pointers,
    we can also identify dynamically allocated percpu memory as well.
    It means we can use bpf_xxx_cpu_ptr() on dynamic percpu memory.
    This would be very convenient when accessing fields like
    "cgroup->rstat_cpu".

Cc: Yonghong Song <yhs@fb.com>
Signed-off-by: Hao Luo <haoluo@google.com>
---
 include/linux/bpf.h   | 11 +++++++++--
 kernel/bpf/btf.c      |  8 +++++++-
 kernel/bpf/verifier.c | 24 ++++++++++++++----------
 3 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index f19abc59b6cd..88449fbbe063 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -334,7 +334,15 @@ enum bpf_type_flag {
 	/* MEM is in user address space. */
 	MEM_USER		= BIT(3 + BPF_BASE_TYPE_BITS),
 
-	__BPF_TYPE_LAST_FLAG	= MEM_USER,
+	/* MEM is a percpu memory. MEM_PERCPU tags PTR_TO_BTF_ID. When tagged
+	 * with MEM_PERCPU, PTR_TO_BTF_ID _cannot_ be directly accessed. In
+	 * order to drop this tag, it must be passed into bpf_per_cpu_ptr()
+	 * or bpf_this_cpu_ptr(), which will return the pointer corresponding
+	 * to the specified cpu.
+	 */
+	MEM_PERCPU		= BIT(4 + BPF_BASE_TYPE_BITS),
+
+	__BPF_TYPE_LAST_FLAG	= MEM_PERCPU,
 };
 
 /* Max number of base types. */
@@ -516,7 +524,6 @@ enum bpf_reg_type {
 	 */
 	PTR_TO_MEM,		 /* reg points to valid memory region */
 	PTR_TO_BUF,		 /* reg points to a read/write buffer */
-	PTR_TO_PERCPU_BTF_ID,	 /* reg points to a percpu kernel variable */
 	PTR_TO_FUNC,		 /* reg points to a bpf program function */
 	__BPF_REG_TYPE_MAX,
 
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index b472cf0c8fdb..f9f2218b27fe 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5057,6 +5057,8 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		tag_value = __btf_name_by_offset(btf, t->name_off);
 		if (strcmp(tag_value, "user") == 0)
 			info->reg_type |= MEM_USER;
+		if (strcmp(tag_value, "percpu") == 0)
+			info->reg_type |= MEM_PERCPU;
 	}
 
 	/* skip modifiers */
@@ -5285,12 +5287,16 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
 				return -EACCES;
 			}
 
-			/* check __user tag */
+			/* check type tag */
 			t = btf_type_by_id(btf, mtype->type);
 			if (btf_type_is_type_tag(t)) {
 				tag_value = __btf_name_by_offset(btf, t->name_off);
+				/* check __user tag */
 				if (strcmp(tag_value, "user") == 0)
 					tmp_flag = MEM_USER;
+				/* check __percpu tag */
+				if (strcmp(tag_value, "percpu") == 0)
+					tmp_flag = MEM_PERCPU;
 			}
 
 			stype = btf_type_skip_modifiers(btf, mtype->type, &id);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d63b1f40e029..093e47360d6d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -554,7 +554,6 @@ static const char *reg_type_str(struct bpf_verifier_env *env,
 		[PTR_TO_TP_BUFFER]	= "tp_buffer",
 		[PTR_TO_XDP_SOCK]	= "xdp_sock",
 		[PTR_TO_BTF_ID]		= "ptr_",
-		[PTR_TO_PERCPU_BTF_ID]	= "percpu_ptr_",
 		[PTR_TO_MEM]		= "mem",
 		[PTR_TO_BUF]		= "buf",
 		[PTR_TO_FUNC]		= "func",
@@ -562,8 +561,7 @@ static const char *reg_type_str(struct bpf_verifier_env *env,
 	};
 
 	if (type & PTR_MAYBE_NULL) {
-		if (base_type(type) == PTR_TO_BTF_ID ||
-		    base_type(type) == PTR_TO_PERCPU_BTF_ID)
+		if (base_type(type) == PTR_TO_BTF_ID)
 			strncpy(postfix, "or_null_", 16);
 		else
 			strncpy(postfix, "_or_null", 16);
@@ -575,6 +573,8 @@ static const char *reg_type_str(struct bpf_verifier_env *env,
 		strncpy(prefix, "alloc_", 32);
 	if (type & MEM_USER)
 		strncpy(prefix, "user_", 32);
+	if (type & MEM_PERCPU)
+		strncpy(prefix, "percpu_", 32);
 
 	snprintf(env->type_str_buf, TYPE_STR_BUF_LEN, "%s%s%s",
 		 prefix, str[base_type(type)], postfix);
@@ -697,8 +697,7 @@ static void print_verifier_state(struct bpf_verifier_env *env,
 			const char *sep = "";
 
 			verbose(env, "%s", reg_type_str(env, t));
-			if (base_type(t) == PTR_TO_BTF_ID ||
-			    base_type(t) == PTR_TO_PERCPU_BTF_ID)
+			if (base_type(t) == PTR_TO_BTF_ID)
 				verbose(env, "%s", kernel_type_name(reg->btf, reg->btf_id));
 			verbose(env, "(");
 /*
@@ -2783,7 +2782,6 @@ static bool is_spillable_regtype(enum bpf_reg_type type)
 	case PTR_TO_XDP_SOCK:
 	case PTR_TO_BTF_ID:
 	case PTR_TO_BUF:
-	case PTR_TO_PERCPU_BTF_ID:
 	case PTR_TO_MEM:
 	case PTR_TO_FUNC:
 	case PTR_TO_MAP_KEY:
@@ -4197,6 +4195,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 		return -EACCES;
 	}
 
+	if (reg->type & MEM_PERCPU) {
+		verbose(env,
+			"R%d is ptr_%s access percpu memory: off=%d\n",
+			regno, tname, off);
+		return -EACCES;
+	}
+
 	if (env->ops->btf_struct_access) {
 		ret = env->ops->btf_struct_access(&env->log, reg->btf, t,
 						  off, size, atype, &btf_id, &flag);
@@ -4803,7 +4808,7 @@ static int check_stack_range_initialized(
 		}
 
 		if (is_spilled_reg(&state->stack[spi]) &&
-		    state->stack[spi].spilled_ptr.type == PTR_TO_BTF_ID)
+		    base_type(state->stack[spi].spilled_ptr.type) == PTR_TO_BTF_ID)
 			goto mark;
 
 		if (is_spilled_reg(&state->stack[spi]) &&
@@ -5259,7 +5264,7 @@ static const struct bpf_reg_types alloc_mem_types = { .types = { PTR_TO_MEM | ME
 static const struct bpf_reg_types const_map_ptr_types = { .types = { CONST_PTR_TO_MAP } };
 static const struct bpf_reg_types btf_ptr_types = { .types = { PTR_TO_BTF_ID } };
 static const struct bpf_reg_types spin_lock_types = { .types = { PTR_TO_MAP_VALUE } };
-static const struct bpf_reg_types percpu_btf_ptr_types = { .types = { PTR_TO_PERCPU_BTF_ID } };
+static const struct bpf_reg_types percpu_btf_ptr_types = { .types = { PTR_TO_BTF_ID | MEM_PERCPU } };
 static const struct bpf_reg_types func_ptr_types = { .types = { PTR_TO_FUNC } };
 static const struct bpf_reg_types stack_ptr_types = { .types = { PTR_TO_STACK } };
 static const struct bpf_reg_types const_str_ptr_types = { .types = { PTR_TO_MAP_VALUE } };
@@ -9639,7 +9644,6 @@ static int check_ld_imm(struct bpf_verifier_env *env, struct bpf_insn *insn)
 			dst_reg->mem_size = aux->btf_var.mem_size;
 			break;
 		case PTR_TO_BTF_ID:
-		case PTR_TO_PERCPU_BTF_ID:
 			dst_reg->btf = aux->btf_var.btf;
 			dst_reg->btf_id = aux->btf_var.btf_id;
 			break;
@@ -11839,7 +11843,7 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
 	type = t->type;
 	t = btf_type_skip_modifiers(btf, type, NULL);
 	if (percpu) {
-		aux->btf_var.reg_type = PTR_TO_PERCPU_BTF_ID;
+		aux->btf_var.reg_type = PTR_TO_BTF_ID | MEM_PERCPU;
 		aux->btf_var.btf = btf;
 		aux->btf_var.btf_id = type;
 	} else if (!btf_type_is_struct(t)) {
-- 
2.35.1.616.g0bdcbb4464-goog

[PATCH bpf-next v1 4/4] selftests/bpf: Add a test for btf_type_tag "percpu"

[Hao Luo]

Add test for percpu btf_type_tag. Similar to the "user" tag, we test
the following cases:

 1. __percpu struct field.
 2. __percpu as function parameter.
 3. per_cpu_ptr() accepts dynamically allocated __percpu memory.

Because the test for "user" and the test for "percpu" are very similar,
a little bit of refactoring has been done in btf_tag.c. Basically, both
tests share the same function for loading vmlinux and module btf.

Example output from log:

 > ./test_progs -v -t btf_tag

 libbpf: prog 'test_percpu1': BPF program load failed: Permission denied
 libbpf: prog 'test_percpu1': -- BEGIN PROG LOAD LOG --
 ...
 ; g = arg->a;
 1: (61) r1 = *(u32 *)(r1 +0)
 R1 is ptr_bpf_testmod_btf_type_tag_1 access percpu memory: off=0
 ...
 test_btf_type_tag_mod_percpu:PASS:btf_type_tag_percpu 0 nsec
 #26/6 btf_tag/btf_type_tag_percpu_mod1:OK

 libbpf: prog 'test_percpu2': BPF program load failed: Permission denied
 libbpf: prog 'test_percpu2': -- BEGIN PROG LOAD LOG --
 ...
 ; g = arg->p->a;
 2: (61) r1 = *(u32 *)(r1 +0)
 R1 is ptr_bpf_testmod_btf_type_tag_1 access percpu memory: off=0
 ...
 test_btf_type_tag_mod_percpu:PASS:btf_type_tag_percpu 0 nsec
 #26/7 btf_tag/btf_type_tag_percpu_mod2:OK

 libbpf: prog 'test_percpu_load': BPF program load failed: Permission denied
 libbpf: prog 'test_percpu_load': -- BEGIN PROG LOAD LOG --
 ...
 ; g = (__u64)cgrp->rstat_cpu->updated_children;
 2: (79) r1 = *(u64 *)(r1 +48)
 R1 is ptr_cgroup_rstat_cpu access percpu memory: off=48
 ...
 test_btf_type_tag_vmlinux_percpu:PASS:btf_type_tag_percpu_load 0 nsec
 #26/8 btf_tag/btf_type_tag_percpu_vmlinux_load:OK

 load_btfs:PASS:could not load vmlinux BTF 0 nsec
 test_btf_type_tag_vmlinux_percpu:PASS:btf_type_tag_percpu 0 nsec
 test_btf_type_tag_vmlinux_percpu:PASS:btf_type_tag_percpu_helper 0 nsec
 #26/9 btf_tag/btf_type_tag_percpu_vmlinux_helper:OK

Signed-off-by: Hao Luo <haoluo@google.com>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  17 ++
 .../selftests/bpf/prog_tests/btf_tag.c        | 164 ++++++++++++++----
 .../selftests/bpf/progs/btf_type_tag_percpu.c |  66 +++++++
 3 files changed, 218 insertions(+), 29 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/btf_type_tag_percpu.c

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 27d63be47b95..17c211f3b924 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -33,6 +33,10 @@ struct bpf_testmod_btf_type_tag_2 {
 	struct bpf_testmod_btf_type_tag_1 __user *p;
 };
 
+struct bpf_testmod_btf_type_tag_3 {
+	struct bpf_testmod_btf_type_tag_1 __percpu *p;
+};
+
 noinline int
 bpf_testmod_test_btf_type_tag_user_1(struct bpf_testmod_btf_type_tag_1 __user *arg) {
 	BTF_TYPE_EMIT(func_proto_typedef);
@@ -46,6 +50,19 @@ bpf_testmod_test_btf_type_tag_user_2(struct bpf_testmod_btf_type_tag_2 *arg) {
 	return arg->p->a;
 }
 
+noinline int
+bpf_testmod_test_btf_type_tag_percpu_1(struct bpf_testmod_btf_type_tag_1 __percpu *arg) {
+	BTF_TYPE_EMIT(func_proto_typedef);
+	BTF_TYPE_EMIT(func_proto_typedef_nested1);
+	BTF_TYPE_EMIT(func_proto_typedef_nested2);
+	return arg->a;
+}
+
+noinline int
+bpf_testmod_test_btf_type_tag_percpu_2(struct bpf_testmod_btf_type_tag_3 *arg) {
+	return arg->p->a;
+}
+
 noinline int bpf_testmod_loop_test(int n)
 {
 	int i, sum = 0;
diff --git a/tools/testing/selftests/bpf/prog_tests/btf_tag.c b/tools/testing/selftests/bpf/prog_tests/btf_tag.c
index f7560b54a6bb..071430cd54de 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_tag.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_tag.c
@@ -10,6 +10,7 @@ struct btf_type_tag_test {
 };
 #include "btf_type_tag.skel.h"
 #include "btf_type_tag_user.skel.h"
+#include "btf_type_tag_percpu.skel.h"
 
 static void test_btf_decl_tag(void)
 {
@@ -43,38 +44,81 @@ static void test_btf_type_tag(void)
 	btf_type_tag__destroy(skel);
 }
 
-static void test_btf_type_tag_mod_user(bool load_test_user1)
+/* loads vmlinux_btf as well as module_btf. If the caller passes NULL as
+ * module_btf, it will not load module btf.
+ *
+ * Returns 0 on success.
+ * Return -1 On error. In case of error, the loaded btf will be freed and the
+ * input parameters will be set to pointing to NULL.
+ */
+static int load_btfs(struct btf **vmlinux_btf, struct btf **module_btf,
+		     bool needs_vmlinux_tag)
 {
 	const char *module_name = "bpf_testmod";
-	struct btf *vmlinux_btf, *module_btf;
-	struct btf_type_tag_user *skel;
 	__s32 type_id;
-	int err;
 
 	if (!env.has_testmod) {
 		test__skip();
-		return;
+		return -1;
 	}
 
-	/* skip the test if the module does not have __user tags */
-	vmlinux_btf = btf__load_vmlinux_btf();
-	if (!ASSERT_OK_PTR(vmlinux_btf, "could not load vmlinux BTF"))
-		return;
+	*vmlinux_btf = btf__load_vmlinux_btf();
+	if (!ASSERT_OK_PTR(*vmlinux_btf, "could not load vmlinux BTF"))
+		return -1;
+
+	if (!needs_vmlinux_tag)
+		goto load_module_btf;
 
-	module_btf = btf__load_module_btf(module_name, vmlinux_btf);
-	if (!ASSERT_OK_PTR(module_btf, "could not load module BTF"))
+	/* skip the test if the vmlinux does not have __user tags */
+	type_id = btf__find_by_name_kind(*vmlinux_btf, "user", BTF_KIND_TYPE_TAG);
+	if (type_id <= 0) {
+		printf("%s:SKIP: btf_type_tag attribute not in vmlinux btf", __func__);
+		test__skip();
 		goto free_vmlinux_btf;
+	}
 
-	type_id = btf__find_by_name_kind(module_btf, "user", BTF_KIND_TYPE_TAG);
+load_module_btf:
+	/* skip loading module_btf, if not requested by caller */
+	if (!module_btf)
+		return 0;
+
+	*module_btf = btf__load_module_btf(module_name, *vmlinux_btf);
+	if (!ASSERT_OK_PTR(*module_btf, "could not load module BTF"))
+		goto free_vmlinux_btf;
+
+	/* skip the test if the module does not have __user tags */
+	type_id = btf__find_by_name_kind(*module_btf, "user", BTF_KIND_TYPE_TAG);
 	if (type_id <= 0) {
 		printf("%s:SKIP: btf_type_tag attribute not in %s", __func__, module_name);
 		test__skip();
 		goto free_module_btf;
 	}
 
+	return 0;
+
+free_module_btf:
+	btf__free(*module_btf);
+free_vmlinux_btf:
+	btf__free(*vmlinux_btf);
+
+	*vmlinux_btf = NULL;
+	if (module_btf)
+		*module_btf = NULL;
+	return -1;
+}
+
+static void test_btf_type_tag_mod_user(bool load_test_user1)
+{
+	struct btf *vmlinux_btf = NULL, *module_btf = NULL;
+	struct btf_type_tag_user *skel;
+	int err;
+
+	if (load_btfs(&vmlinux_btf, &module_btf, /*needs_vmlinux_tag=*/false))
+		return;
+
 	skel = btf_type_tag_user__open();
 	if (!ASSERT_OK_PTR(skel, "btf_type_tag_user"))
-		goto free_module_btf;
+		goto cleanup;
 
 	bpf_program__set_autoload(skel->progs.test_sys_getsockname, false);
 	if (load_test_user1)
@@ -87,34 +131,23 @@ static void test_btf_type_tag_mod_user(bool load_test_user1)
 
 	btf_type_tag_user__destroy(skel);
 
-free_module_btf:
+cleanup:
 	btf__free(module_btf);
-free_vmlinux_btf:
 	btf__free(vmlinux_btf);
 }
 
 static void test_btf_type_tag_vmlinux_user(void)
 {
 	struct btf_type_tag_user *skel;
-	struct btf *vmlinux_btf;
-	__s32 type_id;
+	struct btf *vmlinux_btf = NULL;
 	int err;
 
-	/* skip the test if the vmlinux does not have __user tags */
-	vmlinux_btf = btf__load_vmlinux_btf();
-	if (!ASSERT_OK_PTR(vmlinux_btf, "could not load vmlinux BTF"))
+	if (load_btfs(&vmlinux_btf, NULL, /*needs_vmlinux_tag=*/true))
 		return;
 
-	type_id = btf__find_by_name_kind(vmlinux_btf, "user", BTF_KIND_TYPE_TAG);
-	if (type_id <= 0) {
-		printf("%s:SKIP: btf_type_tag attribute not in vmlinux btf", __func__);
-		test__skip();
-		goto free_vmlinux_btf;
-	}
-
 	skel = btf_type_tag_user__open();
 	if (!ASSERT_OK_PTR(skel, "btf_type_tag_user"))
-		goto free_vmlinux_btf;
+		goto cleanup;
 
 	bpf_program__set_autoload(skel->progs.test_user2, false);
 	bpf_program__set_autoload(skel->progs.test_user1, false);
@@ -124,7 +157,70 @@ static void test_btf_type_tag_vmlinux_user(void)
 
 	btf_type_tag_user__destroy(skel);
 
-free_vmlinux_btf:
+cleanup:
+	btf__free(vmlinux_btf);
+}
+
+static void test_btf_type_tag_mod_percpu(bool load_test_percpu1)
+{
+	struct btf *vmlinux_btf, *module_btf;
+	struct btf_type_tag_percpu *skel;
+	int err;
+
+	if (load_btfs(&vmlinux_btf, &module_btf, /*needs_vmlinux_tag=*/false))
+		return;
+
+	skel = btf_type_tag_percpu__open();
+	if (!ASSERT_OK_PTR(skel, "btf_type_tag_percpu"))
+		goto cleanup;
+
+	bpf_program__set_autoload(skel->progs.test_percpu_load, false);
+	bpf_program__set_autoload(skel->progs.test_percpu_helper, false);
+	if (load_test_percpu1)
+		bpf_program__set_autoload(skel->progs.test_percpu2, false);
+	else
+		bpf_program__set_autoload(skel->progs.test_percpu1, false);
+
+	err = btf_type_tag_percpu__load(skel);
+	ASSERT_ERR(err, "btf_type_tag_percpu");
+
+	btf_type_tag_percpu__destroy(skel);
+
+cleanup:
+	btf__free(module_btf);
+	btf__free(vmlinux_btf);
+}
+
+static void test_btf_type_tag_vmlinux_percpu(bool load_test)
+{
+	struct btf_type_tag_percpu *skel;
+	struct btf *vmlinux_btf = NULL;
+	int err;
+
+	if (load_btfs(&vmlinux_btf, NULL, /*needs_vmlinux_tag=*/true))
+		return;
+
+	skel = btf_type_tag_percpu__open();
+	if (!ASSERT_OK_PTR(skel, "btf_type_tag_percpu"))
+		goto cleanup;
+
+	bpf_program__set_autoload(skel->progs.test_percpu2, false);
+	bpf_program__set_autoload(skel->progs.test_percpu1, false);
+	if (load_test) {
+		bpf_program__set_autoload(skel->progs.test_percpu_helper, false);
+
+		err = btf_type_tag_percpu__load(skel);
+		ASSERT_ERR(err, "btf_type_tag_percpu_load");
+	} else {
+		bpf_program__set_autoload(skel->progs.test_percpu_load, false);
+
+		err = btf_type_tag_percpu__load(skel);
+		ASSERT_OK(err, "btf_type_tag_percpu_helper");
+	}
+
+	btf_type_tag_percpu__destroy(skel);
+
+cleanup:
 	btf__free(vmlinux_btf);
 }
 
@@ -134,10 +230,20 @@ void test_btf_tag(void)
 		test_btf_decl_tag();
 	if (test__start_subtest("btf_type_tag"))
 		test_btf_type_tag();
+
 	if (test__start_subtest("btf_type_tag_user_mod1"))
 		test_btf_type_tag_mod_user(true);
 	if (test__start_subtest("btf_type_tag_user_mod2"))
 		test_btf_type_tag_mod_user(false);
 	if (test__start_subtest("btf_type_tag_sys_user_vmlinux"))
 		test_btf_type_tag_vmlinux_user();
+
+	if (test__start_subtest("btf_type_tag_percpu_mod1"))
+		test_btf_type_tag_mod_percpu(true);
+	if (test__start_subtest("btf_type_tag_percpu_mod2"))
+		test_btf_type_tag_mod_percpu(false);
+	if (test__start_subtest("btf_type_tag_percpu_vmlinux_load"))
+		test_btf_type_tag_vmlinux_percpu(true);
+	if (test__start_subtest("btf_type_tag_percpu_vmlinux_helper"))
+		test_btf_type_tag_vmlinux_percpu(false);
 }
diff --git a/tools/testing/selftests/bpf/progs/btf_type_tag_percpu.c b/tools/testing/selftests/bpf/progs/btf_type_tag_percpu.c
new file mode 100644
index 000000000000..8feddb8289cf
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/btf_type_tag_percpu.c
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Google */
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+struct bpf_testmod_btf_type_tag_1 {
+	int a;
+};
+
+struct bpf_testmod_btf_type_tag_2 {
+	struct bpf_testmod_btf_type_tag_1 *p;
+};
+
+__u64 g;
+
+SEC("fentry/bpf_testmod_test_btf_type_tag_percpu_1")
+int BPF_PROG(test_percpu1, struct bpf_testmod_btf_type_tag_1 *arg)
+{
+	g = arg->a;
+	return 0;
+}
+
+SEC("fentry/bpf_testmod_test_btf_type_tag_percpu_2")
+int BPF_PROG(test_percpu2, struct bpf_testmod_btf_type_tag_2 *arg)
+{
+	g = arg->p->a;
+	return 0;
+}
+
+/* trace_cgroup_mkdir(struct cgroup *cgrp, const char *path)
+ *
+ * struct cgroup_rstat_cpu {
+ *   ...
+ *   struct cgroup *updated_children;
+ *   ...
+ * };
+ *
+ * struct cgroup {
+ *   ...
+ *   struct cgroup_rstat_cpu __percpu *rstat_cpu;
+ *   ...
+ * };
+ */
+SEC("tp_btf/cgroup_mkdir")
+int BPF_PROG(test_percpu_load, struct cgroup *cgrp, const char *path)
+{
+	g = (__u64)cgrp->rstat_cpu->updated_children;
+	return 0;
+}
+
+SEC("tp_btf/cgroup_mkdir")
+int BPF_PROG(test_percpu_helper, struct cgroup *cgrp, const char *path)
+{
+	struct cgroup_rstat_cpu *rstat;
+	__u32 cpu;
+
+	cpu = bpf_get_smp_processor_id();
+	rstat = (struct cgroup_rstat_cpu *)bpf_per_cpu_ptr(cgrp->rstat_cpu, cpu);
+	if (rstat) {
+		/* READ_ONCE */
+		*(volatile int *)rstat;
+	}
+
+	return 0;
+}
-- 
2.35.1.616.g0bdcbb4464-goog

Re: [PATCH bpf-next v1 1/4] bpf: Fix checking PTR_TO_BTF_ID in check_mem_access

[Yonghong Song]

On 3/4/22 11:16 AM, Hao Luo wrote:
> With the introduction of MEM_USER in
> 
>   commit c6f1bfe89ac9 ("bpf: reject program if a __user tagged memory accessed in kernel way")
> 
> PTR_TO_BTF_ID can be combined with a MEM_USER tag. Therefore, most
> likely, when we compare reg_type against PTR_TO_BTF_ID, we want to use
> the reg's base_type. Previously the check in check_mem_access() wants
> to say: if the reg is BTF_ID but not NULL, the execution flow falls
> into the 'then' branch. But now a reg of (BTF_ID | MEM_USER), which
> should go into the 'then' branch, goes into the 'else'.
> 
> The end results before and after this patch are the same: regs tagged
> with MEM_USER get rejected, but not in a way we intended. So fix the
> condition, the error message now is correct.
> 
> Before (log from commit 696c39011538):
> 
>    $ ./test_progs -v -n 22/3
>    ...
>    libbpf: prog 'test_user1': BPF program load failed: Permission denied
>    libbpf: prog 'test_user1': -- BEGIN PROG LOAD LOG --
>    R1 type=ctx expected=fp
>    0: R1=ctx(id=0,off=0,imm=0) R10=fp0
>    ; int BPF_PROG(test_user1, struct bpf_testmod_btf_type_tag_1 *arg)
>    0: (79) r1 = *(u64 *)(r1 +0)
>    func 'bpf_testmod_test_btf_type_tag_user_1' arg0 has btf_id 136561 type STRUCT 'bpf_testmod_btf_type_tag_1'
>    1: R1_w=user_ptr_bpf_testmod_btf_type_tag_1(id=0,off=0,imm=0)
>    ; g = arg->a;
>    1: (61) r1 = *(u32 *)(r1 +0)
>    R1 invalid mem access 'user_ptr_'
> 
> Now:
> 
>    libbpf: prog 'test_user1': BPF program load failed: Permission denied
>    libbpf: prog 'test_user1': -- BEGIN PROG LOAD LOG --
>    R1 type=ctx expected=fp
>    0: R1=ctx(id=0,off=0,imm=0) R10=fp0
>    ; int BPF_PROG(test_user1, struct bpf_testmod_btf_type_tag_1 *arg)
>    0: (79) r1 = *(u64 *)(r1 +0)
>    func 'bpf_testmod_test_btf_type_tag_user_1' arg0 has btf_id 104036 type STRUCT 'bpf_testmod_btf_type_tag_1'
>    1: R1_w=user_ptr_bpf_testmod_btf_type_tag_1(id=0,ref_obj_id=0,off=0,imm=0)
>    ; g = arg->a;
>    1: (61) r1 = *(u32 *)(r1 +0)
>    R1 is ptr_bpf_testmod_btf_type_tag_1 access user memory: off=0
> 
> Note the error message for the reason of rejection.
> 
> Fixes: c6f1bfe89ac9 ("bpf: reject program if a __user tagged memory accessed in kernel way")
> Cc: Yonghong Song <yhs@fb.com>
> Signed-off-by: Hao Luo <haoluo@google.com>

Thanks for the fix!

Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH bpf-next v1 2/4] compiler_types: define __percpu as __attribute__((btf_type_tag("percpu")))

[Yonghong Song]

On 3/4/22 11:16 AM, Hao Luo wrote:
> This is similar to commit 7472d5a642c9 ("compiler_types: define __user as
> __attribute__((btf_type_tag("user")))"), where a type tag "user" was
> introduced to identify the pointers that point to user memory. With that
> change, the newest compile toolchain can encode __user information into
> vmlinux BTF, which can be used by the BPF verifier to enforce safe
> program behaviors.
> 
> Similarly, we have __percpu attribute, which is mainly used to indicate
> memory is allocated in percpu region. The __percpu pointers in kernel
> are supposed to be used together with functions like per_cpu_ptr() and
> this_cpu_ptr(), which perform necessary calculation on the pointer's
> base address. Without the btf_type_tag introduced in this patch,
> __percpu pointers will be treated as regular memory pointers in vmlinux
> BTF and BPF programs are allowed to directly dereference them, generating
> incorrect behaviors. Now with "percpu" btf_type_tag, the BPF verifier is
> able to differentiate __percpu pointers from regular pointers and forbids
> unexpected behaviors like direct load.
> 
> The following is an example similar to the one given in commit
> 7472d5a642c9:
> 
>    [$ ~] cat test.c
>    #define __percpu __attribute__((btf_type_tag("percpu")))
>    int foo(int __percpu *arg) {
>    	return *arg;
>    }
>    [$ ~] clang -O2 -g -c test.c
>    [$ ~] pahole -JV test.o
>    ...
>    File test.o:
>    [1] INT int size=4 nr_bits=32 encoding=SIGNED
>    [2] TYPE_TAG percpu type_id=1
>    [3] PTR (anon) type_id=2
>    [4] FUNC_PROTO (anon) return=1 args=(3 arg)
>    [5] FUNC foo type_id=4
>    [$ ~]
> 
> for the function argument "int __percpu *arg", its type is described as
> 	PTR -> TYPE_TAG(percpu) -> INT
> The kernel can use this information for bpf verification or other
> use cases.
> 
> Like commit 7472d5a642c9, this feature requires clang (>= clang14) and
> pahole (>= 1.23).
> 
> Cc: Yonghong Song <yhs@fb.com>
> Signed-off-by: Hao Luo <haoluo@google.com>

Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH bpf-next v1 3/4] bpf: Reject programs that try to load __percpu memory.

[Yonghong Song]

On 3/4/22 11:16 AM, Hao Luo wrote:
> With the introduction of the btf_type_tag "percpu", we can add a
> MEM_PERCPU to identify those pointers that point to percpu memory.
> The ability of differetiating percpu pointers from regular memory
> pointers have two benefits:
> 
>   1. It forbids unexpected use of percpu pointers, such as direct loads.
>      In kernel, there are special functions used for accessing percpu
>      memory. Directly loading percpu memory is meaningless. We already
>      have BPF helpers like bpf_per_cpu_ptr() and bpf_this_cpu_ptr() that
>      wrap the kernel percpu functions. So we can now convert percpu
>      pointers into regular pointers in a safe way.
> 
>   2. Previously, bpf_per_cpu_ptr() and bpf_this_cpu_ptr() only work on
>      PTR_TO_PERCPU_BTF_ID, a special reg_type which describes static
>      percpu variables in kernel (we rely on pahole to encode them into
>      vmlinux BTF). Now, since we can identify __percpu tagged pointers,
>      we can also identify dynamically allocated percpu memory as well.
>      It means we can use bpf_xxx_cpu_ptr() on dynamic percpu memory.
>      This would be very convenient when accessing fields like
>      "cgroup->rstat_cpu".
> 
> Cc: Yonghong Song <yhs@fb.com>
> Signed-off-by: Hao Luo <haoluo@google.com>

Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH bpf-next v1 4/4] selftests/bpf: Add a test for btf_type_tag "percpu"

[Yonghong Song]

On 3/4/22 11:16 AM, Hao Luo wrote:
> Add test for percpu btf_type_tag. Similar to the "user" tag, we test
> the following cases:
> 
>   1. __percpu struct field.
>   2. __percpu as function parameter.
>   3. per_cpu_ptr() accepts dynamically allocated __percpu memory.
> 
> Because the test for "user" and the test for "percpu" are very similar,
> a little bit of refactoring has been done in btf_tag.c. Basically, both
> tests share the same function for loading vmlinux and module btf.
> 
> Example output from log:
> 
>   > ./test_progs -v -t btf_tag
> 
>   libbpf: prog 'test_percpu1': BPF program load failed: Permission denied
>   libbpf: prog 'test_percpu1': -- BEGIN PROG LOAD LOG --
>   ...
>   ; g = arg->a;
>   1: (61) r1 = *(u32 *)(r1 +0)
>   R1 is ptr_bpf_testmod_btf_type_tag_1 access percpu memory: off=0
>   ...
>   test_btf_type_tag_mod_percpu:PASS:btf_type_tag_percpu 0 nsec
>   #26/6 btf_tag/btf_type_tag_percpu_mod1:OK
> 
>   libbpf: prog 'test_percpu2': BPF program load failed: Permission denied
>   libbpf: prog 'test_percpu2': -- BEGIN PROG LOAD LOG --
>   ...
>   ; g = arg->p->a;
>   2: (61) r1 = *(u32 *)(r1 +0)
>   R1 is ptr_bpf_testmod_btf_type_tag_1 access percpu memory: off=0
>   ...
>   test_btf_type_tag_mod_percpu:PASS:btf_type_tag_percpu 0 nsec
>   #26/7 btf_tag/btf_type_tag_percpu_mod2:OK
> 
>   libbpf: prog 'test_percpu_load': BPF program load failed: Permission denied
>   libbpf: prog 'test_percpu_load': -- BEGIN PROG LOAD LOG --
>   ...
>   ; g = (__u64)cgrp->rstat_cpu->updated_children;
>   2: (79) r1 = *(u64 *)(r1 +48)
>   R1 is ptr_cgroup_rstat_cpu access percpu memory: off=48
>   ...
>   test_btf_type_tag_vmlinux_percpu:PASS:btf_type_tag_percpu_load 0 nsec
>   #26/8 btf_tag/btf_type_tag_percpu_vmlinux_load:OK
> 
>   load_btfs:PASS:could not load vmlinux BTF 0 nsec
>   test_btf_type_tag_vmlinux_percpu:PASS:btf_type_tag_percpu 0 nsec
>   test_btf_type_tag_vmlinux_percpu:PASS:btf_type_tag_percpu_helper 0 nsec
>   #26/9 btf_tag/btf_type_tag_percpu_vmlinux_helper:OK
> 
> Signed-off-by: Hao Luo <haoluo@google.com>

With one nit below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  17 ++
>   .../selftests/bpf/prog_tests/btf_tag.c        | 164 ++++++++++++++----
>   .../selftests/bpf/progs/btf_type_tag_percpu.c |  66 +++++++
>   3 files changed, 218 insertions(+), 29 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/progs/btf_type_tag_percpu.c
> 
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index 27d63be47b95..17c211f3b924 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -33,6 +33,10 @@ struct bpf_testmod_btf_type_tag_2 {
>   	struct bpf_testmod_btf_type_tag_1 __user *p;
>   };
>   
> +struct bpf_testmod_btf_type_tag_3 {
> +	struct bpf_testmod_btf_type_tag_1 __percpu *p;
> +};
> +
>   noinline int
>   bpf_testmod_test_btf_type_tag_user_1(struct bpf_testmod_btf_type_tag_1 __user *arg) {
>   	BTF_TYPE_EMIT(func_proto_typedef);
> @@ -46,6 +50,19 @@ bpf_testmod_test_btf_type_tag_user_2(struct bpf_testmod_btf_type_tag_2 *arg) {
>   	return arg->p->a;
>   }
>   
> +noinline int
> +bpf_testmod_test_btf_type_tag_percpu_1(struct bpf_testmod_btf_type_tag_1 __percpu *arg) {
> +	BTF_TYPE_EMIT(func_proto_typedef);
> +	BTF_TYPE_EMIT(func_proto_typedef_nested1);
> +	BTF_TYPE_EMIT(func_proto_typedef_nested2);

Are these necessary? They have been defined in
bpf_testmod_test_btf_type_tag_user_1().

> +	return arg->a;
> +}
> +
[...]

Re: [PATCH bpf-next v1 4/4] selftests/bpf: Add a test for btf_type_tag "percpu"

[Alexei Starovoitov]

On Sat, Mar 05, 2022 at 01:20:42PM -0800, Yonghong Song wrote:
> 
> On 3/4/22 11:16 AM, Hao Luo wrote:
> > Add test for percpu btf_type_tag. Similar to the "user" tag, we test
> > the following cases:
> > 
> >   1. __percpu struct field.
> >   2. __percpu as function parameter.
> >   3. per_cpu_ptr() accepts dynamically allocated __percpu memory.
> > 
> > Because the test for "user" and the test for "percpu" are very similar,
> > a little bit of refactoring has been done in btf_tag.c. Basically, both
> > tests share the same function for loading vmlinux and module btf.
> > 
> > Example output from log:
> > 
> >   > ./test_progs -v -t btf_tag
> > 
> >   libbpf: prog 'test_percpu1': BPF program load failed: Permission denied
> >   libbpf: prog 'test_percpu1': -- BEGIN PROG LOAD LOG --
> >   ...
> >   ; g = arg->a;
> >   1: (61) r1 = *(u32 *)(r1 +0)
> >   R1 is ptr_bpf_testmod_btf_type_tag_1 access percpu memory: off=0
> >   ...
> >   test_btf_type_tag_mod_percpu:PASS:btf_type_tag_percpu 0 nsec
> >   #26/6 btf_tag/btf_type_tag_percpu_mod1:OK
> > 
> >   libbpf: prog 'test_percpu2': BPF program load failed: Permission denied
> >   libbpf: prog 'test_percpu2': -- BEGIN PROG LOAD LOG --
> >   ...
> >   ; g = arg->p->a;
> >   2: (61) r1 = *(u32 *)(r1 +0)
> >   R1 is ptr_bpf_testmod_btf_type_tag_1 access percpu memory: off=0
> >   ...
> >   test_btf_type_tag_mod_percpu:PASS:btf_type_tag_percpu 0 nsec
> >   #26/7 btf_tag/btf_type_tag_percpu_mod2:OK
> > 
> >   libbpf: prog 'test_percpu_load': BPF program load failed: Permission denied
> >   libbpf: prog 'test_percpu_load': -- BEGIN PROG LOAD LOG --
> >   ...
> >   ; g = (__u64)cgrp->rstat_cpu->updated_children;
> >   2: (79) r1 = *(u64 *)(r1 +48)
> >   R1 is ptr_cgroup_rstat_cpu access percpu memory: off=48
> >   ...
> >   test_btf_type_tag_vmlinux_percpu:PASS:btf_type_tag_percpu_load 0 nsec
> >   #26/8 btf_tag/btf_type_tag_percpu_vmlinux_load:OK
> > 
> >   load_btfs:PASS:could not load vmlinux BTF 0 nsec
> >   test_btf_type_tag_vmlinux_percpu:PASS:btf_type_tag_percpu 0 nsec
> >   test_btf_type_tag_vmlinux_percpu:PASS:btf_type_tag_percpu_helper 0 nsec
> >   #26/9 btf_tag/btf_type_tag_percpu_vmlinux_helper:OK
> > 
> > Signed-off-by: Hao Luo <haoluo@google.com>
> 
> With one nit below.
> 
> Acked-by: Yonghong Song <yhs@fb.com>
> 
> > ---
> >   .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  17 ++
> >   .../selftests/bpf/prog_tests/btf_tag.c        | 164 ++++++++++++++----
> >   .../selftests/bpf/progs/btf_type_tag_percpu.c |  66 +++++++
> >   3 files changed, 218 insertions(+), 29 deletions(-)
> >   create mode 100644 tools/testing/selftests/bpf/progs/btf_type_tag_percpu.c
> > 
> > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > index 27d63be47b95..17c211f3b924 100644
> > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > @@ -33,6 +33,10 @@ struct bpf_testmod_btf_type_tag_2 {
> >   	struct bpf_testmod_btf_type_tag_1 __user *p;
> >   };
> > +struct bpf_testmod_btf_type_tag_3 {
> > +	struct bpf_testmod_btf_type_tag_1 __percpu *p;
> > +};
> > +
> >   noinline int
> >   bpf_testmod_test_btf_type_tag_user_1(struct bpf_testmod_btf_type_tag_1 __user *arg) {
> >   	BTF_TYPE_EMIT(func_proto_typedef);
> > @@ -46,6 +50,19 @@ bpf_testmod_test_btf_type_tag_user_2(struct bpf_testmod_btf_type_tag_2 *arg) {
> >   	return arg->p->a;
> >   }
> > +noinline int
> > +bpf_testmod_test_btf_type_tag_percpu_1(struct bpf_testmod_btf_type_tag_1 __percpu *arg) {
> > +	BTF_TYPE_EMIT(func_proto_typedef);
> > +	BTF_TYPE_EMIT(func_proto_typedef_nested1);
> > +	BTF_TYPE_EMIT(func_proto_typedef_nested2);
> 
> Are these necessary? They have been defined in
> bpf_testmod_test_btf_type_tag_user_1().

Yonghong,
Thanks. Good catch. I've removed those while applying.

Hao,
Great work.
I really liked how patch 3 discovers MEM_PERCPU flag from
two sources: percpu_datasec and clang tag.

Re: [PATCH bpf-next v1 0/4] bpf: add __percpu tagging in vmlinux BTF

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Fri,  4 Mar 2022 11:16:53 -0800 you wrote:
> This patchset is very much similar to Yonghong's patchset on adding
> __user tagging [1], where a "user" btf_type_tag was introduced to
> describe __user memory pointers. Similar approach can be applied on
> __percpu pointers. The __percpu attribute in kernel is used to identify
> pointers that point to memory allocated in percpu region. Normally,
> accessing __percpu memory requires using special functions like
> per_cpu_ptr() etc. Directly accessing __percpu pointer is meaningless.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v1,1/4] bpf: Fix checking PTR_TO_BTF_ID in check_mem_access
    https://git.kernel.org/bpf/bpf-next/c/bff61f6faedb
  - [bpf-next,v1,2/4] compiler_types: define __percpu as __attribute__((btf_type_tag("percpu")))
    https://git.kernel.org/bpf/bpf-next/c/9216c9162378
  - [bpf-next,v1,3/4] bpf: Reject programs that try to load __percpu memory.
    https://git.kernel.org/bpf/bpf-next/c/5844101a1be9
  - [bpf-next,v1,4/4] selftests/bpf: Add a test for btf_type_tag "percpu"
    https://git.kernel.org/bpf/bpf-next/c/50c6b8a9aea2

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

Re: [PATCH bpf-next v1 4/4] selftests/bpf: Add a test for btf_type_tag "percpu"

[Hao Luo]

On Sat, Mar 5, 2022 at 6:49 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Sat, Mar 05, 2022 at 01:20:42PM -0800, Yonghong Song wrote:
> >
> > On 3/4/22 11:16 AM, Hao Luo wrote:
[...]
> > > ---
> > >   .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  17 ++
> > >   .../selftests/bpf/prog_tests/btf_tag.c        | 164 ++++++++++++++----
> > >   .../selftests/bpf/progs/btf_type_tag_percpu.c |  66 +++++++
> > >   3 files changed, 218 insertions(+), 29 deletions(-)
> > >   create mode 100644 tools/testing/selftests/bpf/progs/btf_type_tag_percpu.c
[...]
> > > +noinline int
> > > +bpf_testmod_test_btf_type_tag_percpu_1(struct bpf_testmod_btf_type_tag_1 __percpu *arg) {
> > > +   BTF_TYPE_EMIT(func_proto_typedef);
> > > +   BTF_TYPE_EMIT(func_proto_typedef_nested1);
> > > +   BTF_TYPE_EMIT(func_proto_typedef_nested2);
> >
> > Are these necessary? They have been defined in
> > bpf_testmod_test_btf_type_tag_user_1().
>
> Yonghong,
> Thanks. Good catch. I've removed those while applying.
>

Thanks Yonghong and Alexei. I wasn't sure their purpose and not sure
whether I should include them, so copy-pasted it.

> Hao,
> Great work.
> I really liked how patch 3 discovers MEM_PERCPU flag from
> two sources: percpu_datasec and clang tag.

Thanks Alexei! The BTF infrastructure together with clang tag is
really amazing! [thumbs up]

Re: [PATCH bpf-next v1 2/4] compiler_types: define __percpu as __attribute__((btf_type_tag("percpu")))

[Andrii Nakryiko]

On Fri, Mar 4, 2022 at 11:17 AM Hao Luo <haoluo@google.com> wrote:
>
> This is similar to commit 7472d5a642c9 ("compiler_types: define __user as
> __attribute__((btf_type_tag("user")))"), where a type tag "user" was
> introduced to identify the pointers that point to user memory. With that
> change, the newest compile toolchain can encode __user information into
> vmlinux BTF, which can be used by the BPF verifier to enforce safe
> program behaviors.
>
> Similarly, we have __percpu attribute, which is mainly used to indicate
> memory is allocated in percpu region. The __percpu pointers in kernel
> are supposed to be used together with functions like per_cpu_ptr() and
> this_cpu_ptr(), which perform necessary calculation on the pointer's
> base address. Without the btf_type_tag introduced in this patch,
> __percpu pointers will be treated as regular memory pointers in vmlinux
> BTF and BPF programs are allowed to directly dereference them, generating
> incorrect behaviors. Now with "percpu" btf_type_tag, the BPF verifier is
> able to differentiate __percpu pointers from regular pointers and forbids
> unexpected behaviors like direct load.
>
> The following is an example similar to the one given in commit
> 7472d5a642c9:
>
>   [$ ~] cat test.c
>   #define __percpu __attribute__((btf_type_tag("percpu")))
>   int foo(int __percpu *arg) {
>         return *arg;
>   }
>   [$ ~] clang -O2 -g -c test.c
>   [$ ~] pahole -JV test.o
>   ...
>   File test.o:
>   [1] INT int size=4 nr_bits=32 encoding=SIGNED
>   [2] TYPE_TAG percpu type_id=1
>   [3] PTR (anon) type_id=2
>   [4] FUNC_PROTO (anon) return=1 args=(3 arg)
>   [5] FUNC foo type_id=4
>   [$ ~]
>
> for the function argument "int __percpu *arg", its type is described as
>         PTR -> TYPE_TAG(percpu) -> INT
> The kernel can use this information for bpf verification or other
> use cases.
>
> Like commit 7472d5a642c9, this feature requires clang (>= clang14) and
> pahole (>= 1.23).
>
> Cc: Yonghong Song <yhs@fb.com>
> Signed-off-by: Hao Luo <haoluo@google.com>
> ---
>  include/linux/compiler_types.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 3f31ff400432..223abf43679a 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -38,7 +38,12 @@ static inline void __chk_io_ptr(const volatile void __iomem *ptr) { }
>  #  define __user
>  # endif
>  # define __iomem
> -# define __percpu
> +# if defined(CONFIG_DEBUG_INFO_BTF) && defined(CONFIG_PAHOLE_HAS_BTF_TAG) && \
> +       __has_attribute(btf_type_tag)
> +#  define __percpu     __attribute__((btf_type_tag("percpu")))


Maybe let's add

#if defined(CONFIG_DEBUG_INFO_BTF) &&
defined(CONFIG_PAHOLE_HAS_BTF_TAG) && __has_attribute(btf_type_tag)
#define BTF_TYPE_TAG(value) __attribute__((btf_type_tag(#value)))
#else
#define BTF_TYPE_TAG(value) /* nothing */
#endif

and use BTF_TYPE_TAG() macro unconditionally everywhere?

> +# else
> +#  define __percpu
> +# endif
>  # define __rcu
>  # define __chk_user_ptr(x)     (void)0
>  # define __chk_io_ptr(x)       (void)0
> --
> 2.35.1.616.g0bdcbb4464-goog
>

Re: [PATCH bpf-next v1 2/4] compiler_types: define __percpu as __attribute__((btf_type_tag("percpu")))

[Yonghong Song]

On 3/7/22 5:44 PM, Andrii Nakryiko wrote:
> On Fri, Mar 4, 2022 at 11:17 AM Hao Luo <haoluo@google.com> wrote:
>>
>> This is similar to commit 7472d5a642c9 ("compiler_types: define __user as
>> __attribute__((btf_type_tag("user")))"), where a type tag "user" was
>> introduced to identify the pointers that point to user memory. With that
>> change, the newest compile toolchain can encode __user information into
>> vmlinux BTF, which can be used by the BPF verifier to enforce safe
>> program behaviors.
>>
>> Similarly, we have __percpu attribute, which is mainly used to indicate
>> memory is allocated in percpu region. The __percpu pointers in kernel
>> are supposed to be used together with functions like per_cpu_ptr() and
>> this_cpu_ptr(), which perform necessary calculation on the pointer's
>> base address. Without the btf_type_tag introduced in this patch,
>> __percpu pointers will be treated as regular memory pointers in vmlinux
>> BTF and BPF programs are allowed to directly dereference them, generating
>> incorrect behaviors. Now with "percpu" btf_type_tag, the BPF verifier is
>> able to differentiate __percpu pointers from regular pointers and forbids
>> unexpected behaviors like direct load.
>>
>> The following is an example similar to the one given in commit
>> 7472d5a642c9:
>>
>>    [$ ~] cat test.c
>>    #define __percpu __attribute__((btf_type_tag("percpu")))
>>    int foo(int __percpu *arg) {
>>          return *arg;
>>    }
>>    [$ ~] clang -O2 -g -c test.c
>>    [$ ~] pahole -JV test.o
>>    ...
>>    File test.o:
>>    [1] INT int size=4 nr_bits=32 encoding=SIGNED
>>    [2] TYPE_TAG percpu type_id=1
>>    [3] PTR (anon) type_id=2
>>    [4] FUNC_PROTO (anon) return=1 args=(3 arg)
>>    [5] FUNC foo type_id=4
>>    [$ ~]
>>
>> for the function argument "int __percpu *arg", its type is described as
>>          PTR -> TYPE_TAG(percpu) -> INT
>> The kernel can use this information for bpf verification or other
>> use cases.
>>
>> Like commit 7472d5a642c9, this feature requires clang (>= clang14) and
>> pahole (>= 1.23).
>>
>> Cc: Yonghong Song <yhs@fb.com>
>> Signed-off-by: Hao Luo <haoluo@google.com>
>> ---
>>   include/linux/compiler_types.h | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
>> index 3f31ff400432..223abf43679a 100644
>> --- a/include/linux/compiler_types.h
>> +++ b/include/linux/compiler_types.h
>> @@ -38,7 +38,12 @@ static inline void __chk_io_ptr(const volatile void __iomem *ptr) { }
>>   #  define __user
>>   # endif
>>   # define __iomem
>> -# define __percpu
>> +# if defined(CONFIG_DEBUG_INFO_BTF) && defined(CONFIG_PAHOLE_HAS_BTF_TAG) && \
>> +       __has_attribute(btf_type_tag)
>> +#  define __percpu     __attribute__((btf_type_tag("percpu")))
> 
> 
> Maybe let's add
> 
> #if defined(CONFIG_DEBUG_INFO_BTF) &&
> defined(CONFIG_PAHOLE_HAS_BTF_TAG) && __has_attribute(btf_type_tag)
> #define BTF_TYPE_TAG(value) __attribute__((btf_type_tag(#value)))
> #else
> #define BTF_TYPE_TAG(value) /* nothing */
> #endif
> 
> and use BTF_TYPE_TAG() macro unconditionally everywhere?

Agree that the above suggestion is a good idea, esp. we may
convert others, e.g., __rcu, with btf_type_tag in the future,
and a common checking will simplify things a lot.

Hao, could you send a followup patch with Andrii's suggestion?

> 
>> +# else
>> +#  define __percpu
>> +# endif
>>   # define __rcu
>>   # define __chk_user_ptr(x)     (void)0
>>   # define __chk_io_ptr(x)       (void)0
>> --
>> 2.35.1.616.g0bdcbb4464-goog
>>

Re: [PATCH bpf-next v1 2/4] compiler_types: define __percpu as __attribute__((btf_type_tag("percpu")))

[Hao Luo]

On Tue, Mar 8, 2022 at 11:08 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 3/7/22 5:44 PM, Andrii Nakryiko wrote:
> > On Fri, Mar 4, 2022 at 11:17 AM Hao Luo <haoluo@google.com> wrote:
> >>
[...]
> >>
> >> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> >> index 3f31ff400432..223abf43679a 100644
> >> --- a/include/linux/compiler_types.h
> >> +++ b/include/linux/compiler_types.h
> >> @@ -38,7 +38,12 @@ static inline void __chk_io_ptr(const volatile void __iomem *ptr) { }
> >>   #  define __user
> >>   # endif
> >>   # define __iomem
> >> -# define __percpu
> >> +# if defined(CONFIG_DEBUG_INFO_BTF) && defined(CONFIG_PAHOLE_HAS_BTF_TAG) && \
> >> +       __has_attribute(btf_type_tag)
> >> +#  define __percpu     __attribute__((btf_type_tag("percpu")))
> >
> >
> > Maybe let's add
> >
> > #if defined(CONFIG_DEBUG_INFO_BTF) &&
> > defined(CONFIG_PAHOLE_HAS_BTF_TAG) && __has_attribute(btf_type_tag)
> > #define BTF_TYPE_TAG(value) __attribute__((btf_type_tag(#value)))
> > #else
> > #define BTF_TYPE_TAG(value) /* nothing */
> > #endif
> >
> > and use BTF_TYPE_TAG() macro unconditionally everywhere?
>
> Agree that the above suggestion is a good idea, esp. we may
> convert others, e.g., __rcu, with btf_type_tag in the future,
> and a common checking will simplify things a lot.
>
> Hao, could you send a followup patch with Andrii's suggestion?
>

No problem. Will send the patch this afternoon.