[PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

The patch set is addressing a fallout from
commit 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the verifier.")
It was too aggressive with PTR_UNTRUSTED marks.
Patches 1-6 are cleanup and adding verifier smartness to address real
use cases in bpf programs that broke with too aggressive PTR_UNTRUSTED.
The partial revert is done in patch 7 anyway.

Alexei Starovoitov (8):
  bpf: Invoke btf_struct_access() callback only for writes.
  bpf: Remove unused arguments from btf_struct_access().
  bpf: Refactor btf_nested_type_is_trusted().
  bpf: Teach verifier that certain helpers accept NULL pointer.
  bpf: Refactor NULL-ness check in check_reg_type().
  bpf: Allowlist few fields similar to __rcu tag.
  bpf: Undo strict enforcement for walking untagged fields.
  selftests/bpf: Add tracing tests for walking skb and req.

 include/linux/bpf.h                           | 10 +-
 include/linux/filter.h                        |  3 +-
 kernel/bpf/bpf_cgrp_storage.c                 |  4 +-
 kernel/bpf/bpf_inode_storage.c                |  4 +-
 kernel/bpf/bpf_task_storage.c                 |  8 +-
 kernel/bpf/btf.c                              | 44 ++++-----
 kernel/bpf/verifier.c                         | 91 ++++++++++++++-----
 net/bpf/bpf_dummy_struct_ops.c                | 14 ++-
 net/core/bpf_sk_storage.c                     |  4 +-
 net/core/filter.c                             | 21 ++---
 net/ipv4/bpf_tcp_ca.c                         |  6 +-
 net/netfilter/nf_conntrack_bpf.c              |  3 +-
 .../bpf/progs/test_sk_storage_tracing.c       | 16 ++++
 13 files changed, 131 insertions(+), 97 deletions(-)

-- 
2.34.1

[PATCH bpf-next 1/8] bpf: Invoke btf_struct_access() callback only for writes.

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

Remove duplicated if (atype == BPF_READ) btf_struct_access() from
btf_struct_access() callback and invoke it only for writes.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c          | 2 +-
 net/bpf/bpf_dummy_struct_ops.c | 2 +-
 net/core/filter.c              | 6 ------
 net/ipv4/bpf_tcp_ca.c          | 3 ---
 4 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index eaf9c5291cf0..83984568ccb4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5504,7 +5504,7 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 		return -EACCES;
 	}
 
-	if (env->ops->btf_struct_access && !type_is_alloc(reg->type)) {
+	if (env->ops->btf_struct_access && !type_is_alloc(reg->type) && atype == BPF_WRITE) {
 		if (!btf_is_kernel(reg->btf)) {
 			verbose(env, "verifier internal error: reg->btf must be kernel btf\n");
 			return -EFAULT;
diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index ff4f89a2b02a..9535c8506cda 100644
--- a/net/bpf/bpf_dummy_struct_ops.c
+++ b/net/bpf/bpf_dummy_struct_ops.c
@@ -198,7 +198,7 @@ static int bpf_dummy_ops_btf_struct_access(struct bpf_verifier_log *log,
 	if (err < 0)
 		return err;
 
-	return atype == BPF_READ ? err : NOT_INIT;
+	return NOT_INIT;
 }
 
 static const struct bpf_verifier_ops bpf_dummy_verifier_ops = {
diff --git a/net/core/filter.c b/net/core/filter.c
index 3370efad1dda..8b9f409a2ec3 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -8753,9 +8753,6 @@ static int tc_cls_act_btf_struct_access(struct bpf_verifier_log *log,
 {
 	int ret = -EACCES;
 
-	if (atype == BPF_READ)
-		return btf_struct_access(log, reg, off, size, atype, next_btf_id, flag);
-
 	mutex_lock(&nf_conn_btf_access_lock);
 	if (nfct_btf_struct_access)
 		ret = nfct_btf_struct_access(log, reg, off, size, atype, next_btf_id, flag);
@@ -8830,9 +8827,6 @@ static int xdp_btf_struct_access(struct bpf_verifier_log *log,
 {
 	int ret = -EACCES;
 
-	if (atype == BPF_READ)
-		return btf_struct_access(log, reg, off, size, atype, next_btf_id, flag);
-
 	mutex_lock(&nf_conn_btf_access_lock);
 	if (nfct_btf_struct_access)
 		ret = nfct_btf_struct_access(log, reg, off, size, atype, next_btf_id, flag);
diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index ea21c96c03aa..d6465876bbf6 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -78,9 +78,6 @@ static int bpf_tcp_ca_btf_struct_access(struct bpf_verifier_log *log,
 	const struct btf_type *t;
 	size_t end;
 
-	if (atype == BPF_READ)
-		return btf_struct_access(log, reg, off, size, atype, next_btf_id, flag);
-
 	t = btf_type_by_id(reg->btf, reg->btf_id);
 	if (t != tcp_sock_type) {
 		bpf_log(log, "only read is supported\n");
-- 
2.34.1

[PATCH bpf-next 2/8] bpf: Remove unused arguments from btf_struct_access().

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

Remove unused arguments from btf_struct_access() callback.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h              |  3 +--
 include/linux/filter.h           |  3 +--
 kernel/bpf/verifier.c            |  4 ++--
 net/bpf/bpf_dummy_struct_ops.c   | 12 +++++-------
 net/core/filter.c                | 13 +++++--------
 net/ipv4/bpf_tcp_ca.c            |  3 +--
 net/netfilter/nf_conntrack_bpf.c |  3 +--
 7 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 2d8f3f639e68..4f689dda748f 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -893,8 +893,7 @@ struct bpf_verifier_ops {
 				  struct bpf_prog *prog, u32 *target_size);
 	int (*btf_struct_access)(struct bpf_verifier_log *log,
 				 const struct bpf_reg_state *reg,
-				 int off, int size, enum bpf_access_type atype,
-				 u32 *next_btf_id, enum bpf_type_flag *flag);
+				 int off, int size);
 };
 
 struct bpf_prog_offload_ops {
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 23c08c31bea9..5364b0c52c1d 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -571,8 +571,7 @@ DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
 extern struct mutex nf_conn_btf_access_lock;
 extern int (*nfct_btf_struct_access)(struct bpf_verifier_log *log,
 				     const struct bpf_reg_state *reg,
-				     int off, int size, enum bpf_access_type atype,
-				     u32 *next_btf_id, enum bpf_type_flag *flag);
+				     int off, int size);
 
 typedef unsigned int (*bpf_dispatcher_fn)(const void *ctx,
 					  const struct bpf_insn *insnsi,
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 83984568ccb4..5ca520e5eddf 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5459,7 +5459,7 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 	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 = 0;
-	u32 btf_id;
+	u32 btf_id = 0;
 	int ret;
 
 	if (!env->allow_ptr_leaks) {
@@ -5509,7 +5509,7 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 			verbose(env, "verifier internal error: reg->btf must be kernel btf\n");
 			return -EFAULT;
 		}
-		ret = env->ops->btf_struct_access(&env->log, reg, off, size, atype, &btf_id, &flag);
+		ret = env->ops->btf_struct_access(&env->log, reg, off, size);
 	} else {
 		/* Writes are permitted with default btf_struct_access for
 		 * program allocated objects (which always have ref_obj_id > 0),
diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index 9535c8506cda..5918d1b32e19 100644
--- a/net/bpf/bpf_dummy_struct_ops.c
+++ b/net/bpf/bpf_dummy_struct_ops.c
@@ -173,14 +173,11 @@ static int bpf_dummy_ops_check_member(const struct btf_type *t,
 
 static int bpf_dummy_ops_btf_struct_access(struct bpf_verifier_log *log,
 					   const struct bpf_reg_state *reg,
-					   int off, int size, enum bpf_access_type atype,
-					   u32 *next_btf_id,
-					   enum bpf_type_flag *flag)
+					   int off, int size)
 {
 	const struct btf_type *state;
 	const struct btf_type *t;
 	s32 type_id;
-	int err;
 
 	type_id = btf_find_by_name_kind(reg->btf, "bpf_dummy_ops_state",
 					BTF_KIND_STRUCT);
@@ -194,9 +191,10 @@ static int bpf_dummy_ops_btf_struct_access(struct bpf_verifier_log *log,
 		return -EACCES;
 	}
 
-	err = btf_struct_access(log, reg, off, size, atype, next_btf_id, flag);
-	if (err < 0)
-		return err;
+	if (off + size > sizeof(struct bpf_dummy_ops_state)) {
+		bpf_log(log, "write access at off %d with size %d\n", off, size);
+		return -EACCES;
+	}
 
 	return NOT_INIT;
 }
diff --git a/net/core/filter.c b/net/core/filter.c
index 8b9f409a2ec3..1f2abf0f60e6 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -8742,20 +8742,18 @@ EXPORT_SYMBOL_GPL(nf_conn_btf_access_lock);
 
 int (*nfct_btf_struct_access)(struct bpf_verifier_log *log,
 			      const struct bpf_reg_state *reg,
-			      int off, int size, enum bpf_access_type atype,
-			      u32 *next_btf_id, enum bpf_type_flag *flag);
+			      int off, int size);
 EXPORT_SYMBOL_GPL(nfct_btf_struct_access);
 
 static int tc_cls_act_btf_struct_access(struct bpf_verifier_log *log,
 					const struct bpf_reg_state *reg,
-					int off, int size, enum bpf_access_type atype,
-					u32 *next_btf_id, enum bpf_type_flag *flag)
+					int off, int size)
 {
 	int ret = -EACCES;
 
 	mutex_lock(&nf_conn_btf_access_lock);
 	if (nfct_btf_struct_access)
-		ret = nfct_btf_struct_access(log, reg, off, size, atype, next_btf_id, flag);
+		ret = nfct_btf_struct_access(log, reg, off, size);
 	mutex_unlock(&nf_conn_btf_access_lock);
 
 	return ret;
@@ -8822,14 +8820,13 @@ EXPORT_SYMBOL_GPL(bpf_warn_invalid_xdp_action);
 
 static int xdp_btf_struct_access(struct bpf_verifier_log *log,
 				 const struct bpf_reg_state *reg,
-				 int off, int size, enum bpf_access_type atype,
-				 u32 *next_btf_id, enum bpf_type_flag *flag)
+				 int off, int size)
 {
 	int ret = -EACCES;
 
 	mutex_lock(&nf_conn_btf_access_lock);
 	if (nfct_btf_struct_access)
-		ret = nfct_btf_struct_access(log, reg, off, size, atype, next_btf_id, flag);
+		ret = nfct_btf_struct_access(log, reg, off, size);
 	mutex_unlock(&nf_conn_btf_access_lock);
 
 	return ret;
diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index d6465876bbf6..4406d796cc2f 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -72,8 +72,7 @@ static bool bpf_tcp_ca_is_valid_access(int off, int size,
 
 static int bpf_tcp_ca_btf_struct_access(struct bpf_verifier_log *log,
 					const struct bpf_reg_state *reg,
-					int off, int size, enum bpf_access_type atype,
-					u32 *next_btf_id, enum bpf_type_flag *flag)
+					int off, int size)
 {
 	const struct btf_type *t;
 	size_t end;
diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
index 002e9d24a1e9..3f821b7ba646 100644
--- a/net/netfilter/nf_conntrack_bpf.c
+++ b/net/netfilter/nf_conntrack_bpf.c
@@ -192,8 +192,7 @@ BTF_ID(struct, nf_conn___init)
 /* Check writes into `struct nf_conn` */
 static int _nf_conntrack_btf_struct_access(struct bpf_verifier_log *log,
 					   const struct bpf_reg_state *reg,
-					   int off, int size, enum bpf_access_type atype,
-					   u32 *next_btf_id, enum bpf_type_flag *flag)
+					   int off, int size)
 {
 	const struct btf_type *ncit, *nct, *t;
 	size_t end;
-- 
2.34.1

[PATCH bpf-next 3/8] bpf: Refactor btf_nested_type_is_trusted().

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

btf_nested_type_is_trusted() tries to find a struct member at corresponding offset.
It works for flat structures and falls apart in more complex structs with nested structs.
The offset->member search is already performed by btf_struct_walk() including nested structs.
Reuse this work and pass {field name, field btf id} into btf_nested_type_is_trusted()
instead of offset to make BTF_TYPE_SAFE*() logic more robust.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h   |  7 ++++---
 kernel/bpf/btf.c      | 44 +++++++++++++++++--------------------------
 kernel/bpf/verifier.c | 23 +++++++++++-----------
 3 files changed, 33 insertions(+), 41 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 4f689dda748f..002a811b6b90 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2263,7 +2263,7 @@ static inline bool bpf_tracing_btf_ctx_access(int off, int size,
 int btf_struct_access(struct bpf_verifier_log *log,
 		      const struct bpf_reg_state *reg,
 		      int off, int size, enum bpf_access_type atype,
-		      u32 *next_btf_id, enum bpf_type_flag *flag);
+		      u32 *next_btf_id, enum bpf_type_flag *flag, const char **field_name);
 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,
@@ -2302,7 +2302,7 @@ struct bpf_core_ctx {
 
 bool btf_nested_type_is_trusted(struct bpf_verifier_log *log,
 				const struct bpf_reg_state *reg,
-				int off, const char *suffix);
+				const char *field_name, u32 btf_id, const char *suffix);
 
 bool btf_type_ids_nocast_alias(struct bpf_verifier_log *log,
 			       const struct btf *reg_btf, u32 reg_id,
@@ -2517,7 +2517,8 @@ static inline struct bpf_prog *bpf_prog_by_id(u32 id)
 static inline int btf_struct_access(struct bpf_verifier_log *log,
 				    const struct bpf_reg_state *reg,
 				    int off, int size, enum bpf_access_type atype,
-				    u32 *next_btf_id, enum bpf_type_flag *flag)
+				    u32 *next_btf_id, enum bpf_type_flag *flag,
+				    const char **field_name)
 {
 	return -EACCES;
 }
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index b7e5a5510b91..593c45a294d0 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6166,7 +6166,8 @@ 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, enum bpf_type_flag *flag)
+			   u32 *next_btf_id, enum bpf_type_flag *flag,
+			   const char **field_name)
 {
 	u32 i, moff, mtrue_end, msize = 0, total_nelems = 0;
 	const struct btf_type *mtype, *elem_type = NULL;
@@ -6395,6 +6396,8 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
 			if (btf_type_is_struct(stype)) {
 				*next_btf_id = id;
 				*flag |= tmp_flag;
+				if (field_name)
+					*field_name = mname;
 				return WALK_PTR;
 			}
 		}
@@ -6421,7 +6424,8 @@ 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 bpf_reg_state *reg,
 		      int off, int size, enum bpf_access_type atype __maybe_unused,
-		      u32 *next_btf_id, enum bpf_type_flag *flag)
+		      u32 *next_btf_id, enum bpf_type_flag *flag,
+		      const char **field_name)
 {
 	const struct btf *btf = reg->btf;
 	enum bpf_type_flag tmp_flag = 0;
@@ -6453,7 +6457,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
 
 	t = btf_type_by_id(btf, id);
 	do {
-		err = btf_struct_walk(log, btf, t, off, size, &id, &tmp_flag);
+		err = btf_struct_walk(log, btf, t, off, size, &id, &tmp_flag, field_name);
 
 		switch (err) {
 		case WALK_PTR:
@@ -6528,7 +6532,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, &flag);
+	err = btf_struct_walk(log, btf, type, off, 1, &id, &flag, NULL);
 	if (err != WALK_STRUCT)
 		return false;
 
@@ -8488,16 +8492,15 @@ int bpf_core_apply(struct bpf_core_ctx *ctx, const struct bpf_core_relo *relo,
 
 bool btf_nested_type_is_trusted(struct bpf_verifier_log *log,
 				const struct bpf_reg_state *reg,
-				int off, const char *suffix)
+				const char *field_name, u32 btf_id, const char *suffix)
 {
 	struct btf *btf = reg->btf;
 	const struct btf_type *walk_type, *safe_type;
 	const char *tname;
 	char safe_tname[64];
 	long ret, safe_id;
-	const struct btf_member *member, *m_walk = NULL;
+	const struct btf_member *member;
 	u32 i;
-	const char *walk_name;
 
 	walk_type = btf_type_by_id(btf, reg->btf_id);
 	if (!walk_type)
@@ -8517,30 +8520,17 @@ bool btf_nested_type_is_trusted(struct bpf_verifier_log *log,
 	if (!safe_type)
 		return false;
 
-	for_each_member(i, walk_type, member) {
-		u32 moff;
-
-		/* We're looking for the PTR_TO_BTF_ID member in the struct
-		 * type we're walking which matches the specified offset.
-		 * Below, we'll iterate over the fields in the safe variant of
-		 * the struct and see if any of them has a matching type /
-		 * name.
-		 */
-		moff = __btf_member_bit_offset(walk_type, member) / 8;
-		if (off == moff) {
-			m_walk = member;
-			break;
-		}
-	}
-	if (m_walk == NULL)
-		return false;
-
-	walk_name = __btf_name_by_offset(btf, m_walk->name_off);
 	for_each_member(i, safe_type, member) {
 		const char *m_name = __btf_name_by_offset(btf, member->name_off);
+		const struct btf_type *mtype = btf_type_by_id(btf, member->type);
+		u32 id;
+
+		if (!btf_type_is_ptr(mtype))
+			continue;
 
+		btf_type_skip_modifiers(btf, mtype->type, &id);
 		/* If we match on both type and name, the field is considered trusted. */
-		if (m_walk->type == member->type && !strcmp(walk_name, m_name))
+		if (btf_id == id && !strcmp(field_name, m_name))
 			return true;
 	}
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5ca520e5eddf..2cd2e0b725cd 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5400,12 +5400,12 @@ BTF_TYPE_SAFE_RCU(struct css_set) {
 
 /* full trusted: these fields are trusted even outside of RCU CS and never NULL */
 BTF_TYPE_SAFE_TRUSTED(struct bpf_iter_meta) {
-	__bpf_md_ptr(struct seq_file *, seq);
+	struct seq_file *seq;
 };
 
 BTF_TYPE_SAFE_TRUSTED(struct bpf_iter__task) {
-	__bpf_md_ptr(struct bpf_iter_meta *, meta);
-	__bpf_md_ptr(struct task_struct *, task);
+	struct bpf_iter_meta *meta;
+	struct task_struct *task;
 };
 
 BTF_TYPE_SAFE_TRUSTED(struct linux_binprm) {
@@ -5427,17 +5427,17 @@ BTF_TYPE_SAFE_TRUSTED(struct socket) {
 
 static bool type_is_rcu(struct bpf_verifier_env *env,
 			struct bpf_reg_state *reg,
-			int off)
+			const char *field_name, u32 btf_id)
 {
 	BTF_TYPE_EMIT(BTF_TYPE_SAFE_RCU(struct task_struct));
 	BTF_TYPE_EMIT(BTF_TYPE_SAFE_RCU(struct css_set));
 
-	return btf_nested_type_is_trusted(&env->log, reg, off, "__safe_rcu");
+	return btf_nested_type_is_trusted(&env->log, reg, field_name, btf_id, "__safe_rcu");
 }
 
 static bool type_is_trusted(struct bpf_verifier_env *env,
 			    struct bpf_reg_state *reg,
-			    int off)
+			    const char *field_name, u32 btf_id)
 {
 	BTF_TYPE_EMIT(BTF_TYPE_SAFE_TRUSTED(struct bpf_iter_meta));
 	BTF_TYPE_EMIT(BTF_TYPE_SAFE_TRUSTED(struct bpf_iter__task));
@@ -5446,7 +5446,7 @@ static bool type_is_trusted(struct bpf_verifier_env *env,
 	BTF_TYPE_EMIT(BTF_TYPE_SAFE_TRUSTED(struct dentry));
 	BTF_TYPE_EMIT(BTF_TYPE_SAFE_TRUSTED(struct socket));
 
-	return btf_nested_type_is_trusted(&env->log, reg, off, "__safe_trusted");
+	return btf_nested_type_is_trusted(&env->log, reg, field_name, btf_id, "__safe_trusted");
 }
 
 static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
@@ -5458,6 +5458,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);
+	const char *field_name = NULL;
 	enum bpf_type_flag flag = 0;
 	u32 btf_id = 0;
 	int ret;
@@ -5526,7 +5527,7 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 			return -EFAULT;
 		}
 
-		ret = btf_struct_access(&env->log, reg, off, size, atype, &btf_id, &flag);
+		ret = btf_struct_access(&env->log, reg, off, size, atype, &btf_id, &flag, &field_name);
 	}
 
 	if (ret < 0)
@@ -5554,10 +5555,10 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 		 * A regular RCU-protected pointer with __rcu tag can also be deemed
 		 * trusted if we are in an RCU CS. Such pointer can be NULL.
 		 */
-		if (type_is_trusted(env, reg, off)) {
+		if (type_is_trusted(env, reg, field_name, btf_id)) {
 			flag |= PTR_TRUSTED;
 		} else if (in_rcu_cs(env) && !type_may_be_null(reg->type)) {
-			if (type_is_rcu(env, reg, off)) {
+			if (type_is_rcu(env, reg, field_name, btf_id)) {
 				/* ignore __rcu tag and mark it MEM_RCU */
 				flag |= MEM_RCU;
 			} else if (flag & MEM_RCU) {
@@ -5640,7 +5641,7 @@ static int check_ptr_to_map_access(struct bpf_verifier_env *env,
 	/* Simulate access to a PTR_TO_BTF_ID */
 	memset(&map_reg, 0, sizeof(map_reg));
 	mark_btf_ld_reg(env, &map_reg, 0, PTR_TO_BTF_ID, btf_vmlinux, *map->ops->map_btf_id, 0);
-	ret = btf_struct_access(&env->log, &map_reg, off, size, atype, &btf_id, &flag);
+	ret = btf_struct_access(&env->log, &map_reg, off, size, atype, &btf_id, &flag, NULL);
 	if (ret < 0)
 		return ret;
 
-- 
2.34.1

[PATCH bpf-next 4/8] bpf: Teach verifier that certain helpers accept NULL pointer.

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

bpf_[sk|inode|task|cgrp]_storage_[get|delete]() and bpf_get_socket_cookie() helpers
perform run-time check that sk|inode|task|cgrp pointer != NULL.
Teach verifier about this fact and allow bpf programs to pass
PTR_TO_BTF_ID | PTR_MAYBE_NULL into such helpers.
It will be used in the subsequent patch that will do
bpf_sk_storage_get(.., skb->sk, ...);
Even when 'skb' pointer is trusted the 'sk' pointer may be NULL.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/bpf_cgrp_storage.c  | 4 ++--
 kernel/bpf/bpf_inode_storage.c | 4 ++--
 kernel/bpf/bpf_task_storage.c  | 8 ++++----
 net/core/bpf_sk_storage.c      | 4 ++--
 net/core/filter.c              | 2 +-
 5 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/kernel/bpf/bpf_cgrp_storage.c b/kernel/bpf/bpf_cgrp_storage.c
index d17d5b694668..d44fe8dd9732 100644
--- a/kernel/bpf/bpf_cgrp_storage.c
+++ b/kernel/bpf/bpf_cgrp_storage.c
@@ -224,7 +224,7 @@ const struct bpf_func_proto bpf_cgrp_storage_get_proto = {
 	.gpl_only	= false,
 	.ret_type	= RET_PTR_TO_MAP_VALUE_OR_NULL,
 	.arg1_type	= ARG_CONST_MAP_PTR,
-	.arg2_type	= ARG_PTR_TO_BTF_ID,
+	.arg2_type	= ARG_PTR_TO_BTF_ID_OR_NULL,
 	.arg2_btf_id	= &bpf_cgroup_btf_id[0],
 	.arg3_type	= ARG_PTR_TO_MAP_VALUE_OR_NULL,
 	.arg4_type	= ARG_ANYTHING,
@@ -235,6 +235,6 @@ const struct bpf_func_proto bpf_cgrp_storage_delete_proto = {
 	.gpl_only	= false,
 	.ret_type	= RET_INTEGER,
 	.arg1_type	= ARG_CONST_MAP_PTR,
-	.arg2_type	= ARG_PTR_TO_BTF_ID,
+	.arg2_type	= ARG_PTR_TO_BTF_ID_OR_NULL,
 	.arg2_btf_id	= &bpf_cgroup_btf_id[0],
 };
diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
index e17ad581b9be..a4d93df78c75 100644
--- a/kernel/bpf/bpf_inode_storage.c
+++ b/kernel/bpf/bpf_inode_storage.c
@@ -229,7 +229,7 @@ const struct bpf_func_proto bpf_inode_storage_get_proto = {
 	.gpl_only	= false,
 	.ret_type	= RET_PTR_TO_MAP_VALUE_OR_NULL,
 	.arg1_type	= ARG_CONST_MAP_PTR,
-	.arg2_type	= ARG_PTR_TO_BTF_ID,
+	.arg2_type	= ARG_PTR_TO_BTF_ID_OR_NULL,
 	.arg2_btf_id	= &bpf_inode_storage_btf_ids[0],
 	.arg3_type	= ARG_PTR_TO_MAP_VALUE_OR_NULL,
 	.arg4_type	= ARG_ANYTHING,
@@ -240,6 +240,6 @@ const struct bpf_func_proto bpf_inode_storage_delete_proto = {
 	.gpl_only	= false,
 	.ret_type	= RET_INTEGER,
 	.arg1_type	= ARG_CONST_MAP_PTR,
-	.arg2_type	= ARG_PTR_TO_BTF_ID,
+	.arg2_type	= ARG_PTR_TO_BTF_ID_OR_NULL,
 	.arg2_btf_id	= &bpf_inode_storage_btf_ids[0],
 };
diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c
index d1af0c8f9ce4..adf6dfe0ba68 100644
--- a/kernel/bpf/bpf_task_storage.c
+++ b/kernel/bpf/bpf_task_storage.c
@@ -338,7 +338,7 @@ const struct bpf_func_proto bpf_task_storage_get_recur_proto = {
 	.gpl_only = false,
 	.ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
 	.arg1_type = ARG_CONST_MAP_PTR,
-	.arg2_type = ARG_PTR_TO_BTF_ID,
+	.arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
 	.arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
 	.arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
 	.arg4_type = ARG_ANYTHING,
@@ -349,7 +349,7 @@ const struct bpf_func_proto bpf_task_storage_get_proto = {
 	.gpl_only = false,
 	.ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
 	.arg1_type = ARG_CONST_MAP_PTR,
-	.arg2_type = ARG_PTR_TO_BTF_ID,
+	.arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
 	.arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
 	.arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
 	.arg4_type = ARG_ANYTHING,
@@ -360,7 +360,7 @@ const struct bpf_func_proto bpf_task_storage_delete_recur_proto = {
 	.gpl_only = false,
 	.ret_type = RET_INTEGER,
 	.arg1_type = ARG_CONST_MAP_PTR,
-	.arg2_type = ARG_PTR_TO_BTF_ID,
+	.arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
 	.arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
 };
 
@@ -369,6 +369,6 @@ const struct bpf_func_proto bpf_task_storage_delete_proto = {
 	.gpl_only = false,
 	.ret_type = RET_INTEGER,
 	.arg1_type = ARG_CONST_MAP_PTR,
-	.arg2_type = ARG_PTR_TO_BTF_ID,
+	.arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
 	.arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
 };
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index 085025c7130a..d4172534dfa8 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -412,7 +412,7 @@ const struct bpf_func_proto bpf_sk_storage_get_tracing_proto = {
 	.gpl_only	= false,
 	.ret_type	= RET_PTR_TO_MAP_VALUE_OR_NULL,
 	.arg1_type	= ARG_CONST_MAP_PTR,
-	.arg2_type	= ARG_PTR_TO_BTF_ID,
+	.arg2_type	= ARG_PTR_TO_BTF_ID_OR_NULL,
 	.arg2_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
 	.arg3_type	= ARG_PTR_TO_MAP_VALUE_OR_NULL,
 	.arg4_type	= ARG_ANYTHING,
@@ -424,7 +424,7 @@ const struct bpf_func_proto bpf_sk_storage_delete_tracing_proto = {
 	.gpl_only	= false,
 	.ret_type	= RET_INTEGER,
 	.arg1_type	= ARG_CONST_MAP_PTR,
-	.arg2_type	= ARG_PTR_TO_BTF_ID,
+	.arg2_type	= ARG_PTR_TO_BTF_ID_OR_NULL,
 	.arg2_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
 	.allowed	= bpf_sk_storage_tracing_allowed,
 };
diff --git a/net/core/filter.c b/net/core/filter.c
index 1f2abf0f60e6..727c5269867d 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4998,7 +4998,7 @@ const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto = {
 	.func		= bpf_get_socket_ptr_cookie,
 	.gpl_only	= false,
 	.ret_type	= RET_INTEGER,
-	.arg1_type	= ARG_PTR_TO_BTF_ID_SOCK_COMMON,
+	.arg1_type	= ARG_PTR_TO_BTF_ID_SOCK_COMMON | PTR_MAYBE_NULL,
 };
 
 BPF_CALL_1(bpf_get_socket_cookie_sock_ops, struct bpf_sock_ops_kern *, ctx)
-- 
2.34.1

[PATCH bpf-next 5/8] bpf: Refactor NULL-ness check in check_reg_type().

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

check_reg_type() unconditionally disallows PTR_TO_BTF_ID | PTR_MAYBE_NULL.
It's problematic for helpers that allow ARG_PTR_TO_BTF_ID_OR_NULL like
bpf_sk_storage_get(). Allow passing PTR_TO_BTF_ID | PTR_MAYBE_NULL into such
helpers. That technically includes bpf_kptr_xchg() helper, but in practice:
  bpf_kptr_xchg(..., bpf_cpumask_create());
is still disallowed because bpf_cpumask_create() returns ref counted pointer
with ref_obj_id > 0.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2cd2e0b725cd..4e7d671497f4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7168,6 +7168,8 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
 	case PTR_TO_BTF_ID:
 	case PTR_TO_BTF_ID | PTR_TRUSTED:
 	case PTR_TO_BTF_ID | MEM_RCU:
+	case PTR_TO_BTF_ID | PTR_MAYBE_NULL:
+	case PTR_TO_BTF_ID | PTR_MAYBE_NULL | MEM_RCU:
 	{
 		/* For bpf_sk_release, it needs to match against first member
 		 * 'struct sock_common', hence make an exception for it. This
@@ -7176,6 +7178,12 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
 		bool strict_type_match = arg_type_is_release(arg_type) &&
 					 meta->func_id != BPF_FUNC_sk_release;
 
+		if (type_may_be_null(reg->type) &&
+		    (!type_may_be_null(arg_type) || arg_type_is_release(arg_type))) {
+			verbose(env, "Possibly NULL pointer passed to helper arg%d\n", regno);
+			return -EACCES;
+		}
+
 		if (!arg_btf_id) {
 			if (!compatible->btf_id) {
 				verbose(env, "verifier internal error: missing arg compatible BTF ID\n");
@@ -7206,10 +7214,6 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
 		}
 		break;
 	}
-	case PTR_TO_BTF_ID | PTR_MAYBE_NULL:
-	case PTR_TO_BTF_ID | PTR_MAYBE_NULL | MEM_RCU:
-		verbose(env, "Possibly NULL pointer passed to helper arg%d\n", regno);
-		return -EACCES;
 	case PTR_TO_BTF_ID | MEM_ALLOC:
 		if (meta->func_id != BPF_FUNC_spin_lock && meta->func_id != BPF_FUNC_spin_unlock &&
 		    meta->func_id != BPF_FUNC_kptr_xchg) {
-- 
2.34.1

[PATCH bpf-next 6/8] bpf: Allowlist few fields similar to __rcu tag.

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

Allow bpf program access cgrp->kn, mm->exe_file, skb->sk, req->sk.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 4e7d671497f4..fd90ba498ccc 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5378,6 +5378,7 @@ static int bpf_map_direct_read(struct bpf_map *map, int off, int size, u64 *val)
 }
 
 #define BTF_TYPE_SAFE_RCU(__type)  __PASTE(__type, __safe_rcu)
+#define BTF_TYPE_SAFE_RCU_OR_NULL(__type)  __PASTE(__type, __safe_rcu_or_null)
 #define BTF_TYPE_SAFE_TRUSTED(__type)  __PASTE(__type, __safe_trusted)
 
 /*
@@ -5394,10 +5395,31 @@ BTF_TYPE_SAFE_RCU(struct task_struct) {
 	struct task_struct *group_leader;
 };
 
+BTF_TYPE_SAFE_RCU(struct cgroup) {
+	/* cgrp->kn is always accessible as documented in kernel/cgroup/cgroup.c */
+	struct kernfs_node *kn;
+};
+
 BTF_TYPE_SAFE_RCU(struct css_set) {
 	struct cgroup *dfl_cgrp;
 };
 
+/* RCU trusted: these fields are trusted in RCU CS and can be NULL */
+BTF_TYPE_SAFE_RCU_OR_NULL(struct mm_struct) {
+	struct file __rcu *exe_file;
+};
+
+/* skb->sk, req->sk are not RCU protected, but we mark them as such
+ * because bpf prog accessible sockets are SOCK_RCU_FREE.
+ */
+BTF_TYPE_SAFE_RCU_OR_NULL(struct sk_buff) {
+	struct sock *sk;
+};
+
+BTF_TYPE_SAFE_RCU_OR_NULL(struct request_sock) {
+	struct sock *sk;
+};
+
 /* full trusted: these fields are trusted even outside of RCU CS and never NULL */
 BTF_TYPE_SAFE_TRUSTED(struct bpf_iter_meta) {
 	struct seq_file *seq;
@@ -5430,11 +5452,23 @@ static bool type_is_rcu(struct bpf_verifier_env *env,
 			const char *field_name, u32 btf_id)
 {
 	BTF_TYPE_EMIT(BTF_TYPE_SAFE_RCU(struct task_struct));
+	BTF_TYPE_EMIT(BTF_TYPE_SAFE_RCU(struct cgroup));
 	BTF_TYPE_EMIT(BTF_TYPE_SAFE_RCU(struct css_set));
 
 	return btf_nested_type_is_trusted(&env->log, reg, field_name, btf_id, "__safe_rcu");
 }
 
+static bool type_is_rcu_or_null(struct bpf_verifier_env *env,
+				struct bpf_reg_state *reg,
+				const char *field_name, u32 btf_id)
+{
+	BTF_TYPE_EMIT(BTF_TYPE_SAFE_RCU_OR_NULL(struct mm_struct));
+	BTF_TYPE_EMIT(BTF_TYPE_SAFE_RCU_OR_NULL(struct sk_buff));
+	BTF_TYPE_EMIT(BTF_TYPE_SAFE_RCU_OR_NULL(struct request_sock));
+
+	return btf_nested_type_is_trusted(&env->log, reg, field_name, btf_id, "__safe_rcu_or_null");
+}
+
 static bool type_is_trusted(struct bpf_verifier_env *env,
 			    struct bpf_reg_state *reg,
 			    const char *field_name, u32 btf_id)
@@ -5561,9 +5595,10 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 			if (type_is_rcu(env, reg, field_name, btf_id)) {
 				/* ignore __rcu tag and mark it MEM_RCU */
 				flag |= MEM_RCU;
-			} else if (flag & MEM_RCU) {
+			} else if (flag & MEM_RCU ||
+				   type_is_rcu_or_null(env, reg, field_name, btf_id)) {
 				/* __rcu tagged pointers can be NULL */
-				flag |= PTR_MAYBE_NULL;
+				flag |= MEM_RCU | PTR_MAYBE_NULL;
 			} else if (flag & (MEM_PERCPU | MEM_USER)) {
 				/* keep as-is */
 			} else {
-- 
2.34.1

[PATCH bpf-next 7/8] bpf: Undo strict enforcement for walking untagged fields.

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

The commit 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the verifier.")
broke several tracing bpf programs. Even in clang compiled kernels there are
many fields that are not marked with __rcu that are safe to read and pass into
helpers, but the verifier doesn't know that they're safe. Aggressively marking
them as PTR_UNTRUSTED was premature.

Fixes: 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the verifier.")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index fd90ba498ccc..56f569811f70 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4974,6 +4974,11 @@ static bool is_rcu_reg(const struct bpf_reg_state *reg)
 	return reg->type & MEM_RCU;
 }
 
+static void clear_trusted_flags(enum bpf_type_flag *flag)
+{
+	*flag &= ~(BPF_REG_TRUSTED_MODIFIERS | MEM_RCU);
+}
+
 static int check_pkt_ptr_alignment(struct bpf_verifier_env *env,
 				   const struct bpf_reg_state *reg,
 				   int off, int size, bool strict)
@@ -5602,8 +5607,8 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 			} else if (flag & (MEM_PERCPU | MEM_USER)) {
 				/* keep as-is */
 			} else {
-				/* walking unknown pointers yields untrusted pointer */
-				flag = PTR_UNTRUSTED;
+				/* walking unknown pointers yields old deprecated PTR_TO_BTF_ID */
+				clear_trusted_flags(&flag);
 			}
 		} else {
 			/*
@@ -5617,7 +5622,7 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 		}
 	} else {
 		/* Old compat. Deprecated */
-		flag &= ~PTR_TRUSTED;
+		clear_trusted_flags(&flag);
 	}
 
 	if (atype == BPF_READ && value_regno >= 0)
-- 
2.34.1

[PATCH bpf-next 8/8] selftests/bpf: Add tracing tests for walking skb and req.

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

Add tracing tests for walking skb->sk and req->sk.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 .../bpf/progs/test_sk_storage_tracing.c          | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/test_sk_storage_tracing.c b/tools/testing/selftests/bpf/progs/test_sk_storage_tracing.c
index 6dc1f28fc4b6..02e718f06e0f 100644
--- a/tools/testing/selftests/bpf/progs/test_sk_storage_tracing.c
+++ b/tools/testing/selftests/bpf/progs/test_sk_storage_tracing.c
@@ -92,4 +92,20 @@ int BPF_PROG(inet_csk_accept, struct sock *sk, int flags, int *err, bool kern,
 	return 0;
 }
 
+SEC("tp_btf/tcp_retransmit_synack")
+int BPF_PROG(tcp_retransmit_synack, struct sock* sk, struct request_sock* req)
+{
+	/* load only test */
+	bpf_sk_storage_get(&sk_stg_map, sk, 0, 0);
+	bpf_sk_storage_get(&sk_stg_map, req->sk, 0, 0);
+	return 0;
+}
+
+SEC("tp_btf/tcp_bad_csum")
+int BPF_PROG(tcp_bad_csum, struct sk_buff* skb)
+{
+	bpf_sk_storage_get(&sk_stg_map, skb->sk, 0, 0);
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.34.1

Re: [PATCH bpf-next 4/8] bpf: Teach verifier that certain helpers accept NULL pointer.

[David Vernet]

On Mon, Apr 03, 2023 at 09:50:25PM -0700, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> bpf_[sk|inode|task|cgrp]_storage_[get|delete]() and bpf_get_socket_cookie() helpers
> perform run-time check that sk|inode|task|cgrp pointer != NULL.
> Teach verifier about this fact and allow bpf programs to pass
> PTR_TO_BTF_ID | PTR_MAYBE_NULL into such helpers.
> It will be used in the subsequent patch that will do
> bpf_sk_storage_get(.., skb->sk, ...);
> Even when 'skb' pointer is trusted the 'sk' pointer may be NULL.
> 
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  kernel/bpf/bpf_cgrp_storage.c  | 4 ++--
>  kernel/bpf/bpf_inode_storage.c | 4 ++--
>  kernel/bpf/bpf_task_storage.c  | 8 ++++----
>  net/core/bpf_sk_storage.c      | 4 ++--
>  net/core/filter.c              | 2 +-
>  5 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/bpf/bpf_cgrp_storage.c b/kernel/bpf/bpf_cgrp_storage.c
> index d17d5b694668..d44fe8dd9732 100644
> --- a/kernel/bpf/bpf_cgrp_storage.c
> +++ b/kernel/bpf/bpf_cgrp_storage.c
> @@ -224,7 +224,7 @@ const struct bpf_func_proto bpf_cgrp_storage_get_proto = {
>  	.gpl_only	= false,
>  	.ret_type	= RET_PTR_TO_MAP_VALUE_OR_NULL,
>  	.arg1_type	= ARG_CONST_MAP_PTR,
> -	.arg2_type	= ARG_PTR_TO_BTF_ID,
> +	.arg2_type	= ARG_PTR_TO_BTF_ID_OR_NULL,
>  	.arg2_btf_id	= &bpf_cgroup_btf_id[0],
>  	.arg3_type	= ARG_PTR_TO_MAP_VALUE_OR_NULL,
>  	.arg4_type	= ARG_ANYTHING,
> @@ -235,6 +235,6 @@ const struct bpf_func_proto bpf_cgrp_storage_delete_proto = {
>  	.gpl_only	= false,
>  	.ret_type	= RET_INTEGER,
>  	.arg1_type	= ARG_CONST_MAP_PTR,
> -	.arg2_type	= ARG_PTR_TO_BTF_ID,
> +	.arg2_type	= ARG_PTR_TO_BTF_ID_OR_NULL,
>  	.arg2_btf_id	= &bpf_cgroup_btf_id[0],
>  };
> diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
> index e17ad581b9be..a4d93df78c75 100644
> --- a/kernel/bpf/bpf_inode_storage.c
> +++ b/kernel/bpf/bpf_inode_storage.c
> @@ -229,7 +229,7 @@ const struct bpf_func_proto bpf_inode_storage_get_proto = {
>  	.gpl_only	= false,
>  	.ret_type	= RET_PTR_TO_MAP_VALUE_OR_NULL,
>  	.arg1_type	= ARG_CONST_MAP_PTR,
> -	.arg2_type	= ARG_PTR_TO_BTF_ID,
> +	.arg2_type	= ARG_PTR_TO_BTF_ID_OR_NULL,
>  	.arg2_btf_id	= &bpf_inode_storage_btf_ids[0],
>  	.arg3_type	= ARG_PTR_TO_MAP_VALUE_OR_NULL,
>  	.arg4_type	= ARG_ANYTHING,
> @@ -240,6 +240,6 @@ const struct bpf_func_proto bpf_inode_storage_delete_proto = {
>  	.gpl_only	= false,
>  	.ret_type	= RET_INTEGER,
>  	.arg1_type	= ARG_CONST_MAP_PTR,
> -	.arg2_type	= ARG_PTR_TO_BTF_ID,
> +	.arg2_type	= ARG_PTR_TO_BTF_ID_OR_NULL,
>  	.arg2_btf_id	= &bpf_inode_storage_btf_ids[0],
>  };
> diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c
> index d1af0c8f9ce4..adf6dfe0ba68 100644
> --- a/kernel/bpf/bpf_task_storage.c
> +++ b/kernel/bpf/bpf_task_storage.c
> @@ -338,7 +338,7 @@ const struct bpf_func_proto bpf_task_storage_get_recur_proto = {
>  	.gpl_only = false,
>  	.ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
>  	.arg1_type = ARG_CONST_MAP_PTR,
> -	.arg2_type = ARG_PTR_TO_BTF_ID,
> +	.arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
>  	.arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
>  	.arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
>  	.arg4_type = ARG_ANYTHING,
> @@ -349,7 +349,7 @@ const struct bpf_func_proto bpf_task_storage_get_proto = {
>  	.gpl_only = false,
>  	.ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
>  	.arg1_type = ARG_CONST_MAP_PTR,
> -	.arg2_type = ARG_PTR_TO_BTF_ID,
> +	.arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
>  	.arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
>  	.arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
>  	.arg4_type = ARG_ANYTHING,
> @@ -360,7 +360,7 @@ const struct bpf_func_proto bpf_task_storage_delete_recur_proto = {
>  	.gpl_only = false,
>  	.ret_type = RET_INTEGER,
>  	.arg1_type = ARG_CONST_MAP_PTR,
> -	.arg2_type = ARG_PTR_TO_BTF_ID,
> +	.arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
>  	.arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
>  };
>  
> @@ -369,6 +369,6 @@ const struct bpf_func_proto bpf_task_storage_delete_proto = {
>  	.gpl_only = false,
>  	.ret_type = RET_INTEGER,
>  	.arg1_type = ARG_CONST_MAP_PTR,
> -	.arg2_type = ARG_PTR_TO_BTF_ID,
> +	.arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
>  	.arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
>  };
> diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> index 085025c7130a..d4172534dfa8 100644
> --- a/net/core/bpf_sk_storage.c
> +++ b/net/core/bpf_sk_storage.c
> @@ -412,7 +412,7 @@ const struct bpf_func_proto bpf_sk_storage_get_tracing_proto = {
>  	.gpl_only	= false,
>  	.ret_type	= RET_PTR_TO_MAP_VALUE_OR_NULL,
>  	.arg1_type	= ARG_CONST_MAP_PTR,
> -	.arg2_type	= ARG_PTR_TO_BTF_ID,
> +	.arg2_type	= ARG_PTR_TO_BTF_ID_OR_NULL,
>  	.arg2_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
>  	.arg3_type	= ARG_PTR_TO_MAP_VALUE_OR_NULL,
>  	.arg4_type	= ARG_ANYTHING,
> @@ -424,7 +424,7 @@ const struct bpf_func_proto bpf_sk_storage_delete_tracing_proto = {
>  	.gpl_only	= false,
>  	.ret_type	= RET_INTEGER,
>  	.arg1_type	= ARG_CONST_MAP_PTR,
> -	.arg2_type	= ARG_PTR_TO_BTF_ID,
> +	.arg2_type	= ARG_PTR_TO_BTF_ID_OR_NULL,
>  	.arg2_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
>  	.allowed	= bpf_sk_storage_tracing_allowed,
>  };

Should we also add PTR_MAYBE_NULL to the ARG_PTR_TO_BTF_ID_SOCK_COMMON
arg in bpf_sk_storage_get_proto and bpf_sk_storage_delete_proto?

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 1f2abf0f60e6..727c5269867d 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4998,7 +4998,7 @@ const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto = {
>  	.func		= bpf_get_socket_ptr_cookie,
>  	.gpl_only	= false,
>  	.ret_type	= RET_INTEGER,
> -	.arg1_type	= ARG_PTR_TO_BTF_ID_SOCK_COMMON,
> +	.arg1_type	= ARG_PTR_TO_BTF_ID_SOCK_COMMON | PTR_MAYBE_NULL,
>  };
>  
>  BPF_CALL_1(bpf_get_socket_cookie_sock_ops, struct bpf_sock_ops_kern *, ctx)
> -- 
> 2.34.1
> 

Re: [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[David Vernet]

On Mon, Apr 03, 2023 at 09:50:21PM -0700, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> The patch set is addressing a fallout from
> commit 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the verifier.")
> It was too aggressive with PTR_UNTRUSTED marks.
> Patches 1-6 are cleanup and adding verifier smartness to address real
> use cases in bpf programs that broke with too aggressive PTR_UNTRUSTED.
> The partial revert is done in patch 7 anyway.
> 
> Alexei Starovoitov (8):
>   bpf: Invoke btf_struct_access() callback only for writes.
>   bpf: Remove unused arguments from btf_struct_access().
>   bpf: Refactor btf_nested_type_is_trusted().
>   bpf: Teach verifier that certain helpers accept NULL pointer.
>   bpf: Refactor NULL-ness check in check_reg_type().
>   bpf: Allowlist few fields similar to __rcu tag.
>   bpf: Undo strict enforcement for walking untagged fields.
>   selftests/bpf: Add tracing tests for walking skb and req.

For whole series:

Acked-by: David Vernet <void@manifault.com>

I left one comment on 4/8 in [0], but it's not a blocker and everything
else LGTM.

[0]: https://lore.kernel.org/all/20230404144652.GA3896@maniforge/

Re: [PATCH bpf-next 4/8] bpf: Teach verifier that certain helpers accept NULL pointer.

[Alexei Starovoitov]

On Tue, Apr 4, 2023 at 7:46 AM David Vernet <void@manifault.com> wrote:
>
> On Mon, Apr 03, 2023 at 09:50:25PM -0700, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > bpf_[sk|inode|task|cgrp]_storage_[get|delete]() and bpf_get_socket_cookie() helpers
> > perform run-time check that sk|inode|task|cgrp pointer != NULL.
> > Teach verifier about this fact and allow bpf programs to pass
> > PTR_TO_BTF_ID | PTR_MAYBE_NULL into such helpers.
> > It will be used in the subsequent patch that will do
> > bpf_sk_storage_get(.., skb->sk, ...);
> > Even when 'skb' pointer is trusted the 'sk' pointer may be NULL.
> >
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > ---
> >  kernel/bpf/bpf_cgrp_storage.c  | 4 ++--
> >  kernel/bpf/bpf_inode_storage.c | 4 ++--
> >  kernel/bpf/bpf_task_storage.c  | 8 ++++----
> >  net/core/bpf_sk_storage.c      | 4 ++--
> >  net/core/filter.c              | 2 +-
> >  5 files changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/kernel/bpf/bpf_cgrp_storage.c b/kernel/bpf/bpf_cgrp_storage.c
> > index d17d5b694668..d44fe8dd9732 100644
> > --- a/kernel/bpf/bpf_cgrp_storage.c
> > +++ b/kernel/bpf/bpf_cgrp_storage.c
> > @@ -224,7 +224,7 @@ const struct bpf_func_proto bpf_cgrp_storage_get_proto = {
> >       .gpl_only       = false,
> >       .ret_type       = RET_PTR_TO_MAP_VALUE_OR_NULL,
> >       .arg1_type      = ARG_CONST_MAP_PTR,
> > -     .arg2_type      = ARG_PTR_TO_BTF_ID,
> > +     .arg2_type      = ARG_PTR_TO_BTF_ID_OR_NULL,
> >       .arg2_btf_id    = &bpf_cgroup_btf_id[0],
> >       .arg3_type      = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> >       .arg4_type      = ARG_ANYTHING,
> > @@ -235,6 +235,6 @@ const struct bpf_func_proto bpf_cgrp_storage_delete_proto = {
> >       .gpl_only       = false,
> >       .ret_type       = RET_INTEGER,
> >       .arg1_type      = ARG_CONST_MAP_PTR,
> > -     .arg2_type      = ARG_PTR_TO_BTF_ID,
> > +     .arg2_type      = ARG_PTR_TO_BTF_ID_OR_NULL,
> >       .arg2_btf_id    = &bpf_cgroup_btf_id[0],
> >  };
> > diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
> > index e17ad581b9be..a4d93df78c75 100644
> > --- a/kernel/bpf/bpf_inode_storage.c
> > +++ b/kernel/bpf/bpf_inode_storage.c
> > @@ -229,7 +229,7 @@ const struct bpf_func_proto bpf_inode_storage_get_proto = {
> >       .gpl_only       = false,
> >       .ret_type       = RET_PTR_TO_MAP_VALUE_OR_NULL,
> >       .arg1_type      = ARG_CONST_MAP_PTR,
> > -     .arg2_type      = ARG_PTR_TO_BTF_ID,
> > +     .arg2_type      = ARG_PTR_TO_BTF_ID_OR_NULL,
> >       .arg2_btf_id    = &bpf_inode_storage_btf_ids[0],
> >       .arg3_type      = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> >       .arg4_type      = ARG_ANYTHING,
> > @@ -240,6 +240,6 @@ const struct bpf_func_proto bpf_inode_storage_delete_proto = {
> >       .gpl_only       = false,
> >       .ret_type       = RET_INTEGER,
> >       .arg1_type      = ARG_CONST_MAP_PTR,
> > -     .arg2_type      = ARG_PTR_TO_BTF_ID,
> > +     .arg2_type      = ARG_PTR_TO_BTF_ID_OR_NULL,
> >       .arg2_btf_id    = &bpf_inode_storage_btf_ids[0],
> >  };
> > diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c
> > index d1af0c8f9ce4..adf6dfe0ba68 100644
> > --- a/kernel/bpf/bpf_task_storage.c
> > +++ b/kernel/bpf/bpf_task_storage.c
> > @@ -338,7 +338,7 @@ const struct bpf_func_proto bpf_task_storage_get_recur_proto = {
> >       .gpl_only = false,
> >       .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
> >       .arg1_type = ARG_CONST_MAP_PTR,
> > -     .arg2_type = ARG_PTR_TO_BTF_ID,
> > +     .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> >       .arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
> >       .arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> >       .arg4_type = ARG_ANYTHING,
> > @@ -349,7 +349,7 @@ const struct bpf_func_proto bpf_task_storage_get_proto = {
> >       .gpl_only = false,
> >       .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
> >       .arg1_type = ARG_CONST_MAP_PTR,
> > -     .arg2_type = ARG_PTR_TO_BTF_ID,
> > +     .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> >       .arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
> >       .arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> >       .arg4_type = ARG_ANYTHING,
> > @@ -360,7 +360,7 @@ const struct bpf_func_proto bpf_task_storage_delete_recur_proto = {
> >       .gpl_only = false,
> >       .ret_type = RET_INTEGER,
> >       .arg1_type = ARG_CONST_MAP_PTR,
> > -     .arg2_type = ARG_PTR_TO_BTF_ID,
> > +     .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> >       .arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
> >  };
> >
> > @@ -369,6 +369,6 @@ const struct bpf_func_proto bpf_task_storage_delete_proto = {
> >       .gpl_only = false,
> >       .ret_type = RET_INTEGER,
> >       .arg1_type = ARG_CONST_MAP_PTR,
> > -     .arg2_type = ARG_PTR_TO_BTF_ID,
> > +     .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> >       .arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
> >  };
> > diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> > index 085025c7130a..d4172534dfa8 100644
> > --- a/net/core/bpf_sk_storage.c
> > +++ b/net/core/bpf_sk_storage.c
> > @@ -412,7 +412,7 @@ const struct bpf_func_proto bpf_sk_storage_get_tracing_proto = {
> >       .gpl_only       = false,
> >       .ret_type       = RET_PTR_TO_MAP_VALUE_OR_NULL,
> >       .arg1_type      = ARG_CONST_MAP_PTR,
> > -     .arg2_type      = ARG_PTR_TO_BTF_ID,
> > +     .arg2_type      = ARG_PTR_TO_BTF_ID_OR_NULL,
> >       .arg2_btf_id    = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
> >       .arg3_type      = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> >       .arg4_type      = ARG_ANYTHING,
> > @@ -424,7 +424,7 @@ const struct bpf_func_proto bpf_sk_storage_delete_tracing_proto = {
> >       .gpl_only       = false,
> >       .ret_type       = RET_INTEGER,
> >       .arg1_type      = ARG_CONST_MAP_PTR,
> > -     .arg2_type      = ARG_PTR_TO_BTF_ID,
> > +     .arg2_type      = ARG_PTR_TO_BTF_ID_OR_NULL,
> >       .arg2_btf_id    = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
> >       .allowed        = bpf_sk_storage_tracing_allowed,
> >  };
>
> Should we also add PTR_MAYBE_NULL to the ARG_PTR_TO_BTF_ID_SOCK_COMMON
> arg in bpf_sk_storage_get_proto and bpf_sk_storage_delete_proto?

It makes sense. I'd like to do it in the follow up though.
I haven't seen networking progs passing null-able pointer into these helpers.
Only tracing progs do.
I need to craft a test case, etc.
While this set is good to go imo.

Re: [PATCH bpf-next 4/8] bpf: Teach verifier that certain helpers accept NULL pointer.

[David Vernet]

On Tue, Apr 04, 2023 at 01:17:25PM -0700, Alexei Starovoitov wrote:
> On Tue, Apr 4, 2023 at 7:46 AM David Vernet <void@manifault.com> wrote:
> >
> > On Mon, Apr 03, 2023 at 09:50:25PM -0700, Alexei Starovoitov wrote:
> > > From: Alexei Starovoitov <ast@kernel.org>
> > >
> > > bpf_[sk|inode|task|cgrp]_storage_[get|delete]() and bpf_get_socket_cookie() helpers
> > > perform run-time check that sk|inode|task|cgrp pointer != NULL.
> > > Teach verifier about this fact and allow bpf programs to pass
> > > PTR_TO_BTF_ID | PTR_MAYBE_NULL into such helpers.
> > > It will be used in the subsequent patch that will do
> > > bpf_sk_storage_get(.., skb->sk, ...);
> > > Even when 'skb' pointer is trusted the 'sk' pointer may be NULL.
> > >
> > > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > > ---
> > >  kernel/bpf/bpf_cgrp_storage.c  | 4 ++--
> > >  kernel/bpf/bpf_inode_storage.c | 4 ++--
> > >  kernel/bpf/bpf_task_storage.c  | 8 ++++----
> > >  net/core/bpf_sk_storage.c      | 4 ++--
> > >  net/core/filter.c              | 2 +-
> > >  5 files changed, 11 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/kernel/bpf/bpf_cgrp_storage.c b/kernel/bpf/bpf_cgrp_storage.c
> > > index d17d5b694668..d44fe8dd9732 100644
> > > --- a/kernel/bpf/bpf_cgrp_storage.c
> > > +++ b/kernel/bpf/bpf_cgrp_storage.c
> > > @@ -224,7 +224,7 @@ const struct bpf_func_proto bpf_cgrp_storage_get_proto = {
> > >       .gpl_only       = false,
> > >       .ret_type       = RET_PTR_TO_MAP_VALUE_OR_NULL,
> > >       .arg1_type      = ARG_CONST_MAP_PTR,
> > > -     .arg2_type      = ARG_PTR_TO_BTF_ID,
> > > +     .arg2_type      = ARG_PTR_TO_BTF_ID_OR_NULL,
> > >       .arg2_btf_id    = &bpf_cgroup_btf_id[0],
> > >       .arg3_type      = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> > >       .arg4_type      = ARG_ANYTHING,
> > > @@ -235,6 +235,6 @@ const struct bpf_func_proto bpf_cgrp_storage_delete_proto = {
> > >       .gpl_only       = false,
> > >       .ret_type       = RET_INTEGER,
> > >       .arg1_type      = ARG_CONST_MAP_PTR,
> > > -     .arg2_type      = ARG_PTR_TO_BTF_ID,
> > > +     .arg2_type      = ARG_PTR_TO_BTF_ID_OR_NULL,
> > >       .arg2_btf_id    = &bpf_cgroup_btf_id[0],
> > >  };
> > > diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
> > > index e17ad581b9be..a4d93df78c75 100644
> > > --- a/kernel/bpf/bpf_inode_storage.c
> > > +++ b/kernel/bpf/bpf_inode_storage.c
> > > @@ -229,7 +229,7 @@ const struct bpf_func_proto bpf_inode_storage_get_proto = {
> > >       .gpl_only       = false,
> > >       .ret_type       = RET_PTR_TO_MAP_VALUE_OR_NULL,
> > >       .arg1_type      = ARG_CONST_MAP_PTR,
> > > -     .arg2_type      = ARG_PTR_TO_BTF_ID,
> > > +     .arg2_type      = ARG_PTR_TO_BTF_ID_OR_NULL,
> > >       .arg2_btf_id    = &bpf_inode_storage_btf_ids[0],
> > >       .arg3_type      = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> > >       .arg4_type      = ARG_ANYTHING,
> > > @@ -240,6 +240,6 @@ const struct bpf_func_proto bpf_inode_storage_delete_proto = {
> > >       .gpl_only       = false,
> > >       .ret_type       = RET_INTEGER,
> > >       .arg1_type      = ARG_CONST_MAP_PTR,
> > > -     .arg2_type      = ARG_PTR_TO_BTF_ID,
> > > +     .arg2_type      = ARG_PTR_TO_BTF_ID_OR_NULL,
> > >       .arg2_btf_id    = &bpf_inode_storage_btf_ids[0],
> > >  };
> > > diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c
> > > index d1af0c8f9ce4..adf6dfe0ba68 100644
> > > --- a/kernel/bpf/bpf_task_storage.c
> > > +++ b/kernel/bpf/bpf_task_storage.c
> > > @@ -338,7 +338,7 @@ const struct bpf_func_proto bpf_task_storage_get_recur_proto = {
> > >       .gpl_only = false,
> > >       .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
> > >       .arg1_type = ARG_CONST_MAP_PTR,
> > > -     .arg2_type = ARG_PTR_TO_BTF_ID,
> > > +     .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> > >       .arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
> > >       .arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> > >       .arg4_type = ARG_ANYTHING,
> > > @@ -349,7 +349,7 @@ const struct bpf_func_proto bpf_task_storage_get_proto = {
> > >       .gpl_only = false,
> > >       .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
> > >       .arg1_type = ARG_CONST_MAP_PTR,
> > > -     .arg2_type = ARG_PTR_TO_BTF_ID,
> > > +     .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> > >       .arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
> > >       .arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> > >       .arg4_type = ARG_ANYTHING,
> > > @@ -360,7 +360,7 @@ const struct bpf_func_proto bpf_task_storage_delete_recur_proto = {
> > >       .gpl_only = false,
> > >       .ret_type = RET_INTEGER,
> > >       .arg1_type = ARG_CONST_MAP_PTR,
> > > -     .arg2_type = ARG_PTR_TO_BTF_ID,
> > > +     .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> > >       .arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
> > >  };
> > >
> > > @@ -369,6 +369,6 @@ const struct bpf_func_proto bpf_task_storage_delete_proto = {
> > >       .gpl_only = false,
> > >       .ret_type = RET_INTEGER,
> > >       .arg1_type = ARG_CONST_MAP_PTR,
> > > -     .arg2_type = ARG_PTR_TO_BTF_ID,
> > > +     .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> > >       .arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
> > >  };
> > > diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> > > index 085025c7130a..d4172534dfa8 100644
> > > --- a/net/core/bpf_sk_storage.c
> > > +++ b/net/core/bpf_sk_storage.c
> > > @@ -412,7 +412,7 @@ const struct bpf_func_proto bpf_sk_storage_get_tracing_proto = {
> > >       .gpl_only       = false,
> > >       .ret_type       = RET_PTR_TO_MAP_VALUE_OR_NULL,
> > >       .arg1_type      = ARG_CONST_MAP_PTR,
> > > -     .arg2_type      = ARG_PTR_TO_BTF_ID,
> > > +     .arg2_type      = ARG_PTR_TO_BTF_ID_OR_NULL,
> > >       .arg2_btf_id    = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
> > >       .arg3_type      = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> > >       .arg4_type      = ARG_ANYTHING,
> > > @@ -424,7 +424,7 @@ const struct bpf_func_proto bpf_sk_storage_delete_tracing_proto = {
> > >       .gpl_only       = false,
> > >       .ret_type       = RET_INTEGER,
> > >       .arg1_type      = ARG_CONST_MAP_PTR,
> > > -     .arg2_type      = ARG_PTR_TO_BTF_ID,
> > > +     .arg2_type      = ARG_PTR_TO_BTF_ID_OR_NULL,
> > >       .arg2_btf_id    = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
> > >       .allowed        = bpf_sk_storage_tracing_allowed,
> > >  };
> >
> > Should we also add PTR_MAYBE_NULL to the ARG_PTR_TO_BTF_ID_SOCK_COMMON
> > arg in bpf_sk_storage_get_proto and bpf_sk_storage_delete_proto?
> 
> It makes sense. I'd like to do it in the follow up though.
> I haven't seen networking progs passing null-able pointer into these helpers.
> Only tracing progs do.
> I need to craft a test case, etc.

Sounds good

> While this set is good to go imo.

Agreed, the series LGTM.

Re: [PATCH bpf-next 1/8] bpf: Invoke btf_struct_access() callback only for writes.

[Andrii Nakryiko]

On Mon, Apr 3, 2023 at 9:50 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> Remove duplicated if (atype == BPF_READ) btf_struct_access() from
> btf_struct_access() callback and invoke it only for writes.

It would be nice to elaborate a bit why this is ok. As far as I can
tell, it's because custom btf_struct_access() callbacks are only
checking and overriding write accesses, delegating reads to generic
btf_struct_access(). Is that right? If so, can you please note it down
in the commit message?

Further, given btf_struct_access *callbacks* are now write-only, while
we still keep generic btf_struct_access for reads, should we
distinguish callback's write-only nature by renaming it to something
like "btf_struct_write_access"?

>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  kernel/bpf/verifier.c          | 2 +-
>  net/bpf/bpf_dummy_struct_ops.c | 2 +-
>  net/core/filter.c              | 6 ------
>  net/ipv4/bpf_tcp_ca.c          | 3 ---
>  4 files changed, 2 insertions(+), 11 deletions(-)
>

[...]

Re: [PATCH bpf-next 2/8] bpf: Remove unused arguments from btf_struct_access().

[Andrii Nakryiko]

On Mon, Apr 3, 2023 at 9:50 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> Remove unused arguments from btf_struct_access() callback.
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---

expected this change in patch #1, tbh, even wrote a comment about
dropping enum bpf_access_type, but then guessed to check next patch.
Anyways, I'm fine either way, but patch 1 and 2 together make for much
less confusing change, IMO. See also my note about renaming *callback*
to make its write access nature clear.

>  include/linux/bpf.h              |  3 +--
>  include/linux/filter.h           |  3 +--
>  kernel/bpf/verifier.c            |  4 ++--
>  net/bpf/bpf_dummy_struct_ops.c   | 12 +++++-------
>  net/core/filter.c                | 13 +++++--------
>  net/ipv4/bpf_tcp_ca.c            |  3 +--
>  net/netfilter/nf_conntrack_bpf.c |  3 +--
>  7 files changed, 16 insertions(+), 25 deletions(-)
>

[...]

Re: [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[Andrii Nakryiko]

On Tue, Apr 4, 2023 at 7:51 AM David Vernet <void@manifault.com> wrote:
>
> On Mon, Apr 03, 2023 at 09:50:21PM -0700, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > The patch set is addressing a fallout from
> > commit 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the verifier.")
> > It was too aggressive with PTR_UNTRUSTED marks.
> > Patches 1-6 are cleanup and adding verifier smartness to address real
> > use cases in bpf programs that broke with too aggressive PTR_UNTRUSTED.
> > The partial revert is done in patch 7 anyway.
> >
> > Alexei Starovoitov (8):
> >   bpf: Invoke btf_struct_access() callback only for writes.
> >   bpf: Remove unused arguments from btf_struct_access().
> >   bpf: Refactor btf_nested_type_is_trusted().
> >   bpf: Teach verifier that certain helpers accept NULL pointer.
> >   bpf: Refactor NULL-ness check in check_reg_type().
> >   bpf: Allowlist few fields similar to __rcu tag.
> >   bpf: Undo strict enforcement for walking untagged fields.
> >   selftests/bpf: Add tracing tests for walking skb and req.
>
> For whole series:
>
> Acked-by: David Vernet <void@manifault.com>

Added David's acks manually (we really need to teach pw-apply to do
this automatically...) and applied. I've added a single sentence to
patch #1 with why (I think) btf_struct_access() callback
simplification was done, I didn't want to hold the patch set just due
to that, as the rest looked good. But please do consider renaming the
callback to more write-access implying name as a follow up, as current
situation with the same name but different semantics is confusing.

Applied to bpf-next, thanks.

>
> I left one comment on 4/8 in [0], but it's not a blocker and everything
> else LGTM.
>
> [0]: https://lore.kernel.org/all/20230404144652.GA3896@maniforge/

Re: [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Mon,  3 Apr 2023 21:50:21 -0700 you wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> The patch set is addressing a fallout from
> commit 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the verifier.")
> It was too aggressive with PTR_UNTRUSTED marks.
> Patches 1-6 are cleanup and adding verifier smartness to address real
> use cases in bpf programs that broke with too aggressive PTR_UNTRUSTED.
> The partial revert is done in patch 7 anyway.
> 
> [...]

Here is the summary with links:
  - [bpf-next,1/8] bpf: Invoke btf_struct_access() callback only for writes.
    https://git.kernel.org/bpf/bpf-next/c/7d64c5132844
  - [bpf-next,2/8] bpf: Remove unused arguments from btf_struct_access().
    https://git.kernel.org/bpf/bpf-next/c/b7e852a9ec96
  - [bpf-next,3/8] bpf: Refactor btf_nested_type_is_trusted().
    https://git.kernel.org/bpf/bpf-next/c/63260df13965
  - [bpf-next,4/8] bpf: Teach verifier that certain helpers accept NULL pointer.
    https://git.kernel.org/bpf/bpf-next/c/91571a515d1b
  - [bpf-next,5/8] bpf: Refactor NULL-ness check in check_reg_type().
    https://git.kernel.org/bpf/bpf-next/c/add68b843f33
  - [bpf-next,6/8] bpf: Allowlist few fields similar to __rcu tag.
    https://git.kernel.org/bpf/bpf-next/c/30ee9821f943
  - [bpf-next,7/8] bpf: Undo strict enforcement for walking untagged fields.
    https://git.kernel.org/bpf/bpf-next/c/afeebf9f57a4
  - [bpf-next,8/8] selftests/bpf: Add tracing tests for walking skb and req.
    https://git.kernel.org/bpf/bpf-next/c/69f41a787761

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

Re: [PATCH bpf-next 4/8] bpf: Teach verifier that certain helpers accept NULL pointer.

[Martin KaFai Lau]

On 4/3/23 9:50 PM, Alexei Starovoitov wrote:
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 1f2abf0f60e6..727c5269867d 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4998,7 +4998,7 @@ const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto = {
>   	.func		= bpf_get_socket_ptr_cookie,
>   	.gpl_only	= false,
>   	.ret_type	= RET_INTEGER,
> -	.arg1_type	= ARG_PTR_TO_BTF_ID_SOCK_COMMON,
> +	.arg1_type	= ARG_PTR_TO_BTF_ID_SOCK_COMMON | PTR_MAYBE_NULL,

I think the bpf_skc_to_* helpers (eg. bpf_skc_to_tcp_sock) also need similar 
change. They are available to tracing also. It can be a follow-up. The patch set 
lgtm.

Re: [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[Alexei Starovoitov]

On Tue, Apr 4, 2023 at 5:02 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Apr 4, 2023 at 7:51 AM David Vernet <void@manifault.com> wrote:
> >
> > On Mon, Apr 03, 2023 at 09:50:21PM -0700, Alexei Starovoitov wrote:
> > > From: Alexei Starovoitov <ast@kernel.org>
> > >
> > > The patch set is addressing a fallout from
> > > commit 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the verifier.")
> > > It was too aggressive with PTR_UNTRUSTED marks.
> > > Patches 1-6 are cleanup and adding verifier smartness to address real
> > > use cases in bpf programs that broke with too aggressive PTR_UNTRUSTED.
> > > The partial revert is done in patch 7 anyway.
> > >
> > > Alexei Starovoitov (8):
> > >   bpf: Invoke btf_struct_access() callback only for writes.
> > >   bpf: Remove unused arguments from btf_struct_access().
> > >   bpf: Refactor btf_nested_type_is_trusted().
> > >   bpf: Teach verifier that certain helpers accept NULL pointer.
> > >   bpf: Refactor NULL-ness check in check_reg_type().
> > >   bpf: Allowlist few fields similar to __rcu tag.
> > >   bpf: Undo strict enforcement for walking untagged fields.
> > >   selftests/bpf: Add tracing tests for walking skb and req.
> >
> > For whole series:
> >
> > Acked-by: David Vernet <void@manifault.com>
>
> Added David's acks manually (we really need to teach pw-apply to do
> this automatically...) and applied.

+1
I was hoping that patchwork will add this feature eventually,
but it seems faster to hack the pw-apply script instead.

> I've added a single sentence to
> patch #1 with why (I think) btf_struct_access() callback
> simplification was done, I didn't want to hold the patch set just due
> to that, as the rest looked good. But please do consider renaming the
> callback to more write-access implying name as a follow up, as current
> situation with the same name but different semantics is confusing.
>
> Applied to bpf-next, thanks.

Thanks.
Renaming either the btf_struct_access() function or (*btf_struct_access) field
was on the todo list as a potential workaround,
since this name caused a weird issue with clang in LTO build.
For some reason two global symbols were generated.
Yonghong is investigating.

fwiw btf_struct_write_access sounds fine as a new name.

Re: [PATCH bpf-next 4/8] bpf: Teach verifier that certain helpers accept NULL pointer.

[Alexei Starovoitov]

On Tue, Apr 4, 2023 at 5:10 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 4/3/23 9:50 PM, Alexei Starovoitov wrote:
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 1f2abf0f60e6..727c5269867d 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -4998,7 +4998,7 @@ const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto = {
> >       .func           = bpf_get_socket_ptr_cookie,
> >       .gpl_only       = false,
> >       .ret_type       = RET_INTEGER,
> > -     .arg1_type      = ARG_PTR_TO_BTF_ID_SOCK_COMMON,
> > +     .arg1_type      = ARG_PTR_TO_BTF_ID_SOCK_COMMON | PTR_MAYBE_NULL,
>
> I think the bpf_skc_to_* helpers (eg. bpf_skc_to_tcp_sock) also need similar
> change. They are available to tracing also. It can be a follow-up. The patch set
> lgtm.

Ok. I'll take a look.

Re: [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[Jakub Kicinski]

On Tue, 4 Apr 2023 17:16:27 -0700 Alexei Starovoitov wrote:
> > Added David's acks manually (we really need to teach pw-apply to do
> > this automatically...) and applied.  
> 
> +1
> I was hoping that patchwork will add this feature eventually,
> but it seems faster to hack the pw-apply script instead.

pw-apply can kind of do it. It exports an env variable called ADD_TAGS
if it spots any tags in reply to the cover letter.

You need to add a snippet like this to your .git/hooks/applypatch-msg:

  while IFS= read -r tag; do
    echo -e Adding tag: '\e[35m'$tag'\e[0m'
      git interpret-trailers --in-place \
          --if-exists=addIfDifferent \
          --trailer "$tag" \
          "$1"
  done <<< "$ADD_TAGS"

to transfer those tags onto the commits.

Looking at the code you may also need to use -M to get ADD_TAGS
exported. I'm guessing I put this code under -M so that the extra curl
requests don't slow down the script for everyone. But we can probably
"graduate" that into the main body if you find it useful and hate -M :)

Re: [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[Andrii Nakryiko]

On Tue, Apr 4, 2023 at 6:51 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 4 Apr 2023 17:16:27 -0700 Alexei Starovoitov wrote:
> > > Added David's acks manually (we really need to teach pw-apply to do
> > > this automatically...) and applied.
> >
> > +1
> > I was hoping that patchwork will add this feature eventually,
> > but it seems faster to hack the pw-apply script instead.
>
> pw-apply can kind of do it. It exports an env variable called ADD_TAGS
> if it spots any tags in reply to the cover letter.
>
> You need to add a snippet like this to your .git/hooks/applypatch-msg:
>
>   while IFS= read -r tag; do
>     echo -e Adding tag: '\e[35m'$tag'\e[0m'
>       git interpret-trailers --in-place \
>           --if-exists=addIfDifferent \
>           --trailer "$tag" \
>           "$1"
>   done <<< "$ADD_TAGS"
>
> to transfer those tags onto the commits.
>
> Looking at the code you may also need to use -M to get ADD_TAGS
> exported. I'm guessing I put this code under -M so that the extra curl
> requests don't slow down the script for everyone. But we can probably
> "graduate" that into the main body if you find it useful and hate -M :)

So I'm exclusively using `pw-apply -c <patchworks-url>` to apply
everything locally. I'd expect that at this time the script would
detect any Acked-by replies on *cover letter patch*, and apply them
across all patches in the series. Such that we (humans) can look at
them, fix them, add them, etc. Doing something like this in git hook
seems unnecessary?

So I think the only thing that's missing is the code that would fetch
all replies on the cover letter "patch" (e.g., like on [0]) and just
apply it across everything. We must be doing something like this for
acks on individual patches, so I imagine we are not far off to make
this work, but I haven't looked at pw-apply carefully enough to know
for sure.

  [0] https://patchwork.kernel.org/project/netdevbpf/cover/20230404045029.82870-1-alexei.starovoitov@gmail.com/

Re: [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[Jakub Kicinski]

On Wed, 5 Apr 2023 10:22:16 -0700 Andrii Nakryiko wrote:
> So I'm exclusively using `pw-apply -c <patchworks-url>` to apply
> everything locally.

I think you can throw -M after -c $url? It can only help... :)

> I'd expect that at this time the script would
> detect any Acked-by replies on *cover letter patch*, and apply them
> across all patches in the series. Such that we (humans) can look at
> them, fix them, add them, etc. Doing something like this in git hook
> seems unnecessary?

Maybe mb2q can do it, IDK. I don't use the mb2q thing.
I don't think git has a way of doing git am and insert these tags if
they don't exist, in a single command.

> So I think the only thing that's missing is the code that would fetch
> all replies on the cover letter "patch" (e.g., like on [0]) and just
> apply it across everything. We must be doing something like this for
> acks on individual patches, so I imagine we are not far off to make
> this work, but I haven't looked at pw-apply carefully enough to know
> for sure.

The individual patches are handled by patchwork.

Don't get me wrong, I'm not disagreeing with you. Just trying to help
and point out existing workarounds..

Re: [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[Daniel Borkmann]

On 4/5/23 7:22 PM, Andrii Nakryiko wrote:
> On Tue, Apr 4, 2023 at 6:51 PM Jakub Kicinski <kuba@kernel.org> wrote:
>> On Tue, 4 Apr 2023 17:16:27 -0700 Alexei Starovoitov wrote:
>>>> Added David's acks manually (we really need to teach pw-apply to do
>>>> this automatically...) and applied.
>>>
>>> +1
>>> I was hoping that patchwork will add this feature eventually,
>>> but it seems faster to hack the pw-apply script instead.
>>
>> pw-apply can kind of do it. It exports an env variable called ADD_TAGS
>> if it spots any tags in reply to the cover letter.
>>
>> You need to add a snippet like this to your .git/hooks/applypatch-msg:
>>
>>    while IFS= read -r tag; do
>>      echo -e Adding tag: '\e[35m'$tag'\e[0m'
>>        git interpret-trailers --in-place \
>>            --if-exists=addIfDifferent \
>>            --trailer "$tag" \
>>            "$1"
>>    done <<< "$ADD_TAGS"
>>
>> to transfer those tags onto the commits.
>>
>> Looking at the code you may also need to use -M to get ADD_TAGS
>> exported. I'm guessing I put this code under -M so that the extra curl
>> requests don't slow down the script for everyone. But we can probably
>> "graduate" that into the main body if you find it useful and hate -M :)
> 
> So I'm exclusively using `pw-apply -c <patchworks-url>` to apply
> everything locally. I'd expect that at this time the script would
> detect any Acked-by replies on *cover letter patch*, and apply them
> across all patches in the series. Such that we (humans) can look at
> them, fix them, add them, etc. Doing something like this in git hook
> seems unnecessary?
> 
> So I think the only thing that's missing is the code that would fetch
> all replies on the cover letter "patch" (e.g., like on [0]) and just
> apply it across everything. We must be doing something like this for
> acks on individual patches, so I imagine we are not far off to make
> this work, but I haven't looked at pw-apply carefully enough to know
> for sure.

I always use pw-apply based on the mbox, e.g. ...

   pw-apply -b https://patchwork.kernel.org/[...]/mbox/ -m <branch-name> -- -a "Foo Bar <foo@bar.com>"

... still manual, but it will propagate the -a (Acked-by) / -r (Reviewed-by) /
-t (Tested-by) to the individual patches.

Thanks,
Daniel

Re: [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[Andrii Nakryiko]

On Wed, Apr 5, 2023 at 11:19 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 5 Apr 2023 10:22:16 -0700 Andrii Nakryiko wrote:
> > So I'm exclusively using `pw-apply -c <patchworks-url>` to apply
> > everything locally.
>
> I think you can throw -M after -c $url? It can only help... :)
>
> > I'd expect that at this time the script would
> > detect any Acked-by replies on *cover letter patch*, and apply them
> > across all patches in the series. Such that we (humans) can look at
> > them, fix them, add them, etc. Doing something like this in git hook
> > seems unnecessary?
>
> Maybe mb2q can do it, IDK. I don't use the mb2q thing.
> I don't think git has a way of doing git am and insert these tags if
> they don't exist, in a single command.
>
> > So I think the only thing that's missing is the code that would fetch
> > all replies on the cover letter "patch" (e.g., like on [0]) and just
> > apply it across everything. We must be doing something like this for
> > acks on individual patches, so I imagine we are not far off to make
> > this work, but I haven't looked at pw-apply carefully enough to know
> > for sure.
>
> The individual patches are handled by patchwork.
>

Ah, I see, so yeah, it would be harder to do in pw-apply then.

> Don't get me wrong, I'm not disagreeing with you. Just trying to help
> and point out existing workarounds..

Oh, absolutely, I think we are all on the same page here. Daniel
mentioned -a in another reply, I didn't know about that and will use
that for the time being, still will save a bunch of time next time.

When I feel inspired to deal with a bit of bash, curl, and stuff, I'll
see if we can extend `pw-apply -c` to do this automatically. Thanks
for the discussion!

Re: [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[Alexei Starovoitov]

On Wed, Apr 5, 2023 at 11:19 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 5 Apr 2023 10:22:16 -0700 Andrii Nakryiko wrote:
> > So I'm exclusively using `pw-apply -c <patchworks-url>` to apply
> > everything locally.
>
> I think you can throw -M after -c $url? It can only help... :)

Yeah. If only...
I'm exclusively using -c.
-M only works with -s, but I couldn't make -s -M work either.
Do you pass the series as a number?
but then series_json=$(curl -s $srv/series/$1/) line
doesn't look right, since it's missing "/mbox/" ?
User error on my side, I guess.
My bash skills were too weak to make -c and -M work,
but .git/hooks tip is great!
Thank you.

Re: [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[Jakub Kicinski]

On Wed, 5 Apr 2023 22:13:26 -0700 Alexei Starovoitov wrote:
> On Wed, Apr 5, 2023 at 11:19 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Wed, 5 Apr 2023 10:22:16 -0700 Andrii Nakryiko wrote:  
> > > So I'm exclusively using `pw-apply -c <patchworks-url>` to apply
> > > everything locally.  
> >
> > I think you can throw -M after -c $url? It can only help... :)  
> 
> Yeah. If only...
> I'm exclusively using -c.
> -M only works with -s, but I couldn't make -s -M work either.
> Do you pass the series as a number?

Yes, it copy just the numerical ID into the terminal.

> but then series_json=$(curl -s $srv/series/$1/) line
> doesn't look right, since it's missing "/mbox/" ?

That's loading JSON from the patchwork's REST API.

> User error on my side, I guess.
> My bash skills were too weak to make -c and -M work,
> but .git/hooks tip is great!
> Thank you.

FWIW I think the below may work for using -c instead of -s.
But it is mixing "Daniel paths" and "Jakub paths" in the script.
The output is still a bit different than when using -s.

diff --git a/pw-apply b/pw-apply
index 5fc37a24b027..c9cec94a4a8c 100755
--- a/pw-apply
+++ b/pw-apply
@@ -81,17 +81,15 @@ accept_series()
 }
 
 cover_from_url()
 {
   curl -s $1 | gunzip -f -c > tmp.i
-  series_num=`grep "href=\"/series" tmp.i|cut -d/ -f3|head -1`
+  series=`grep "href=\"/series" tmp.i|cut -d/ -f3|head -1`
   cover_url=`grep "href=\"/project/netdevbpf/cover" tmp.i|cut -d\" -f2`
   if [ ! -z "$cover_url" ]; then
-    curl -s https://patchwork.kernel.org${cover_url}mbox/ | gunzip -f -c > cover.i
     merge="1"
   fi
-  curl -s https://patchwork.kernel.org/series/$series_num/mbox/ | gunzip -f -c > mbox.i
 }
 
 edits=""
 am_flags=""
 branch="mbox"
@@ -118,18 +116,20 @@ while true; do
     -h | --help ) usage; break ;;
     -- ) shift; break ;;
     * )  break ;;
   esac
 done
+# Load the info from cover first, it may will populate $series and $merge
+[ ! -z "$cover" ]  && cover_from_url $cover
+
 [ ! -z "$auto_branch" ] && [ -z "$series" ] && usage
 [ ! -z "$mbox" ]   && [ ! -z "$series" ] && usage
 [   -z "$mbox" ]   && [   -z "$series" ] && [ -z "$cover" ] && usage
 [ ! -z "$accept" ] && [ ! -z "$mbox" ]   && usage
 [ ! -z "$series" ] && mbox_from_series $series
 [ ! -z "$mbox" ]   && mbox_from_url $mbox
 [ ! -z "$accept" ] && accept_series $series
-[ ! -z "$cover" ]  && cover_from_url $cover
 
 target=$(git branch --show-current)
 
 body=
 author=XYZ

Re: [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[Alexei Starovoitov]

On Thu, Apr 6, 2023 at 8:42 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 5 Apr 2023 22:13:26 -0700 Alexei Starovoitov wrote:
> > On Wed, Apr 5, 2023 at 11:19 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > On Wed, 5 Apr 2023 10:22:16 -0700 Andrii Nakryiko wrote:
> > > > So I'm exclusively using `pw-apply -c <patchworks-url>` to apply
> > > > everything locally.
> > >
> > > I think you can throw -M after -c $url? It can only help... :)
> >
> > Yeah. If only...
> > I'm exclusively using -c.
> > -M only works with -s, but I couldn't make -s -M work either.
> > Do you pass the series as a number?
>
> Yes, it copy just the numerical ID into the terminal.
>
> > but then series_json=$(curl -s $srv/series/$1/) line
> > doesn't look right, since it's missing "/mbox/" ?
>
> That's loading JSON from the patchwork's REST API.

This line still doesn't work for me.
curl -s https://patchwork.kernel.org/series/736654/
returns:
The page URL requested (<code>/series/736654/</code>) does not exist.

while
curl -s https://patchwork.kernel.org/series/736654/mbox/
returns proper mbox format.

> > User error on my side, I guess.
> > My bash skills were too weak to make -c and -M work,
> > but .git/hooks tip is great!
> > Thank you.
>
> FWIW I think the below may work for using -c instead of -s.
> But it is mixing "Daniel paths" and "Jakub paths" in the script.
> The output is still a bit different than when using -s.
>
> diff --git a/pw-apply b/pw-apply
> index 5fc37a24b027..c9cec94a4a8c 100755
> --- a/pw-apply
> +++ b/pw-apply
> @@ -81,17 +81,15 @@ accept_series()
>  }
>
>  cover_from_url()
>  {
>    curl -s $1 | gunzip -f -c > tmp.i
> -  series_num=`grep "href=\"/series" tmp.i|cut -d/ -f3|head -1`
> +  series=`grep "href=\"/series" tmp.i|cut -d/ -f3|head -1`
>    cover_url=`grep "href=\"/project/netdevbpf/cover" tmp.i|cut -d\" -f2`
>    if [ ! -z "$cover_url" ]; then
> -    curl -s https://patchwork.kernel.org${cover_url}mbox/ | gunzip -f -c > cover.i
>      merge="1"
>    fi
> -  curl -s https://patchwork.kernel.org/series/$series_num/mbox/ | gunzip -f -c > mbox.i
>  }
>
>  edits=""
>  am_flags=""
>  branch="mbox"
> @@ -118,18 +116,20 @@ while true; do
>      -h | --help ) usage; break ;;
>      -- ) shift; break ;;
>      * )  break ;;
>    esac
>  done
> +# Load the info from cover first, it may will populate $series and $merge
> +[ ! -z "$cover" ]  && cover_from_url $cover
> +
>  [ ! -z "$auto_branch" ] && [ -z "$series" ] && usage
>  [ ! -z "$mbox" ]   && [ ! -z "$series" ] && usage
>  [   -z "$mbox" ]   && [   -z "$series" ] && [ -z "$cover" ] && usage
>  [ ! -z "$accept" ] && [ ! -z "$mbox" ]   && usage
>  [ ! -z "$series" ] && mbox_from_series $series
>  [ ! -z "$mbox" ]   && mbox_from_url $mbox
>  [ ! -z "$accept" ] && accept_series $series
> -[ ! -z "$cover" ]  && cover_from_url $cover

This part completely makes sense! Would be great to converge
on common usage, but json fetch doesn't work for me for some reason.
While mbox via original -c works fine.

Re: [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[Jakub Kicinski]

On Thu, 6 Apr 2023 18:17:57 -0700 Alexei Starovoitov wrote:
> On Thu, Apr 6, 2023 at 8:42 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > Yeah. If only...
> > > I'm exclusively using -c.
> > > -M only works with -s, but I couldn't make -s -M work either.
> > > Do you pass the series as a number?  
> >
> > Yes, it copy just the numerical ID into the terminal.
> >  
> > > but then series_json=$(curl -s $srv/series/$1/) line
> > > doesn't look right, since it's missing "/mbox/" ?  
> >
> > That's loading JSON from the patchwork's REST API.  
> 
> This line still doesn't work for me.
> curl -s https://patchwork.kernel.org/series/736654/
> returns:
> The page URL requested (<code>/series/736654/</code>) does not exist.
> 
> while
> curl -s https://patchwork.kernel.org/series/736654/mbox/
> returns proper mbox format.

Check if your git config is right:

$ git config --get pw.server
https://patchwork.kernel.org/api/1.1/

that's where $srv comes from

Re: [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[Alexei Starovoitov]

On Thu, Apr 6, 2023 at 6:23 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 6 Apr 2023 18:17:57 -0700 Alexei Starovoitov wrote:
> > On Thu, Apr 6, 2023 at 8:42 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > > Yeah. If only...
> > > > I'm exclusively using -c.
> > > > -M only works with -s, but I couldn't make -s -M work either.
> > > > Do you pass the series as a number?
> > >
> > > Yes, it copy just the numerical ID into the terminal.
> > >
> > > > but then series_json=$(curl -s $srv/series/$1/) line
> > > > doesn't look right, since it's missing "/mbox/" ?
> > >
> > > That's loading JSON from the patchwork's REST API.
> >
> > This line still doesn't work for me.
> > curl -s https://patchwork.kernel.org/series/736654/
> > returns:
> > The page URL requested (<code>/series/736654/</code>) does not exist.
> >
> > while
> > curl -s https://patchwork.kernel.org/series/736654/mbox/
> > returns proper mbox format.
>
> Check if your git config is right:
>
> $ git config --get pw.server
> https://patchwork.kernel.org/api/1.1/
>
> that's where $srv comes from

Ahh. All works now!
I like the new output.
I'll play with it more.
Should -M be a default? Any downside?

Re: [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier.

[Jakub Kicinski]

On Thu, 6 Apr 2023 18:32:33 -0700 Alexei Starovoitov wrote:
> > Check if your git config is right:
> >
> > $ git config --get pw.server
> > https://patchwork.kernel.org/api/1.1/
> >
> > that's where $srv comes from  
> 
> Ahh. All works now!
> I like the new output.
> I'll play with it more.
> Should -M be a default? Any downside?

There should be no difference, AFAICT. I'm happy with making it 
the default.

There's a minor difference in the merge-message formatting between
-c and -s we could possibly remove if we make -M the default. 
Daniel uses the subject of the series as a fake branch name on the
 
  Merge branch '$branch_name'

line, while I convert the subject to a format which can be a real
branch name in git (no spaces, special chars etc) and put the subject
as the first line of the merge text.