From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933468AbcFQSZD (ORCPT ); Fri, 17 Jun 2016 14:25:03 -0400 Received: from smtprelay0105.hostedemail.com ([216.40.44.105]:45650 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753440AbcFQSZA (ORCPT ); Fri, 17 Jun 2016 14:25:00 -0400 X-Session-Marker: 726F737465647440676F6F646D69732E6F7267 X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,rostedt@goodmis.org,:::::::,RULES_HIT:41:355:379:541:599:800:960:968:973:988:989:1260:1277:1311:1313:1314:1345:1359:1437:1515:1516:1518:1534:1542:1593:1594:1711:1730:1747:1777:1792:2393:2553:2559:2562:2693:2898:3138:3139:3140:3141:3142:3354:3622:3865:3866:3867:3868:3870:3871:3872:3873:3874:4321:4362:5007:6119:6261:7576:7875:9010:9040:10004:10400:10848:10967:11026:11232:11658:11914:12043:12295:12296:12438:12517:12519:12555:12740:13439:14096:14097:14181:14659:14721:21080:21324:21434:30054:30064:30070:30090:30091,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:fn,MSBL:0,DNSBL:none,Custom_rules:0:0:0,LFtime:2,LUA_SUMMARY:none X-HE-Tag: board25_82c1647d5b31f X-Filterd-Recvd-Size: 3712 Date: Fri, 17 Jun 2016 14:24:57 -0400 From: Steven Rostedt To: zhengjun.xing@intel.com Cc: mingo@redhat.com, linux-kernel@vger.kernel.org, Namhyung Kim Subject: Re: [PATCH] tracing: Add *iter check for NULL Message-ID: <20160617142457.48a37b6c@gandalf.local.home> In-Reply-To: <20160617123841.21b1a7e1@gandalf.local.home> References: <1464769870-18344-1-git-send-email-zhengjun.xing@intel.com> <20160617123841.21b1a7e1@gandalf.local.home> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 17 Jun 2016 12:38:41 -0400 Steven Rostedt wrote: > On Wed, 1 Jun 2016 16:31:10 +0800 > zhengjun.xing@intel.com wrote: > > > From: xingzhen > > > > 3debb0a9ddb adding a "__used" to the variable in the > > __trace_printk_fmt section. Sometimes it will cause > > *iter to be NULL, then strcmp in lookup_format and > > strcpy in hold_module_trace_bprintk_format will panic. > > Could you show an example of this happening? Ha! While adding a trace_printk() test module (to test someone else's change) I triggered this bug. > > > > > Signed-off-by: xingzhen > > --- > > kernel/trace/trace_printk.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c > > index f96f038..82ecfb5 100644 > > --- a/kernel/trace/trace_printk.c > > +++ b/kernel/trace/trace_printk.c > > @@ -55,6 +55,8 @@ void hold_module_trace_bprintk_format(const char **start, const char **end) > > > > mutex_lock(&btrace_mutex); > > for (iter = start; iter < end; iter++) { > > + if (!*iter) > > + goto err; > > struct trace_bprintk_fmt *tb_fmt = lookup_format(*iter); > > First, you can't place logic before a declaration. Not all compilers > will allow that. > > Also, do we really want to error on this or just skip it? Because it > will miss out on all other trace_printks in the module. I tried your patch and it works until you remove the module and try reading the trace again. As I said, you left out later processing. This should not exit on error. Below is a patch I wrote, and it works well. I'll add you as reported by. Thanks! -- Steve > > > > if (tb_fmt) { > > *iter = tb_fmt->fmt; > > @@ -75,6 +77,7 @@ void hold_module_trace_bprintk_format(const char **start, const char **end) > > *iter = fmt; > > > > } > > +err: > > mutex_unlock(&btrace_mutex); > > } > > > diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c index f96f0383f6c6..ad1d6164e946 100644 --- a/kernel/trace/trace_printk.c +++ b/kernel/trace/trace_printk.c @@ -36,6 +36,10 @@ struct trace_bprintk_fmt { static inline struct trace_bprintk_fmt *lookup_format(const char *fmt) { struct trace_bprintk_fmt *pos; + + if (!fmt) + return ERR_PTR(-EINVAL); + list_for_each_entry(pos, &trace_bprintk_fmt_list, list) { if (!strcmp(pos->fmt, fmt)) return pos; @@ -57,7 +61,8 @@ void hold_module_trace_bprintk_format(const char **start, const char **end) for (iter = start; iter < end; iter++) { struct trace_bprintk_fmt *tb_fmt = lookup_format(*iter); if (tb_fmt) { - *iter = tb_fmt->fmt; + if (!IS_ERR(tb_fmt)) + *iter = tb_fmt->fmt; continue; }