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

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

At 2025-06-03 17:20:04, "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.
>>
>> Oh! I was wrong, before commit a3c3c6667, "change state" happened *after* list_del_event()
>> >
>> > 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.
>>
>> I cannot agree with the reasoning,
>> The panic dose not happened when exit, it happened when reboot/shutdown.
>> (I close perf_event_open before reboot)
>> >
>> > 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().
>>
>> Any procedure to bring out the impact of this missed perf_cgroup_disable()?
>> My system seems all normal, where should I check it?
>
>Here is possible senario:
>  1. perf event open with cgroup.
>  2. perf event open with cpu event (no cgroup).
>  3. above task sets the cpuctx->cgrp the same to (1).
>  3. close (1) events.
>     here, perf_cgroup_event_disable() isn't called,
>     cpuctx->cgrp still point the cgroup.
>  4. by other task, the cgroup and is destroied.
>  5. close (2) events.
>     here, it is last event, in __perf_remove_from_context()
>     and last event, it calls update_cgrp_time_from_cpuctx(),
>     And this refers invalid pointer.

... seems too complicated to me to give it a try.
Would not destroying cgroup trigger some reaction to cpuctx? 
There has to be some connection between cgroup lifecycle and perf cpuctx->cgrp.
Have you try it out? or it is just theoretical? 


>
>> But to fix it,  isn't following change less aggressive?
>>         event_sched_out(event, ctx);
>> -       perf_event_set_state(event, min(event->state, state));
>>         if (flags & DETACH_GROUP)
>>                 perf_group_detach(event);
>>         if (flags & DETACH_CHILD)
>>                perf_child_detach(event);
>>         list_del_event(event, ctx);
>> +       perf_event_set_state(event, min(event->state, state));
>
>If perf_child_detach() is called first and perf_event_set_state() call,
>since the parent is removed in perf_child_detatced,
>It would be failed to account the total_enable_time which caculating
>child_event's enable_time too.

Thanks for clarifying this, 
So the whole point  in commit a3c3c6667 is to make  perf_event_set_state() happens before perf_child_detach(), right?
I feel I got lost somewhere when I rush to this suggestion. But I still don't understand why my patchv1  breaks commit 
a3c3c6667, really confused.


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