linux-kernel - Re: [tip:tracing/core] tracing: trace_output.c, fix false positive compiler warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.00.0905061041450.3239@gandalf.stny.rr.com>
Date:	Wed, 6 May 2009 10:49:30 -0400 (EDT)
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Ingo Molnar <mingo@...e.hu>
cc:	mingo@...hat.com, hpa@...or.com, linux-kernel@...r.kernel.org,
	jaswinder@...nel.org, jaswinderrajput@...il.com,
	tglx@...utronix.de, linux-tip-commits@...r.kernel.org
Subject: Re: [tip:tracing/core] tracing: trace_output.c, fix false positive
 compiler warning


On Wed, 6 May 2009, Ingo Molnar wrote:
> > > ---
> > >  kernel/trace/trace_output.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
> > > index 5fc51f0..8bd9a2c 100644
> > > --- a/kernel/trace/trace_output.c
> > > +++ b/kernel/trace/trace_output.c
> > > @@ -541,7 +541,7 @@ int register_ftrace_event(struct trace_event *event)
> > >  	INIT_LIST_HEAD(&event->list);
> > >  
> > >  	if (!event->type) {
> > > -		struct list_head *list;
> > > +		struct list_head *list = NULL;
> > 
> > Actually this is the wrong place to initialize. The correct place 
> > is in the function that is expected to.
> 
> does it really matter? It's far more robust to initialize at the 
> definition site, because there we can be sure there's no 
> side-effects. This one:
> 
> >  	/* Did we used up all 65 thousand events??? */
> > -	if ((last + 1) > FTRACE_MAX_EVENT)
> > +	if ((last + 1) > FTRACE_MAX_EVENT) {
> > +		*list = NULL;
> >  		return 0;
> > +	}
> 
> Is correct too but needs a semantic check (and ongoing maintenance, 
> etc.).

Actually, to answer this, we need to look at the entire code. Just looking 
at the changes of a patch does not include the big picture.

The original code is:

static int trace_search_list(struct list_head **list)
{
        struct trace_event *e;
        int last = __TRACE_LAST_TYPE;

        if (list_empty(&ftrace_event_list)) {
                *list = &ftrace_event_list;
                return last + 1;
        }

        /*
         * We used up all possible max events,
         * lets see if somebody freed one.
         */
        list_for_each_entry(e, &ftrace_event_list, list) {
                if (e->type != last + 1)
                        break;
                last++;
        }

        /* Did we used up all 65 thousand events??? */
        if ((last + 1) > FTRACE_MAX_EVENT)
                return 0;

        *list = &e->list;
        return last + 1;
}

[...]
                struct list_head *list;

                if (next_event_type > FTRACE_MAX_EVENT) {

                        event->type = trace_search_list(&list);
                        if (!event->type)
                                goto out;

                } else {

                        event->type = next_event_type++;
                        list = &ftrace_event_list;
                }

                if (WARN_ON(ftrace_find_event(event->type)))
                        goto out;

                list_add_tail(&event->list, list);



The caller is:

	struct list_head *list;

	if () {
		event->type = trace_seach_list(&list);
	} else {
		[...]
		list = &ftrace_event_list;
	}

This code shows that list is initialized by either trace_seach_list() or 
set manually.

Thus, my change makes trace_search_list always initialize the list 
variable. Thus if trace_search_list() is used someplace else, it will not 
cause us this error again.

If gcc can not figure out that trace_search_list initializes the code 
(from the original code), the

	struct list_head *list = NULL;

will always be performed, because gcc thinks that's the only way to 
guarantee that it will be initialized.

My solution, gcc can easily see that trace_search_list will always 
initialize list, and will not set it needlessly to NULL.

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/