Re: NULL pointer deref when running BPF monitor program (6.11.0-rc1)

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Aug 16, 2024 at 10:10:31AM -0400, Steven Rostedt wrote:
> On Thu, 15 Aug 2024 14:37:18 +0200
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> > > I checked bit more and there are more tracepoints with the same issue,
> > > the first diff stat looks like:
> > >
> > >          include/trace/events/afs.h                            | 44 ++++++++++++++++++++++----------------------
> > >          include/trace/events/cachefiles.h                     | 96 ++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------
> > >          include/trace/events/ext4.h                           |  6 +++---
> > >          include/trace/events/fib.h                            | 16 ++++++++--------
> > >          include/trace/events/filelock.h                       | 38 +++++++++++++++++++-------------------
> > >          include/trace/events/host1x.h                         | 10 +++++-----
> > >          include/trace/events/huge_memory.h                    | 24 ++++++++++++------------
> > >          include/trace/events/kmem.h                           | 18 +++++++++---------
> > >          include/trace/events/netfs.h                          | 16 ++++++++--------
> > >          include/trace/events/power.h                          |  6 +++---
> > >          include/trace/events/qdisc.h                          |  8 ++++----
> > >          include/trace/events/rxrpc.h                          | 12 ++++++------
> > >          include/trace/events/sched.h                          | 12 ++++++------
> > >          include/trace/events/sunrpc.h                         |  8 ++++----
> > >          include/trace/events/tcp.h                            | 14 +++++++-------
> > >          include/trace/events/tegra_apb_dma.h                  |  6 +++---
> > >          include/trace/events/timer_migration.h                | 10 +++++-----
> > >          include/trace/events/writeback.h                      | 16 ++++++++--------
> > >
> > > plus there's one case where pointer needs to be checked with IS_ERR in
> > > include/trace/events/rdma_core.h trace_mr_alloc/mr_integ_alloc
> > >
> > > I'm not excited about the '_nullable' argument suffix, because it's lot
> > > of extra changes/renames in TP_fast_assign and it does not solve the
> > > IS_ERR case above
> > >
> > > I checked on the type tag and with llvm build we get the TYPE_TAG info
> > > nicely in BTF:
> > >
> > >         [119148] TYPEDEF 'btf_trace_sched_pi_setprio' type_id=119149
> > >         [119149] PTR '(anon)' type_id=119150
> > >         [119150] FUNC_PROTO '(anon)' ret_type_id=0 vlen=3
> > >                 '(anon)' type_id=27
> > >                 '(anon)' type_id=678
> > >                 '(anon)' type_id=119152
> > >         [119151] TYPE_TAG 'nullable' type_id=679
> > >         [119152] PTR '(anon)' type_id=119151
> > >
> > >         [679] STRUCT 'task_struct' size=15424 vlen=277
> > >
> > > which we can easily check in verifier.. the tracepoint definition would look like:
> > >
> > >         -       TP_PROTO(struct task_struct *tsk, struct task_struct *pi_task),
> > >         +       TP_PROTO(struct task_struct *tsk, struct task_struct __nullable *pi_task),
> > >
> > > and no other change in TP_fast_assign is needed
> > >
> > > I think using the type tag for this is nicer, but I'm not sure where's
> > > gcc at with btf_type_tag implementation, need to check on that  
> > 
> > Unfortunately last time I heard gcc was still far.
> > So we cannot rely on decl_tag or type_tag yet.
> > Aside from __nullable we would need another suffix to indicate is_err.
> > 
> > Maybe we can do something with the TP* macro?
> > So the suffix only seen one place instead of search-and-replace
> > through the body?
> > 
> > but imo above diff stat doesn't look too bad.
> 
> I'm fine with annotation of parameters, but I really don't want this
> being part of the TP_fast_assign() content. What would that look like
> anyway?

best option would be using btf_type_tag annotation, which would look like:

	-       TP_PROTO(struct task_struct *tsk, struct task_struct *pi_task),
	+       TP_PROTO(struct task_struct *tsk, struct task_struct __nullable *pi_task),

but gcc does not support that yet, just clang

so far the only working solution I have is adding '__nullable' suffix
to argument name:

	diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
	index 9ea4c404bd4e..fc46f0b42741 100644
	--- a/include/trace/events/sched.h
	+++ b/include/trace/events/sched.h
	@@ -559,9 +559,9 @@ DEFINE_EVENT(sched_stat_runtime, sched_stat_runtime,
	  */
	 TRACE_EVENT(sched_pi_setprio,
	 
	-	TP_PROTO(struct task_struct *tsk, struct task_struct *pi_task),
	+	TP_PROTO(struct task_struct *tsk, struct task_struct *pi_task__nullable),
	 
	-	TP_ARGS(tsk, pi_task),
	+	TP_ARGS(tsk, pi_task__nullable),
	 
		TP_STRUCT__entry(
			__array( char,	comm,	TASK_COMM_LEN	)
	@@ -574,8 +574,8 @@ TRACE_EVENT(sched_pi_setprio,
			memcpy(__entry->comm, tsk->comm, TASK_COMM_LEN);
			__entry->pid		= tsk->pid;
			__entry->oldprio	= tsk->prio;
	-		__entry->newprio	= pi_task ?
	-				min(tsk->normal_prio, pi_task->prio) :
	+		__entry->newprio	= pi_task__nullable ?
	+				min(tsk->normal_prio, pi_task__nullable->prio) :
					tsk->normal_prio;
			/* XXX SCHED_DEADLINE bits missing */
		),


now I'm trying to make work something like:

	diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
	index 9ea4c404bd4e..4e4aae2d5700 100644
	--- a/include/trace/events/sched.h
	+++ b/include/trace/events/sched.h
	@@ -559,9 +559,9 @@ DEFINE_EVENT(sched_stat_runtime, sched_stat_runtime,
	  */
	 TRACE_EVENT(sched_pi_setprio,
	 
	-	TP_PROTO(struct task_struct *tsk, struct task_struct *pi_task),
	+	TP_PROTO(struct task_struct *tsk, struct task_struct *__nullable(pi_task)),
	 
	-	TP_ARGS(tsk, pi_task),
	+	TP_ARGS(tsk, __nullable(pi_task)),
	 
		TP_STRUCT__entry(
			__array( char,	comm,	TASK_COMM_LEN	)


jirka