* Re: [bug report] tracing: Factorize filter creation
2018-03-23 8:46 [bug report] tracing: Factorize filter creation Dan Carpenter
@ 2018-03-23 8:49 ` Dan Carpenter
2018-03-26 14:32 ` Tejun Heo
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2018-03-23 8:49 UTC (permalink / raw)
To: kernel-janitors
Hm... No. I get a second warning:
kernel/trace/trace_events_filter.c:2052 ftrace_profile_set_filter()
error: uninitialized symbol 'filter'.
kernel/trace/trace_events_filter.c
2036
2037 err = -EEXIST;
2038 if (event->filter)
2039 goto out_unlock;
2040
2041 err = create_filter(call, filter_str, false, &filter);
2042 if (err)
2043 goto free_filter;
^^^^^^^^^^^
filter is uninitialized. What are we supposed to be freeing?
2044
2045 if (ftrace_event_is_function(call))
2046 err = ftrace_function_set_filter(event, filter);
2047 else
2048 event->filter = filter;
2049
2050 free_filter:
2051 if (err || ftrace_event_is_function(call))
2052 __free_filter(filter);
2053
2054 out_unlock:
2055 mutex_unlock(&event_mutex);
2056
2057 return err;
2058 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [bug report] tracing: Factorize filter creation
2018-03-23 8:46 [bug report] tracing: Factorize filter creation Dan Carpenter
2018-03-23 8:49 ` Dan Carpenter
@ 2018-03-26 14:32 ` Tejun Heo
2018-03-26 14:36 ` Tejun Heo
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Tejun Heo @ 2018-03-26 14:32 UTC (permalink / raw)
To: kernel-janitors
On Fri, Mar 23, 2018 at 11:46:12AM +0300, Dan Carpenter wrote:
> Hello Tejun Heo,
>
> The patch 38b78eb85540: "tracing: Factorize filter creation" from Dec
> 15, 2011, leads to the following static checker warning:
>
> kernel/trace/trace_events_filter.c:1794 apply_event_filter()
> error: uninitialized symbol 'filter'.
>
> kernel/trace/trace_events_filter.c
> 1764 int apply_event_filter(struct trace_event_file *file, char *filter_string)
...
> 1785
> 1786 err = create_filter(call, filter_string, true, &filter);
> ^^^^^^^
> 1787
> 1788 /*
> 1789 * Always swap the call filter with the new filter
> 1790 * even if there was an error. If there was an error
> 1791 * in the filter, we disable the filter and show the error
> 1792 * string
> 1793 */
> 1794 if (filter) {
> ^^^^^^
> I guess the fix is probably to set filter to NULL in create_filter()?
@filterp is the outparam and create_filter() always sets it, so the
code doesn't look wrong to me.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [bug report] tracing: Factorize filter creation
2018-03-23 8:46 [bug report] tracing: Factorize filter creation Dan Carpenter
2018-03-23 8:49 ` Dan Carpenter
2018-03-26 14:32 ` Tejun Heo
@ 2018-03-26 14:36 ` Tejun Heo
2018-03-26 16:07 ` Dan Carpenter
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Tejun Heo @ 2018-03-26 14:36 UTC (permalink / raw)
To: kernel-janitors
Hey, Dan.
On Fri, Mar 23, 2018 at 11:49:32AM +0300, Dan Carpenter wrote:
> Hm... No. I get a second warning:
>
> kernel/trace/trace_events_filter.c:2052 ftrace_profile_set_filter()
> error: uninitialized symbol 'filter'.
>
> kernel/trace/trace_events_filter.c
> 2036
> 2037 err = -EEXIST;
> 2038 if (event->filter)
> 2039 goto out_unlock;
> 2040
> 2041 err = create_filter(call, filter_str, false, &filter);
> 2042 if (err)
> 2043 goto free_filter;
> ^^^^^^^^^^^
> filter is uninitialized. What are we supposed to be freeing?
>
> 2044
> 2045 if (ftrace_event_is_function(call))
> 2046 err = ftrace_function_set_filter(event, filter);
> 2047 else
> 2048 event->filter = filter;
> 2049
> 2050 free_filter:
> 2051 if (err || ftrace_event_is_function(call))
> 2052 __free_filter(filter);
This does look suspicious but __free_filter() checks for NULL input
and please take a look at the following comment from create_filter().
* On success, returns 0 and *@filterp points to the new filter. On
* failure, returns -errno and *@filterp may point to %NULL or to a new
* filter. In the latter case, the returned filter contains error
* information if @set_str is %true and the caller is responsible for
* freeing it.
It looks like the function could return !NULL filter w/ error. Kinda
confusing but the code doesn't look wrong.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [bug report] tracing: Factorize filter creation
2018-03-23 8:46 [bug report] tracing: Factorize filter creation Dan Carpenter
` (2 preceding siblings ...)
2018-03-26 14:36 ` Tejun Heo
@ 2018-03-26 16:07 ` Dan Carpenter
2018-03-26 16:35 ` Tejun Heo
2018-03-27 7:23 ` Dan Carpenter
5 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2018-03-26 16:07 UTC (permalink / raw)
To: kernel-janitors
On Mon, Mar 26, 2018 at 07:32:26AM -0700, Tejun Heo wrote:
> On Fri, Mar 23, 2018 at 11:46:12AM +0300, Dan Carpenter wrote:
> > Hello Tejun Heo,
> >
> > The patch 38b78eb85540: "tracing: Factorize filter creation" from Dec
> > 15, 2011, leads to the following static checker warning:
> >
> > kernel/trace/trace_events_filter.c:1794 apply_event_filter()
> > error: uninitialized symbol 'filter'.
> >
> > kernel/trace/trace_events_filter.c
> > 1764 int apply_event_filter(struct trace_event_file *file, char *filter_string)
> ...
> > 1785
> > 1786 err = create_filter(call, filter_string, true, &filter);
> > ^^^^^^^
> > 1787
> > 1788 /*
> > 1789 * Always swap the call filter with the new filter
> > 1790 * even if there was an error. If there was an error
> > 1791 * in the filter, we disable the filter and show the error
> > 1792 * string
> > 1793 */
> > 1794 if (filter) {
> > ^^^^^^
> > I guess the fix is probably to set filter to NULL in create_filter()?
>
> @filterp is the outparam and create_filter() always sets it, so the
> code doesn't look wrong to me.
It's not set if create_filter_start() fails at the start of
create_filter(). I'm looking at today's linux-next.
1686 /**
1687 * create_filter - create a filter for a trace_event_call
1688 * @call: trace_event_call to create a filter for
1689 * @filter_str: filter string
1690 * @set_str: remember @filter_str and enable detailed error in filter
1691 * @filterp: out param for created filter (always updated on return)
^^^^^^^^^^^^^^^^^^^^^^^^
Ah... You're right about the comment.
1692 *
1693 * Creates a filter for @call with @filter_str. If @set_str is %true,
1694 * @filter_str is copied and recorded in the new filter.
1695 *
1696 * On success, returns 0 and *@filterp points to the new filter. On
1697 * failure, returns -errno and *@filterp may point to %NULL or to a new
1698 * filter. In the latter case, the returned filter contains error
1699 * information if @set_str is %true and the caller is responsible for
1700 * freeing it.
1701 */
1702 static int create_filter(struct trace_event_call *call,
1703 char *filter_string, bool set_str,
1704 struct event_filter **filterp)
1705 {
1706 struct filter_parse_error *pe = NULL;
1707 struct event_filter *filter = NULL;
1708 int err;
1709
1710 err = create_filter_start(filter_string, set_str, &pe, &filter);
1711 if (err)
1712 return err;
^^^^^^^^^^
But it's not set here.
1713
1714 err = process_preds(call, filter_string, filter, pe);
1715 if (err && set_str)
1716 append_filter_err(pe, filter);
1717
1718 *filterp = filter;
1719 return err;
1720 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [bug report] tracing: Factorize filter creation
2018-03-23 8:46 [bug report] tracing: Factorize filter creation Dan Carpenter
` (3 preceding siblings ...)
2018-03-26 16:07 ` Dan Carpenter
@ 2018-03-26 16:35 ` Tejun Heo
2018-03-27 7:23 ` Dan Carpenter
5 siblings, 0 replies; 7+ messages in thread
From: Tejun Heo @ 2018-03-26 16:35 UTC (permalink / raw)
To: kernel-janitors
On Mon, Mar 26, 2018 at 07:07:35PM +0300, Dan Carpenter wrote:
> 1710 err = create_filter_start(filter_string, set_str, &pe, &filter);
> 1711 if (err)
> 1712 return err;
> ^^^^^^^^^^
> But it's not set here.
Hah, that looks different from linus#master. Anyways, yeah, if
anything isn't setting it, it'd be a bug. I'm not very familiar with
events code tho. I think it'd be best to report them to the perf
maintainers.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [bug report] tracing: Factorize filter creation
2018-03-23 8:46 [bug report] tracing: Factorize filter creation Dan Carpenter
` (4 preceding siblings ...)
2018-03-26 16:35 ` Tejun Heo
@ 2018-03-27 7:23 ` Dan Carpenter
5 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2018-03-27 7:23 UTC (permalink / raw)
To: kernel-janitors
On Mon, Mar 26, 2018 at 09:35:32AM -0700, Tejun Heo wrote:
> On Mon, Mar 26, 2018 at 07:07:35PM +0300, Dan Carpenter wrote:
> > 1710 err = create_filter_start(filter_string, set_str, &pe, &filter);
> > 1711 if (err)
> > 1712 return err;
> > ^^^^^^^^^^
> > But it's not set here.
>
> Hah, that looks different from linus#master. Anyways, yeah, if
> anything isn't setting it, it'd be a bug. I'm not very familiar with
> events code tho. I think it'd be best to report them to the perf
> maintainers.
>
Oh wow... Your patch is from 2011. Sorry about that. :P
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread