Re: [PATCH] ftrace: remove check of list iterator against head past the loop body

[Jakob Koschel <jakobkoschel@gmail.com>]

> On 1. Apr 2022, at 01:10, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Fri, 1 Apr 2022 00:37:52 +0200
> Jakob Koschel <jakobkoschel@gmail.com> wrote:
> 
> Hi Jakob,
> 
> The patch looks fine, I have some small nits though.
> 
> First, the subject should actually be:
> 
> Subject: [PATCH] tracing: Remove check of list iterator against head past the loop body
> 
> Changes that only deal specifically with the function probes are labeled as
> "ftrace:", but the more generic changes that touches files outside of
> ftrace.c and fgraph.c should be "tracing:". Also, Linus prefers to have the
> next part of the subject start with a capital letter: "Remove" instead of
> "remove".

Thanks for the input. I've just seen that I have a few more changes that
I classified as 'tracing: *' so I'll bundle these up and resent altogether.

Also thanks for the advice on using a capital letter, I'll incorporate that.

> 
>> 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/ [1]
>> 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..096f5a83358d 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);
>> @@ -4704,7 +4706,7 @@ unregister_ftrace_function_probe_func(char *glob, struct trace_array *tr,
>> {
>> 	struct ftrace_ops_hash old_hash_ops;
>> 	struct ftrace_func_entry *entry;
>> -	struct ftrace_func_probe *probe;
>> +	struct ftrace_func_probe *probe = NULL, *iter;
> 
> Can you move this to the first declaration to keep the nice "upside-down
> x-mas tree" look.

Thanks, I'll fix that up. It seems like this is not applied on the entire kernel
making treewide changes a bit more difficult. Is it documented somewhere, which
parts of the kernel enforce this? Just looking two lines down from here it
seems to be 'broken' already so just from looking at existing code it's often
hard to judge.

> 
>> 	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 e11e167b7809..fe3dc157e635 100644
>> --- a/kernel/trace/trace_events.c
>> +++ b/kernel/trace/trace_events.c
>> @@ -2281,7 +2281,7 @@ event_subsystem_dir(struct trace_array *tr, const char *name,
>> 		 struct trace_event_file *file, struct dentry *parent)
>> {
>> 	struct trace_subsystem_dir *dir;
>> -	struct event_subsystem *system;
>> +	struct event_subsystem *system, *iter;
> 
> And move this above dir as well, for the same reason.
> 
> Thanks,
> 
> -- Steve
> 
> 
>> 	struct dentry *entry;
>> 
>> 	/* First see if we did not already create this dir */
>> @@ -2295,13 +2295,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)
>> 
>> base-commit: f82da161ea75dc4db21b2499e4b1facd36dab275


	Jakob