[PATCH bpf v3 0/3] Fix for raw_tp PTR_MAYBE_NULL handling

[PATCH bpf v3 0/3] Fix for raw_tp PTR_MAYBE_NULL handling

[Kumar Kartikeya Dwivedi]

A production BPF program had the following code produced by LLVM.

r0 = 1024;
r1 = ...; // r1 = trusted_or_null_(id=1)
r3 = r1;  // r3 = trusted_or_null_(id=1) r1 = trusted_or_null_(id=1)
r3 += r0; // r3 = trusted_or_null_(id=1, off=1024)
if r1 == 0 goto pc+X;

After cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL"),
the production BPF program began throwing a warning in the verifier
because for the code above, when unmarking null mark from r1, the
verifier will notice another register r3 with same id but off != 0,
which is unexpected, since offset modification on PTR_MAYBE_NULL is not
permitted, but the aforementioned commit relaxed that restriction to
preserve compatibility with non-NULL raw_tp args.

Another production program hit a case where generic code it was calling
into would perform a NULL check, while the program knows and is written
with the knowledge that the raw_tp arg can never be NULL.

In earlier versions before the raw_tp change, verifier would never walk
the path where raw_tp arg was seen as scalar zero, but now it will,
hence code in the program that operates on the raw_tp arg later on will
fail on dereferencing a scalar.

Provide a fix to suppress the warning for raw_tp args, and not mark NULL
checked raw_tp args as scalars. We will follow up with a more generic
fix to handle such patterns for all pointer types in the verifier, which
currently involves playing whack-a-mole with suppressing such LLVM
optimizations and reworking BPF programs to avoid verifier errors.

Changelog:
----------
v2 -> v3
v2: https://lore.kernel.org/bpf/20241205223152.2434683-1-memxor@gmail.com

 * Add Acked-by for Patch 1
 * Add fix for scalar dereference issue
 * Roll both fixes into one, as second fix undoes first
 * Fix nits

v1 -> v2
v1: https://lore.kernel.org/bpf/20241204024154.21386-1-memxor@gmail.com

 * Fix eager unmarking bug (Eduard)
 * Generalize approach, always unmark NULL when off == 0 is checked
 * Make NULL check noop if operand has off != 0
 * Do not reset id when treating as noop
 * Trim comment (Alexei)
 * Adjust selftests

Kumar Kartikeya Dwivedi (3):
  bpf: Suppress warning for non-zero off raw_tp arg NULL check
  bpf: Do not mark NULL-checked raw_tp arg as scalar
  selftests/bpf: Add raw_tp tests for PTR_MAYBE_NULL marking

 kernel/bpf/verifier.c                         | 44 +++++++--
 .../selftests/bpf/prog_tests/raw_tp_null.c    |  6 ++
 .../selftests/bpf/progs/raw_tp_null_fail.c    | 90 +++++++++++++++++++
 3 files changed, 133 insertions(+), 7 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/raw_tp_null_fail.c


base-commit: 5a6ea7022ff4d2a65ae328619c586d6a8909b48b
-- 
2.43.5

[PATCH bpf v3 1/3] bpf: Suppress warning for non-zero off raw_tp arg NULL check

[Kumar Kartikeya Dwivedi]

The fixed commit began marking raw_tp arguments as PTR_MAYBE_NULL to
avoid dead code elimination in the verifier, since raw_tp arguments
may actually be NULL at runtime. However, to preserve compatibility,
it simulated the raw_tp accesses as if the NULL marking was not present.

One of the behaviors permitted by this simulation is offset modification
for NULL pointers. Typically, this pattern is rejected by the verifier,
and users make workarounds to prevent the compiler from producing such
patterns. However, now that it is allowed, when the compiler emits such
code, the offset modification is allowed and a PTR_MAYBE_NULL raw_tp arg
with non-zero off can be formed.

The failing example program had the following pseudo-code:

r0 = 1024;
r1 = ...; // r1 = trusted_or_null_(id=1)
r3 = r1;  // r3 = trusted_or_null_(id=1) r1 = trusted_or_null_(id=1)
r3 += r0; // r3 = trusted_or_null_(id=1, off=1024)
if r1 == 0 goto pc+X;

At this point, while mark_ptr_or_null_reg will see PTR_MAYBE_NULL and
off == 0 for r1, it will notice non-zero off for r3, and the
WARN_ON_ONCE will fire, as the condition checks excluding register types
do not include raw_tp argument type.

This is a pattern produced by LLVM, therefore it is hard to suppress it
everywhere in BPF programs.

The right "generic" fix for this issue in general, will be permitting
offset modification for PTR_MAYBE_NULL pointers everywhere, and
enforcing that the instruction operand of a conditional jump has the
offset as zero. It's other copies may still have non-zero offset, and
that is fine. But this is more involved and will take longer to
integrate.

If a zero offset pointer is NULL checked, all copies can be marked
non-NULL, while checking non-zero offset PTR_MAYBE_NULL is a no-op.

For now, only make this change for raw_tp arguments, and table the
generic fix for later.

Dereferencing such pointers will still work as the fixed commit allowed
it for raw_tp args.

Fixes: cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
Reported-by: Manu Bretelle <chantra@meta.com>
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/verifier.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2fd35465d650..82f40d63ad7b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -15340,7 +15340,8 @@ static int reg_set_min_max(struct bpf_verifier_env *env,
 	return err;
 }
 
-static void mark_ptr_or_null_reg(struct bpf_func_state *state,
+static void mark_ptr_or_null_reg(struct bpf_verifier_env *env,
+				 struct bpf_func_state *state,
 				 struct bpf_reg_state *reg, u32 id,
 				 bool is_null)
 {
@@ -15357,7 +15358,9 @@ static void mark_ptr_or_null_reg(struct bpf_func_state *state,
 		 */
 		if (WARN_ON_ONCE(reg->smin_value || reg->smax_value || !tnum_equals_const(reg->var_off, 0)))
 			return;
-		if (!(type_is_ptr_alloc_obj(reg->type) || type_is_non_owning_ref(reg->type)) &&
+		if (!type_is_ptr_alloc_obj(reg->type) &&
+		    !type_is_non_owning_ref(reg->type) &&
+		    !mask_raw_tp_reg_cond(env, reg) &&
 		    WARN_ON_ONCE(reg->off))
 			return;
 
@@ -15390,11 +15393,12 @@ static void mark_ptr_or_null_reg(struct bpf_func_state *state,
 /* The logic is similar to find_good_pkt_pointers(), both could eventually
  * be folded together at some point.
  */
-static void mark_ptr_or_null_regs(struct bpf_verifier_state *vstate, u32 regno,
+static void mark_ptr_or_null_regs(struct bpf_verifier_env *env,
+				  struct bpf_verifier_state *vstate, u32 regno,
 				  bool is_null)
 {
 	struct bpf_func_state *state = vstate->frame[vstate->curframe];
-	struct bpf_reg_state *regs = state->regs, *reg;
+	struct bpf_reg_state *regs = state->regs, *reg = &regs[regno];
 	u32 ref_obj_id = regs[regno].ref_obj_id;
 	u32 id = regs[regno].id;
 
@@ -15405,8 +15409,28 @@ static void mark_ptr_or_null_regs(struct bpf_verifier_state *vstate, u32 regno,
 		 */
 		WARN_ON_ONCE(release_reference_state(state, id));
 
+	/* For raw_tp args, compiler can produce code of the following
+	 * pattern:
+	 * r3 = r1; // r1 = trusted_or_null_(id=1) r3 = trusted_or_null_(id=1)
+	 * r3 += 8; // r3 = trusted_or_null_(id=1,off=8)
+	 * if r1 == 0 goto pc+N; // r1 = trusted_(id=1)
+	 *
+	 * But we musn't remove the or_null mark from r3, as it won't be
+	 * NULL.
+	 *
+	 * Only do unmarking of everything sharing id if operand of NULL check
+	 * has off = 0.
+	 */
+	if (mask_raw_tp_reg_cond(env, reg) && reg->off) {
+		/* We don't reset reg->id back to 0, as it's unexpected
+		 * when PTR_MAYBE_NULL is set. Simply avoid performing
+		 * a walk for other registers with the same id.
+		 */
+		return;
+	}
+
 	bpf_for_each_reg_in_vstate(vstate, state, reg, ({
-		mark_ptr_or_null_reg(state, reg, id, is_null);
+		mark_ptr_or_null_reg(env, state, reg, id, is_null);
 	}));
 }
 
@@ -15832,9 +15856,9 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
 		/* Mark all identical registers in each branch as either
 		 * safe or unknown depending R == 0 or R != 0 conditional.
 		 */
-		mark_ptr_or_null_regs(this_branch, insn->dst_reg,
+		mark_ptr_or_null_regs(env, this_branch, insn->dst_reg,
 				      opcode == BPF_JNE);
-		mark_ptr_or_null_regs(other_branch, insn->dst_reg,
+		mark_ptr_or_null_regs(env, other_branch, insn->dst_reg,
 				      opcode == BPF_JEQ);
 	} else if (!try_match_pkt_pointers(insn, dst_reg, &regs[insn->src_reg],
 					   this_branch, other_branch) &&
-- 
2.43.5

[PATCH bpf v3 2/3] bpf: Do not mark NULL-checked raw_tp arg as scalar

[Kumar Kartikeya Dwivedi]

Commit cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
began marking raw_tp arguments as PTR_MAYBE_NULL, which caused the
values of these pointers in the branch where they are seen to be NULL to
be marked as scalar zero.

In comparison, raw_tp using programs which did the NULL check never
explored the NULL branch, hence successor instructions following the
NULL check always saw a non-NULL raw_tp pointer and performed memory
accesses on it.

To preserve compatibility, we need to leave a NULL raw_tp arg as is,
such that it can be allowed to be dereferenced. Otherwise, the verifer
will notice the dereference of a zero scalar in the path following the
NULL branch and reject existing programs.

This can allow programs that do bogus things with a NULL pointer to be
able to access memory (with PROBE_MEM) correctly, for instance a program
only having:

	if (!skb) __builtin_memcpy(dst, &skb->mark, sizeof(skb->mark));

However, such programs would also not fail on earlier kernels, since the
verifier would simply never explore this branch. Now, it will permit
behavior where such memory is accessed and explore the branch.

The correct way to fix this is to simply introduce the right annotations
per-tracepoint, and remove the masking/unmasking hack, until then the
raw_tp stop-gap allows programs to continue passing without deleting
code that at runtime can cause safety violations.

An implication of this fix, which follows from the way the raw_tp fixes
were implemented, is that all PTR_MAYBE_NULL trusted PTR_TO_BTF_ID are
engulfed by these checks, and PROBE_MEM will apply to all of them, incl.
those coming from helpers with KF_ACQUIRE returning maybe null trusted
pointers. This NULL tagging after this commit will be sticky. Compared
to a solution which only specially tagged raw_tp args with a different
special maybe null tag (like PTR_SOFT_NULL), it's a consequence of
overloading PTR_MAYBE_NULL with this meaning.

Fixes: cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
Reported-by: Manu Bretelle <chantra@meta.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/verifier.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 82f40d63ad7b..556fb609d4a4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -15365,6 +15365,12 @@ static void mark_ptr_or_null_reg(struct bpf_verifier_env *env,
 			return;
 
 		if (is_null) {
+			/* We never mark a raw_tp trusted pointer as scalar, to
+			 * preserve backwards compatibility, instead just leave
+			 * it as is.
+			 */
+			if (mask_raw_tp_reg_cond(env, reg))
+				return;
 			reg->type = SCALAR_VALUE;
 			/* We don't need id and ref_obj_id from this point
 			 * onwards anymore, thus we should better reset it,
-- 
2.43.5

[PATCH bpf v3 3/3] selftests/bpf: Add raw_tp tests for PTR_MAYBE_NULL marking

[Kumar Kartikeya Dwivedi]

Ensure that pointers with off != 0 are never unmarked as PTR_MAYBE_NULL
when doing NULL checks, while pointers that have off == 0 continue to
get unmarked and propagate unmarking to all other registers sharing id.
Lastly, ensure that in the path where pointer is NULL, the unmarking is
not performed for any registers sharing the same id.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 .../selftests/bpf/prog_tests/raw_tp_null.c    |  6 ++
 .../selftests/bpf/progs/raw_tp_null_fail.c    | 90 +++++++++++++++++++
 2 files changed, 96 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/raw_tp_null_fail.c

diff --git a/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c b/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c
index 6fa19449297e..13fcd4c31034 100644
--- a/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c
+++ b/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c
@@ -3,6 +3,12 @@
 
 #include <test_progs.h>
 #include "raw_tp_null.skel.h"
+#include "raw_tp_null_fail.skel.h"
+
+void test_raw_tp_null_fail(void)
+{
+	RUN_TESTS(raw_tp_null_fail);
+}
 
 void test_raw_tp_null(void)
 {
diff --git a/tools/testing/selftests/bpf/progs/raw_tp_null_fail.c b/tools/testing/selftests/bpf/progs/raw_tp_null_fail.c
new file mode 100644
index 000000000000..a87f25ee61ff
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/raw_tp_null_fail.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+
+/* r1 with off=0 is checked, which marks r0 with off=8 as non-null */
+SEC("tp_btf/bpf_testmod_test_raw_tp_null")
+__success
+__log_level(2)
+__msg("3: (07) r0 += 8                       ; R0_w=trusted_ptr_or_null_sk_buff(id=1,off=8)")
+__msg("4: (15) if r1 == 0x0 goto pc+4        ; R1_w=trusted_ptr_sk_buff()")
+__msg("5: (bf) r2 = r0                       ; R0_w=trusted_ptr_sk_buff(off=8)")
+/* For the path where we saw r1 as != NULL, we will see this state */
+__msg("6: (79) r2 = *(u64 *)(r1 +0)          ; R1_w=trusted_ptr_sk_buff()")
+/* In the NULL path, ensure registers are not marked as scalar */
+/* For the path where we saw r1 as NULL, we will see this state */
+__msg("from 4 to 9: R0=trusted_ptr_or_null_sk_buff(id=1,off=8) R1=trusted_ptr_or_null_sk_buff(id=1)")
+__msg("9: (79) r2 = *(u64 *)(r1 +0)          ; R1=trusted_ptr_or_null_sk_buff(id=1)")
+int BPF_PROG(test_raw_tp_null_check_zero_off, struct sk_buff *skb)
+{
+	asm volatile (
+		"r1 = *(u64 *)(r1 +0);			\
+		 r0 = r1;				\
+		 r2 = 0;				\
+		 r0 += 8;				\
+		 if r1 == 0 goto jmp;			\
+		 r2 = r0;				\
+		 r2 = *(u64 *)(r1 +0);			\
+		 r0 = 0;				\
+		 exit;					\
+		 jmp:					\
+		 r2 = *(u64 *)(r1 +0)"
+		::
+		: __clobber_all
+	);
+	return 0;
+}
+
+/* r2 with offset is checked, which won't mark r1 with off=0 as non-NULL */
+SEC("tp_btf/bpf_testmod_test_raw_tp_null")
+__success
+__log_level(2)
+__msg("3: (07) r2 += 8                       ; R2_w=trusted_ptr_or_null_sk_buff(id=1,off=8)")
+__msg("4: (15) if r2 == 0x0 goto pc+1        ; R2_w=trusted_ptr_or_null_sk_buff(id=1,off=8)")
+__msg("5: (bf) r2 = r1                       ; R1_w=trusted_ptr_or_null_sk_buff(id=1)")
+int BPF_PROG(test_raw_tp_null_copy_check_with_off, struct sk_buff *skb)
+{
+	asm volatile (
+		"r1 = *(u64 *)(r1 +0);			\
+		 r2 = r1;				\
+		 r3 = 0;				\
+		 r2 += 8;				\
+		 if r2 == 0 goto jmp2;			\
+		 r2 = r1;				\
+		 jmp2:					"
+		::
+		: __clobber_all
+	);
+	return 0;
+}
+
+/* Ensure state doesn't change for r0 and r1 when performing repeated checks.. */
+SEC("tp_btf/bpf_testmod_test_raw_tp_null")
+__success
+__log_level(2)
+__msg("2: (07) r0 += 8                       ; R0_w=trusted_ptr_or_null_sk_buff(id=1,off=8)")
+__msg("3: (15) if r0 == 0x0 goto pc+3        ; R0_w=trusted_ptr_or_null_sk_buff(id=1,off=8)")
+__msg("4: (15) if r0 == 0x0 goto pc+2        ; R0_w=trusted_ptr_or_null_sk_buff(id=1,off=8)")
+__msg("5: (15) if r0 == 0x0 goto pc+1        ; R0_w=trusted_ptr_or_null_sk_buff(id=1,off=8)")
+__msg("6: (bf) r2 = r1                       ; R1=trusted_ptr_or_null_sk_buff(id=1)")
+int BPF_PROG(test_raw_tp_check_with_off, struct sk_buff *skb)
+{
+	asm volatile (
+		"r1 = *(u64 *)(r1 +0);			\
+		 r0 = r1;				\
+		 r0 += 8;				\
+		 if r0 == 0 goto jmp3;			\
+		 if r0 == 0 goto jmp3;			\
+		 if r0 == 0 goto jmp3;			\
+		 r2 = r1;				\
+		 jmp3:					"
+		::
+		: __clobber_all
+	);
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.43.5

Re: [PATCH bpf v3 2/3] bpf: Do not mark NULL-checked raw_tp arg as scalar

[Alexei Starovoitov]

On Fri, Dec 6, 2024 at 8:11 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> An implication of this fix, which follows from the way the raw_tp fixes
> were implemented, is that all PTR_MAYBE_NULL trusted PTR_TO_BTF_ID are
> engulfed by these checks, and PROBE_MEM will apply to all of them, incl.
> those coming from helpers with KF_ACQUIRE returning maybe null trusted
> pointers. This NULL tagging after this commit will be sticky. Compared
> to a solution which only specially tagged raw_tp args with a different
> special maybe null tag (like PTR_SOFT_NULL), it's a consequence of
> overloading PTR_MAYBE_NULL with this meaning.
>
> Fixes: cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> Reported-by: Manu Bretelle <chantra@meta.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  kernel/bpf/verifier.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 82f40d63ad7b..556fb609d4a4 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -15365,6 +15365,12 @@ static void mark_ptr_or_null_reg(struct bpf_verifier_env *env,
>                         return;
>
>                 if (is_null) {
> +                       /* We never mark a raw_tp trusted pointer as scalar, to
> +                        * preserve backwards compatibility, instead just leave
> +                        * it as is.
> +                        */
> +                       if (mask_raw_tp_reg_cond(env, reg))
> +                               return;

The blast radius is getting too big.
Patch 1 is ok, but here we're doubling down on
the hack in commit
cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")

I think we need to revert the raw_tp masking hack and
go with denylist the way Jiri proposed:
https://lore.kernel.org/bpf/ZrIj9jkXqpKXRuS7@krava/

denylist is certainly less safer and it's a whack-a-mole
comparing to allowlist, but it's much much shorter
according to Jiri's analysis:
https://lore.kernel.org/bpf/Zr3q8ihbe8cUdpfp@krava/

Eduard had an idea how to auto generate such allow/denylist
during the build.
That could be a follow up.

Re: [PATCH bpf v3 2/3] bpf: Do not mark NULL-checked raw_tp arg as scalar

[Kumar Kartikeya Dwivedi]

On Fri, 6 Dec 2024 at 18:59, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Dec 6, 2024 at 8:11 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> >
> > An implication of this fix, which follows from the way the raw_tp fixes
> > were implemented, is that all PTR_MAYBE_NULL trusted PTR_TO_BTF_ID are
> > engulfed by these checks, and PROBE_MEM will apply to all of them, incl.
> > those coming from helpers with KF_ACQUIRE returning maybe null trusted
> > pointers. This NULL tagging after this commit will be sticky. Compared
> > to a solution which only specially tagged raw_tp args with a different
> > special maybe null tag (like PTR_SOFT_NULL), it's a consequence of
> > overloading PTR_MAYBE_NULL with this meaning.
> >
> > Fixes: cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> > Reported-by: Manu Bretelle <chantra@meta.com>
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  kernel/bpf/verifier.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 82f40d63ad7b..556fb609d4a4 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -15365,6 +15365,12 @@ static void mark_ptr_or_null_reg(struct bpf_verifier_env *env,
> >                         return;
> >
> >                 if (is_null) {
> > +                       /* We never mark a raw_tp trusted pointer as scalar, to
> > +                        * preserve backwards compatibility, instead just leave
> > +                        * it as is.
> > +                        */
> > +                       if (mask_raw_tp_reg_cond(env, reg))
> > +                               return;
>
> The blast radius is getting too big.
> Patch 1 is ok, but here we're doubling down on
> the hack in commit
> cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")

There are two concerns:
First, it applies whether or not a register is a raw_tp arg. There is
a way to detect that (with some register state, instead of using a
separate tag).
Second, we treat the program in the == NULL branch as if the pointer
_maybe_ null, and in the != NULL as definitively not NULL.
I don't really see how that's too different, given we already allow direct
access etc. when the pointer is _unchecked_ after entry, and the state
is same as
the case where == NULL branch is explored.

>
> I think we need to revert the raw_tp masking hack and
> go with denylist the way Jiri proposed:
> https://lore.kernel.org/bpf/ZrIj9jkXqpKXRuS7@krava/
>
> denylist is certainly less safer and it's a whack-a-mole
> comparing to allowlist, but it's much much shorter
> according to Jiri's analysis:
> https://lore.kernel.org/bpf/Zr3q8ihbe8cUdpfp@krava/

Ok, let's revert.
Jiri, do you have the diff around for that attempt? Could you post a
revert of the patches and then the diff you shared?
If not, I can carry it as well with the revert, if you share it with
me (keeping the attribution etc.). Either is fine, lmk.

Thanks


>
> Eduard had an idea how to auto generate such allow/denylist
> during the build.
> That could be a follow up.

Re: [PATCH bpf v3 2/3] bpf: Do not mark NULL-checked raw_tp arg as scalar

[Eduard Zingerman]

On Fri, 2024-12-06 at 09:59 -0800, Alexei Starovoitov wrote:
> On Fri, Dec 6, 2024 at 8:11 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > 
> > An implication of this fix, which follows from the way the raw_tp fixes
> > were implemented, is that all PTR_MAYBE_NULL trusted PTR_TO_BTF_ID are
> > engulfed by these checks, and PROBE_MEM will apply to all of them, incl.
> > those coming from helpers with KF_ACQUIRE returning maybe null trusted
> > pointers. This NULL tagging after this commit will be sticky. Compared
> > to a solution which only specially tagged raw_tp args with a different
> > special maybe null tag (like PTR_SOFT_NULL), it's a consequence of
> > overloading PTR_MAYBE_NULL with this meaning.
> > 
> > Fixes: cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> > Reported-by: Manu Bretelle <chantra@meta.com>
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  kernel/bpf/verifier.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 82f40d63ad7b..556fb609d4a4 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -15365,6 +15365,12 @@ static void mark_ptr_or_null_reg(struct bpf_verifier_env *env,
> >                         return;
> > 
> >                 if (is_null) {
> > +                       /* We never mark a raw_tp trusted pointer as scalar, to
> > +                        * preserve backwards compatibility, instead just leave
> > +                        * it as is.
> > +                        */
> > +                       if (mask_raw_tp_reg_cond(env, reg))
> > +                               return;
> 
> The blast radius is getting too big.
> Patch 1 is ok, but here we're doubling down on
> the hack in commit
> cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> 
> I think we need to revert the raw_tp masking hack and
> go with denylist the way Jiri proposed:
> https://lore.kernel.org/bpf/ZrIj9jkXqpKXRuS7@krava/
> 
> denylist is certainly less safer and it's a whack-a-mole
> comparing to allowlist, but it's much much shorter
> according to Jiri's analysis:
> https://lore.kernel.org/bpf/Zr3q8ihbe8cUdpfp@krava/
> 
> Eduard had an idea how to auto generate such allow/denylist
> during the build.
> That could be a follow up.

If the sole goal is to avoid dead code elimination for tracepoint
parameter null check, there might be another hack. Not sure if it was
discussed:
- don't add PTR_MAYBE_NULL (but maybe add a new tag, PTR_SOFT_NULL
  from Kumar's original RFC);
- in is_branch_taken() don't predict anything when tracepoint
  parameters are compared;
- in mark_ptr_or_null_regs() don't propagate null for pointers to
  tracepoint parameters (as in this patch).

Seems pretty confined but can't catch nullable tracepoint parameters
being passed to kfuncs.

Re: [PATCH bpf v3 2/3] bpf: Do not mark NULL-checked raw_tp arg as scalar

[Kumar Kartikeya Dwivedi]

On Fri, 6 Dec 2024 at 19:15, Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Fri, 2024-12-06 at 09:59 -0800, Alexei Starovoitov wrote:
> > On Fri, Dec 6, 2024 at 8:11 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > >
> > > An implication of this fix, which follows from the way the raw_tp fixes
> > > were implemented, is that all PTR_MAYBE_NULL trusted PTR_TO_BTF_ID are
> > > engulfed by these checks, and PROBE_MEM will apply to all of them, incl.
> > > those coming from helpers with KF_ACQUIRE returning maybe null trusted
> > > pointers. This NULL tagging after this commit will be sticky. Compared
> > > to a solution which only specially tagged raw_tp args with a different
> > > special maybe null tag (like PTR_SOFT_NULL), it's a consequence of
> > > overloading PTR_MAYBE_NULL with this meaning.
> > >
> > > Fixes: cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> > > Reported-by: Manu Bretelle <chantra@meta.com>
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > ---
> > >  kernel/bpf/verifier.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 82f40d63ad7b..556fb609d4a4 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -15365,6 +15365,12 @@ static void mark_ptr_or_null_reg(struct bpf_verifier_env *env,
> > >                         return;
> > >
> > >                 if (is_null) {
> > > +                       /* We never mark a raw_tp trusted pointer as scalar, to
> > > +                        * preserve backwards compatibility, instead just leave
> > > +                        * it as is.
> > > +                        */
> > > +                       if (mask_raw_tp_reg_cond(env, reg))
> > > +                               return;
> >
> > The blast radius is getting too big.
> > Patch 1 is ok, but here we're doubling down on
> > the hack in commit
> > cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> >
> > I think we need to revert the raw_tp masking hack and
> > go with denylist the way Jiri proposed:
> > https://lore.kernel.org/bpf/ZrIj9jkXqpKXRuS7@krava/
> >
> > denylist is certainly less safer and it's a whack-a-mole
> > comparing to allowlist, but it's much much shorter
> > according to Jiri's analysis:
> > https://lore.kernel.org/bpf/Zr3q8ihbe8cUdpfp@krava/
> >
> > Eduard had an idea how to auto generate such allow/denylist
> > during the build.
> > That could be a follow up.
>
> If the sole goal is to avoid dead code elimination for tracepoint
> parameter null check, there might be another hack. Not sure if it was
> discussed:
> - don't add PTR_MAYBE_NULL (but maybe add a new tag, PTR_SOFT_NULL
>   from Kumar's original RFC);
> - in is_branch_taken() don't predict anything when tracepoint
>   parameters are compared;
> - in mark_ptr_or_null_regs() don't propagate null for pointers to
>   tracepoint parameters (as in this patch).

That was pretty much the first attempt with a soft null tag, it was a
special tag indicating provenance of the argument, so it only applied
to raw_tp args. I was trying to warn when being passed into helpers
and kfuncs too, but that needs a more complete fix anyway (with
PTR_TO_BTF_ID...) so we decided to address it later.

But at this point I think we'll keep digging ourselves into a deeper
hole the more we try to address it this way.
I think the various issues, breakage, and corner cases are evidence
this is probably not a good approach to take.

Longer term we have to do explicit annotations and know when it is
NULL and not NULL, so it is probably better to bite the bullet now and
do it explicitly.
Once we denylist or handle the ones Jiri found, we can keep discussing
how to keep the list up to date.


>
> Seems pretty confined but can't catch nullable tracepoint parameters
> being passed to kfuncs.
>

Re: [PATCH bpf v3 2/3] bpf: Do not mark NULL-checked raw_tp arg as scalar

[Alexei Starovoitov]

On Fri, Dec 6, 2024 at 10:15 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Fri, 2024-12-06 at 09:59 -0800, Alexei Starovoitov wrote:
> > On Fri, Dec 6, 2024 at 8:11 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > >
> > > An implication of this fix, which follows from the way the raw_tp fixes
> > > were implemented, is that all PTR_MAYBE_NULL trusted PTR_TO_BTF_ID are
> > > engulfed by these checks, and PROBE_MEM will apply to all of them, incl.
> > > those coming from helpers with KF_ACQUIRE returning maybe null trusted
> > > pointers. This NULL tagging after this commit will be sticky. Compared
> > > to a solution which only specially tagged raw_tp args with a different
> > > special maybe null tag (like PTR_SOFT_NULL), it's a consequence of
> > > overloading PTR_MAYBE_NULL with this meaning.
> > >
> > > Fixes: cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> > > Reported-by: Manu Bretelle <chantra@meta.com>
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > ---
> > >  kernel/bpf/verifier.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 82f40d63ad7b..556fb609d4a4 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -15365,6 +15365,12 @@ static void mark_ptr_or_null_reg(struct bpf_verifier_env *env,
> > >                         return;
> > >
> > >                 if (is_null) {
> > > +                       /* We never mark a raw_tp trusted pointer as scalar, to
> > > +                        * preserve backwards compatibility, instead just leave
> > > +                        * it as is.
> > > +                        */
> > > +                       if (mask_raw_tp_reg_cond(env, reg))
> > > +                               return;
> >
> > The blast radius is getting too big.
> > Patch 1 is ok, but here we're doubling down on
> > the hack in commit
> > cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> >
> > I think we need to revert the raw_tp masking hack and
> > go with denylist the way Jiri proposed:
> > https://lore.kernel.org/bpf/ZrIj9jkXqpKXRuS7@krava/
> >
> > denylist is certainly less safer and it's a whack-a-mole
> > comparing to allowlist, but it's much much shorter
> > according to Jiri's analysis:
> > https://lore.kernel.org/bpf/Zr3q8ihbe8cUdpfp@krava/
> >
> > Eduard had an idea how to auto generate such allow/denylist
> > during the build.
> > That could be a follow up.
>
> If the sole goal is to avoid dead code elimination for tracepoint
> parameter null check, there might be another hack. Not sure if it was
> discussed:
> - don't add PTR_MAYBE_NULL (but maybe add a new tag, PTR_SOFT_NULL
>   from Kumar's original RFC);
> - in is_branch_taken() don't predict anything when tracepoint
>   parameters are compared;

this part was discussed, but we didn't realize we need below bit...

> - in mark_ptr_or_null_regs() don't propagate null for pointers to
>   tracepoint parameters (as in this patch).

... and here the 'for tp args' filter is hard to do.
mark_ptr_or_null_regs() is generic. arg vs non-arg is lost long ago.

Re: [PATCH bpf v3 2/3] bpf: Do not mark NULL-checked raw_tp arg as scalar

[Alexei Starovoitov]

On Fri, Dec 6, 2024 at 10:11 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
> > I think we need to revert the raw_tp masking hack and
> > go with denylist the way Jiri proposed:
> > https://lore.kernel.org/bpf/ZrIj9jkXqpKXRuS7@krava/

...

> Jiri, do you have the diff around for that attempt? Could you post a
> revert of the patches and then the diff you shared?

the link above.

Re: [PATCH bpf v3 2/3] bpf: Do not mark NULL-checked raw_tp arg as scalar

[Kumar Kartikeya Dwivedi]

On Fri, 6 Dec 2024 at 19:37, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Dec 6, 2024 at 10:11 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> > > I think we need to revert the raw_tp masking hack and
> > > go with denylist the way Jiri proposed:
> > > https://lore.kernel.org/bpf/ZrIj9jkXqpKXRuS7@krava/
>
> ...
>
> > Jiri, do you have the diff around for that attempt? Could you post a
> > revert of the patches and then the diff you shared?
>
> the link above.

The link only has information about one tracepoint, but further down
the thread Jiri found more examples and the case of IS_ERR.
https://lore.kernel.org/bpf/Zr3q8ihbe8cUdpfp@krava
It would probably be good to add all of them? Or did I misunderstand
and you just add PTR_MAYBE_NULL for the scheduler tracepoint in the
report?

Re: [PATCH bpf v3 2/3] bpf: Do not mark NULL-checked raw_tp arg as scalar

[Kumar Kartikeya Dwivedi]

On Fri, 6 Dec 2024 at 19:37, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Dec 6, 2024 at 10:15 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> >
> > On Fri, 2024-12-06 at 09:59 -0800, Alexei Starovoitov wrote:
> > > On Fri, Dec 6, 2024 at 8:11 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > > >
> > > > An implication of this fix, which follows from the way the raw_tp fixes
> > > > were implemented, is that all PTR_MAYBE_NULL trusted PTR_TO_BTF_ID are
> > > > engulfed by these checks, and PROBE_MEM will apply to all of them, incl.
> > > > those coming from helpers with KF_ACQUIRE returning maybe null trusted
> > > > pointers. This NULL tagging after this commit will be sticky. Compared
> > > > to a solution which only specially tagged raw_tp args with a different
> > > > special maybe null tag (like PTR_SOFT_NULL), it's a consequence of
> > > > overloading PTR_MAYBE_NULL with this meaning.
> > > >
> > > > Fixes: cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> > > > Reported-by: Manu Bretelle <chantra@meta.com>
> > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > > ---
> > > >  kernel/bpf/verifier.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > > index 82f40d63ad7b..556fb609d4a4 100644
> > > > --- a/kernel/bpf/verifier.c
> > > > +++ b/kernel/bpf/verifier.c
> > > > @@ -15365,6 +15365,12 @@ static void mark_ptr_or_null_reg(struct bpf_verifier_env *env,
> > > >                         return;
> > > >
> > > >                 if (is_null) {
> > > > +                       /* We never mark a raw_tp trusted pointer as scalar, to
> > > > +                        * preserve backwards compatibility, instead just leave
> > > > +                        * it as is.
> > > > +                        */
> > > > +                       if (mask_raw_tp_reg_cond(env, reg))
> > > > +                               return;
> > >
> > > The blast radius is getting too big.
> > > Patch 1 is ok, but here we're doubling down on
> > > the hack in commit
> > > cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> > >
> > > I think we need to revert the raw_tp masking hack and
> > > go with denylist the way Jiri proposed:
> > > https://lore.kernel.org/bpf/ZrIj9jkXqpKXRuS7@krava/
> > >
> > > denylist is certainly less safer and it's a whack-a-mole
> > > comparing to allowlist, but it's much much shorter
> > > according to Jiri's analysis:
> > > https://lore.kernel.org/bpf/Zr3q8ihbe8cUdpfp@krava/
> > >
> > > Eduard had an idea how to auto generate such allow/denylist
> > > during the build.
> > > That could be a follow up.
> >
> > If the sole goal is to avoid dead code elimination for tracepoint
> > parameter null check, there might be another hack. Not sure if it was
> > discussed:
> > - don't add PTR_MAYBE_NULL (but maybe add a new tag, PTR_SOFT_NULL
> >   from Kumar's original RFC);
> > - in is_branch_taken() don't predict anything when tracepoint
> >   parameters are compared;
>
> this part was discussed, but we didn't realize we need below bit...
>
> > - in mark_ptr_or_null_regs() don't propagate null for pointers to
> >   tracepoint parameters (as in this patch).
>
> ... and here the 'for tp args' filter is hard to do.
> mark_ptr_or_null_regs() is generic. arg vs non-arg is lost long ago.

It is not lost. If only args are marked PTR_SOFT_NULL or
reg->btf.is_raw_tp_arg (or w/e else), it can still be seen when we are
in that function, and all its copies will have the same information.

Re: [PATCH bpf v3 2/3] bpf: Do not mark NULL-checked raw_tp arg as scalar

[Alexei Starovoitov]

On Fri, Dec 6, 2024 at 11:09 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Fri, 6 Dec 2024 at 19:37, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Dec 6, 2024 at 10:11 AM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > > > I think we need to revert the raw_tp masking hack and
> > > > go with denylist the way Jiri proposed:
> > > > https://lore.kernel.org/bpf/ZrIj9jkXqpKXRuS7@krava/
> >
> > ...
> >
> > > Jiri, do you have the diff around for that attempt? Could you post a
> > > revert of the patches and then the diff you shared?
> >
> > the link above.
>
> The link only has information about one tracepoint, but further down
> the thread Jiri found more examples and the case of IS_ERR.
> https://lore.kernel.org/bpf/Zr3q8ihbe8cUdpfp@krava
> It would probably be good to add all of them? Or did I misunderstand
> and you just add PTR_MAYBE_NULL for the scheduler tracepoint in the
> report?

I mean Jiri's patch fixes one tracepoint, but the same approach
can be used for the rest of tp-s Jiri found in his 2nd link.
IS_ERR can be handled as well as info->reg_type = SCALAR;

So revert plus one patch to fix them all.

Re: [PATCH bpf v3 2/3] bpf: Do not mark NULL-checked raw_tp arg as scalar

[Alexei Starovoitov]

On Fri, Dec 6, 2024 at 11:10 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Fri, 6 Dec 2024 at 19:37, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Dec 6, 2024 at 10:15 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > >
> > > On Fri, 2024-12-06 at 09:59 -0800, Alexei Starovoitov wrote:
> > > > On Fri, Dec 6, 2024 at 8:11 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > > > >
> > > > > An implication of this fix, which follows from the way the raw_tp fixes
> > > > > were implemented, is that all PTR_MAYBE_NULL trusted PTR_TO_BTF_ID are
> > > > > engulfed by these checks, and PROBE_MEM will apply to all of them, incl.
> > > > > those coming from helpers with KF_ACQUIRE returning maybe null trusted
> > > > > pointers. This NULL tagging after this commit will be sticky. Compared
> > > > > to a solution which only specially tagged raw_tp args with a different
> > > > > special maybe null tag (like PTR_SOFT_NULL), it's a consequence of
> > > > > overloading PTR_MAYBE_NULL with this meaning.
> > > > >
> > > > > Fixes: cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> > > > > Reported-by: Manu Bretelle <chantra@meta.com>
> > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > > > ---
> > > > >  kernel/bpf/verifier.c | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > > > index 82f40d63ad7b..556fb609d4a4 100644
> > > > > --- a/kernel/bpf/verifier.c
> > > > > +++ b/kernel/bpf/verifier.c
> > > > > @@ -15365,6 +15365,12 @@ static void mark_ptr_or_null_reg(struct bpf_verifier_env *env,
> > > > >                         return;
> > > > >
> > > > >                 if (is_null) {
> > > > > +                       /* We never mark a raw_tp trusted pointer as scalar, to
> > > > > +                        * preserve backwards compatibility, instead just leave
> > > > > +                        * it as is.
> > > > > +                        */
> > > > > +                       if (mask_raw_tp_reg_cond(env, reg))
> > > > > +                               return;
> > > >
> > > > The blast radius is getting too big.
> > > > Patch 1 is ok, but here we're doubling down on
> > > > the hack in commit
> > > > cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> > > >
> > > > I think we need to revert the raw_tp masking hack and
> > > > go with denylist the way Jiri proposed:
> > > > https://lore.kernel.org/bpf/ZrIj9jkXqpKXRuS7@krava/
> > > >
> > > > denylist is certainly less safer and it's a whack-a-mole
> > > > comparing to allowlist, but it's much much shorter
> > > > according to Jiri's analysis:
> > > > https://lore.kernel.org/bpf/Zr3q8ihbe8cUdpfp@krava/
> > > >
> > > > Eduard had an idea how to auto generate such allow/denylist
> > > > during the build.
> > > > That could be a follow up.
> > >
> > > If the sole goal is to avoid dead code elimination for tracepoint
> > > parameter null check, there might be another hack. Not sure if it was
> > > discussed:
> > > - don't add PTR_MAYBE_NULL (but maybe add a new tag, PTR_SOFT_NULL
> > >   from Kumar's original RFC);
> > > - in is_branch_taken() don't predict anything when tracepoint
> > >   parameters are compared;
> >
> > this part was discussed, but we didn't realize we need below bit...
> >
> > > - in mark_ptr_or_null_regs() don't propagate null for pointers to
> > >   tracepoint parameters (as in this patch).
> >
> > ... and here the 'for tp args' filter is hard to do.
> > mark_ptr_or_null_regs() is generic. arg vs non-arg is lost long ago.
>
> It is not lost. If only args are marked PTR_SOFT_NULL or
> reg->btf.is_raw_tp_arg (or w/e else), it can still be seen when we are
> in that function, and all its copies will have the same information.

ok. fair. still such PTR_SOFT_NULL can only be a temporary workaround.
We still need to revert cb4158ce8ec8.
And if we're reverting and adding soft_null knowingly to revert
later that's just too much.
The cb4158ce8ec8 approach felt ok-ish initially, but two issues
were found. soft_null looks ok-ish today, but it may have issues too.
revert plus denylist is a better way long term.
Especially with automation of allow/deny lists.

Re: [PATCH bpf v3 2/3] bpf: Do not mark NULL-checked raw_tp arg as scalar

[Jiri Olsa]

On Fri, Dec 06, 2024 at 07:10:48PM +0100, Kumar Kartikeya Dwivedi wrote:
> On Fri, 6 Dec 2024 at 18:59, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Dec 6, 2024 at 8:11 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > >
> > > An implication of this fix, which follows from the way the raw_tp fixes
> > > were implemented, is that all PTR_MAYBE_NULL trusted PTR_TO_BTF_ID are
> > > engulfed by these checks, and PROBE_MEM will apply to all of them, incl.
> > > those coming from helpers with KF_ACQUIRE returning maybe null trusted
> > > pointers. This NULL tagging after this commit will be sticky. Compared
> > > to a solution which only specially tagged raw_tp args with a different
> > > special maybe null tag (like PTR_SOFT_NULL), it's a consequence of
> > > overloading PTR_MAYBE_NULL with this meaning.
> > >
> > > Fixes: cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> > > Reported-by: Manu Bretelle <chantra@meta.com>
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > ---
> > >  kernel/bpf/verifier.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 82f40d63ad7b..556fb609d4a4 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -15365,6 +15365,12 @@ static void mark_ptr_or_null_reg(struct bpf_verifier_env *env,
> > >                         return;
> > >
> > >                 if (is_null) {
> > > +                       /* We never mark a raw_tp trusted pointer as scalar, to
> > > +                        * preserve backwards compatibility, instead just leave
> > > +                        * it as is.
> > > +                        */
> > > +                       if (mask_raw_tp_reg_cond(env, reg))
> > > +                               return;
> >
> > The blast radius is getting too big.
> > Patch 1 is ok, but here we're doubling down on
> > the hack in commit
> > cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> 
> There are two concerns:
> First, it applies whether or not a register is a raw_tp arg. There is
> a way to detect that (with some register state, instead of using a
> separate tag).
> Second, we treat the program in the == NULL branch as if the pointer
> _maybe_ null, and in the != NULL as definitively not NULL.
> I don't really see how that's too different, given we already allow direct
> access etc. when the pointer is _unchecked_ after entry, and the state
> is same as
> the case where == NULL branch is explored.
> 
> >
> > I think we need to revert the raw_tp masking hack and
> > go with denylist the way Jiri proposed:
> > https://lore.kernel.org/bpf/ZrIj9jkXqpKXRuS7@krava/
> >
> > denylist is certainly less safer and it's a whack-a-mole
> > comparing to allowlist, but it's much much shorter
> > according to Jiri's analysis:
> > https://lore.kernel.org/bpf/Zr3q8ihbe8cUdpfp@krava/
> 
> Ok, let's revert.
> Jiri, do you have the diff around for that attempt? Could you post a
> revert of the patches and then the diff you shared?
> If not, I can carry it as well with the revert, if you share it with
> me (keeping the attribution etc.). Either is fine, lmk.

hi,
sorry for late reply.. I rebased it, there were some conflicts, it's compile tested,
and perhaps not up2date:
  https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git/log/?h=bpf/tp_fix

jirka