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 <[email protected]>
Closes: https://lore.kernel.org/lkml/[email protected]/
Reported-by: Pengfei Xu <[email protected]>
Closes: https://lore.kernel.org/lkml/ZXpm6gQ%[email protected]/
Signed-off-by: Mark Rutland <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Peter Zijlstra <[email protected]>
---
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.
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
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 <[email protected]>
> Closes: https://lore.kernel.org/lkml/[email protected]/
> Reported-by: Pengfei Xu <[email protected]>
> Closes: https://lore.kernel.org/lkml/ZXpm6gQ%[email protected]/
> Signed-off-by: Mark Rutland <[email protected]>
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
>
The following commit has been merged into the perf/urgent branch of tip:
Commit-ID: 7e2c1e4b34f07d9aa8937fab88359d4a0fce468e
Gitweb: https://git.kernel.org/tip/7e2c1e4b34f07d9aa8937fab88359d4a0fce468e
Author: Mark Rutland <[email protected]>
AuthorDate: Fri, 15 Dec 2023 11:24:50
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Fri, 15 Dec 2023 12:33:23 +01:00
perf: Fix perf_event_validate_size() lockdep splat
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()")
Closes: https://lore.kernel.org/lkml/[email protected]/
Closes: https://lore.kernel.org/lkml/ZXpm6gQ%[email protected]/
Reported-by: Lucas De Marchi <[email protected]>
Reported-by: Pengfei Xu <[email protected]>
Signed-off-by: Mark Rutland <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/events/core.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index c9d123e..9efd0d7 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)
Hi,
I tested the following patch on top of v6.7-rc5 kernel.
This perf issue was gone.
It's fixed.
Attached is the fixed dmesg log.
Thanks!
On 2023-12-15 at 19:42:58 +0800, tip-bot2 for Mark Rutland wrote:
> The following commit has been merged into the perf/urgent branch of tip:
>
> Commit-ID: 7e2c1e4b34f07d9aa8937fab88359d4a0fce468e
> Gitweb: https://git.kernel.org/tip/7e2c1e4b34f07d9aa8937fab88359d4a0fce468e
> Author: Mark Rutland <[email protected]>
> AuthorDate: Fri, 15 Dec 2023 11:24:50
> Committer: Peter Zijlstra <[email protected]>
> CommitterDate: Fri, 15 Dec 2023 12:33:23 +01:00
>
> perf: Fix perf_event_validate_size() lockdep splat
>
> 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()")
> Closes: https://lore.kernel.org/lkml/[email protected]/
> Closes: https://lore.kernel.org/lkml/ZXpm6gQ%[email protected]/
> Reported-by: Lucas De Marchi <[email protected]>
> Reported-by: Pengfei Xu <[email protected]>
> Signed-off-by: Mark Rutland <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Link: https://lkml.kernel.org/r/[email protected]
> ---
> kernel/events/core.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index c9d123e..9efd0d7 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)
Tested-by: Pengfei Xu <[email protected]>