On Wed, 2022-04-06 at 20:55 +0000, Chuck Lever III wrote:
>
>
> > On Apr 6, 2022, at 4:34 PM, Trond Myklebust
> > <trondmy@xxxxxxxxxxxxxxx> wrote:
> >
> > On Wed, 2022-04-06 at 14:08 -0400, Chuck Lever wrote:
> > > Fix a NULL deref crash that occurs when an svc_rqst is deferred
> > > while the sunrpc tracing subsystem is enabled. svc_revisit() sets
> > > dr->xprt to NULL, so it can't be relied upon in the tracepoint to
> > > provide the remote's address.
> > >
> > > Since __sockaddr() and friends are not available before v5.18,
> > > this
> > > is just a partial revert of commit ece200ddd54b ("sunrpc: Save
> > > remote presentation address in svc_xprt for trace events") in
> > > order
> > > to enable backports of the fix. It can be cleaned up during a
> > > future merge window.
> > >
> > > Fixes: ece200ddd54b ("sunrpc: Save remote presentation address in
> > > svc_xprt for trace events")
> > > Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> > > ---
> > > include/trace/events/sunrpc.h | 9 +++++----
> > > 1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/trace/events/sunrpc.h
> > > b/include/trace/events/sunrpc.h
> > > index ab8ae1f6ba84..4abc2fddd3b8 100644
> > > --- a/include/trace/events/sunrpc.h
> > > +++ b/include/trace/events/sunrpc.h
> > > @@ -2017,18 +2017,19 @@ DECLARE_EVENT_CLASS(svc_deferred_event,
> > > TP_STRUCT__entry(
> > > __field(const void *, dr)
> > > __field(u32, xid)
> > > - __string(addr, dr->xprt->xpt_remotebuf)
> > > + __dynamic_array(u8, addr, dr->addrlen)
> > > ),
> > >
> > > TP_fast_assign(
> > > __entry->dr = dr;
> > > __entry->xid = be32_to_cpu(*(__be32 *)(dr->args +
> > > (dr-
> > > > xprt_hlen>>2)));
> > > - __assign_str(addr, dr->xprt->xpt_remotebuf);
> > > + memcpy(__get_dynamic_array(addr), &dr->addr, dr-
> > > > addrlen);
> > > ),
> > >
> > > - TP_printk("addr=%s dr=%p xid=0x%08x", __get_str(addr),
> > > __entry->dr,
> > > - __entry->xid)
> > > + TP_printk("addr=%pISpc dr=%p xid=0x%08x",
> > > + (struct sockaddr *)__get_dynamic_array(addr),
> > > + __entry->dr, __entry->xid)
> > > );
> > >
> >
> > Have you tested this? I found myself hitting the warning
> >
> > "event %s has unsafe dereference of argument %d"
> >
> > in test_event_printk() when I tried to use something similar for
> > the
> > NFS code.
>
> The warning is fixed by c6ced22997ad ("tracing: Update print
> fmt check to handle new __get_sockaddr() macro"). I think
> this means that along with the above patch, c6ced22997ad will
> need to be backported to some recent stable kernels.
>
> If you're adding dynamically-sized sockaddr fields in trace
> records, I invite you to consider __sockaddr/__get_sockaddr(),
> added in v5.18.
>
Interesting... I hadn't seen that change. It looks however as if that
is explicitly checking for the __get_sockaddr() string, so you'd have
to convert this patch.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
