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

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] [day] [month] [year] [list]

Message-ID: <580484ba-88d0-4f69-a52c-d078e04c67fe@amd.com>
Date: Thu, 26 Sep 2024 09:35:13 +0530
From: Ravi Bangoria <ravi.bangoria@....com>
To: George Kennedy <george.kennedy@...cle.com>
Cc: harshit.m.mogalapalli@...cle.com, peterz@...radead.org, mingo@...hat.com,
 acme@...nel.org, namhyung@...nel.org, mark.rutland@....com,
 alexander.shishkin@...ux.intel.com, jolsa@...nel.org, irogers@...gle.com,
 adrian.hunter@...el.com, kan.liang@...ux.intel.com, tglx@...utronix.de,
 bp@...en8.de, dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
 linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
 Dongli Zhang <dongli.zhang@...cle.com>, Ravi Bangoria <ravi.bangoria@....com>
Subject: Re: [PATCH] perf/x86/amd: check event before enable to avoid GPF

On 24-Sep-24 10:43 PM, George Kennedy wrote:
> 
> 
> On 7/2/2024 12:03 AM, Ravi Bangoria wrote:
>> On 24-Jun-24 9:48 PM, George Kennedy wrote:
>>>
>>> On 6/10/2024 6:51 AM, Ravi Bangoria wrote:
>>>> On 6/8/2024 12:43 AM, George Kennedy wrote:
>>>>> Hi Ravi,
>>>>>
>>>>> On 6/4/2024 9:40 AM, Ravi Bangoria wrote:
>>>>>>> On 6/4/2024 9:16 AM, Ravi Bangoria wrote:
>>>>>>>>>>>> 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
>>>>>>>> There are subtle differences between Intel and AMD pmu implementation.
>>>>>>>> __intel_pmu_enable_all() enables all event with single WRMSR whereas
>>>>>>>> amd_pmu_enable_all() loops over each PMC and enables it individually.
>>>>>>>>
>>>>>>>> The WARN_ON_ONCE() is important because it will warn about potential
>>>>>>>> sw bug somewhere else.
>>>>>>> We could add a similar WARN_ON_ONCE() to the proposed patch.
>>>>>> Sure, that would help in future. But for current splat, can you please
>>>>>> try to rootcause the underlying race condition?
>>>>> Were you able to reproduce the crash on the AMD machine?
>>>> I'm able to reproduce within the KVM guest. Will try to investigate further.
>>> Hi Ravi,
>>>
>>> Any new status?
>> I was able to reproduce it with passthrough pmu[1] as well on a Zen4 machine
>> where Host has PerfMonV2 support (GlobalCtrl etc) but guest do not. I've
>> debugged it at some extent and seeing some race conditions, but not working
>> on this with top priority since this requires root/CAP_PERFMON privileges to
>> cause a crash. I'll resume investigation once I get some time. Sorry about
>> the delay.
> 
> Hi Ravi,
> 
> Anything new on this issue?

Apologies for not spending time on this.

Can you please resend the patch, rebased on latest tree and a comment
stating that there is a race condition which needs to be fixed.

Something like:
  /*
   * FIXME: cpuc->events[idx] can become NULL in a subtle race
   * condition with NMI->throttle->x86_pmu_stop().
   */

I'll try to root cause and fix it properly once I get some free time.

Thanks,
Ravi