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

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Aug 19, 2024 at 01:47:20PM +0200, Jiri Olsa wrote:
> On Fri, Aug 16, 2024 at 03:30:40PM -0400, Steven Rostedt wrote:
> > 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.
> 
> verifier assumes that programs attached to the tracepoint can access
> pointer arguments without checking them for null and some of those
> programs most likely access such arguments directly
> 
> changing that globally and require bpf program to do null check for all
> pointer arguments will make verifier fail to load existing programs
> 
> > 
> > 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?
> 
> IIUC you mean to store extra data for each tracepoint that would
> annotate the argument? as Alexei pointed out earlier it might be
> too much, because we'd be fine with just adding suffix to annotated
> arguments in __bpf_trace_##call:
> 
> 	__bpf_trace_##call(void *__data, proto)                                 \

nah.. it's defined for class template, so tracepoints like cgroup_mkdir
won't have its own __bpf_trace_cgroup_mkdir function that we could use

we need to do something else

jirka


> 	{                                                                       \
> 		CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(__data, CAST_TO_U64(args));        \
> 	}
> 
> with that verifier could easily get suffix information from BTF and
> once gcc implements btf_type_tag we can easily switch to that
> 
> jirka