linux-kernel - Re: [PATCH] perf/core: Prevent WARN_ON(!ctx) in __free

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]

Message-ID: <CAKUZ0zKwPoh6MrtpN+gM_rsKV7_+yy-MgWNU=fs98BhwWKa-qQ@mail.gmail.com>
Date: Sun, 6 Apr 2025 03:20:00 -0400
From: Gabriel <gshahrouzi@...il.com>
To: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	linux-perf-users@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Ian Rogers <irogers@...gle.com>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	Shuah Khan <skhan@...uxfoundation.org>, linux-kernel-mentees@...ts.linux.dev, 
	syzkaller-bugs@...glegroups.com, 
	syzbot+ff3aa851d46ab82953a3@...kaller.appspotmail.com
Subject: Re: [PATCH] perf/core: Prevent WARN_ON(!ctx) in __free_event for
 partial init

On Sat, Apr 5, 2025 at 4:30 PM Gabriel Shahrouzi <gshahrouzi@...il.com> wrote:
>
> Move the get_ctx(child_ctx) call and the child_event->ctx assignment to
> occur immediately after the child event is allocated. Ensure that
> child_event->ctx is non-NULL before any subsequent error path within
> inherit_event calls free_event(), satisfying the assumptions of the
> cleanup code.
>
> Reported-by: syzbot+ff3aa851d46ab82953a3@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=ff3aa851d46ab82953a3
> Signed-off-by: Gabriel Shahrouzi <gshahrouzi@...il.com>
> ---
>  kernel/events/core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 0bb21659e252..153ba622cfa0 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -14016,6 +14016,9 @@ inherit_event(struct perf_event *parent_event,
>         if (IS_ERR(child_event))
>                 return child_event;
>
> +       get_ctx(child_ctx);
> +       child_event->ctx = child_ctx;
> +
>         pmu_ctx = find_get_pmu_context(child_event->pmu, child_ctx, child_event);
>         if (IS_ERR(pmu_ctx)) {
>                 free_event(child_event);
> @@ -14037,8 +14040,6 @@ inherit_event(struct perf_event *parent_event,
>                 return NULL;
>         }
>
> -       get_ctx(child_ctx);
> -
>         /*
>          * Make the child state follow the state of the parent event,
>          * not its attr.disabled bit.  We hold the parent's mutex,
> @@ -14059,7 +14060,6 @@ inherit_event(struct perf_event *parent_event,
>                 local64_set(&hwc->period_left, sample_period);
>         }
>
> -       child_event->ctx = child_ctx;
>         child_event->overflow_handler = parent_event->overflow_handler;
>         child_event->overflow_handler_context
>                 = parent_event->overflow_handler_context;
> --
> 2.43.0
>
I want to clarify three things [1] why the Fixes: tag was not used;
[2] what the author’s original intent was and how to maintain it; [3]
how to improve my patch.

[1] This bug is a side-effect of multiple interacting commits over
time (up to 15 years old), not a single regression. Bisection yielded
a false positive
(https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=00b35530811f2aa3d7ceec2dbada80861c7632a8),
and testing on an earlier branch (6.14-c8) hit unrelated issues,
confirming no single commit is responsible. A Fixes: tag is therefore
unsuitable.

[2] The code initially incremented refcount then assigned context
immediately after the child_event was created. Later, an early
validity check for child_event was added before the
refcount/assignment. Even later, a WARN_ON_ONCE cleanup check was
added, assuming event->ctx is valid if the pmu_ctx is valid.
The problem is that the WARN_ON_ONCE could trigger after the initial
check passed but before child_event->ctx was assigned, violating its
precondition. The solution is to assign child_event->ctx right after
its initial validation. This ensures the context exists for any
subsequent checks or cleanup routines, resolving the WARN_ON_ONCE.

[3]. Defer the refcount update and child_event->ctx assignment
directly after child_event->pmu_ctx is set but before checking if the
parent event is orphaned. The cleanup routine depends on
event->pmu_ctx being non-NULL before it verifies event->ctx is
non-NULL. This also maintains the author's original intent of passing
in child_ctx to find_get_pmu_context before its refcount/assignment.