Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752416AbbBXNqG (ORCPT ); Tue, 24 Feb 2015 08:46:06 -0500 Received: from mailapp01.imgtec.com ([195.59.15.196]:1512 "EHLO imgpgp01.kl.imgtec.org" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751044AbbBXNqD (ORCPT ); Tue, 24 Feb 2015 08:46:03 -0500 X-PGP-Universal: processed; by imgpgp01.kl.imgtec.org on Tue, 24 Feb 2015 13:46:02 +0000 Message-ID: <54EC8098.6070709@imgtec.com> Date: Tue, 24 Feb 2015 13:46:00 +0000 From: James Hogan User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Trond Myklebust CC: Jeff Layton , "J. Bruce Fields" , Linux Kernel Mailing List , Steven Rostedt , "Ingo Molnar" , Stable Tree Mailing List Subject: Re: [PATCH] sunrpc: Fix trace events to store data in the struct References: <1424778476-28242-1-git-send-email-james.hogan@imgtec.com> In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="nSRASCiVTxq6sbGlCFtVgvb7OrBlJB46i" X-Originating-IP: [192.168.154.110] X-ESG-ENCRYPT-TAG: b93fcccb Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5719 Lines: 156 --nSRASCiVTxq6sbGlCFtVgvb7OrBlJB46i Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi Trond, On 24/02/15 13:36, Trond Myklebust wrote: > On Tue, Feb 24, 2015 at 6:47 AM, James Hogan w= rote: >> Commit 83a712e0afef ("sunrpc: add some tracepoints around enqueue and >> dequeue of svc_xprt") merged in v3.19-rc1 added some new trace events,= >> however a couple of them printed data from dereferenced pointers rathe= r >> than storing the data in the struct. In general this isn't safe as the= >> print may not happen until later when the data may have changed or bee= n >> freed, and nor is it portable as userland won't have access to that >> other data in order to interpret the trace data itself. >> >> Fix by copying the data into the struct and printing from there. >> >> Fixes: 83a712e0afef ("sunrpc: add some tracepoints around enqueue ..."= ) >> Signed-off-by: James Hogan >> Cc: Jeff Layton >> Cc: J. Bruce Fields >> Cc: Trond Myklebust >> Cc: Steven Rostedt >> Cc: Ingo Molnar >> Cc: # v3.19+ >> --- >> Build tested only. Perhaps somebody familiar with the code could give = it >> a spin to sanity check the trace output. >> --- >> include/trace/events/sunrpc.h | 22 +++++++++++++++------- >> 1 file changed, 15 insertions(+), 7 deletions(-) >> >> diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunr= pc.h >> index b9c1dc6c825a..47dfcaebfaaf 100644 >> --- a/include/trace/events/sunrpc.h >> +++ b/include/trace/events/sunrpc.h >> @@ -503,18 +503,22 @@ TRACE_EVENT(svc_xprt_do_enqueue, >> >> TP_STRUCT__entry( >> __field(struct svc_xprt *, xprt) >> - __field(struct svc_rqst *, rqst) >> + __field_struct(struct sockaddr_storage, ss) >> + __field(unsigned long, flags); >> + __field(int, pid) >> ), >> >> TP_fast_assign( >> __entry->xprt =3D xprt; >> - __entry->rqst =3D rqst; >> + xprt ? memcpy(&__entry->ss, &xprt->xpt_remote, sizeof(= __entry->ss)) : memset(&__entry->ss, 0, sizeof(__entry->ss)); >=20 > How could xprt ever be NULL here, and even if it was, why the esoteric > C instead of a simple 'if' statement? Yeh, I had a straight forward unconditional assignment before, but I changed it purely for consistency with the svc_xprt_dequeue trace event. I don't pretend to understand the details of what is being traced though, so I'm happy to change it if required. Thanks James >=20 >> + __entry->flags =3D xprt ? xprt->xpt_flags : 0; >> + __entry->pid =3D rqst ? rqst->rq_task->pid : 0; >> ), >> >> TP_printk("xprt=3D0x%p addr=3D%pIScp pid=3D%d flags=3D%s", __e= ntry->xprt, >> - (struct sockaddr *)&__entry->xprt->xpt_remote, >> - __entry->rqst ? __entry->rqst->rq_task->pid : 0, >> - show_svc_xprt_flags(__entry->xprt->xpt_flags)) >> + (struct sockaddr *)&__entry->ss, >> + __entry->pid, >> + show_svc_xprt_flags(__entry->flags)) >> ); >> >> TRACE_EVENT(svc_xprt_dequeue, >> @@ -562,17 +566,21 @@ TRACE_EVENT(svc_handle_xprt, >> >> TP_STRUCT__entry( >> __field(struct svc_xprt *, xprt) >> + __field_struct(struct sockaddr_storage, ss) >> + __field(unsigned long, flags); >> __field(int, len) >> ), >> >> TP_fast_assign( >> __entry->xprt =3D xprt; >> + xprt ? memcpy(&__entry->ss, &xprt->xpt_remote, sizeof(= __entry->ss)) : memset(&__entry->ss, 0, sizeof(__entry->ss)); >=20 > Ditto. >=20 >> + __entry->flags =3D xprt ? xprt->xpt_flags : 0; >> __entry->len =3D len; >> ), >> >> TP_printk("xprt=3D0x%p addr=3D%pIScp len=3D%d flags=3D%s", __e= ntry->xprt, >> - (struct sockaddr *)&__entry->xprt->xpt_remote, __entry= ->len, >> - show_svc_xprt_flags(__entry->xprt->xpt_flags)) >> + (struct sockaddr *)&__entry->ss, __entry->len, >> + show_svc_xprt_flags(__entry->flags)) >> ); >> #endif /* _TRACE_SUNRPC_H */ >> >> -- >> 2.0.5 >> >=20 >=20 >=20 --nSRASCiVTxq6sbGlCFtVgvb7OrBlJB46i Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJU7ICZAAoJEGwLaZPeOHZ6PS0P/jWy39XRyUMttwUlYK9/NtZH UM6A4MxJwj4v/srkWRWERfsdEKMuxruFLdRU0SRhkUu2x/tND8EuN1McFYfu58+c 6Nl5pWoWgrLgjjoERADA2Xw6ajaorFVJmgx6WzgL4iYV2qyfnoMdB280H5kgopSj fd6TkPoj6UkV6B9y5W7uph3Mg1BmHwxx50tRoEGQ2ZyCxplOhg0B9PKJX2FgtVRU EczdaDNptCA1PLRN4x+ckl98jhvuB1FexcZq2MLy9/oj+TfrPqET+NYZm0HqK0jp Jhs37C5SgCF1RApIbvSTI8RZX+9hvXLFwpQ4N+Mlx2GHxVYS+J100yaYbCuHrD2D HlOiVOAGt/oB+qXexzHPbzWaRKw8Mtzhoq8oy55SbdMtPcAv0kFQ+8WH24O+YSGg lIyBgOC+nrx/N3+i7CEsU4pO9a9hfzVhZIbPk2RjZd/YnqeccGxX26clLtFOorlp vmoxwqWBd40gOV1oZMFAywBYaYR1K9F1ox2RvQMe+S0LZRjnSmGNkS+tQ0SFJrEn Vnq9+kMuAbcZ0/rt3LlhdK/07IpDNiqGeGsXY1Jv/WTvutyhykR9NpGd741FL5hD dfwIrdNGc4cpgZpSzEA8Gw5hoDl3RoK7T6nbDj+uN6EXa0xNEcEZq5I2Nt3zFOiZ mUVd1BVzLF1/QYSoK5F/ =GZYM -----END PGP SIGNATURE----- --nSRASCiVTxq6sbGlCFtVgvb7OrBlJB46i-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/