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

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 16 Aug 2024 20:59:47 +0200
Jiri Olsa <olsajiri@gmail.com> wrote:

> 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	)

Hmm, that's really ugly though. Both versions.

Now when Alexei said:

> > > > > We cannot make all tracepoint pointers to be PTR_TRUSTED | PTR_MAYBE_NULL
> > > > > by default, since it will break a bunch of progs.
> > > > > Instead we can annotate this tracepoint arg as __nullable and
> > > > > teach the verifier to recognize such special arguments of tracepoints. 

I'm not familiar with the verifier, so I don't know how the above is
implemented, and why it would break a bunch of progs.

If you had a macro around the parameter:

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

Could having that go through another macro pass in trace_events.h work?
That is, could we associate the trace event with "nullable" parameters
that could be stored someplace else for you?

-- Steve