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

From: Chuck Lever III <chuck.lever@oracle.com>
To: Trond Myklebust <trondmy@hammerspace.com>
Cc: "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v2 2/2] SUNRPC: Fix the svc_deferred_event trace class
Date: Thu, 7 Apr 2022 00:37:36 +0000	[thread overview]
Message-ID: <CF55C9CB-539D-4A65-913E-E339A05CD6F0@oracle.com> (raw)
In-Reply-To: <c4bea3892c7d219138c3a9ee961bab40e3d1c246.camel@hammerspace.com>

> 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?