ACK/Cmnt: [PATCH 1/1][linux-5.15] tracing/perf: Fix double put of trace event when init fails

[Stefan Bader]

On 28.08.22 04:02, Krister Johansen wrote:

BugLink: https://bugs.launchpad.net/bugs/1987232

> commit 7249921d94ff64f67b733eca0b68853a62032b3d upstream.
> 
> If in perf_trace_event_init(), the perf_trace_event_open() fails, then it
> will call perf_trace_event_unreg() which will not only unregister the perf
> trace event, but will also call the put() function of the tp_event.
> 
> The problem here is that the trace_event_try_get_ref() is called by the
> caller of perf_trace_event_init() and if perf_trace_event_init() returns a
> failure, it will then call trace_event_put(). But since the
> perf_trace_event_unreg() already called the trace_event_put() function, it
> triggers a WARN_ON().
> 
>   WARNING: CPU: 1 PID: 30309 at kernel/trace/trace_dynevent.c:46 trace_event_dyn_put_ref+0x15/0x20
> 
> If perf_trace_event_reg() does not call the trace_event_try_get_ref() then
> the perf_trace_event_unreg() should not be calling trace_event_put(). This
> breaks symmetry and causes bugs like these.
> 
> Pull out the trace_event_put() from perf_trace_event_unreg() and call it
> in the locations that perf_trace_event_unreg() is called. This not only
> fixes this bug, but also brings back the proper symmetry of the reg/unreg
> vs get/put logic.
> 
> Link: https://lore.kernel.org/all/cover.1660347763.git.kjlx@templeofstupid.com/
> Link: https://lkml.kernel.org/r/20220816192817.43d5e17f@gandalf.local.home
> 
> Cc: stable at vger.kernel.org
> Fixes: 1d18538e6a092 ("tracing: Have dynamic events have a ref counter")
> Reported-by: Krister Johansen <kjlx at templeofstupid.com>
> Reviewed-by: Krister Johansen <kjlx at templeofstupid.com>
> Tested-by: Krister Johansen <kjlx at templeofstupid.com>
> Acked-by: Jiri Olsa <jolsa at kernel.org>
> Signed-off-by: Steven Rostedt (Google) <rostedt at goodmis.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>

(cherry picked from commit 0d7970e8702b7d15293a844cb59ddd352c298f26 linux-5.15.y)
> Signed-off-by: Krister Johansen <kjlx at templeofstupid.com>
Acked-by: Stefan Bader <stefan.bader at canonical.com>
> ---

Hi Krister,

since a new cycle just stated and we are still about 10 releases of upstream 
stable behind I would guess this would probably still be a rather long time 
until it hits the normal way. So picking it up before sounds reasonable. Things 
need a little addition for process but that is not a big problem.

-Stefan

>   kernel/trace/trace_event_perf.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> index fba8cb77a73a..083f648e3265 100644
> --- a/kernel/trace/trace_event_perf.c
> +++ b/kernel/trace/trace_event_perf.c
> @@ -157,7 +157,7 @@ static void perf_trace_event_unreg(struct perf_event *p_event)
>   	int i;
>   
>   	if (--tp_event->perf_refcount > 0)
> -		goto out;
> +		return;
>   
>   	tp_event->class->reg(tp_event, TRACE_REG_PERF_UNREGISTER, NULL);
>   
> @@ -176,8 +176,6 @@ static void perf_trace_event_unreg(struct perf_event *p_event)
>   			perf_trace_buf[i] = NULL;
>   		}
>   	}
> -out:
> -	trace_event_put_ref(tp_event);
>   }
>   
>   static int perf_trace_event_open(struct perf_event *p_event)
> @@ -241,6 +239,7 @@ void perf_trace_destroy(struct perf_event *p_event)
>   	mutex_lock(&event_mutex);
>   	perf_trace_event_close(p_event);
>   	perf_trace_event_unreg(p_event);
> +	trace_event_put_ref(p_event->tp_event);
>   	mutex_unlock(&event_mutex);
>   }
>   
> @@ -292,6 +291,7 @@ void perf_kprobe_destroy(struct perf_event *p_event)
>   	mutex_lock(&event_mutex);
>   	perf_trace_event_close(p_event);
>   	perf_trace_event_unreg(p_event);
> +	trace_event_put_ref(p_event->tp_event);
>   	mutex_unlock(&event_mutex);
>   
>   	destroy_local_trace_kprobe(p_event->tp_event);
> @@ -347,6 +347,7 @@ void perf_uprobe_destroy(struct perf_event *p_event)
>   	mutex_lock(&event_mutex);
>   	perf_trace_event_close(p_event);
>   	perf_trace_event_unreg(p_event);
> +	trace_event_put_ref(p_event->tp_event);
>   	mutex_unlock(&event_mutex);
>   	destroy_local_trace_uprobe(p_event->tp_event);
>   }

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20220829/58bbcc03/attachment-0001.sig>