bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v2 0/5] bpf: add __user tagging support in vmlinux BTF
@ 2022-01-12 20:14 Yonghong Song
  2022-01-12 20:14 ` [PATCH bpf-next v2 1/5] compiler_types: define __user as __attribute__((btf_type_tag("user"))) Yonghong Song
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Yonghong Song @ 2022-01-12 20:14 UTC (permalink / raw)
  To: Alexei Starovoitov, Andrii Nakryiko, bpf, Daniel Borkmann,
	Linus Torvalds
  Cc: Arnaldo Carvalho de Melo, Jose E . Marchesi, kernel-team,
	Masami Hiramatsu

The __user attribute is currently mainly used by sparse for type checking.
The attribute indicates whether a memory access is in user memory address
space or not. Such information is important during tracing kernel
internal functions or data structures as accessing user memory often
has different mechanisms compared to accessing kernel memory. For example,
the perf-probe needs explicit command line specification to indicate a
particular argument or string in user-space memory ([1], [2], [3]).
Currently, vmlinux BTF is available in kernel with many distributions.
If __user attribute information is available in vmlinux BTF, the explicit
user memory access information from users will not be necessary as
the kernel can figure it out by itself with vmlinux BTF.

Besides the above possible use for perf/probe, another use case is
for bpf verifier. Currently, for bpf BPF_PROG_TYPE_TRACING type of bpf
programs, users can write direct code like
  p->m1->m2
and "p" could be a function parameter. Without __user information in BTF,
the verifier will assume p->m1 accessing kernel memory and will generate
normal loads. Let us say "p" actually tagged with __user in the source
code.  In such cases, p->m1 is actually accessing user memory and direct
load is not right and may produce incorrect result. For such cases,
bpf_probe_read_user() will be the correct way to read p->m1.

To support encoding __user information in BTF, a new attribute
  __attribute__((btf_type_tag("<arbitrary_string>")))
is implemented in clang ([4]). For example, if we have
  #define __user __attribute__((btf_type_tag("user")))
during kernel compilation, the attribute "user" information will
be preserved in dwarf. After pahole converting dwarf to BTF, __user
information will be available in vmlinux BTF and such information
can be used by bpf verifier, perf/probe or other use cases.

Currently btf_type_tag is only supported in clang (>= clang14) and
pahole (>= 1.23). gcc support is also proposed and under development ([5]).

In the rest of patch set, Patch 1 added support of __user btf_type_tag
during compilation. Patch 2 added bpf verifier support to utilize __user
tag information to reject bpf programs not using proper helper to access
user memories. Patches 3-5 are for bpf selftests which demonstrate verifier
can reject direct user memory accesses.

  [1] http://lkml.kernel.org/r/155789874562.26965.10836126971405890891.stgit@devnote2
  [2] http://lkml.kernel.org/r/155789872187.26965.4468456816590888687.stgit@devnote2
  [3] http://lkml.kernel.org/r/155789871009.26965.14167558859557329331.stgit@devnote2
  [4] https://reviews.llvm.org/D111199
  [5] https://lore.kernel.org/bpf/0cbeb2fb-1a18-f690-e360-24b1c90c2a91@fb.com/

Changelog:
  v1 -> v2:
    - use MEM_USER flag for PTR_TO_BTF_ID reg type instead of a separate 
      field to encode __user tag.
    - add a test with kernel function __sys_getsockname which has __user tagged
      argument.

Yonghong Song (5):
  compiler_types: define __user as __attribute__((btf_type_tag("user")))
  bpf: reject program if a __user tagged memory accessed in kernel way
  selftests/bpf: rename btf_decl_tag.c to test_btf_decl_tag.c
  selftests/bpf: add a selftest with __user tag
  selftests/bpf: specify pahole version requirement for btf_tag test

 include/linux/bpf.h                           |  11 +-
 include/linux/btf.h                           |   5 +
 include/linux/compiler_types.h                |   3 +
 kernel/bpf/btf.c                              |  34 ++++--
 kernel/bpf/verifier.c                         |  30 ++++--
 lib/Kconfig.debug                             |   8 ++
 net/bpf/bpf_dummy_struct_ops.c                |   6 +-
 net/ipv4/bpf_tcp_ca.c                         |   6 +-
 tools/testing/selftests/bpf/README.rst        |   2 +
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  18 ++++
 .../selftests/bpf/prog_tests/btf_tag.c        | 101 +++++++++++++++++-
 .../selftests/bpf/progs/btf_type_tag_user.c   |  40 +++++++
 .../{btf_decl_tag.c => test_btf_decl_tag.c}   |   0
 13 files changed, 239 insertions(+), 25 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/btf_type_tag_user.c
 rename tools/testing/selftests/bpf/progs/{btf_decl_tag.c => test_btf_decl_tag.c} (100%)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH bpf-next v2 1/5] compiler_types: define __user as __attribute__((btf_type_tag("user")))
  2022-01-12 20:14 [PATCH bpf-next v2 0/5] bpf: add __user tagging support in vmlinux BTF Yonghong Song
@ 2022-01-12 20:14 ` Yonghong Song
  2022-01-12 20:15 ` [PATCH bpf-next v2 2/5] bpf: reject program if a __user tagged memory accessed in kernel way Yonghong Song
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Yonghong Song @ 2022-01-12 20:14 UTC (permalink / raw)
  To: Alexei Starovoitov, Andrii Nakryiko, bpf, Daniel Borkmann,
	Linus Torvalds
  Cc: Arnaldo Carvalho de Melo, Jose E . Marchesi, kernel-team,
	Masami Hiramatsu

The __user attribute is currently mainly used by sparse for type checking.
The attribute indicates whether a memory access is in user memory address
space or not. Such information is important during tracing kernel
internal functions or data structures as accessing user memory often
has different mechanisms compared to accessing kernel memory. For example,
the perf-probe needs explicit command line specification to indicate a
particular argument or string in user-space memory ([1], [2], [3]).
Currently, vmlinux BTF is available in kernel with many distributions.
If __user attribute information is available in vmlinux BTF, the explicit
user memory access information from users will not be necessary as
the kernel can figure it out by itself with vmlinux BTF.

Besides the above possible use for perf/probe, another use case is
for bpf verifier. Currently, for bpf BPF_PROG_TYPE_TRACING type of bpf
programs, users can write direct code like
  p->m1->m2
and "p" could be a function parameter. Without __user information in BTF,
the verifier will assume p->m1 accessing kernel memory and will generate
normal loads. Let us say "p" actually tagged with __user in the source
code.  In such cases, p->m1 is actually accessing user memory and direct
load is not right and may produce incorrect result. For such cases,
bpf_probe_read_user() will be the correct way to read p->m1.

To support encoding __user information in BTF, a new attribute
  __attribute__((btf_type_tag("<arbitrary_string>")))
is implemented in clang ([4]). For example, if we have
  #define __user __attribute__((btf_type_tag("user")))
during kernel compilation, the attribute "user" information will
be preserved in dwarf. After pahole converting dwarf to BTF, __user
information will be available in vmlinux BTF.

The following is an example with latest upstream clang (clang14) and
pahole 1.23:

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

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

Current btf_type_tag is only supported in clang (>= clang14) and
pahole (>= 1.23). gcc support is also proposed and under development ([5]).

  [1] http://lkml.kernel.org/r/155789874562.26965.10836126971405890891.stgit@devnote2
  [2] http://lkml.kernel.org/r/155789872187.26965.4468456816590888687.stgit@devnote2
  [3] http://lkml.kernel.org/r/155789871009.26965.14167558859557329331.stgit@devnote2
  [4] https://reviews.llvm.org/D111199
  [5] https://lore.kernel.org/bpf/0cbeb2fb-1a18-f690-e360-24b1c90c2a91@fb.com/

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 include/linux/compiler_types.h | 3 +++
 lib/Kconfig.debug              | 8 ++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 1d32f4c03c9e..67e5d29cd2a1 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -31,6 +31,9 @@ static inline void __chk_io_ptr(const volatile void __iomem *ptr) { }
 # define __kernel
 # ifdef STRUCTLEAK_PLUGIN
 #  define __user	__attribute__((user))
+# elif defined(CONFIG_DEBUG_INFO_BTF) && defined(CONFIG_PAHOLE_HAS_BTF_TAG) && \
+	__has_attribute(btf_type_tag)
+#  define __user	__attribute__((btf_type_tag("user")))
 # else
 #  define __user
 # endif
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index c77fe36bb3d8..84981ecb4075 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -325,6 +325,14 @@ config DEBUG_INFO_BTF
 config PAHOLE_HAS_SPLIT_BTF
 	def_bool $(success, test `$(PAHOLE) --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'` -ge "119")
 
+config PAHOLE_HAS_BTF_TAG
+	def_bool $(success, test `$(PAHOLE) --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'` -ge "123")
+	depends on CC_IS_CLANG
+	help
+	  Decide whether pahole emits btf_tag attributes (btf_type_tag and
+	  btf_decl_tag) or not. Currently only clang compiler implements
+	  these attributes, so make the config depend on CC_IS_CLANG.
+
 config DEBUG_INFO_BTF_MODULES
 	def_bool y
 	depends on DEBUG_INFO_BTF && MODULES && PAHOLE_HAS_SPLIT_BTF
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH bpf-next v2 2/5] bpf: reject program if a __user tagged memory accessed in kernel way
  2022-01-12 20:14 [PATCH bpf-next v2 0/5] bpf: add __user tagging support in vmlinux BTF Yonghong Song
  2022-01-12 20:14 ` [PATCH bpf-next v2 1/5] compiler_types: define __user as __attribute__((btf_type_tag("user"))) Yonghong Song
@ 2022-01-12 20:15 ` Yonghong Song
  2022-01-19 17:47   ` Alexei Starovoitov
  2022-01-12 20:15 ` [PATCH bpf-next v2 3/5] selftests/bpf: rename btf_decl_tag.c to test_btf_decl_tag.c Yonghong Song
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Yonghong Song @ 2022-01-12 20:15 UTC (permalink / raw)
  To: Alexei Starovoitov, Andrii Nakryiko, bpf, Daniel Borkmann,
	Linus Torvalds
  Cc: Arnaldo Carvalho de Melo, Jose E . Marchesi, kernel-team,
	Masami Hiramatsu

BPF verifier supports direct memory access for BPF_PROG_TYPE_TRACING type
of bpf programs, e.g., a->b. If "a" is a pointer
pointing to kernel memory, bpf verifier will allow user to write
code in C like a->b and the verifier will translate it to a kernel
load properly. If "a" is a pointer to user memory, it is expected
that bpf developer should be bpf_probe_read_user() helper to
get the value a->b. Without utilizing BTF __user tagging information,
current verifier will assume that a->b is a kernel memory access
and this may generate incorrect result.

Now BTF contains __user information, it can check whether the
pointer points to a user memory or not. If it is, the verifier
can reject the program and force users to use bpf_probe_read_user()
helper explicitly.

In the future, we can easily extend btf_add_space for other
address space tagging, for example, rcu/percpu etc.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 include/linux/bpf.h            | 11 ++++++++---
 include/linux/btf.h            |  5 +++++
 kernel/bpf/btf.c               | 34 ++++++++++++++++++++++++++++------
 kernel/bpf/verifier.c          | 30 ++++++++++++++++++++++--------
 net/bpf/bpf_dummy_struct_ops.c |  6 ++++--
 net/ipv4/bpf_tcp_ca.c          |  6 ++++--
 6 files changed, 71 insertions(+), 21 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 6e947cd91152..c97ec30f2f12 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -308,6 +308,8 @@ extern const struct bpf_map_ops bpf_map_offload_ops;
 #define BPF_BASE_TYPE_BITS	8
 
 enum bpf_type_flag {
+	FLAG_DONTCARE		= 0,
+
 	/* PTR may be NULL. */
 	PTR_MAYBE_NULL		= BIT(0 + BPF_BASE_TYPE_BITS),
 
@@ -316,7 +318,10 @@ enum bpf_type_flag {
 	 */
 	MEM_RDONLY		= BIT(1 + BPF_BASE_TYPE_BITS),
 
-	__BPF_TYPE_LAST_FLAG	= MEM_RDONLY,
+	/* MEM is in user address space. */
+	MEM_USER		= BIT(2 + BPF_BASE_TYPE_BITS),
+
+	__BPF_TYPE_LAST_FLAG	= MEM_USER,
 };
 
 /* Max number of base types. */
@@ -572,7 +577,7 @@ struct bpf_verifier_ops {
 				 const struct btf *btf,
 				 const struct btf_type *t, int off, int size,
 				 enum bpf_access_type atype,
-				 u32 *next_btf_id);
+				 u32 *next_btf_id, enum bpf_type_flag *flag);
 	bool (*check_kfunc_call)(u32 kfunc_btf_id, struct module *owner);
 };
 
@@ -1749,7 +1754,7 @@ static inline bool bpf_tracing_btf_ctx_access(int off, int size,
 int btf_struct_access(struct bpf_verifier_log *log, const struct btf *btf,
 		      const struct btf_type *t, int off, int size,
 		      enum bpf_access_type atype,
-		      u32 *next_btf_id);
+		      u32 *next_btf_id, enum bpf_type_flag *flag);
 bool btf_struct_ids_match(struct bpf_verifier_log *log,
 			  const struct btf *btf, u32 id, int off,
 			  const struct btf *need_btf, u32 need_type_id);
diff --git a/include/linux/btf.h b/include/linux/btf.h
index 0c74348cbc9d..e4e2f7124fe6 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -216,6 +216,11 @@ static inline bool btf_type_is_var(const struct btf_type *t)
 	return BTF_INFO_KIND(t->info) == BTF_KIND_VAR;
 }
 
+static inline bool btf_type_is_type_tag(const struct btf_type *t)
+{
+	return BTF_INFO_KIND(t->info) == BTF_KIND_TYPE_TAG;
+}
+
 /* union is only a special case of struct:
  * all its offsetof(member) == 0
  */
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 33bb8ae4a804..54bf483d01f3 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4848,6 +4848,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 	const char *tname = prog->aux->attach_func_name;
 	struct bpf_verifier_log *log = info->log;
 	const struct btf_param *args;
+	const char *tag_value;
 	u32 nr_args, arg;
 	int i, ret;
 
@@ -5000,6 +5001,13 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 	info->btf = btf;
 	info->btf_id = t->type;
 	t = btf_type_by_id(btf, t->type);
+
+	if (btf_type_is_type_tag(t)) {
+		tag_value = __btf_name_by_offset(btf, t->name_off);
+		if (strcmp(tag_value, "user") == 0)
+			info->reg_type |= MEM_USER;
+	}
+
 	/* skip modifiers */
 	while (btf_type_is_modifier(t)) {
 		info->btf_id = t->type;
@@ -5026,12 +5034,12 @@ enum bpf_struct_walk_result {
 
 static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
 			   const struct btf_type *t, int off, int size,
-			   u32 *next_btf_id)
+			   u32 *next_btf_id, enum bpf_type_flag *flag)
 {
 	u32 i, moff, mtrue_end, msize = 0, total_nelems = 0;
 	const struct btf_type *mtype, *elem_type = NULL;
 	const struct btf_member *member;
-	const char *tname, *mname;
+	const char *tname, *mname, *tag_value;
 	u32 vlen, elem_id, mid;
 
 again:
@@ -5215,7 +5223,8 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
 		}
 
 		if (btf_type_is_ptr(mtype)) {
-			const struct btf_type *stype;
+			enum bpf_type_flag tmp_flag = FLAG_DONTCARE;
+			const struct btf_type *stype, *t;
 			u32 id;
 
 			if (msize != size || off != moff) {
@@ -5224,9 +5233,19 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
 					mname, moff, tname, off, size);
 				return -EACCES;
 			}
+
+			/* check __user 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);
+				if (strcmp(tag_value, "user") == 0)
+					tmp_flag = MEM_USER;
+			}
+
 			stype = btf_type_skip_modifiers(btf, mtype->type, &id);
 			if (btf_type_is_struct(stype)) {
 				*next_btf_id = id;
+				*flag = tmp_flag;
 				return WALK_PTR;
 			}
 		}
@@ -5253,13 +5272,14 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
 int btf_struct_access(struct bpf_verifier_log *log, const struct btf *btf,
 		      const struct btf_type *t, int off, int size,
 		      enum bpf_access_type atype __maybe_unused,
-		      u32 *next_btf_id)
+		      u32 *next_btf_id, enum bpf_type_flag *flag)
 {
+	enum bpf_type_flag tmp_flag = FLAG_DONTCARE;
 	int err;
 	u32 id;
 
 	do {
-		err = btf_struct_walk(log, btf, t, off, size, &id);
+		err = btf_struct_walk(log, btf, t, off, size, &id, &tmp_flag);
 
 		switch (err) {
 		case WALK_PTR:
@@ -5267,6 +5287,7 @@ int btf_struct_access(struct bpf_verifier_log *log, const struct btf *btf,
 			 * we're done.
 			 */
 			*next_btf_id = id;
+			*flag = tmp_flag;
 			return PTR_TO_BTF_ID;
 		case WALK_SCALAR:
 			return SCALAR_VALUE;
@@ -5311,6 +5332,7 @@ bool btf_struct_ids_match(struct bpf_verifier_log *log,
 			  const struct btf *need_btf, u32 need_type_id)
 {
 	const struct btf_type *type;
+	enum bpf_type_flag flag;
 	int err;
 
 	/* Are we already done? */
@@ -5321,7 +5343,7 @@ bool btf_struct_ids_match(struct bpf_verifier_log *log,
 	type = btf_type_by_id(btf, id);
 	if (!type)
 		return false;
-	err = btf_struct_walk(log, btf, type, off, 1, &id);
+	err = btf_struct_walk(log, btf, type, off, 1, &id, &flag);
 	if (err != WALK_STRUCT)
 		return false;
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index bfb45381fb3f..6b78642ea437 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -568,6 +568,9 @@ static const char *reg_type_str(struct bpf_verifier_env *env,
 			strncpy(postfix, "_or_null", 16);
 	}
 
+	if (type & MEM_USER)
+		strncpy(prefix, "user_", 16);
+
 	if (type & MEM_RDONLY)
 		strncpy(prefix, "rdonly_", 16);
 
@@ -1544,14 +1547,15 @@ static void mark_reg_not_init(struct bpf_verifier_env *env,
 static void mark_btf_ld_reg(struct bpf_verifier_env *env,
 			    struct bpf_reg_state *regs, u32 regno,
 			    enum bpf_reg_type reg_type,
-			    struct btf *btf, u32 btf_id)
+			    struct btf *btf, u32 btf_id,
+			    enum bpf_type_flag flag)
 {
 	if (reg_type == SCALAR_VALUE) {
 		mark_reg_unknown(env, regs, regno);
 		return;
 	}
 	mark_reg_known_zero(env, regs, regno);
-	regs[regno].type = PTR_TO_BTF_ID;
+	regs[regno].type = PTR_TO_BTF_ID | flag;
 	regs[regno].btf = btf;
 	regs[regno].btf_id = btf_id;
 }
@@ -4149,6 +4153,7 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 	struct bpf_reg_state *reg = regs + regno;
 	const struct btf_type *t = btf_type_by_id(reg->btf, reg->btf_id);
 	const char *tname = btf_name_by_offset(reg->btf, t->name_off);
+	enum bpf_type_flag flag = FLAG_DONTCARE;
 	u32 btf_id;
 	int ret;
 
@@ -4168,9 +4173,16 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 		return -EACCES;
 	}
 
+	if (reg->type & MEM_USER) {
+		verbose(env,
+			"R%d is ptr_%s access user 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);
+						  off, size, atype, &btf_id, &flag);
 	} else {
 		if (atype != BPF_READ) {
 			verbose(env, "only read is supported\n");
@@ -4178,14 +4190,14 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 		}
 
 		ret = btf_struct_access(&env->log, reg->btf, t, off, size,
-					atype, &btf_id);
+					atype, &btf_id, &flag);
 	}
 
 	if (ret < 0)
 		return ret;
 
 	if (atype == BPF_READ && value_regno >= 0)
-		mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id);
+		mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag);
 
 	return 0;
 }
@@ -4198,6 +4210,7 @@ static int check_ptr_to_map_access(struct bpf_verifier_env *env,
 {
 	struct bpf_reg_state *reg = regs + regno;
 	struct bpf_map *map = reg->map_ptr;
+	enum bpf_type_flag flag = FLAG_DONTCARE;
 	const struct btf_type *t;
 	const char *tname;
 	u32 btf_id;
@@ -4235,12 +4248,12 @@ static int check_ptr_to_map_access(struct bpf_verifier_env *env,
 		return -EACCES;
 	}
 
-	ret = btf_struct_access(&env->log, btf_vmlinux, t, off, size, atype, &btf_id);
+	ret = btf_struct_access(&env->log, btf_vmlinux, t, off, size, atype, &btf_id, &flag);
 	if (ret < 0)
 		return ret;
 
 	if (value_regno >= 0)
-		mark_btf_ld_reg(env, regs, value_regno, ret, btf_vmlinux, btf_id);
+		mark_btf_ld_reg(env, regs, value_regno, ret, btf_vmlinux, btf_id, flag);
 
 	return 0;
 }
@@ -4441,7 +4454,8 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
 		if (err < 0)
 			return err;
 
-		err = check_ctx_access(env, insn_idx, off, size, t, &reg_type, &btf, &btf_id);
+		err = check_ctx_access(env, insn_idx, off, size, t, &reg_type, &btf,
+				       &btf_id);
 		if (err)
 			verbose_linfo(env, insn_idx, "; ");
 		if (!err && t == BPF_READ && value_regno >= 0) {
diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index fbc896323bec..d0e54e30658a 100644
--- a/net/bpf/bpf_dummy_struct_ops.c
+++ b/net/bpf/bpf_dummy_struct_ops.c
@@ -145,7 +145,8 @@ static int bpf_dummy_ops_btf_struct_access(struct bpf_verifier_log *log,
 					   const struct btf *btf,
 					   const struct btf_type *t, int off,
 					   int size, enum bpf_access_type atype,
-					   u32 *next_btf_id)
+					   u32 *next_btf_id,
+					   enum bpf_type_flag *flag)
 {
 	const struct btf_type *state;
 	s32 type_id;
@@ -162,7 +163,8 @@ static int bpf_dummy_ops_btf_struct_access(struct bpf_verifier_log *log,
 		return -EACCES;
 	}
 
-	err = btf_struct_access(log, btf, t, off, size, atype, next_btf_id);
+	err = btf_struct_access(log, btf, t, off, size, atype, next_btf_id,
+				flag);
 	if (err < 0)
 		return err;
 
diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index de610cb83694..6b781eead784 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -95,12 +95,14 @@ static int bpf_tcp_ca_btf_struct_access(struct bpf_verifier_log *log,
 					const struct btf *btf,
 					const struct btf_type *t, int off,
 					int size, enum bpf_access_type atype,
-					u32 *next_btf_id)
+					u32 *next_btf_id,
+					enum bpf_type_flag *flag)
 {
 	size_t end;
 
 	if (atype == BPF_READ)
-		return btf_struct_access(log, btf, t, off, size, atype, next_btf_id);
+		return btf_struct_access(log, btf, t, off, size, atype, next_btf_id,
+					 flag);
 
 	if (t != tcp_sock_type) {
 		bpf_log(log, "only read is supported\n");
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH bpf-next v2 3/5] selftests/bpf: rename btf_decl_tag.c to test_btf_decl_tag.c
  2022-01-12 20:14 [PATCH bpf-next v2 0/5] bpf: add __user tagging support in vmlinux BTF Yonghong Song
  2022-01-12 20:14 ` [PATCH bpf-next v2 1/5] compiler_types: define __user as __attribute__((btf_type_tag("user"))) Yonghong Song
  2022-01-12 20:15 ` [PATCH bpf-next v2 2/5] bpf: reject program if a __user tagged memory accessed in kernel way Yonghong Song
@ 2022-01-12 20:15 ` Yonghong Song
  2022-01-12 20:15 ` [PATCH bpf-next v2 4/5] selftests/bpf: add a selftest with __user tag Yonghong Song
  2022-01-12 20:15 ` [PATCH bpf-next v2 5/5] selftests/bpf: specify pahole version requirement for btf_tag test Yonghong Song
  4 siblings, 0 replies; 10+ messages in thread
From: Yonghong Song @ 2022-01-12 20:15 UTC (permalink / raw)
  To: Alexei Starovoitov, Andrii Nakryiko, bpf, Daniel Borkmann,
	Linus Torvalds
  Cc: Arnaldo Carvalho de Melo, Jose E . Marchesi, kernel-team,
	Masami Hiramatsu

The uapi btf.h contains the following declaration:
  struct btf_decl_tag {
       __s32   component_idx;
  };

The skeleton will also generate a struct with name
"btf_decl_tag" for bpf program btf_decl_tag.c.

Rename btf_decl_tag.c to test_btf_decl_tag.c so
the corresponding skeleton struct name becomes
"test_btf_decl_tag". This way, we could include
uapi btf.h in prog_tests/btf_tag.c.
There is no functionality change for this patch.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/testing/selftests/bpf/prog_tests/btf_tag.c          | 8 ++++----
 .../bpf/progs/{btf_decl_tag.c => test_btf_decl_tag.c}     | 0
 2 files changed, 4 insertions(+), 4 deletions(-)
 rename tools/testing/selftests/bpf/progs/{btf_decl_tag.c => test_btf_decl_tag.c} (100%)

diff --git a/tools/testing/selftests/bpf/prog_tests/btf_tag.c b/tools/testing/selftests/bpf/prog_tests/btf_tag.c
index 88d63e23e35f..c4cf27777ff7 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_tag.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_tag.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2021 Facebook */
 #include <test_progs.h>
-#include "btf_decl_tag.skel.h"
+#include "test_btf_decl_tag.skel.h"
 
 /* struct btf_type_tag_test is referenced in btf_type_tag.skel.h */
 struct btf_type_tag_test {
@@ -11,9 +11,9 @@ struct btf_type_tag_test {
 
 static void test_btf_decl_tag(void)
 {
-	struct btf_decl_tag *skel;
+	struct test_btf_decl_tag *skel;
 
-	skel = btf_decl_tag__open_and_load();
+	skel = test_btf_decl_tag__open_and_load();
 	if (!ASSERT_OK_PTR(skel, "btf_decl_tag"))
 		return;
 
@@ -22,7 +22,7 @@ static void test_btf_decl_tag(void)
 		test__skip();
 	}
 
-	btf_decl_tag__destroy(skel);
+	test_btf_decl_tag__destroy(skel);
 }
 
 static void test_btf_type_tag(void)
diff --git a/tools/testing/selftests/bpf/progs/btf_decl_tag.c b/tools/testing/selftests/bpf/progs/test_btf_decl_tag.c
similarity index 100%
rename from tools/testing/selftests/bpf/progs/btf_decl_tag.c
rename to tools/testing/selftests/bpf/progs/test_btf_decl_tag.c
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH bpf-next v2 4/5] selftests/bpf: add a selftest with __user tag
  2022-01-12 20:14 [PATCH bpf-next v2 0/5] bpf: add __user tagging support in vmlinux BTF Yonghong Song
                   ` (2 preceding siblings ...)
  2022-01-12 20:15 ` [PATCH bpf-next v2 3/5] selftests/bpf: rename btf_decl_tag.c to test_btf_decl_tag.c Yonghong Song
@ 2022-01-12 20:15 ` Yonghong Song
  2022-01-12 20:15 ` [PATCH bpf-next v2 5/5] selftests/bpf: specify pahole version requirement for btf_tag test Yonghong Song
  4 siblings, 0 replies; 10+ messages in thread
From: Yonghong Song @ 2022-01-12 20:15 UTC (permalink / raw)
  To: Alexei Starovoitov, Andrii Nakryiko, bpf, Daniel Borkmann,
	Linus Torvalds
  Cc: Arnaldo Carvalho de Melo, Jose E . Marchesi, kernel-team,
	Masami Hiramatsu

Added a selftest with three__user usages: a __user pointer-type argument
in bpf_testmod, a __user pointer-type struct member in bpf_testmod,
and a __user pointer-type struct member in vmlinux. In all cases,
directly accessing the user memory will result verification failure.

  $ ./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_'
  ...
  #22/3 btf_tag/btf_type_tag_user_mod1:OK

  $ ./test_progs -v -n 22/4
  ...
  libbpf: prog 'test_user2': BPF program load failed: Permission denied
  libbpf: prog 'test_user2': -- BEGIN PROG LOAD LOG --
  R1 type=ctx expected=fp
  0: R1=ctx(id=0,off=0,imm=0) R10=fp0
  ; int BPF_PROG(test_user2, struct bpf_testmod_btf_type_tag_2 *arg)
  0: (79) r1 = *(u64 *)(r1 +0)
  func 'bpf_testmod_test_btf_type_tag_user_2' arg0 has btf_id 136563 type STRUCT 'bpf_testmod_btf_type_tag_2'
  1: R1_w=ptr_bpf_testmod_btf_type_tag_2(id=0,off=0,imm=0)
  ; g = arg->p->a;
  1: (79) r1 = *(u64 *)(r1 +0)          ; R1_w=user_ptr_bpf_testmod_btf_type_tag_1(id=0,off=0,imm=0)
  ; g = arg->p->a;
  2: (61) r1 = *(u32 *)(r1 +0)
  R1 invalid mem access 'user_ptr_'
  ...
  #22/4 btf_tag/btf_type_tag_user_mod2:OK

  $ ./test_progs -v -n 22/5
  ...
  libbpf: prog 'test_sys_getsockname': BPF program load failed: Permission denied
  libbpf: prog 'test_sys_getsockname': -- BEGIN PROG LOAD LOG --
  R1 type=ctx expected=fp
  0: R1=ctx(id=0,off=0,imm=0) R10=fp0
  ; int BPF_PROG(test_sys_getsockname, int fd, struct sockaddr *usockaddr,
  0: (79) r1 = *(u64 *)(r1 +8)
  func '__sys_getsockname' arg1 has btf_id 2319 type STRUCT 'sockaddr'
  1: R1_w=user_ptr_sockaddr(id=0,off=0,imm=0)
  ; g = usockaddr->sa_family;
  1: (69) r1 = *(u16 *)(r1 +0)
  R1 invalid mem access 'user_ptr_'
  ...
  #22/5 btf_tag/btf_type_tag_user_vmlinux:OK

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 18 ++++
 .../selftests/bpf/prog_tests/btf_tag.c        | 93 +++++++++++++++++++
 .../selftests/bpf/progs/btf_type_tag_user.c   | 40 ++++++++
 3 files changed, 151 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/btf_type_tag_user.c

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index df3b292a8ffe..4efe3eee0908 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -21,6 +21,24 @@ bpf_testmod_test_mod_kfunc(int i)
 	*(int *)this_cpu_ptr(&bpf_testmod_ksym_percpu) = i;
 }
 
+struct bpf_testmod_btf_type_tag_1 {
+	int a;
+};
+
+struct bpf_testmod_btf_type_tag_2 {
+	struct bpf_testmod_btf_type_tag_1 __user *p;
+};
+
+noinline int
+bpf_testmod_test_btf_type_tag_user_1(struct bpf_testmod_btf_type_tag_1 __user *arg) {
+	return arg->a;
+}
+
+noinline int
+bpf_testmod_test_btf_type_tag_user_2(struct bpf_testmod_btf_type_tag_2 *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 c4cf27777ff7..ee13acb44893 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_tag.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_tag.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2021 Facebook */
 #include <test_progs.h>
+#include <bpf/btf.h>
 #include "test_btf_decl_tag.skel.h"
 
 /* struct btf_type_tag_test is referenced in btf_type_tag.skel.h */
@@ -8,6 +9,7 @@ struct btf_type_tag_test {
         int **p;
 };
 #include "btf_type_tag.skel.h"
+#include "btf_type_tag_user.skel.h"
 
 static void test_btf_decl_tag(void)
 {
@@ -41,10 +43,101 @@ static void test_btf_type_tag(void)
 	btf_type_tag__destroy(skel);
 }
 
+static void test_btf_type_tag_mod_user(bool load_test_user1)
+{
+	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;
+	}
+
+	/* 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;
+
+	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;
+
+	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;
+	}
+
+	skel = btf_type_tag_user__open();
+	if (!ASSERT_OK_PTR(skel, "btf_type_tag_user"))
+		goto free_module_btf;
+
+	bpf_program__set_autoload(skel->progs.test_sys_getsockname, false);
+	if (load_test_user1)
+		bpf_program__set_autoload(skel->progs.test_user2, false);
+	else
+		bpf_program__set_autoload(skel->progs.test_user1, false);
+
+	err = btf_type_tag_user__load(skel);
+	ASSERT_ERR(err, "btf_type_tag_user");
+
+	btf_type_tag_user__destroy(skel);
+
+free_module_btf:
+	btf__free(module_btf);
+free_vmlinux_btf:
+	btf__free(vmlinux_btf);
+}
+
+static void test_btf_type_tag_vmlinux_user()
+{
+	struct btf_type_tag_user *skel;
+	struct btf *vmlinux_btf;
+	__s32 type_id;
+	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"))
+		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;
+
+	bpf_program__set_autoload(skel->progs.test_user2, false);
+	bpf_program__set_autoload(skel->progs.test_user1, false);
+
+	err = btf_type_tag_user__load(skel);
+	ASSERT_ERR(err, "btf_type_tag_user");
+
+	btf_type_tag_user__destroy(skel);
+
+free_vmlinux_btf:
+	btf__free(vmlinux_btf);
+}
+
 void test_btf_tag(void)
 {
 	if (test__start_subtest("btf_decl_tag"))
 		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();
 }
diff --git a/tools/testing/selftests/bpf/progs/btf_type_tag_user.c b/tools/testing/selftests/bpf/progs/btf_type_tag_user.c
new file mode 100644
index 000000000000..5523f77c5a44
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/btf_type_tag_user.c
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Facebook */
+#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;
+};
+
+int g;
+
+SEC("fentry/bpf_testmod_test_btf_type_tag_user_1")
+int BPF_PROG(test_user1, struct bpf_testmod_btf_type_tag_1 *arg)
+{
+	g = arg->a;
+	return 0;
+}
+
+SEC("fentry/bpf_testmod_test_btf_type_tag_user_2")
+int BPF_PROG(test_user2, struct bpf_testmod_btf_type_tag_2 *arg)
+{
+	g = arg->p->a;
+	return 0;
+}
+
+/* int __sys_getsockname(int fd, struct sockaddr __user *usockaddr,
+ *                       int __user *usockaddr_len);
+ */
+SEC("fentry/__sys_getsockname")
+int BPF_PROG(test_sys_getsockname, int fd, struct sockaddr *usockaddr,
+	     int *usockaddr_len)
+{
+	g = usockaddr->sa_family;
+	return 0;
+}
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH bpf-next v2 5/5] selftests/bpf: specify pahole version requirement for btf_tag test
  2022-01-12 20:14 [PATCH bpf-next v2 0/5] bpf: add __user tagging support in vmlinux BTF Yonghong Song
                   ` (3 preceding siblings ...)
  2022-01-12 20:15 ` [PATCH bpf-next v2 4/5] selftests/bpf: add a selftest with __user tag Yonghong Song
@ 2022-01-12 20:15 ` Yonghong Song
  4 siblings, 0 replies; 10+ messages in thread
From: Yonghong Song @ 2022-01-12 20:15 UTC (permalink / raw)
  To: Alexei Starovoitov, Andrii Nakryiko, bpf, Daniel Borkmann,
	Linus Torvalds
  Cc: Arnaldo Carvalho de Melo, Jose E . Marchesi, kernel-team,
	Masami Hiramatsu

Specify pahole version requirement (1.23) for btf_tag subtests
btf_type_tag_user_{mod1, mod2, vmlinux}.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/testing/selftests/bpf/README.rst | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/bpf/README.rst b/tools/testing/selftests/bpf/README.rst
index 42ef250c7acc..d099d91adc3b 100644
--- a/tools/testing/selftests/bpf/README.rst
+++ b/tools/testing/selftests/bpf/README.rst
@@ -206,6 +206,8 @@ btf_tag test and Clang version
 
 The btf_tag selftest requires LLVM support to recognize the btf_decl_tag and
 btf_type_tag attributes. They are introduced in `Clang 14` [0_, 1_].
+The subtests ``btf_type_tag_user_{mod1, mod2, vmlinux}`` also requires
+pahole version ``1.23``.
 
 Without them, the btf_tag selftest will be skipped and you will observe:
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v2 2/5] bpf: reject program if a __user tagged memory accessed in kernel way
  2022-01-12 20:15 ` [PATCH bpf-next v2 2/5] bpf: reject program if a __user tagged memory accessed in kernel way Yonghong Song
@ 2022-01-19 17:47   ` Alexei Starovoitov
  2022-01-20  4:10     ` Yonghong Song
  0 siblings, 1 reply; 10+ messages in thread
From: Alexei Starovoitov @ 2022-01-19 17:47 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Alexei Starovoitov, Andrii Nakryiko, bpf, Daniel Borkmann,
	Linus Torvalds, Arnaldo Carvalho de Melo, Jose E . Marchesi,
	Kernel Team, Masami Hiramatsu

On Wed, Jan 12, 2022 at 12:16 PM Yonghong Song <yhs@fb.com> wrote:
> +
> +                       /* check __user 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);
> +                               if (strcmp(tag_value, "user") == 0)
> +                                       tmp_flag = MEM_USER;
> +                       }
> +
>                         stype = btf_type_skip_modifiers(btf, mtype->type, &id);

Does LLVM guarantee that btf_tag will be the first in the modifiers?
Looking at the selftest:
+struct bpf_testmod_btf_type_tag_2 {
+       struct bpf_testmod_btf_type_tag_1 __user *p;
+};

What if there are 'const' or 'volatile' modifiers on that pointer too?
And in different order with btf_tag?
BTF gets normalized or not?
I wonder whether we should introduce something like
btf_type_collect_modifiers() instead of btf_type_skip_modifiers() ?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v2 2/5] bpf: reject program if a __user tagged memory accessed in kernel way
  2022-01-19 17:47   ` Alexei Starovoitov
@ 2022-01-20  4:10     ` Yonghong Song
  2022-01-20  4:27       ` Alexei Starovoitov
  0 siblings, 1 reply; 10+ messages in thread
From: Yonghong Song @ 2022-01-20  4:10 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Alexei Starovoitov, Andrii Nakryiko, bpf, Daniel Borkmann,
	Linus Torvalds, Arnaldo Carvalho de Melo, Jose E . Marchesi,
	Kernel Team, Masami Hiramatsu



On 1/19/22 9:47 AM, Alexei Starovoitov wrote:
> On Wed, Jan 12, 2022 at 12:16 PM Yonghong Song <yhs@fb.com> wrote:
>> +
>> +                       /* check __user 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);
>> +                               if (strcmp(tag_value, "user") == 0)
>> +                                       tmp_flag = MEM_USER;
>> +                       }
>> +
>>                          stype = btf_type_skip_modifiers(btf, mtype->type, &id);
> 
> Does LLVM guarantee that btf_tag will be the first in the modifiers?
> Looking at the selftest:
> +struct bpf_testmod_btf_type_tag_2 {
> +       struct bpf_testmod_btf_type_tag_1 __user *p;
> +};
> 
> What if there are 'const' or 'volatile' modifiers on that pointer too?
> And in different order with btf_tag?
> BTF gets normalized or not?
> I wonder whether we should introduce something like
> btf_type_collect_modifiers() instead of btf_type_skip_modifiers() ?

Yes, LLVM guarantees that btf_tag will be the first in the modifiers.
The type chain format looks like below:
   ptr -> [btf_type_tag ->]* (zero or more btf_type_tag's)
       -> [other modifiers: const and/or volatile and/or restrict]
       -> base_type

I only handled zero/one btf_type_tag case as we don't have use case
in kernel with two btf_type_tags for one pointer yet.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v2 2/5] bpf: reject program if a __user tagged memory accessed in kernel way
  2022-01-20  4:10     ` Yonghong Song
@ 2022-01-20  4:27       ` Alexei Starovoitov
  2022-01-20  6:51         ` Yonghong Song
  0 siblings, 1 reply; 10+ messages in thread
From: Alexei Starovoitov @ 2022-01-20  4:27 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Alexei Starovoitov, Andrii Nakryiko, bpf, Daniel Borkmann,
	Linus Torvalds, Arnaldo Carvalho de Melo, Jose E . Marchesi,
	Kernel Team, Masami Hiramatsu

On Wed, Jan 19, 2022 at 08:10:27PM -0800, Yonghong Song wrote:
> 
> 
> On 1/19/22 9:47 AM, Alexei Starovoitov wrote:
> > On Wed, Jan 12, 2022 at 12:16 PM Yonghong Song <yhs@fb.com> wrote:
> > > +
> > > +                       /* check __user 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);
> > > +                               if (strcmp(tag_value, "user") == 0)
> > > +                                       tmp_flag = MEM_USER;
> > > +                       }
> > > +
> > >                          stype = btf_type_skip_modifiers(btf, mtype->type, &id);
> > 
> > Does LLVM guarantee that btf_tag will be the first in the modifiers?
> > Looking at the selftest:
> > +struct bpf_testmod_btf_type_tag_2 {
> > +       struct bpf_testmod_btf_type_tag_1 __user *p;
> > +};
> > 
> > What if there are 'const' or 'volatile' modifiers on that pointer too?
> > And in different order with btf_tag?
> > BTF gets normalized or not?
> > I wonder whether we should introduce something like
> > btf_type_collect_modifiers() instead of btf_type_skip_modifiers() ?
> 
> Yes, LLVM guarantees that btf_tag will be the first in the modifiers.
> The type chain format looks like below:
>   ptr -> [btf_type_tag ->]* (zero or more btf_type_tag's)
>       -> [other modifiers: const and/or volatile and/or restrict]
>       -> base_type
> 
> I only handled zero/one btf_type_tag case as we don't have use case
> in kernel with two btf_type_tags for one pointer yet.

Makes sense. Would be good to document this LLVM behavior somewhere.
When GCC adds support for btf_tag it would need to do the same.
Or is it more of a pahole guarantee when it converts LLVM dwarf tags to BTF?

Separately... looking at:
FLAG_DONTCARE           = 0
It's not quite right.
bpf_types already have an enum value at zero:
enum bpf_reg_type {
        NOT_INIT = 0,            /* nothing was written into register */
and other bpf_*_types too.
So empty flag should really mean zeros in bits after BPF_BASE_TYPE_BITS.
But there is no good way to express it as enum.
So maybe use 0 directly when you init:
enum bpf_type_flag tmp_flag = 0;
?

Another bit.. this patch will conflict with
commit a672b2e36a64 ("bpf: Fix ringbuf memory type confusion when passing to helpers")
so please resubmit when that patch appears in bpf-next.
Thanks!

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v2 2/5] bpf: reject program if a __user tagged memory accessed in kernel way
  2022-01-20  4:27       ` Alexei Starovoitov
@ 2022-01-20  6:51         ` Yonghong Song
  0 siblings, 0 replies; 10+ messages in thread
From: Yonghong Song @ 2022-01-20  6:51 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Alexei Starovoitov, Andrii Nakryiko, bpf, Daniel Borkmann,
	Linus Torvalds, Arnaldo Carvalho de Melo, Jose E . Marchesi,
	Kernel Team, Masami Hiramatsu



On 1/19/22 8:27 PM, Alexei Starovoitov wrote:
> On Wed, Jan 19, 2022 at 08:10:27PM -0800, Yonghong Song wrote:
>>
>>
>> On 1/19/22 9:47 AM, Alexei Starovoitov wrote:
>>> On Wed, Jan 12, 2022 at 12:16 PM Yonghong Song <yhs@fb.com> wrote:
>>>> +
>>>> +                       /* check __user 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);
>>>> +                               if (strcmp(tag_value, "user") == 0)
>>>> +                                       tmp_flag = MEM_USER;
>>>> +                       }
>>>> +
>>>>                           stype = btf_type_skip_modifiers(btf, mtype->type, &id);
>>>
>>> Does LLVM guarantee that btf_tag will be the first in the modifiers?
>>> Looking at the selftest:
>>> +struct bpf_testmod_btf_type_tag_2 {
>>> +       struct bpf_testmod_btf_type_tag_1 __user *p;
>>> +};
>>>
>>> What if there are 'const' or 'volatile' modifiers on that pointer too?
>>> And in different order with btf_tag?
>>> BTF gets normalized or not?
>>> I wonder whether we should introduce something like
>>> btf_type_collect_modifiers() instead of btf_type_skip_modifiers() ?
>>
>> Yes, LLVM guarantees that btf_tag will be the first in the modifiers.
>> The type chain format looks like below:
>>    ptr -> [btf_type_tag ->]* (zero or more btf_type_tag's)
>>        -> [other modifiers: const and/or volatile and/or restrict]
>>        -> base_type
>>
>> I only handled zero/one btf_type_tag case as we don't have use case
>> in kernel with two btf_type_tags for one pointer yet.
> 
> Makes sense. Would be good to document this LLVM behavior somewhere.
> When GCC adds support for btf_tag it would need to do the same.
> Or is it more of a pahole guarantee when it converts LLVM dwarf tags to BTF?

Yes, this property is guaranteed by both llvm (for bpf target) and
pahole (for non bpf target). I will document this behavior in
btf.rst.


> 
> Separately... looking at:
> FLAG_DONTCARE           = 0
> It's not quite right.
> bpf_types already have an enum value at zero:
> enum bpf_reg_type {
>          NOT_INIT = 0,            /* nothing was written into register */
> and other bpf_*_types too.
> So empty flag should really mean zeros in bits after BPF_BASE_TYPE_BITS.
> But there is no good way to express it as enum.
> So maybe use 0 directly when you init:
> enum bpf_type_flag tmp_flag = 0;
> ?

I thought about this before and that is why I added FLAG_DONTCARE
to match value to the type. But I agree that is not elegent, I will
use 0 as you suggested.

> 
> Another bit.. this patch will conflict with
> commit a672b2e36a64 ("bpf: Fix ringbuf memory type confusion when passing to helpers")
> so please resubmit when that patch appears in bpf-next.

Thanks for heads-up. Will fix the above two issues and
resubmit once commit a672b2e36a64 arrives in bpf-next.

> Thanks!

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-01-20  6:51 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-12 20:14 [PATCH bpf-next v2 0/5] bpf: add __user tagging support in vmlinux BTF Yonghong Song
2022-01-12 20:14 ` [PATCH bpf-next v2 1/5] compiler_types: define __user as __attribute__((btf_type_tag("user"))) Yonghong Song
2022-01-12 20:15 ` [PATCH bpf-next v2 2/5] bpf: reject program if a __user tagged memory accessed in kernel way Yonghong Song
2022-01-19 17:47   ` Alexei Starovoitov
2022-01-20  4:10     ` Yonghong Song
2022-01-20  4:27       ` Alexei Starovoitov
2022-01-20  6:51         ` Yonghong Song
2022-01-12 20:15 ` [PATCH bpf-next v2 3/5] selftests/bpf: rename btf_decl_tag.c to test_btf_decl_tag.c Yonghong Song
2022-01-12 20:15 ` [PATCH bpf-next v2 4/5] selftests/bpf: add a selftest with __user tag Yonghong Song
2022-01-12 20:15 ` [PATCH bpf-next v2 5/5] selftests/bpf: specify pahole version requirement for btf_tag test Yonghong Song

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).