tracing: Factorize filter creation

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

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

...

@filterp is the outparam and create_filter() always sets it, so the code doesn't look wrong to me.

Thanks.

tejun

Hey, Dan.

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

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

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

Oh wow... Your patch is from 2011. Sorry about that. :P

regards,
dan carpenter