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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Sat, 2 Apr 2022 12:33:38 +0200
Jakob Koschel <[email protected]> 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 <[email protected]>
> ---
> 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;
> }
>
On Sat, 2 Apr 2022 12:33:39 +0200
Jakob Koschel <[email protected]> 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;
> }
> }