re: tracing: Add 'hist' event trigger command

re: tracing: Add 'hist' event trigger command

[Dan Carpenter]

Hello Tom Zanussi,

This is a semi-automatic email about new static checker warnings.

The patch 7ef224d1d0e3: "tracing: Add 'hist' event trigger command" 
from Mar 3, 2016, leads to the following Smatch complaint:

kernel/trace/trace_events_hist.c:374 create_hist_field()
	 error: we previously assumed 'field' could be null (see line 352)

kernel/trace/trace_events_hist.c
   351	
   352		if (field && is_function_field(field))
                    ^^^^^
New check for NULL.

   353			return NULL;
   354	
   355		hist_field = kzalloc(sizeof(struct hist_field), GFP_KERNEL);
   356		if (!hist_field)
   357			return NULL;
   358	
   359		if (flags & HIST_FIELD_FL_HITCOUNT) {
   360			hist_field->fn = hist_field_counter;
   361			goto out;
   362		}
   363	
   364		if (flags & HIST_FIELD_FL_STACKTRACE) {
   365			hist_field->fn = hist_field_none;
   366			goto out;
   367		}
   368	
   369		if (flags & HIST_FIELD_FL_LOG2) {
   370			hist_field->fn = hist_field_log2;
   371			goto out;
   372		}
   373	
   374		if (is_string_field(field)) {
                                    ^^^^^
New unchecked dereference inside function.

   375			flags |= HIST_FIELD_FL_STRING;
   376	

regards,
dan carpenter

Re: tracing: Add 'hist' event trigger command

[Tom Zanussi]

Hi Dan,

Thanks for flagging this, but I don't think it's really a problem in 
this case - the cases where field could be NULL are checked and the 
function exits before is_string_field() is called.

Though I agree it would be better to make an explicit check - I'll write 
a patch to do that..

Thanks,

Tom

On 04/22/2016 04:54 AM, Dan Carpenter wrote:
> Hello Tom Zanussi,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 7ef224d1d0e3: "tracing: Add 'hist' event trigger command"
> from Mar 3, 2016, leads to the following Smatch complaint:
>
> kernel/trace/trace_events_hist.c:374 create_hist_field()
> 	 error: we previously assumed 'field' could be null (see line 352)
>
> kernel/trace/trace_events_hist.c
>     351	
>     352		if (field && is_function_field(field))
>                      ^^^^^
> New check for NULL.
>
>     353			return NULL;
>     354	
>     355		hist_field = kzalloc(sizeof(struct hist_field), GFP_KERNEL);
>     356		if (!hist_field)
>     357			return NULL;
>     358	
>     359		if (flags & HIST_FIELD_FL_HITCOUNT) {
>     360			hist_field->fn = hist_field_counter;
>     361			goto out;
>     362		}
>     363	
>     364		if (flags & HIST_FIELD_FL_STACKTRACE) {
>     365			hist_field->fn = hist_field_none;
>     366			goto out;
>     367		}
>     368	
>     369		if (flags & HIST_FIELD_FL_LOG2) {
>     370			hist_field->fn = hist_field_log2;
>     371			goto out;
>     372		}
>     373	
>     374		if (is_string_field(field)) {
>                                      ^^^^^
> New unchecked dereference inside function.
>
>     375			flags |= HIST_FIELD_FL_STRING;
>     376	
>
> regards,
> dan carpenter
>

Re: tracing: Add 'hist' event trigger command

[Steven Rostedt]

On Fri, 22 Apr 2016 10:14:30 -0500
Tom Zanussi <tom.zanussi@linux.intel.com> wrote:


> On 04/22/2016 04:54 AM, Dan Carpenter wrote:
> > Hello Tom Zanussi,
> >
> > This is a semi-automatic email about new static checker warnings.
> >
> > The patch 7ef224d1d0e3: "tracing: Add 'hist' event trigger command"
> > from Mar 3, 2016, leads to the following Smatch complaint:
> >
> > kernel/trace/trace_events_hist.c:374 create_hist_field()
> > 	 error: we previously assumed 'field' could be null (see line 352)
> >
> > kernel/trace/trace_events_hist.c
> >     351	
> >     352		if (field && is_function_field(field))
> >                      ^^^^^
> > New check for NULL.
> >
> >     353			return NULL;
> >     354	
> >     355		hist_field = kzalloc(sizeof(struct hist_field), GFP_KERNEL);
> >     356		if (!hist_field)
> >     357			return NULL;
> >     358	
> >     359		if (flags & HIST_FIELD_FL_HITCOUNT) {
> >     360			hist_field->fn = hist_field_counter;
> >     361			goto out;
> >     362		}
> >     363	
> >     364		if (flags & HIST_FIELD_FL_STACKTRACE) {
> >     365			hist_field->fn = hist_field_none;
> >     366			goto out;
> >     367		}
> >     368	
> >     369		if (flags & HIST_FIELD_FL_LOG2) {
> >     370			hist_field->fn = hist_field_log2;
> >     371			goto out;
> >     372		}
> >     373	

Perhaps add here:

	if (WARN_ON_ONCE(!field))
		goto out;

-- Steve

> >     374		if (is_string_field(field)) {
> >                                      ^^^^^
> > New unchecked dereference inside function.
> >
> >     375			flags |= HIST_FIELD_FL_STRING;
> >     376	
> >
> > regards,
> > dan carpenter
> >  

Re: tracing: Add 'hist' event trigger command

[Dan Carpenter]

On Fri, Apr 22, 2016 at 10:14:30AM -0500, Tom Zanussi wrote:
> Hi Dan,
> 
> Thanks for flagging this, but I don't think it's really a problem in
> this case - the cases where field could be NULL are checked and the
> function exits before is_string_field() is called.
> 

It's likely that I will be able to silence that false positive in Smatch
sometime before the end of the year.

regards,
dan carpenter