| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Zr-ho0ncAk__sZiX@krava> Date: Fri, 16 Aug 2024 20:59:47 +0200 From: Jiri Olsa <olsajiri@...il.com> To: Steven Rostedt <rostedt@...dmis.org> Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>, Jiri Olsa <olsajiri@...il.com>, Juri Lelli <juri.lelli@...hat.com>, bpf <bpf@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, Artem Savkov <asavkov@...hat.com>, "Jose E. Marchesi" <jose.marchesi@...cle.com> Subject: Re: NULL pointer deref when running BPF monitor program (6.11.0-rc1) 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@...il.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
Powered by blists - more mailing lists