Re: [PATCH v2 2/2] SUNRPC: Fix the svc_deferred_event trace class

[Trond Myklebust <trondmy@hammerspace.com>]

On Thu, 2022-04-07 at 00:37 +0000, Chuck Lever III wrote:
> 
> > On Apr 6, 2022, at 5:17 PM, Trond Myklebust
> > <trondmy@hammerspace.com> wrote:
> > 
> > On Wed, 2022-04-06 at 20:55 +0000, Chuck Lever III wrote:
> > > 
> > > 
> > > > On Apr 6, 2022, at 4:34 PM, Trond Myklebust
> > > > <trondmy@hammerspace.com> 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@oracle.com>
> > > > > > ---
> > > > > >  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.
> 
> Well… v1 of this patch /did/ use __get_sockaddr(). I thought I could
> construct a back-portable version of the patch that would not need
> __get_sockaddr() because it’s not available in -stable kernels.
> 
> It sounds like the only choice for fixing this issue in -stable
> kernels is to remove the addr field from these tracepoints entirely?

In retrospect, after trying the above, I found it was easiest just to
do the conversion to a string representation up front in the
TP_fast_assign() if necessary, and then store the result.

IOW: in TP_fast_assign(), allocate a character buffer of size
INET6_ADDRSTRLEN + 10 on the stack and do the snprintf("%pISpc"...,
then apply __assign_str() to the result.

That has the extra advantage that it doesn't require you to reserve an
entire sockaddr_storage worth of ring buffer space. 🙂

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com