linux-kernel - Re: [PATCH v2 1/4] tracing: Remove usage of list iterator after the loop body

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220422185922.15fe4614@gandalf.local.home>
Date:   Fri, 22 Apr 2022 18:59:22 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Jakob Koschel <jakobkoschel@...il.com>
Cc:     Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
        Mike Rapoport <rppt@...nel.org>,
        "Brian Johannesmeyer" <bjohannesmeyer@...il.com>,
        Cristiano Giuffrida <c.giuffrida@...nl>,
        "Bos, H.J." <h.j.bos@...nl>
Subject: Re: [PATCH v2 1/4] tracing: Remove usage of list iterator after the
 loop body

On Sat,  2 Apr 2022 12:33:38 +0200
Jakob Koschel <jakobkoschel@...il.com> wrote:

> In preparation to limit the scope of the list iterator variable to the
> traversal loop, use a dedicated pointer to point to the found element
> [1].
> 
> Before, the code implicitly used the head when no element was found
> when using &pos->list. Since the new variable is only set if an
> element was found, the head needs to be used explicitly if the
> variable is NULL.
> 
> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <jakobkoschel@...il.com>
> ---
>  kernel/trace/trace_output.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
> index 8aa493d25c73..733a4d6c20e2 100644
> --- a/kernel/trace/trace_output.c
> +++ b/kernel/trace/trace_output.c
> @@ -692,7 +692,7 @@ static LIST_HEAD(ftrace_event_list);
>  
>  static int trace_search_list(struct list_head **list)
>  {
> -	struct trace_event *e;
> +	struct trace_event *e = NULL, *iter;
>  	int next = __TRACE_LAST_TYPE;
>  
>  	if (list_empty(&ftrace_event_list)) {
> @@ -704,9 +704,11 @@ static int trace_search_list(struct list_head **list)
>  	 * 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 != next)
> +	list_for_each_entry(iter, &ftrace_event_list, list) {
> +		if (iter->type != next) {
> +			e = iter;
>  			break;
> +		}
>  		next++;
>  	}
>  
> @@ -714,7 +716,10 @@ static int trace_search_list(struct list_head **list)
>  	if (next > TRACE_EVENT_TYPE_MAX)
>  		return 0;
>  
> -	*list = &e->list;
> +	if (e)

Technically, if e was NULL, then next would be too big. But as that's very
subtle with the algorithm, I'm fine adding this.

-- Steve


> +		*list = &e->list;
> +	else
> +		*list = &ftrace_event_list;
>  	return next;
>  }
>