diff mbox series

perf: Fix perf_event_validate_size() lockdep splat

Message ID 20231215112450.3972309-1-mark.rutland@arm.com
State New
Headers
Series perf: Fix perf_event_validate_size() lockdep splat |

Commit Message

Mark Rutland Dec. 15, 2023, 11:24 a.m. UTC 
  When lockdep is enabled, the for_each_sibling_event(sibling, event)
macro checks that event->ctx->mutex is held. When creating a new group
leader event, we call perf_event_validate_size() on a partially
initialized event where event->ctx is NULL, and so when
for_each_sibling_event() attempts to check event->ctx->mutex, we get a
splat, as reported by Lucas De Marchi:

  WARNING: CPU: 8 PID: 1471 at kernel/events/core.c:1950 __do_sys_perf_event_open+0xf37/0x1080

This only happens for a new event which is its own group_leader, and in
this case there cannot be any sibling events. Thus it's safe to skip the
check for siblings, which avoids having to make invasive and ugly
changes to for_each_sibling_event().

Avoid the splat by bailing out early when the new event is its own
group_leader.

Fixes: 382c27f4ed28f803 ("perf: Fix perf_event_validate_size()")
Reported-by: Lucas De Marchi <lucas.demarchi@intel.com>
Closes: https://lore.kernel.org/lkml/20231214000620.3081018-1-lucas.demarchi@intel.com/
Reported-by: Pengfei Xu <pengfei.xu@intel.com>
Closes: https://lore.kernel.org/lkml/ZXpm6gQ%2Fd59jGsuW@xpf.sh.intel.com/
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 kernel/events/core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Hi Ingo, Boris, Peter,

I'm not sure who's still around and who has disappeared for the
holidays, but I'm hoping at least one of you is able to queue this. I've
tested the patch on arm64 with Syzkaller (and syz-repro); before this
patch it hits the splat near-instantly, and after this patch all seems
well.

The broken commit was merged in v6.7-rc5 via:

  https://lore.kernel.org/lkml/20231210105949.GAZXWaJe6DeHU9+ofl@fat_crate.local/

... in merge commit:

  537ccb5d28d6f398 ("Merge tag 'perf_urgent_for_v6.7_rc5' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip")

Mark.

Comments

Peter Zijlstra Dec. 15, 2023, 11:35 a.m. UTC | #1 
On Fri, Dec 15, 2023 at 11:24:50AM +0000, Mark Rutland wrote:
> When lockdep is enabled, the for_each_sibling_event(sibling, event)
> macro checks that event->ctx->mutex is held. When creating a new group
> leader event, we call perf_event_validate_size() on a partially
> initialized event where event->ctx is NULL, and so when
> for_each_sibling_event() attempts to check event->ctx->mutex, we get a
> splat, as reported by Lucas De Marchi:
> 
>   WARNING: CPU: 8 PID: 1471 at kernel/events/core.c:1950 __do_sys_perf_event_open+0xf37/0x1080
> 
> This only happens for a new event which is its own group_leader, and in
> this case there cannot be any sibling events. Thus it's safe to skip the
> check for siblings, which avoids having to make invasive and ugly
> changes to for_each_sibling_event().
> 
> Avoid the splat by bailing out early when the new event is its own
> group_leader.
> 
> Fixes: 382c27f4ed28f803 ("perf: Fix perf_event_validate_size()")
> Reported-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Closes: https://lore.kernel.org/lkml/20231214000620.3081018-1-lucas.demarchi@intel.com/
> Reported-by: Pengfei Xu <pengfei.xu@intel.com>
> Closes: https://lore.kernel.org/lkml/ZXpm6gQ%2Fd59jGsuW@xpf.sh.intel.com/
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>

Urgh, my bad indeed.

> ---
>  kernel/events/core.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> Hi Ingo, Boris, Peter,
> 
> I'm not sure who's still around and who has disappeared for the
> holidays, but I'm hoping at least one of you is able to queue this. I've
> tested the patch on arm64 with Syzkaller (and syz-repro); before this
> patch it hits the splat near-instantly, and after this patch all seems
> well.

I'm still here today, let me stick this in perf/urgent.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index c9d123e13b579..9efd0d7775e7c 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -1947,6 +1947,16 @@ static bool perf_event_validate_size(struct perf_event *event)
>  				   group_leader->nr_siblings + 1) > 16*1024)
>  		return false;
>  
> +	/*
> +	 * When creating a new group leader, group_leader->ctx is initialized
> +	 * after the size has been validated, but we cannot safely use
> +	 * for_each_sibling_event() until group_leader->ctx is set. A new group
> +	 * leader cannot have any siblings yet, so we can safely skip checking
> +	 * the non-existent siblings.
> +	 */
> +	if (event == group_leader)
> +		return true;
> +
>  	for_each_sibling_event(sibling, group_leader) {
>  		if (__perf_event_read_size(sibling->attr.read_format,
>  					   group_leader->nr_siblings + 1) > 16*1024)
> -- 
> 2.30.2
>
diff mbox series

Patch

diff --git a/kernel/events/core.c b/kernel/events/core.c
index c9d123e13b579..9efd0d7775e7c 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1947,6 +1947,16 @@  static bool perf_event_validate_size(struct perf_event *event)
 				   group_leader->nr_siblings + 1) > 16*1024)
 		return false;
 
+	/*
+	 * When creating a new group leader, group_leader->ctx is initialized
+	 * after the size has been validated, but we cannot safely use
+	 * for_each_sibling_event() until group_leader->ctx is set. A new group
+	 * leader cannot have any siblings yet, so we can safely skip checking
+	 * the non-existent siblings.
+	 */
+	if (event == group_leader)
+		return true;
+
 	for_each_sibling_event(sibling, group_leader) {
 		if (__perf_event_read_size(sibling->attr.read_format,
 					   group_leader->nr_siblings + 1) > 16*1024)