| Message ID | 164926848846.12216.6872977249610829189.stgit@klimt.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix request deferral for NFS/RDMA | expand |
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.
> 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
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.
> 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?
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.
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) \
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(-)