linux-kernel - Re: [PATCH v3] perf/core: Fix reentry problem in perf_output_read

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 16 Aug 2022 17:31:27 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Mark Rutland <mark.rutland@....com>
Cc:     Yang Jihong <yangjihong1@...wei.com>, rostedt@...dmis.org,
        mingo@...hat.com, acme@...nel.org,
        alexander.shishkin@...ux.intel.com, jolsa@...nel.org,
        namhyung@...nel.org, linux-kernel@...r.kernel.org,
        linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v3] perf/core: Fix reentry problem in
 perf_output_read_group

On Tue, Aug 16, 2022 at 03:54:19PM +0100, Mark Rutland wrote:
> > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> > index ee8b9ecdc03b..d4d53b9ba71e 100644
> > --- a/include/linux/perf_event.h
> > +++ b/include/linux/perf_event.h
> > @@ -631,7 +631,12 @@ struct pmu_event_list {
> >  	struct list_head	list;
> >  };
> >  
> > +/*
> > + * Iterating the sibling list requires this list to be stable; by ensuring IRQs
> > + * are disabled IPIs from perf_{install_in,remove_from}_context() are held off.
> > + */
> >  #define for_each_sibling_event(sibling, event)			\
> > +	lockdep_assert_irqs_disabled();				\
> >  	if ((event)->group_leader == (event))			\
> >  		list_for_each_entry((sibling), &(event)->sibling_list, sibling_list)
> >  
> 
> I had a go with v6.0-rc1 and Vince's perf fuzzer immediately triggered a bunch
> of cases (dump below).
> 
> I had thought holding the context mutex protected some of these cases, even
> with IRQs unmasked?

Ah yes.. duh. How's this then?


diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index ee8b9ecdc03b..4d9cf508c510 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -631,7 +631,21 @@ struct pmu_event_list {
 	struct list_head	list;
 };
 
+#ifdef CONFIG_LOCKDEP
+#define LOCKDEP_ASSERT_EVENT_CTX(event)				\
+	WARN_ON_ONCE(__lockdep_enabled &&			\
+		     (this_cpu_read(hardirqs_enabled) ||	\
+		      lockdep_is_held(&(event)->ctx->mutex) != LOCK_STATE_HELD))
+#else
+#define LOCKDEP_ASSERT_EVENT_CTX(event)
+#endif
+
+/*
+ * Iterating the sibling list requires this list to be stable; by ensuring IRQs
+ * are disabled IPIs from perf_{install_in,remove_from}_context() are held off.
+ */
 #define for_each_sibling_event(sibling, event)			\
+	LOCKDEP_ASSERT_EVENT_CTX(event);			\
 	if ((event)->group_leader == (event))			\
 		list_for_each_entry((sibling), &(event)->sibling_list, sibling_list)