* [PATCH v2 0/2] Fix request deferral for NFS/RDMA
@ 2022-04-06 18:07 Chuck Lever
2022-04-06 18:08 ` [PATCH v2 1/2] SUNRPC: Fix NFSD's request deferral on RDMA transports Chuck Lever
2022-04-06 18:08 ` [PATCH v2 2/2] SUNRPC: Fix the svc_deferred_event trace class Chuck Lever
0 siblings, 2 replies; 8+ messages in thread
From: Chuck Lever @ 2022-04-06 18:07 UTC (permalink / raw)
To: linux-nfs
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/2] SUNRPC: Fix NFSD's request deferral on RDMA transports
2022-04-06 18:07 [PATCH v2 0/2] Fix request deferral for NFS/RDMA Chuck Lever
@ 2022-04-06 18:08 ` Chuck Lever
2022-04-06 18:08 ` [PATCH v2 2/2] SUNRPC: Fix the svc_deferred_event trace class Chuck Lever
1 sibling, 0 replies; 8+ messages in thread
From: Chuck Lever @ 2022-04-06 18:08 UTC (permalink / raw)
To: linux-nfs
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;
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] SUNRPC: Fix the svc_deferred_event trace class
2022-04-06 18:07 [PATCH v2 0/2] Fix request deferral for NFS/RDMA Chuck Lever
2022-04-06 18:08 ` [PATCH v2 1/2] SUNRPC: Fix NFSD's request deferral on RDMA transports Chuck Lever
@ 2022-04-06 18:08 ` Chuck Lever
2022-04-06 20:34 ` Trond Myklebust
1 sibling, 1 reply; 8+ messages in thread
From: Chuck Lever @ 2022-04-06 18:08 UTC (permalink / raw)
To: linux-nfs
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) \
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] SUNRPC: Fix the svc_deferred_event trace class
2022-04-06 18:08 ` [PATCH v2 2/2] SUNRPC: Fix the svc_deferred_event trace class Chuck Lever
@ 2022-04-06 20:34 ` Trond Myklebust
2022-04-06 20:55 ` Chuck Lever III
0 siblings, 1 reply; 8+ messages in thread
From: Trond Myklebust @ 2022-04-06 20:34 UTC (permalink / raw)
To: linux-nfs, chuck.lever
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] SUNRPC: Fix the svc_deferred_event trace class
2022-04-06 20:34 ` Trond Myklebust
@ 2022-04-06 20:55 ` Chuck Lever III
2022-04-06 21:17 ` Trond Myklebust
0 siblings, 1 reply; 8+ messages in thread
From: Chuck Lever III @ 2022-04-06 20:55 UTC (permalink / raw)
To: Trond Myklebust; +Cc: Linux NFS Mailing List
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] SUNRPC: Fix the svc_deferred_event trace class
2022-04-06 20:55 ` Chuck Lever III
@ 2022-04-06 21:17 ` Trond Myklebust
2022-04-07 0:37 ` Chuck Lever III
0 siblings, 1 reply; 8+ messages in thread
From: Trond Myklebust @ 2022-04-06 21:17 UTC (permalink / raw)
To: chuck.lever; +Cc: linux-nfs
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] SUNRPC: Fix the svc_deferred_event trace class
2022-04-06 21:17 ` Trond Myklebust
@ 2022-04-07 0:37 ` Chuck Lever III
2022-04-07 1:27 ` Trond Myklebust
0 siblings, 1 reply; 8+ messages in thread
From: Chuck Lever III @ 2022-04-07 0:37 UTC (permalink / raw)
To: Trond Myklebust; +Cc: linux-nfs
> 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?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] SUNRPC: Fix the svc_deferred_event trace class
2022-04-07 0:37 ` Chuck Lever III
@ 2022-04-07 1:27 ` Trond Myklebust
0 siblings, 0 replies; 8+ messages in thread
From: Trond Myklebust @ 2022-04-07 1:27 UTC (permalink / raw)
To: chuck.lever; +Cc: linux-nfs
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-04-07 1:32 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-06 18:07 [PATCH v2 0/2] Fix request deferral for NFS/RDMA Chuck Lever
2022-04-06 18:08 ` [PATCH v2 1/2] SUNRPC: Fix NFSD's request deferral on RDMA transports Chuck Lever
2022-04-06 18:08 ` [PATCH v2 2/2] SUNRPC: Fix the svc_deferred_event trace class Chuck Lever
2022-04-06 20:34 ` Trond Myklebust
2022-04-06 20:55 ` Chuck Lever III
2022-04-06 21:17 ` Trond Myklebust
2022-04-07 0:37 ` Chuck Lever III
2022-04-07 1:27 ` Trond Myklebust
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.