linux-kernel - Re: [PATCH 1/1] tracing: fix a WARN from trace_event_dyn_put

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 16 Aug 2022 13:48:43 -0700
From:   Krister Johansen <kjlx@...pleofstupid.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Ingo Molnar <mingo@...hat.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        David Reaver <me@...idreaver.com>,
        linux-kernel@...r.kernel.org, Jiri Olsa <jolsa@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Frederic Weisbecker <fweisbec@...il.com>
Subject: Re: [PATCH 1/1] tracing: fix a WARN from trace_event_dyn_put_ref

On Tue, Aug 16, 2022 at 03:24:11PM -0400, Steven Rostedt wrote:
> Actually, I think the issue is that the unreg() calls trace_event_put_ref()
> but the reg() does not do the try_get_ref(), but the perf_trace_init()
> does. Which is the broken symmetry.
> 
> I think if we pull out the trace_event_put_ref() from the unreg() function,
> we fix the bug and also put back the symmetry.
> 
> Does this patch work?

Yes, thanks, this change looks correct.  It also does build and pass the
tests that I included in the original e-mail.

> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> index a114549720d6..61e3a2620fa3 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);
>  }
> 

If it matters:

Reviewed-by: Krister Johansen <kjlx@...pleofstupid.com>
Tested-by: Krister Johansen <kjlx@...pleofstupid.com>

Thanks again,

-K