[PATCH v2 0/2] Fix request deferral for NFS/RDMA

[PATCH v2 0/2] Fix request deferral for NFS/RDMA

[Chuck Lever]

Last week Trond reported a server crash in svc_rdma_sendto() that
seemed related to deferral of a request incoming on an RPC/RDMA
transport. A link to that report appears in the first patch below.

The first patch in this series addresses the crash, and the second
fixes a related observability crash. The other patches from v1
have been postponed to 5.19.

Comments, opinions, and Reviewed-by's are appreciated!

Since v1:
- Series trimmed down to just what will go to 5.18-rc
- 2nd patch re-worked so it can be backported to recent stable
- Addressed a few checkpatch.pl nits

---

Chuck Lever (2):
      SUNRPC: Fix NFSD's request deferral on RDMA transports
      SUNRPC: Fix the svc_deferred_event trace class


 include/linux/sunrpc/svc.h              | 1 +
 include/trace/events/sunrpc.h           | 9 +++++----
 net/sunrpc/svc_xprt.c                   | 3 +++
 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 2 +-
 4 files changed, 10 insertions(+), 5 deletions(-)

--
Chuck Lever

[PATCH v2 1/2] SUNRPC: Fix NFSD's request deferral on RDMA transports

[Chuck Lever]

Trond Myklebust reports an NFSD crash in svc_rdma_sendto(). Further
investigation shows that the crash occurred while NFSD was handling
a deferred request.

This patch addresses two inter-related issues that prevent request
deferral from working correctly for RPC/RDMA requests:

1. Prevent the crash by ensuring that the original
   svc_rqst::rq_xprt_ctxt value is available when the request is
   revisited. Otherwise svc_rdma_sendto() does not have a Receive
   context available with which to construct its reply.

2. Possibly since before commit 71641d99ce03 ("svcrdma: Properly
   compute .len and .buflen for received RPC Calls"),
   svc_rdma_recvfrom() did not include the transport header in the
   returned xdr_buf. There should have been no need for svc_defer()
   and friends to save and restore that header, as of that commit.
   This issue is addressed in a backport-friendly way by simply
   having svc_rdma_recvfrom() set rq_xprt_hlen to zero
   unconditionally, just as svc_tcp_recvfrom() does. This enables
   svc_deferred_recv() to correctly reconstruct an RPC message
   received via RPC/RDMA.

Reported-by: Trond Myklebust <trondmy@hammerspace.com>
Link: https://lore.kernel.org/linux-nfs/82662b7190f26fb304eb0ab1bb04279072439d4e.camel@hammerspace.com/
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Cc: <stable@vger.kernel.org>
---
 include/linux/sunrpc/svc.h              |    1 +
 net/sunrpc/svc_xprt.c                   |    3 +++
 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c |    2 +-
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index a5dda4987e8b..217711fc9cac 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -395,6 +395,7 @@ struct svc_deferred_req {
 	size_t			addrlen;
 	struct sockaddr_storage	daddr;	/* where reply must come from */
 	size_t			daddrlen;
+	void			*xprt_ctxt;
 	struct cache_deferred_req handle;
 	size_t			xprt_hlen;
 	int			argslen;
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 0c117d3bfda8..b42cfffa7395 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -1231,6 +1231,8 @@ static struct cache_deferred_req *svc_defer(struct cache_req *req)
 		dr->daddr = rqstp->rq_daddr;
 		dr->argslen = rqstp->rq_arg.len >> 2;
 		dr->xprt_hlen = rqstp->rq_xprt_hlen;
+		dr->xprt_ctxt = rqstp->rq_xprt_ctxt;
+		rqstp->rq_xprt_ctxt = NULL;
 
 		/* back up head to the start of the buffer and copy */
 		skip = rqstp->rq_arg.len - rqstp->rq_arg.head[0].iov_len;
@@ -1269,6 +1271,7 @@ static noinline int svc_deferred_recv(struct svc_rqst *rqstp)
 	rqstp->rq_xprt_hlen   = dr->xprt_hlen;
 	rqstp->rq_daddr       = dr->daddr;
 	rqstp->rq_respages    = rqstp->rq_pages;
+	rqstp->rq_xprt_ctxt   = dr->xprt_ctxt;
 	svc_xprt_received(rqstp->rq_xprt);
 	return (dr->argslen<<2) - dr->xprt_hlen;
 }
diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
index cf76a6ad127b..864131a9fc6e 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
@@ -831,7 +831,7 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
 		goto out_err;
 	if (ret == 0)
 		goto out_drop;
-	rqstp->rq_xprt_hlen = ret;
+	rqstp->rq_xprt_hlen = 0;
 
 	if (svc_rdma_is_reverse_direction_reply(xprt, ctxt))
 		goto out_backchannel;

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

[Chuck Lever]

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)
 );
 
 #define DEFINE_SVC_DEFERRED_EVENT(name) \

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

[Trond Myklebust]

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.

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

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

[Chuck Lever III]

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


--
Chuck Lever

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

[Trond Myklebust]

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.

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

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

[Chuck Lever III]

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

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

[Trond Myklebust]

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