[PATCH 0/5] ftrace: Use guard to take ftrace_lock

[PATCH 0/5] ftrace: Use guard to take ftrace_lock

[Steven Rostedt]

The ftrace infrastructure uses ftrace_lock to protect several critical
paths. But the functions have that take the ftrace_lock has error paths that
require the lock to be released. Use the guard() helper to take the
ftrace_lock in theses functions which simplify the early exit paths.

Steven Rostedt (5):
      fgraph: Use guard(mutex)(&ftrace_lock) for unregister_ftrace_graph()
      ftrace: Use guard for match_records()
      ftrace: Use guard to lock ftrace_lock in cache_mod()
      ftrace: Use guard to take the ftrace_lock in release_probe()
      ftrace: Use guard to take ftrace_lock in ftrace_graph_set_hash()

----
 kernel/trace/fgraph.c | 10 ++++------
 kernel/trace/ftrace.c | 53 +++++++++++++++++----------------------------------
 2 files changed, 21 insertions(+), 42 deletions(-)

[PATCH 1/5] fgraph: Use guard(mutex)(&ftrace_lock) for unregister_ftrace_graph()

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

The ftrace_lock is held throughout unregister_ftrace_graph(), use a guard
to simplify the error paths.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/fgraph.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index 001abf376c0c..0bf78517b5d4 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -1381,17 +1381,17 @@ void unregister_ftrace_graph(struct fgraph_ops *gops)
 {
 	int command = 0;
 
-	mutex_lock(&ftrace_lock);
+	guard(mutex)(&ftrace_lock);
 
 	if (unlikely(!ftrace_graph_active))
-		goto out;
+		return;
 
 	if (unlikely(gops->idx < 0 || gops->idx >= FGRAPH_ARRAY_SIZE ||
 		     fgraph_array[gops->idx] != gops))
-		goto out;
+		return;
 
 	if (fgraph_lru_release_index(gops->idx) < 0)
-		goto out;
+		return;
 
 	fgraph_array[gops->idx] = &fgraph_stub;
 
@@ -1413,7 +1413,5 @@ void unregister_ftrace_graph(struct fgraph_ops *gops)
 		unregister_pm_notifier(&ftrace_suspend_notifier);
 		unregister_trace_sched_switch(ftrace_graph_probe_sched_switch, NULL);
 	}
- out:
 	gops->saved_func = NULL;
-	mutex_unlock(&ftrace_lock);
 }
-- 
2.45.2

[PATCH 2/5] ftrace: Use guard for match_records()

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

The ftrace_lock is held for most of match_records() until the end of the
function. Use guard to make error paths simpler.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index e9fd4fb2769e..44adc34643c9 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4829,15 +4829,13 @@ match_records(struct ftrace_hash *hash, char *func, int len, char *mod)
 		mod_g.len = strlen(mod_g.search);
 	}
 
-	mutex_lock(&ftrace_lock);
+	guard(mutex)(&ftrace_lock);
 
 	if (unlikely(ftrace_disabled))
-		goto out_unlock;
+		return 0;
 
-	if (func_g.type == MATCH_INDEX) {
-		found = add_rec_by_index(hash, &func_g, clear_filter);
-		goto out_unlock;
-	}
+	if (func_g.type == MATCH_INDEX)
+		return add_rec_by_index(hash, &func_g, clear_filter);
 
 	do_for_each_ftrace_rec(pg, rec) {
 
@@ -4846,16 +4844,12 @@ match_records(struct ftrace_hash *hash, char *func, int len, char *mod)
 
 		if (ftrace_match_record(rec, &func_g, mod_match, exclude_mod)) {
 			ret = enter_record(hash, rec, clear_filter);
-			if (ret < 0) {
-				found = ret;
-				goto out_unlock;
-			}
+			if (ret < 0)
+				return ret;
 			found = 1;
 		}
 		cond_resched();
 	} while_for_each_ftrace_rec();
- out_unlock:
-	mutex_unlock(&ftrace_lock);
 
 	return found;
 }
-- 
2.45.2

[PATCH 3/5] ftrace: Use guard to lock ftrace_lock in cache_mod()

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

The ftrace_lock is held throughout cache_mod(), use guard to simplify the
error paths.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 44adc34643c9..64997416415e 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4947,14 +4947,14 @@ static int cache_mod(struct trace_array *tr,
 {
 	struct ftrace_mod_load *ftrace_mod, *n;
 	struct list_head *head = enable ? &tr->mod_trace : &tr->mod_notrace;
-	int ret;
 
-	mutex_lock(&ftrace_lock);
+	guard(mutex)(&ftrace_lock);
 
 	/* We do not cache inverse filters */
 	if (func[0] == '!') {
+		int ret = -EINVAL;
+
 		func++;
-		ret = -EINVAL;
 
 		/* Look to remove this hash */
 		list_for_each_entry_safe(ftrace_mod, n, head, list) {
@@ -4970,20 +4970,15 @@ static int cache_mod(struct trace_array *tr,
 				continue;
 			}
 		}
-		goto out;
+		return ret;
 	}
 
-	ret = -EINVAL;
 	/* We only care about modules that have not been loaded yet */
 	if (module_exists(module))
-		goto out;
+		return -EINVAL;
 
 	/* Save this string off, and execute it when the module is loaded */
-	ret = ftrace_add_mod(tr, func, module, enable);
- out:
-	mutex_unlock(&ftrace_lock);
-
-	return ret;
+	return ftrace_add_mod(tr, func, module, enable);
 }
 
 static int
-- 
2.45.2

[PATCH 4/5] ftrace: Use guard to take the ftrace_lock in release_probe()

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

The ftrace_lock is held throughout the entire release_probe() function.
Use guard to simplify any exit paths.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 64997416415e..c0fabd7da5b2 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5288,7 +5288,7 @@ static void release_probe(struct ftrace_func_probe *probe)
 {
 	struct ftrace_probe_ops *probe_ops;
 
-	mutex_lock(&ftrace_lock);
+	guard(mutex)(&ftrace_lock);
 
 	WARN_ON(probe->ref <= 0);
 
@@ -5306,7 +5306,6 @@ static void release_probe(struct ftrace_func_probe *probe)
 		list_del(&probe->list);
 		kfree(probe);
 	}
-	mutex_unlock(&ftrace_lock);
 }
 
 static void acquire_probe_locked(struct ftrace_func_probe *probe)
-- 
2.45.2

[PATCH 5/5] ftrace: Use guard to take ftrace_lock in ftrace_graph_set_hash()

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

The ftrace_lock is taken for most of the ftrace_graph_set_hash() function
throughout the end. Use guard to take the ftrace_lock to simplify the exit
paths.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index c0fabd7da5b2..b4ef469f4fd2 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -6816,12 +6816,10 @@ ftrace_graph_set_hash(struct ftrace_hash *hash, char *buffer)
 
 	func_g.len = strlen(func_g.search);
 
-	mutex_lock(&ftrace_lock);
+	guard(mutex)(&ftrace_lock);
 
-	if (unlikely(ftrace_disabled)) {
-		mutex_unlock(&ftrace_lock);
+	if (unlikely(ftrace_disabled))
 		return -ENODEV;
-	}
 
 	do_for_each_ftrace_rec(pg, rec) {
 
@@ -6837,7 +6835,7 @@ ftrace_graph_set_hash(struct ftrace_hash *hash, char *buffer)
 				if (entry)
 					continue;
 				if (add_hash_entry(hash, rec->ip) == NULL)
-					goto out;
+					return 0;
 			} else {
 				if (entry) {
 					free_hash_entry(hash, entry);
@@ -6846,13 +6844,8 @@ ftrace_graph_set_hash(struct ftrace_hash *hash, char *buffer)
 			}
 		}
 	} while_for_each_ftrace_rec();
-out:
-	mutex_unlock(&ftrace_lock);
 
-	if (fail)
-		return -EINVAL;
-
-	return 0;
+	return fail ? -EINVAL : 0;
 }
 
 static ssize_t
-- 
2.45.2

Re: [PATCH 5/5] ftrace: Use guard to take ftrace_lock in ftrace_graph_set_hash()

[Peter Zijlstra]

On Mon, Oct 28, 2024 at 03:12:33AM -0400, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@goodmis.org>
> 
> The ftrace_lock is taken for most of the ftrace_graph_set_hash() function
> throughout the end. Use guard to take the ftrace_lock to simplify the exit
> paths.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  kernel/trace/ftrace.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index c0fabd7da5b2..b4ef469f4fd2 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -6816,12 +6816,10 @@ ftrace_graph_set_hash(struct ftrace_hash *hash, char *buffer)
>  
>  	func_g.len = strlen(func_g.search);
>  
> -	mutex_lock(&ftrace_lock);
> +	guard(mutex)(&ftrace_lock);
>  
> -	if (unlikely(ftrace_disabled)) {
> -		mutex_unlock(&ftrace_lock);
> +	if (unlikely(ftrace_disabled))
>  		return -ENODEV;
> -	}
>  
>  	do_for_each_ftrace_rec(pg, rec) {
>  
> @@ -6837,7 +6835,7 @@ ftrace_graph_set_hash(struct ftrace_hash *hash, char *buffer)
>  				if (entry)
>  					continue;
>  				if (add_hash_entry(hash, rec->ip) == NULL)
> -					goto out;
> +					return 0;
>  			} else {
>  				if (entry) {
>  					free_hash_entry(hash, entry);
> @@ -6846,13 +6844,8 @@ ftrace_graph_set_hash(struct ftrace_hash *hash, char *buffer)
>  			}
>  		}
>  	} while_for_each_ftrace_rec();
> -out:
> -	mutex_unlock(&ftrace_lock);
>  
> -	if (fail)
> -		return -EINVAL;
> -
> -	return 0;
> +	return fail ? -EINVAL : 0;
>  }

Isn't the fail case more a case of -ESRCH / -ENOENT rather than -EINVAL?

Anyway, that's orthogonal, the patch preserves existing semantics and
looks okay (as do the others fwiw).

Re: [PATCH 5/5] ftrace: Use guard to take ftrace_lock in ftrace_graph_set_hash()

[Steven Rostedt]

On Mon, 28 Oct 2024 10:16:56 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> > @@ -6846,13 +6844,8 @@ ftrace_graph_set_hash(struct ftrace_hash *hash, char *buffer)
> >  			}
> >  		}
> >  	} while_for_each_ftrace_rec();
> > -out:
> > -	mutex_unlock(&ftrace_lock);
> >  
> > -	if (fail)
> > -		return -EINVAL;
> > -
> > -	return 0;
> > +	return fail ? -EINVAL : 0;
> >  }  
> 
> Isn't the fail case more a case of -ESRCH / -ENOENT rather than -EINVAL?

Could be. Although this is mostly for internal use. I should check to
see if this gets back to user space. And yeah, it probably should be
changed.

> 
> Anyway, that's orthogonal, the patch preserves existing semantics and
> looks okay (as do the others fwiw).

Thanks for the review!

-- Steve