[PATCH v2] tracing/eprobe: Install the trigger for every instance that enables the eprobe

[PATCH v2] tracing/eprobe: Install the trigger for every instance that enables the eprobe

[David Carlier]

The eprobe trigger lives in the instance of the enabling file, but it is
only added when the eprobe is first enabled and only removed when the
last file goes away. A second instance then records nothing, and the
first instance's trigger is left behind once the eprobe is disabled
there, so the eprobe can be deleted while that trigger still uses it.

Add and remove the trigger for each file. The file is never NULL here,
so warn if it is and drop the dead perf paths.

Fixes: 7491e2c44278 ("tracing: Add a probe that attaches to trace events")
Cc: stable@vger.kernel.org

Signed-off-by: David Carlier <devnexen@gmail.com>
---
Changes in v2:
- WARN and return -EINVAL on a NULL file, drop the dead !file paths (Masami)
- v1: https://lore.kernel.org/linux-trace-kernel/20261031203915.491866-1-devnexen@gmail.com/

 kernel/trace/trace_eprobe.c | 56 ++++++++++++-------------------------
 1 file changed, 18 insertions(+), 38 deletions(-)

diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 998e6390937a..474be2f5ccd9 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -644,37 +644,32 @@ static int enable_trace_eprobe(struct trace_event_call *call,
 {
 	struct trace_probe *tp;
 	struct trace_eprobe *ep;
-	bool enabled;
 	int ret = 0;
 	int cnt = 0;
 
+	if (WARN_ON_ONCE(!file))
+		return -EINVAL;
+
 	tp = trace_probe_primary_from_call(call);
 	if (WARN_ON_ONCE(!tp))
 		return -ENODEV;
-	enabled = trace_probe_is_enabled(tp);
 
 	/* This also changes "enabled" state */
-	if (file) {
-		ret = trace_probe_add_file(tp, file);
-		if (ret)
-			return ret;
-	} else
-		trace_probe_set_flag(tp, TP_FLAG_PROFILE);
-
-	if (enabled)
-		return 0;
+	ret = trace_probe_add_file(tp, file);
+	if (ret)
+		return ret;
 
+	/* The trigger lives in the instance of the file, add it for each file */
 	for_each_trace_eprobe_tp(ep, tp) {
 		ret = enable_eprobe(ep, file);
 		if (ret)
 			break;
-		enabled = true;
 		cnt++;
 	}
 
 	if (ret) {
 		/* Failed to enable one of them. Roll back all */
-		if (enabled) {
+		if (cnt) {
 			/*
 			 * It's a bug if one failed for something other than memory
 			 * not being available but another eprobe succeeded.
@@ -687,10 +682,7 @@ static int enable_trace_eprobe(struct trace_event_call *call,
 					break;
 			}
 		}
-		if (file)
-			trace_probe_remove_file(tp, file);
-		else
-			trace_probe_clear_flag(tp, TP_FLAG_PROFILE);
+		trace_probe_remove_file(tp, file);
 	}
 
 	return ret;
@@ -702,33 +694,21 @@ static int disable_trace_eprobe(struct trace_event_call *call,
 	struct trace_probe *tp;
 	struct trace_eprobe *ep;
 
+	if (WARN_ON_ONCE(!file))
+		return -EINVAL;
+
 	tp = trace_probe_primary_from_call(call);
 	if (WARN_ON_ONCE(!tp))
 		return -ENODEV;
 
-	if (file) {
-		if (!trace_probe_get_file_link(tp, file))
-			return -ENOENT;
-		if (!trace_probe_has_single_file(tp))
-			goto out;
-		trace_probe_clear_flag(tp, TP_FLAG_TRACE);
-	} else
-		trace_probe_clear_flag(tp, TP_FLAG_PROFILE);
+	if (!trace_probe_get_file_link(tp, file))
+		return -ENOENT;
 
-	if (!trace_probe_is_enabled(tp)) {
-		for_each_trace_eprobe_tp(ep, tp)
-			disable_eprobe(ep, file->tr);
-	}
+	for_each_trace_eprobe_tp(ep, tp)
+		disable_eprobe(ep, file->tr);
 
- out:
-	if (file)
-		/*
-		 * Synchronization is done in below function. For perf event,
-		 * file == NULL and perf_trace_event_unreg() calls
-		 * tracepoint_synchronize_unregister() to ensure synchronize
-		 * event. We don't need to care about it.
-		 */
-		trace_probe_remove_file(tp, file);
+	/* Synchronization is done in below function. */
+	trace_probe_remove_file(tp, file);
 
 	return 0;
 }
-- 
2.55.0

Re: [PATCH v2] tracing/eprobe: Install the trigger for every instance that enables the eprobe

[Masami Hiramatsu (Google)]

On Sat,  7 Nov 2026 10:12:32 +0000, David Carlier <devnexen@gmail.com> wrote:
> The eprobe trigger lives in the instance of the enabling file, but it is
> only added when the eprobe is first enabled and only removed when the
> last file goes away. A second instance then records nothing, and the
> first instance's trigger is left behind once the eprobe is disabled
> there, so the eprobe can be deleted while that trigger still uses it.
>
> Add and remove the trigger for each file. The file is never NULL here,
> so warn if it is and drop the dead perf paths.
>
> Fixes: 7491e2c44278 ("tracing: Add a probe that attaches to trace events")
> Cc: stable@vger.kernel.org
>
> Signed-off-by: David Carlier <devnexen@gmail.com>

This looks good to me. let me pick it.

Thanks,

> ---
> Changes in v2:
> - WARN and return -EINVAL on a NULL file, drop the dead !file paths (Masami)
> - v1: https://lore.kernel.org/linux-trace-kernel/20261031203915.491866-1-devnexen@gmail.com/
>
>  kernel/trace/trace_eprobe.c | 56 ++++++++++++-------------------------
>  1 file changed, 18 insertions(+), 38 deletions(-)
>
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index 998e6390937a..474be2f5ccd9 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -644,37 +644,32 @@ static int enable_trace_eprobe(struct trace_event_call *call,
>  {
>  	struct trace_probe *tp;
>  	struct trace_eprobe *ep;
> -	bool enabled;
>  	int ret = 0;
>  	int cnt = 0;
>  
> +	if (WARN_ON_ONCE(!file))
> +		return -EINVAL;
> +
>  	tp = trace_probe_primary_from_call(call);
>  	if (WARN_ON_ONCE(!tp))
>  		return -ENODEV;
> -	enabled = trace_probe_is_enabled(tp);
>  
>  	/* This also changes "enabled" state */
> -	if (file) {
> -		ret = trace_probe_add_file(tp, file);
> -		if (ret)
> -			return ret;
> -	} else
> -		trace_probe_set_flag(tp, TP_FLAG_PROFILE);
> -
> -	if (enabled)
> -		return 0;
> +	ret = trace_probe_add_file(tp, file);
> +	if (ret)
> +		return ret;
>  
> +	/* The trigger lives in the instance of the file, add it for each file */
>  	for_each_trace_eprobe_tp(ep, tp) {
>  		ret = enable_eprobe(ep, file);
>  		if (ret)
>  			break;
> -		enabled = true;
>  		cnt++;
>  	}
>  
>  	if (ret) {
>  		/* Failed to enable one of them. Roll back all */
> -		if (enabled) {
> +		if (cnt) {
>  			/*
>  			 * It's a bug if one failed for something other than memory
>  			 * not being available but another eprobe succeeded.
> @@ -687,10 +682,7 @@ static int enable_trace_eprobe(struct trace_event_call *call,
>  					break;
>  			}
>  		}
> -		if (file)
> -			trace_probe_remove_file(tp, file);
> -		else
> -			trace_probe_clear_flag(tp, TP_FLAG_PROFILE);
> +		trace_probe_remove_file(tp, file);
>  	}
>  
>  	return ret;
> @@ -702,33 +694,21 @@ static int disable_trace_eprobe(struct trace_event_call *call,
>  	struct trace_probe *tp;
>  	struct trace_eprobe *ep;
>  
> +	if (WARN_ON_ONCE(!file))
> +		return -EINVAL;
> +
>  	tp = trace_probe_primary_from_call(call);
>  	if (WARN_ON_ONCE(!tp))
>  		return -ENODEV;
>  
> -	if (file) {
> -		if (!trace_probe_get_file_link(tp, file))
> -			return -ENOENT;
> -		if (!trace_probe_has_single_file(tp))
> -			goto out;
> -		trace_probe_clear_flag(tp, TP_FLAG_TRACE);
> -	} else
> -		trace_probe_clear_flag(tp, TP_FLAG_PROFILE);
> +	if (!trace_probe_get_file_link(tp, file))
> +		return -ENOENT;
>  
> -	if (!trace_probe_is_enabled(tp)) {
> -		for_each_trace_eprobe_tp(ep, tp)
> -			disable_eprobe(ep, file->tr);
> -	}
> +	for_each_trace_eprobe_tp(ep, tp)
> +		disable_eprobe(ep, file->tr);
>  
> - out:
> -	if (file)
> -		/*
> -		 * Synchronization is done in below function. For perf event,
> -		 * file == NULL and perf_trace_event_unreg() calls
> -		 * tracepoint_synchronize_unregister() to ensure synchronize
> -		 * event. We don't need to care about it.
> -		 */
> -		trace_probe_remove_file(tp, file);
> +	/* Synchronization is done in below function. */
> +	trace_probe_remove_file(tp, file);
>  
>  	return 0;
>  }
>

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>