[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240816101031.6dd1361b@rorschach.local.home>
Date: Fri, 16 Aug 2024 10:10:31 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: 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 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?
-- Steve
Powered by blists - more mailing lists