Re: [PATCH] perf/x86/amd: check event before enable to avoid GPF

[George Kennedy <george.kennedy@oracle.com>]

Hi Ravi,

On 5/30/2024 2:30 PM, George Kennedy wrote:
> Hi Ravi,
>
> On 5/29/2024 11:37 PM, Ravi Bangoria wrote:
>> Hi George,
>>
>>> Events can be deleted and the entry can be NULL.
>> Can you please also explain "how".
> It looks like x86_pmu_stop() is clearing the bit in active_mask and 
> setting the events entry to NULL (and doing it in the correct order) 
> for the same events index that amd_pmu_enable_all() is trying to enable.
>>
>>> Check event for NULL in amd_pmu_enable_all() before enable to avoid 
>>> a GPF.
>>> This appears to be an AMD only issue.
>>>
>>> Syzkaller reported a GPF in amd_pmu_enable_all.
>> Can you please provide a bug report link? Also, any reproducer?
> The Syzkaller reproducer can be found in this link:
> https://lore.kernel.org/netdev/CAMt6jhyec7-TSFpr3F+_ikjpu39WV3jnCBBGwpzpBrPx55w20g@mail.gmail.com/T/#u 
>
>>
>>> @@ -760,7 +760,8 @@ static void amd_pmu_enable_all(int added)
>>>           if (!test_bit(idx, cpuc->active_mask))
>>>               continue;
>>>   -        amd_pmu_enable_event(cpuc->events[idx]);
>>> +        if (cpuc->events[idx])
>>> +            amd_pmu_enable_event(cpuc->events[idx]);
>> What if cpuc->events[idx] becomes NULL after if (cpuc->events[idx]) but
>> before amd_pmu_enable_event(cpuc->events[idx])?
> Good question, but the crash has not reproduced with the proposed fix 
> in hours of testing. It usually reproduces within minutes without the 
> fix.

Also, a similar fix is done in __intel_pmu_enable_all() in 
arch/x86/events/intel/core.c except that a WARN_ON_ONCE is done as well.
See: 
https://elixir.bootlin.com/linux/v6.10-rc1/source/arch/x86/events/intel/core.c#L2256

>
> Thank you,
> George
>>
>> Thanks,
>> Ravi
>