linux-kernel - Re: [PATCH 1/1] perf/core: fix dangling cgroup pointer in cpuctx

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: <aD6hVAuHGNZjrKpr@e129823.arm.com>
Date: Tue, 3 Jun 2025 08:16:36 +0100
From: Yeoreum Yun <yeoreum.yun@....com>
To: peterz@...radead.org, mingo@...hat.com, mingo@...nel.org,
	acme@...nel.org, namhyung@...nel.org, leo.yan@....com,
	mark.rutland@....com, alexander.shishkin@...ux.intel.com,
	jolsa@...nel.org, irogers@...gle.com, adrian.hunter@...el.com,
	kan.liang@...ux.intel.com
Cc: linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
	David Wang <00107082@....com>
Subject: Re: [PATCH 1/1] perf/core: fix dangling cgroup pointer in cpuctx

Hi David,

> >
> > >
> > > Also, your patch couldn't solve a problem describe in
> > > commit a3c3c6667("perf/core: Fix child_total_time_enabled accounting bug at task exit")
> > > for INCATIVE event's total_enable_time.
> >
> > I do not think so.
> > Correct me if I am making silly  mistakes,
> > The patch, https://lore.kernel.org/lkml/20250603032651.3988-1-00107082@163.com/
> > calls perf_event_set_state() based on DETACH_EXIT flag, which cover the INACTIVE state, right?
> > If DETACH_EXIT is not used for this purpose? Then why should it exist at the first place?
> > I think I does not revert the purpose of commit a3c3c6667.....But I could be wrong
> > Would you show a call path where DETACH_EXIT is not set, but the changes in commit a3c3c6667 is still needed?
> >
> > Sorry for my bad explaination without detail.
> > Think about cpu specific event and closed by task.
> > If there is specific child cpu event specified in cpu 0.
> > 1. cpu 0 -> active
> > 2. scheulded to cpu1 -> inactive
> > 3. close the cpu event from parent -> inactive close
> >
> > Can be failed to count total_enable_time.
>
> Is this explaining the purpose of commit a3c3c6667 ?
> I am not arguing with it. And I also not suggest reverting it. (it is just that reverting it can fix the kernel panic.)

In commit a3c3c6667, I explain the specific case but not with above
case. But the commit's purpose is "account total_enable_time" properly.

> > And also, considering the your patch, for DETACH_EXIT case,
> > If it changes the state before list_del_event() that wouldn't disable
> > related to the cgroup. So it would make cpuctx->cgrp pointer could be dangled
> > as patch describe...
> No, I don't think so.
> change state before list_del_event(), this is the same behavior before commit a3c3c6667, right?
> And no such kernel panic happened  before commit a3c3c6667.

That's why list_del_event() handle the perf_cgroup_disable() before the
commit a3c3c6667. However because of *my mistake*, I've forget to
perf_cgroup_disable() properly before change the event state.
Yes, your patch can make avoid the panic since as soon as exit,
the event->cgrp switched.

However, as I said, the INACTIVE event could be failed to count
total_enable_time.

So, set event should be occured before list_del_event().
And since it's event->state change on remove.
It shouldn't have any side effect the state change isn't cause of your
panic. But missed perf_cgroup_disable().

Thanks.
--
Sincerely,
Yeoreum Yun