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

["David Wang" <00107082@163.com>]

At 2025-06-03 15:16:36, "Yeoreum Yun" <yeoreum.yun@arm.com> wrote:
>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.

This time, you mean my patch, right?
Still have trouble understand this. My patch dose not change the logic along  DETACH_EXIT at all.
Could you elaborate on it more? What change my patch made to break commit  a3c3c6667?
(Hope you are not still talking the purpose of commit a3c3c6667)

>
>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().

I may understand it wrong, but I think the kernel panic is caused by missing 
perf_cgroup_event_disable(), 
when reboot/shutdown, kernel were waiting cgroup ctx reference drop to 0, for ever, and hence the panic.




>
>Thanks.
>--
>Sincerely,
>Yeoreum Yun