[bug report] tracing: Factorize filter creation

[bug report] tracing: Factorize filter creation

[Dan Carpenter]

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)
  1765  {
  1766          struct trace_event_call *call = file->event_call;
  1767          struct event_filter *filter;
                                    ^^^^^^^
  1768          int err;
  1769  
  1770          if (!strcmp(strstrip(filter_string), "0")) {
  1771                  filter_disable(file);
  1772                  filter = event_filter(file);
  1773  
  1774                  if (!filter)
  1775                          return 0;
  1776  
  1777                  event_clear_filter(file);
  1778  
  1779                  /* Make sure the filter is not being used */
  1780                  synchronize_sched();
  1781                  __free_filter(filter);
  1782  
  1783                  return 0;
  1784          }
  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()?

  1795                  struct event_filter *tmp;
  1796  
  1797                  tmp = event_filter(file);
  1798                  if (!err)
  1799                          event_set_filtered_flag(file);
  1800                  else
  1801                          filter_disable(file);
  1802  
  1803                  event_set_filter(file, filter);
  1804  
  1805                  if (tmp) {
  1806                          /* Make sure the call is done with the filter */
  1807                          synchronize_sched();

regards,
dan carpenter

Re: [bug report] tracing: Factorize filter creation

[Dan Carpenter]

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

Re: [bug report] tracing: Factorize filter creation

[Tejun Heo]

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

Re: [bug report] tracing: Factorize filter creation

[Tejun Heo]

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

Re: [bug report] tracing: Factorize filter creation

[Dan Carpenter]

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

Re: [bug report] tracing: Factorize filter creation

[Tejun Heo]

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

Re: [bug report] tracing: Factorize filter creation

[Dan Carpenter]

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