* [PATCH] tracing: Add *iter check for NULL @ 2016-06-01 8:31 zhengjun.xing 2016-06-02 15:41 ` Namhyung Kim 2016-06-17 16:38 ` Steven Rostedt 0 siblings, 2 replies; 5+ messages in thread From: zhengjun.xing @ 2016-06-01 8:31 UTC (permalink / raw) To: rostedt, mingo; +Cc: linux-kernel, zhengjun.xing From: xingzhen <zhengjun.xing@intel.com> 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. Signed-off-by: xingzhen <zhengjun.xing@intel.com> --- 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); 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); } -- 1.9.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tracing: Add *iter check for NULL 2016-06-01 8:31 [PATCH] tracing: Add *iter check for NULL zhengjun.xing @ 2016-06-02 15:41 ` Namhyung Kim 2016-06-17 16:38 ` Steven Rostedt 1 sibling, 0 replies; 5+ messages in thread From: Namhyung Kim @ 2016-06-02 15:41 UTC (permalink / raw) To: zhengjun.xing; +Cc: Steven Rostedt, Ingo Molnar, LKML Hello, On Wed, Jun 1, 2016 at 5:31 PM, <zhengjun.xing@intel.com> wrote: > From: xingzhen <zhengjun.xing@intel.com> > > 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. > > Signed-off-by: xingzhen <zhengjun.xing@intel.com> Acked-by: Namhyung Kim <namhyung@kernel.org> Thanks, Namhyung > --- > 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); > 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); > } > > -- > 1.9.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tracing: Add *iter check for NULL 2016-06-01 8:31 [PATCH] tracing: Add *iter check for NULL zhengjun.xing 2016-06-02 15:41 ` Namhyung Kim @ 2016-06-17 16:38 ` Steven Rostedt 2016-06-17 18:24 ` Steven Rostedt 1 sibling, 1 reply; 5+ messages in thread From: Steven Rostedt @ 2016-06-17 16:38 UTC (permalink / raw) To: zhengjun.xing; +Cc: mingo, linux-kernel, Namhyung Kim On Wed, 1 Jun 2016 16:31:10 +0800 zhengjun.xing@intel.com wrote: > From: xingzhen <zhengjun.xing@intel.com> > > 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? > > Signed-off-by: xingzhen <zhengjun.xing@intel.com> > --- > 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. -- 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); > } > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tracing: Add *iter check for NULL 2016-06-17 16:38 ` Steven Rostedt @ 2016-06-17 18:24 ` Steven Rostedt 2016-06-20 0:12 ` Namhyung Kim 0 siblings, 1 reply; 5+ messages in thread From: Steven Rostedt @ 2016-06-17 18:24 UTC (permalink / raw) To: zhengjun.xing; +Cc: mingo, linux-kernel, Namhyung Kim On Fri, 17 Jun 2016 12:38:41 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Wed, 1 Jun 2016 16:31:10 +0800 > zhengjun.xing@intel.com wrote: > > > From: xingzhen <zhengjun.xing@intel.com> > > > > 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 <zhengjun.xing@intel.com> > > --- > > 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; } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tracing: Add *iter check for NULL 2016-06-17 18:24 ` Steven Rostedt @ 2016-06-20 0:12 ` Namhyung Kim 0 siblings, 0 replies; 5+ messages in thread From: Namhyung Kim @ 2016-06-20 0:12 UTC (permalink / raw) To: Steven Rostedt; +Cc: zhengjun.xing, mingo, linux-kernel On Fri, Jun 17, 2016 at 02:24:57PM -0400, Steven Rostedt wrote: > 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! Acked-by: Namhyung Kim <namhyung@kernel.org> Thanks, Namhyung > 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; > } > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-06-20 0:28 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-01 8:31 [PATCH] tracing: Add *iter check for NULL zhengjun.xing 2016-06-02 15:41 ` Namhyung Kim 2016-06-17 16:38 ` Steven Rostedt 2016-06-17 18:24 ` Steven Rostedt 2016-06-20 0:12 ` Namhyung Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
Powered by JetHome