Received-SPF: pass (google.com: domain of linux-nfs+bounces-2742-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 10 Apr 2024 11:41:18 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Justin Stitt <justinstitt@google.com>, Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Jeff Layton <jlayton@kernel.org>, Neil Brown <neilb@suse.de>,
        Olga Kornievskaia <kolga@netapp.com>, Dai Ngo <Dai.Ngo@oracle.com>,
        Tom Talpey <tom@talpey.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org,
        linux-nfs@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] trace: events: cleanup deprecated strncpy uses
Message-ID: <ZhazHrz4SxBs+BFD@tissot.1015granger.net>
References: <20240401-strncpy-include-trace-events-mdio-h-v1-1-9cb5a4cda116@google.com>
 <20240410113614.39b61b0d@gandalf.local.home>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240410113614.39b61b0d@gandalf.local.home>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?+HxlQiuvGaKtde7yq1JJVtxQuu43qPzL+R0OaPa08lnPnkTYv5Q5elB/bm2h?=
 =?us-ascii?Q?zQcucD30deVLYL7/ZsT93q9c6U4EW9O12EAhs4Sln5QIBF67NjzmUZMiCura?=
 =?us-ascii?Q?obLoylwzT0F3Es+7X8cy8QJj9U6WAHmmm11GnTRR4MrI1fG5BEb2borccrAw?=
 =?us-ascii?Q?XUmfvAp0NGbaD4AbQmGL6o8FB4O926gPGDEszFZuJoA8U35weRDN1nOlkSD4?=
 =?us-ascii?Q?8MjY02rEXdFKFmOhDxEt+m1QH9Si0Qmf6o71xLFJNBpGeSklRf1awydj8cM8?=
 =?us-ascii?Q?iWOkkq0zIzfZ6r6AGxWSdzeu3Q1DkRz9zWvSHV3fdYOgoNNZ4A811DOruXWX?=
 =?us-ascii?Q?bQIScGsXHeXvIQ/OQy9crAlOaWJ8dLYGfGCnLPL22COzGTfrKIGldOjVz/tc?=
 =?us-ascii?Q?IQRU4xgFZkVI33TVORlG39tUBeHdke8l7F/96Z86YZaYCBHoyE/+FgDMYQc2?=
 =?us-ascii?Q?gMJJAa0HArNrpCLywnZ8Ocua1UvXsIvyiubi92CwMV8k7YRyIgglshVjZ2MG?=
 =?us-ascii?Q?ACDQ2vQV7oT7ASnndf6L/7NAOpGv+zNbr7y3zNd4tj7DuRZOnSbiI6YaxMuJ?=
 =?us-ascii?Q?GINxFIbGLM6vJI0ykb+HoDTjIQKV/GDK9okFadIC4J/CE1MOt94e4JfRKJjM?=
 =?us-ascii?Q?tKQjPkT/l9xmYdorHlorpQmfrTOPSbTZGNpHgYeblQpnIgeF+mBkcGCqOp9/?=
 =?us-ascii?Q?SDz9Zfu2JaUJj95bP6MivZJh30ZWMQ82NsZ29DAkx1iW9rrXaBm+KGZoQaRQ?=
 =?us-ascii?Q?2ZL4AmplYScXDnkhEobk5Y1FK8eFkNsYzBluKwLZNq5Vbh5P8iRknK9qsGI8?=
 =?us-ascii?Q?WNq9AI83+rxW6P6d11lPzOIlrDzAwyAbzwuPrDnfozOQnd//wV44rNMKZPvL?=
 =?us-ascii?Q?dUovaRnynDtGGx7m1lncGFkKhWXEDPOl4oduG+w8g1qqvwiAUdNVLoRkDaYc?=
 =?us-ascii?Q?bYHgQBEVSURNSwYdOZHtNnH1cAazn5TNNmqzHylsCQnn7ZvTubvIsym7wWD6?=
 =?us-ascii?Q?+85EwdUItUpkLmTD3cUbpow1mqf7Ii8D7v/P3loSe4047lvmOWlQFm0oggTo?=
 =?us-ascii?Q?yUmZyeCQ8mv0Ls27w/N6mKcphDZxpZxTyMogUJp5JKUSGlAK5/XWYcVJsWsI?=
 =?us-ascii?Q?MMF5EiG+WQw/Qa2g+PUvohLSRklmAoLkGzPRgCMldYnHjCDxcT0U1K55ddZ8?=
 =?us-ascii?Q?9eTWx3mDDwYW5Ihw0mRaUUB9K6zW2baAfOWY3xWKrYuDJk9g44s7yjITmCaF?=
 =?us-ascii?Q?/1LL61nN3xV5GT9RPMAfW76hNDlzFyouxIf7KV35NlbIYd6ab6HkXJq64TmT?=
 =?us-ascii?Q?ldW+NGPkfOHQGdpO3tAW01/Q5ud7ewIovlSX9r6L1Pb80Ruix28eL8UdbQBc?=
 =?us-ascii?Q?2d00IFw/0QwblBh80yNGqoAV3hpZZxQmpD62NJemwfH0PTCTX58WVObqnoWf?=
 =?us-ascii?Q?n4mAINgHNjaKni/FA7/qRzGVyNdYqmDFWpPnIyjsqeMi+rapxeh38x+X1Cys?=
 =?us-ascii?Q?G7jUSP/nHdCCun3P6OLTGZ0PXv2iixIQ8Ykkmpds9HWHVUo01aIMg8SpefeK?=
 =?us-ascii?Q?E8j80tIa0PWkhOKkKTM+JpJ3ZgDLkZtzzpjD7lsCxiOM7pvEgzaqgn6XkhY/?=
 =?us-ascii?Q?6Q=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	juELbf0R+gjJHB6pRHNYpRUzR++yURXYP84hsE7KUWe+zGljK6a8YTZN/YfxfgVnV+RQpQq9gmT50D8Hsgqaz5tOIpQqKiFY/80JPypu6Jm1aEcgJU7Nfjr+XRf5urlofxXp2RmtSLo7IyKyud5GEim6cwd4I6SgJwgoBGCAv1Qe0Q/QVo0qWfZ3UcYOX2iQOZnX1FhKLd2ww5npuv742OqDySF0PddSd5qqzKeQfvXYgadrBZR7nsTpb6AOt8Mj5v3fwhcem7nOLjK4QCbjPjvsL+V2uFTrNrx8xwW3XEAyPARAnY3tFAOKH0e27UTqrUZP6gcjkdF/E1LrHtmvHwOjyQUU/GSwYQQsTfRvgBd8wbVrdsvXTKUZVzyGbxIhc9T6arRQK+llB396oxGHWHk5Gtv/+ZnasvgPVttMWXJCa7hG8kNgTHADVj8PFO9UuqPRt5hQsT3gjf6gBJ6AbMiDcG/HagNW3t/o/jAEdEY4Ro9YbpIlPemJ9cTPFnkfq/3ZLUkDBP+M7zRAfRRT2AZSHaVYVKvedleRTGkYK4w/JqmSIvQb5tJlaiKM1kkk2oY/FoRV8IjVrxx6DWi6z1qZ89fCSplqhiQgYrEk39U=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7768daf5-ba1f-40d0-4cf6-08dc5974abad
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Apr 2024 15:41:22.1891
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: yDu+Rpo3cQh8/uqft770gVjXP6ET2QZWUUsggpw16ALYSAne2ym+P79OM/vIHZSWymt+sxo+9IFQd+RGX7enjQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4709
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-04-10_04,2024-04-09_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 mlxscore=0 adultscore=0
 phishscore=0 spamscore=0 malwarescore=0 mlxlogscore=999 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2404010000
 definitions=main-2404100114
X-Proofpoint-GUID: DC8y-Z9N4BiXGJ-z__Jb2IpokFtmM6fg
X-Proofpoint-ORIG-GUID: DC8y-Z9N4BiXGJ-z__Jb2IpokFtmM6fg

On Wed, Apr 10, 2024 at 11:36:14AM -0400, Steven Rostedt wrote:
> On Mon, 01 Apr 2024 23:48:52 +0000
> Justin Stitt <justinstitt@google.com> wrote:
> 
> > diff --git a/include/trace/events/rpcgss.h b/include/trace/events/rpcgss.h
> > index ba2d96a1bc2f..274c297f1b15 100644
> > --- a/include/trace/events/rpcgss.h
> > +++ b/include/trace/events/rpcgss.h
> > @@ -618,7 +618,7 @@ TRACE_EVENT(rpcgss_context,
> >  		__entry->timeout = timeout;
> >  		__entry->window_size = window_size;
> >  		__entry->len = len;
> > -		strncpy(__get_str(acceptor), data, len);
> > +		memcpy(__get_str(acceptor), data, len);
> >  	),
> >  
> >  	TP_printk("win_size=%u expiry=%lu now=%lu timeout=%u acceptor=%.*s",
> 
> WTF, that code is just buggy. Looking at the rpcgss_context event we have:
> 
> > TRACE_EVENT(rpcgss_context,
> >         TP_PROTO(
> >                 u32 window_size,
> >                 unsigned long expiry,
> >                 unsigned long now,
> >                 unsigned int timeout,
> >                 unsigned int len,
> >                 const u8 *data
> >         ),
> > 
> >         TP_ARGS(window_size, expiry, now, timeout, len, data),
> > 
> >         TP_STRUCT__entry(
> >                 __field(unsigned long, expiry)
> >                 __field(unsigned long, now)
> >                 __field(unsigned int, timeout)
> >                 __field(u32, window_size)
> >                 __field(int, len)
> >                 __string(acceptor, data)
> 
> The __string() macro expects "data" to be a string and does *not* check
> length when copying.
> 
> If anything, it needs to be:
> 
> 		__string_len(acceptor, data, len)
> 
> as the macro code has changed recently, and the current code will crash!

A general question:

Is there a test suite we should run regularly to build some
confidence in the kernel's observability apparatus? We're building
a menagerie of tests around kdevops, and one area where we know
there is a testing gap is the tracepoints in NFSD and SunRPC.


> >         ),
> > 
> >         TP_fast_assign(
> >                 __entry->expiry = expiry;
> >                 __entry->now = now;
> >                 __entry->timeout = timeout;
> >                 __entry->window_size = window_size;
> >                 __entry->len = len;
> >                 strncpy(__get_str(acceptor), data, len);
> 
> Then this needs to be:
> 
> 		__assign_str(acceptor, data);
> 
> Note, the length is now saved via __string_len() and not needed here.
> 
> I'll go send a patch to fix this.
> 
> -- Steve
> 
> 
> >         ),

-- 
Chuck Lever