lkml.org 
[lkml]   [2022]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3] perf/core: Fix reentry problem in perf_output_read_group
On Tue, Aug 16, 2022 at 05:31:27PM +0200, Peter Zijlstra wrote:
> 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))

Uh, should that `||` be `&&`, or am I missing the plot?

This'll warn if IRQs are enabled regardless of whether the mutex is held.

Thanks,
Mark.

> +#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)
>

\
 
 \ /
  Last update: 2022-08-16 18:41    [W:0.138 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site