Re: NULL pointer deref when running BPF monitor program (6.11.0-rc1)
From: Jiri Olsa
Date: Fri Aug 16 2024 - 15:00:02 EST
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@xxxxxxxxx> 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