[PATCH v2 0/4] Remove usage of list iterator after the loop body

[PATCH v2 0/4] Remove usage of list iterator after the loop body

[Jakob Koschel]

In preparation to limit the scope of a list iterator to the list
traversal loop, use a dedicated pointer as the iterator [1].

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/

v1->v2:
- fix reverse x-mas tree (Steven Rostedt)
- rename subject line (Steven Rostedt)
- add additional tracing patches to bundle them all up in one go

Sorry, I corrupted the Subject line on the previous send, please ignore :(

Jakob Koschel (4):
  tracing: Remove usage of list iterator after the loop body
  tracing: Remove usage of list iterator variable after the loop
  tracing: Replace usage of found with dedicated list iterator variable
  tracing: Remove check of list iterator against head past the loop body

 kernel/trace/ftrace.c               | 20 ++++++++++++--------
 kernel/trace/trace_eprobe.c         | 14 ++++++++------
 kernel/trace/trace_events.c         | 29 ++++++++++++++---------------
 kernel/trace/trace_events_hist.c    | 17 ++++++++---------
 kernel/trace/trace_events_trigger.c | 28 +++++++++++++---------------
 kernel/trace/trace_output.c         | 13 +++++++++----
 6 files changed, 64 insertions(+), 57 deletions(-)


base-commit: 7a3ecddc571cc3294e5d6bb5948ff2b0cfa12735
--
2.25.1

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

[Jakob Koschel]

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@gmail.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)
+		*list = &e->list;
+	else
+		*list = &ftrace_event_list;
 	return next;
 }
 
-- 
2.25.1

[PATCH v2 2/4] tracing: Remove usage of list iterator variable after the loop

[Jakob Koschel]

In preparation to limit the scope of a list iterator to the list
traversal loop, use a dedicated pointer to point to the found element
[1].

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 kernel/trace/trace_events.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index e11e167b7809..97c7eb2f55e5 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1723,9 +1723,9 @@ static LIST_HEAD(event_subsystems);
 
 static int subsystem_open(struct inode *inode, struct file *filp)
 {
+	struct trace_subsystem_dir *dir = NULL, *iter_dir;
+	struct trace_array *tr = NULL, *iter_tr;
 	struct event_subsystem *system = NULL;
-	struct trace_subsystem_dir *dir = NULL; /* Initialize for gcc */
-	struct trace_array *tr;
 	int ret;
 
 	if (tracing_is_disabled())
@@ -1734,14 +1734,16 @@ static int subsystem_open(struct inode *inode, struct file *filp)
 	/* Make sure the system still exists */
 	mutex_lock(&event_mutex);
 	mutex_lock(&trace_types_lock);
-	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
-		list_for_each_entry(dir, &tr->systems, list) {
-			if (dir == inode->i_private) {
+	list_for_each_entry(iter_tr, &ftrace_trace_arrays, list) {
+		list_for_each_entry(iter_dir, &iter_tr->systems, list) {
+			if (iter_dir == inode->i_private) {
 				/* Don't open systems with no events */
-				if (dir->nr_events) {
+				if (iter_dir->nr_events) {
 					__get_system_dir(dir);
 					system = dir->subsystem;
 				}
+				tr = iter_tr;
+				dir = iter_dir;
 				goto exit_loop;
 			}
 		}
@@ -1753,9 +1755,6 @@ static int subsystem_open(struct inode *inode, struct file *filp)
 	if (!system)
 		return -ENODEV;
 
-	/* Some versions of gcc think dir can be uninitialized here */
-	WARN_ON(!dir);
-
 	/* Still need to increment the ref count of the system */
 	if (trace_array_get(tr) < 0) {
 		put_system(dir);
-- 
2.25.1

[PATCH v2 3/4] tracing: Replace usage of found with dedicated list iterator variable

[Jakob Koschel]

To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.

To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].

This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 kernel/trace/trace_events_hist.c    | 17 ++++++++---------
 kernel/trace/trace_events_trigger.c | 28 +++++++++++++---------------
 2 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 44db5ba9cabb..7f60d04d5b6e 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -6089,32 +6089,31 @@ static void hist_unregister_trigger(char *glob,
 				    struct event_trigger_data *data,
 				    struct trace_event_file *file)
 {
+	struct event_trigger_data *test = NULL, *iter, *named_data = NULL;
 	struct hist_trigger_data *hist_data = data->private_data;
-	struct event_trigger_data *test, *named_data = NULL;
-	bool unregistered = false;
 
 	lockdep_assert_held(&event_mutex);
 
 	if (hist_data->attrs->name)
 		named_data = find_named_trigger(hist_data->attrs->name);
 
-	list_for_each_entry(test, &file->triggers, list) {
-		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
-			if (!hist_trigger_match(data, test, named_data, false))
+	list_for_each_entry(iter, &file->triggers, list) {
+		if (iter->cmd_ops->trigger_type == ETT_EVENT_HIST) {
+			if (!hist_trigger_match(data, iter, named_data, false))
 				continue;
-			unregistered = true;
-			list_del_rcu(&test->list);
+			test = iter;
+			list_del_rcu(&iter->list);
 			trace_event_trigger_enable_disable(file, 0);
 			update_cond_flag(file);
 			break;
 		}
 	}
 
-	if (unregistered && test->ops->free)
+	if (test && test->ops->free)
 		test->ops->free(test->ops, test);
 
 	if (hist_data->enable_timestamps) {
-		if (!hist_data->remove || unregistered)
+		if (!hist_data->remove || test)
 			tracing_set_filter_buffering(file->tr, false);
 	}
 }
diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
index 7eb9d04f1c2e..1ba54a489416 100644
--- a/kernel/trace/trace_events_trigger.c
+++ b/kernel/trace/trace_events_trigger.c
@@ -614,22 +614,21 @@ static void unregister_trigger(char *glob,
 			       struct event_trigger_data *test,
 			       struct trace_event_file *file)
 {
-	struct event_trigger_data *data;
-	bool unregistered = false;
+	struct event_trigger_data *data = NULL, *iter;
 
 	lockdep_assert_held(&event_mutex);
 
-	list_for_each_entry(data, &file->triggers, list) {
-		if (data->cmd_ops->trigger_type == test->cmd_ops->trigger_type) {
-			unregistered = true;
-			list_del_rcu(&data->list);
+	list_for_each_entry(iter, &file->triggers, list) {
+		if (iter->cmd_ops->trigger_type == test->cmd_ops->trigger_type) {
+			data = iter;
+			list_del_rcu(&iter->list);
 			trace_event_trigger_enable_disable(file, 0);
 			update_cond_flag(file);
 			break;
 		}
 	}
 
-	if (unregistered && data->ops->free)
+	if (data && data->ops->free)
 		data->ops->free(data->ops, data);
 }
 
@@ -1976,27 +1975,26 @@ void event_enable_unregister_trigger(char *glob,
 				     struct trace_event_file *file)
 {
 	struct enable_trigger_data *test_enable_data = test->private_data;
+	struct event_trigger_data *data = NULL, *iter;
 	struct enable_trigger_data *enable_data;
-	struct event_trigger_data *data;
-	bool unregistered = false;
 
 	lockdep_assert_held(&event_mutex);
 
-	list_for_each_entry(data, &file->triggers, list) {
-		enable_data = data->private_data;
+	list_for_each_entry(iter, &file->triggers, list) {
+		enable_data = iter->private_data;
 		if (enable_data &&
-		    (data->cmd_ops->trigger_type ==
+		    (iter->cmd_ops->trigger_type ==
 		     test->cmd_ops->trigger_type) &&
 		    (enable_data->file == test_enable_data->file)) {
-			unregistered = true;
-			list_del_rcu(&data->list);
+			data = iter;
+			list_del_rcu(&iter->list);
 			trace_event_trigger_enable_disable(file, 0);
 			update_cond_flag(file);
 			break;
 		}
 	}
 
-	if (unregistered && data->ops->free)
+	if (data && data->ops->free)
 		data->ops->free(data->ops, data);
 }
 
-- 
2.25.1

[PATCH v2 4/4] tracing: Remove check of list iterator against head past the loop body

[Jakob Koschel]

When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator value will be a bogus pointer
computed based on the head element.

While it is safe to use the pointer to determine if it was computed
based on the head element, either with list_entry_is_head() or
&pos->member == head, using the iterator variable after the loop should
be avoided.

In preparation to limit the scope of a list iterator to the list
traversal loop, use a dedicated pointer to point to the found element [1].

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 kernel/trace/ftrace.c       | 20 ++++++++++++--------
 kernel/trace/trace_eprobe.c | 14 ++++++++------
 kernel/trace/trace_events.c | 12 ++++++------
 3 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 4f1d2f5e7263..5c465e70d146 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4560,8 +4560,8 @@ register_ftrace_function_probe(char *glob, struct trace_array *tr,
 			       struct ftrace_probe_ops *probe_ops,
 			       void *data)
 {
+	struct ftrace_func_probe *probe = NULL, *iter;
 	struct ftrace_func_entry *entry;
-	struct ftrace_func_probe *probe;
 	struct ftrace_hash **orig_hash;
 	struct ftrace_hash *old_hash;
 	struct ftrace_hash *hash;
@@ -4580,11 +4580,13 @@ register_ftrace_function_probe(char *glob, struct trace_array *tr,
 
 	mutex_lock(&ftrace_lock);
 	/* Check if the probe_ops is already registered */
-	list_for_each_entry(probe, &tr->func_probes, list) {
-		if (probe->probe_ops == probe_ops)
+	list_for_each_entry(iter, &tr->func_probes, list) {
+		if (iter->probe_ops == probe_ops) {
+			probe = iter;
 			break;
+		}
 	}
-	if (&probe->list == &tr->func_probes) {
+	if (!probe) {
 		probe = kzalloc(sizeof(*probe), GFP_KERNEL);
 		if (!probe) {
 			mutex_unlock(&ftrace_lock);
@@ -4702,9 +4704,9 @@ int
 unregister_ftrace_function_probe_func(char *glob, struct trace_array *tr,
 				      struct ftrace_probe_ops *probe_ops)
 {
+	struct ftrace_func_probe *probe = NULL, *iter;
 	struct ftrace_ops_hash old_hash_ops;
 	struct ftrace_func_entry *entry;
-	struct ftrace_func_probe *probe;
 	struct ftrace_glob func_g;
 	struct ftrace_hash **orig_hash;
 	struct ftrace_hash *old_hash;
@@ -4732,11 +4734,13 @@ unregister_ftrace_function_probe_func(char *glob, struct trace_array *tr,
 
 	mutex_lock(&ftrace_lock);
 	/* Check if the probe_ops is already registered */
-	list_for_each_entry(probe, &tr->func_probes, list) {
-		if (probe->probe_ops == probe_ops)
+	list_for_each_entry(iter, &tr->func_probes, list) {
+		if (iter->probe_ops == probe_ops) {
+			probe = iter;
 			break;
+		}
 	}
-	if (&probe->list == &tr->func_probes)
+	if (!probe)
 		goto err_unlock_ftrace;
 
 	ret = -EINVAL;
diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 541aa13581b9..63e901a28425 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -650,7 +650,7 @@ static struct trace_event_functions eprobe_funcs = {
 static int disable_eprobe(struct trace_eprobe *ep,
 			  struct trace_array *tr)
 {
-	struct event_trigger_data *trigger;
+	struct event_trigger_data *trigger = NULL, *iter;
 	struct trace_event_file *file;
 	struct eprobe_data *edata;
 
@@ -658,14 +658,16 @@ static int disable_eprobe(struct trace_eprobe *ep,
 	if (!file)
 		return -ENOENT;
 
-	list_for_each_entry(trigger, &file->triggers, list) {
-		if (!(trigger->flags & EVENT_TRIGGER_FL_PROBE))
+	list_for_each_entry(iter, &file->triggers, list) {
+		if (!(iter->flags & EVENT_TRIGGER_FL_PROBE))
 			continue;
-		edata = trigger->private_data;
-		if (edata->ep == ep)
+		edata = iter->private_data;
+		if (edata->ep == ep) {
+			trigger = iter;
 			break;
+		}
 	}
-	if (list_entry_is_head(trigger, &file->triggers, list))
+	if (!trigger)
 		return -ENODEV;
 
 	list_del_rcu(&trigger->list);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 97c7eb2f55e5..c86cbb25879d 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2279,8 +2279,8 @@ static struct dentry *
 event_subsystem_dir(struct trace_array *tr, const char *name,
 		    struct trace_event_file *file, struct dentry *parent)
 {
+	struct event_subsystem *system, *iter;
 	struct trace_subsystem_dir *dir;
-	struct event_subsystem *system;
 	struct dentry *entry;
 
 	/* First see if we did not already create this dir */
@@ -2294,13 +2294,13 @@ event_subsystem_dir(struct trace_array *tr, const char *name,
 	}
 
 	/* Now see if the system itself exists. */
-	list_for_each_entry(system, &event_subsystems, list) {
-		if (strcmp(system->name, name) == 0)
+	system = NULL;
+	list_for_each_entry(iter, &event_subsystems, list) {
+		if (strcmp(iter->name, name) == 0) {
+			system = iter;
 			break;
+		}
 	}
-	/* Reset system variable when not found */
-	if (&system->list == &event_subsystems)
-		system = NULL;
 
 	dir = kmalloc(sizeof(*dir), GFP_KERNEL);
 	if (!dir)
-- 
2.25.1

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

[Steven Rostedt]

On Sat,  2 Apr 2022 12:33:38 +0200
Jakob Koschel <jakobkoschel@gmail.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@gmail.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;
>  }
>  

Re: [PATCH v2 2/4] tracing: Remove usage of list iterator variable after the loop

[Steven Rostedt]

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

This patch crashed in my testing.

> @@ -1734,14 +1734,16 @@ static int subsystem_open(struct inode *inode, struct file *filp)
>  	/* Make sure the system still exists */
>  	mutex_lock(&event_mutex);
>  	mutex_lock(&trace_types_lock);
> -	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
> -		list_for_each_entry(dir, &tr->systems, list) {
> -			if (dir == inode->i_private) {
> +	list_for_each_entry(iter_tr, &ftrace_trace_arrays, list) {
> +		list_for_each_entry(iter_dir, &iter_tr->systems, list) {
> +			if (iter_dir == inode->i_private) {
>  				/* Don't open systems with no events */
> -				if (dir->nr_events) {
> +				if (iter_dir->nr_events) {
>  					__get_system_dir(dir);
>  					system = dir->subsystem;

					system = NULL->subsystem


>  				}
> +				tr = iter_tr;
> +				dir = iter_dir;

But do not change that dir, move the setting above it. That is:

				tr = iter_tr;
				dir = iter_dir;
				if (iter_dir->nr_events) {
 					__get_system_dir(dir);
 					system = dir->subsystem;
				}

-- Steve

				
>  				goto exit_loop;
>  			}
>  		}

Re: [PATCH v2 3/4] tracing: Replace usage of found with dedicated list iterator variable

[Steven Rostedt]

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

> To move the list iterator variable into the list_for_each_entry_*()
> macro in the future it should be avoided to use the list iterator
> variable after the loop body.
> 
> To *never* use the list iterator variable after the loop it was
> concluded to use a separate iterator variable instead of a
> found boolean [1].
> 
> This removes the need to use a found variable and simply checking if
> the variable was set, can determine if the break/goto was hit.
> 
> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
> ---
>  kernel/trace/trace_events_hist.c    | 17 ++++++++---------
>  kernel/trace/trace_events_trigger.c | 28 +++++++++++++---------------
>  2 files changed, 21 insertions(+), 24 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 44db5ba9cabb..7f60d04d5b6e 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -6089,32 +6089,31 @@ static void hist_unregister_trigger(char *glob,
>  				    struct event_trigger_data *data,
>  				    struct trace_event_file *file)
>  {
> +	struct event_trigger_data *test = NULL, *iter, *named_data = NULL;
>  	struct hist_trigger_data *hist_data = data->private_data;
> -	struct event_trigger_data *test, *named_data = NULL;
> -	bool unregistered = false;
>  
>  	lockdep_assert_held(&event_mutex);
>  
>  	if (hist_data->attrs->name)
>  		named_data = find_named_trigger(hist_data->attrs->name);
>  
> -	list_for_each_entry(test, &file->triggers, list) {
> -		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
> -			if (!hist_trigger_match(data, test, named_data, false))
> +	list_for_each_entry(iter, &file->triggers, list) {
> +		if (iter->cmd_ops->trigger_type == ETT_EVENT_HIST) {
> +			if (!hist_trigger_match(data, iter, named_data, false))
>  				continue;
> -			unregistered = true;
> -			list_del_rcu(&test->list);
> +			test = iter;
> +			list_del_rcu(&iter->list);
>  			trace_event_trigger_enable_disable(file, 0);
>  			update_cond_flag(file);
>  			break;
>  		}
>  	}
>  
> -	if (unregistered && test->ops->free)
> +	if (test && test->ops->free)
>  		test->ops->free(test->ops, test);
>  
>  	if (hist_data->enable_timestamps) {
> -		if (!hist_data->remove || unregistered)
> +		if (!hist_data->remove || test)
>  			tracing_set_filter_buffering(file->tr, false);
>  	}
>  }
> diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
> index 7eb9d04f1c2e..1ba54a489416 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -614,22 +614,21 @@ static void unregister_trigger(char *glob,
>  			       struct event_trigger_data *test,
>  			       struct trace_event_file *file)
>  {
> -	struct event_trigger_data *data;
> -	bool unregistered = false;
> +	struct event_trigger_data *data = NULL, *iter;
>  
>  	lockdep_assert_held(&event_mutex);
>  
> -	list_for_each_entry(data, &file->triggers, list) {
> -		if (data->cmd_ops->trigger_type == test->cmd_ops->trigger_type) {
> -			unregistered = true;
> -			list_del_rcu(&data->list);
> +	list_for_each_entry(iter, &file->triggers, list) {
> +		if (iter->cmd_ops->trigger_type == test->cmd_ops->trigger_type) {
> +			data = iter;
> +			list_del_rcu(&iter->list);

Don't use the iterator in the logic.

			data = iter;
			list_del_rcu(&data->list);

  is just fine.

>  			trace_event_trigger_enable_disable(file, 0);
>  			update_cond_flag(file);
>  			break;
>  		}
>  	}
>  
> -	if (unregistered && data->ops->free)
> +	if (data && data->ops->free)
>  		data->ops->free(data->ops, data);
>  }
>  
> @@ -1976,27 +1975,26 @@ void event_enable_unregister_trigger(char *glob,
>  				     struct trace_event_file *file)
>  {
>  	struct enable_trigger_data *test_enable_data = test->private_data;
> +	struct event_trigger_data *data = NULL, *iter;
>  	struct enable_trigger_data *enable_data;
> -	struct event_trigger_data *data;
> -	bool unregistered = false;
>  
>  	lockdep_assert_held(&event_mutex);
>  
> -	list_for_each_entry(data, &file->triggers, list) {
> -		enable_data = data->private_data;
> +	list_for_each_entry(iter, &file->triggers, list) {
> +		enable_data = iter->private_data;
>  		if (enable_data &&
> -		    (data->cmd_ops->trigger_type ==
> +		    (iter->cmd_ops->trigger_type ==
>  		     test->cmd_ops->trigger_type) &&
>  		    (enable_data->file == test_enable_data->file)) {
> -			unregistered = true;
> -			list_del_rcu(&data->list);
> +			data = iter;
> +			list_del_rcu(&iter->list);

Same here.

And please rebase on top of Linus's lastest tree.

-- Steve


>  			trace_event_trigger_enable_disable(file, 0);
>  			update_cond_flag(file);
>  			break;
>  		}
>  	}
>  
> -	if (unregistered && data->ops->free)
> +	if (data && data->ops->free)
>  		data->ops->free(data->ops, data);
>  }
>  

[PATCH v2 0/4] Remove usage of list iterator after the loop body

[Jakob Koschel]

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].

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]

v1->v2:
- fix NULL ptr dereference in subsystem_open() (Steven Rostedt)
- don't use the iterator in PATCH 3/4 when not necessary (Steven Rostedt)

Jakob Koschel (4):
  tracing: Remove usage of list iterator after the loop body
  tracing: Remove usage of list iterator variable after the loop
  tracing: Replace usage of found with dedicated list iterator variable
  tracing: Remove check of list iterator against head past the loop body

 kernel/trace/ftrace.c               | 20 ++++++++++++--------
 kernel/trace/trace_eprobe.c         | 14 ++++++++------
 kernel/trace/trace_events.c         | 27 +++++++++++++--------------
 kernel/trace/trace_events_hist.c    | 15 +++++++--------
 kernel/trace/trace_events_trigger.c | 24 +++++++++++-------------
 kernel/trace/trace_output.c         | 13 +++++++++----
 6 files changed, 60 insertions(+), 53 deletions(-)


base-commit: 46cf2c613f4b10eb12f749207b0fd2c1bfae3088
--
2.25.1