Received-SPF: pass (google.com: domain of linux-kernel+bounces-200683-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <3ec1cb26-c889-45ab-b8cc-92f933a2b5d1@amd.com>
Date: Tue, 4 Jun 2024 18:46:27 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] perf/x86/amd: check event before enable to avoid GPF
To: George Kennedy <george.kennedy@oracle.com>
Cc: harshit.m.mogalapalli@oracle.com, peterz@infradead.org, mingo@redhat.com,
 acme@kernel.org, namhyung@kernel.org, mark.rutland@arm.com,
 alexander.shishkin@linux.intel.com, jolsa@kernel.org, irogers@google.com,
 adrian.hunter@intel.com, kan.liang@linux.intel.com, tglx@linutronix.de,
 bp@alien8.de, dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com,
 linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
 ravi.bangoria@amd.com
References: <1716990659-2427-1-git-send-email-george.kennedy@oracle.com>
 <75f6aba1-8ed6-4ef8-8811-de40ae40be90@amd.com>
 <1e14ca4b-6e3e-4d57-acec-bc3ee2bed6ed@oracle.com>
 <0494aedf-9759-4427-a7f3-39a91bd5771d@oracle.com>
Content-Language: en-US
From: Ravi Bangoria <ravi.bangoria@amd.com>
In-Reply-To: <0494aedf-9759-4427-a7f3-39a91bd5771d@oracle.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?T2xXUkYxc3h6bmhoNHFPcGhpTy9YUUc4RVQ2dTY2NmdwYWNHekxPUXJWZEQw?=
 =?utf-8?B?aXE0dml6NzJuc25ZVGRsR3FMSUpkZTVNNEMxbitZZjhBeGl5eElZNEJEeC9F?=
 =?utf-8?B?UXJGVTBQeTBGajhtekk4Rnc4S0J5U2ZhazYxWmJlOVVNc2M0SGMxM2JQVTZF?=
 =?utf-8?B?clkzeEZmN3EwWjRTakZ5SHF5ZUhQNWtOVzNhRlI5ZDVTM3pQNGdiUnRYWStN?=
 =?utf-8?B?Qlo4clh5bU1iaWlkSVlUTTZWY0V1czVwd1dsay95czJDaExWWlB6a2FuZWhB?=
 =?utf-8?B?K3lidkVraFBlNVU5U2xGelowNEFnWWM1ZEtlYjdpNlBuRjI0Wi9JMk1qMXBJ?=
 =?utf-8?B?cHFsUURkRnhmNysvWVBtUUI4bmk2cStQa2xSSHFFTWgwWUoybU9KVGttaTAx?=
 =?utf-8?B?dit4MktCYnEzN0doTWxsZ0JCU2Y5MFBEbTFvRncxb0FGT2JJRU5SaEJONVFh?=
 =?utf-8?B?ZHRIOGhubVBQQmdGNmlSOStFQk96dm4wNnpKQTNIczYyOERJSm84VVVWajJF?=
 =?utf-8?B?UzdCekJ4WmxqYnFmWHJtbTdFNG9kTHJsdzVBMFlGbGpuYmk1U3Z5Wm4wdzNi?=
 =?utf-8?B?b1lVMHZMa04vOWhkeVdrV3BWb0prTUV6MEo1b1E2TzZOcEQwc0JuSUdqaXNY?=
 =?utf-8?B?dnFmL3ZBYTZycTVBcDRxaDhySHJoZ0hkQWNiSmRlU0hlQ00vZ2RDTWpQVXJW?=
 =?utf-8?B?QnRTZEE5aG5Ta3FOZGN5anJjbWZQLytmSjBJUnFIb0NtRmdIaHovWlVkeG1D?=
 =?utf-8?B?Vk1MRWlVZjdmWnd6TXpQR2VHNTlFOFVMbjFvZUtFR0thdW95Y1QrNkZRNVly?=
 =?utf-8?B?Nm5CRmdCWm05ZjU1cVl5eE42Y3dmOHJjTEIvc0E0WGNVQ2xnZjIvZWlvUGl6?=
 =?utf-8?B?aXNwczFvUkVwZ0RiNVRRMEdxdEt6OUx4VVowQVNVVEhJcnZ3akJqZXZxT1p1?=
 =?utf-8?B?cUswamFiQTRENjBmSkF6b0lxODA3N0pvWm1XbU84R2NqQTd1RU1sSWM4eUI3?=
 =?utf-8?B?S0ZSbEtLRE9CbEtvQnNqb3IxNE5Ua2JWL2FyNkkwaVJtRXFKcUFSUDdLTTRq?=
 =?utf-8?B?a0FNbmsrTDlISGR0bm9wbnl3VnRURExlelRuYjZHZkVsMWtOZTZwREtYcVVr?=
 =?utf-8?B?ZkpsUzdTU0xwbGYrbDJMRGZ2bnZDOFJkZ0xyVmJLOStKYThBSk1PUHhyZWNv?=
 =?utf-8?B?N2tSTE1wYkpsOHhjWDNpYi9zdnBNbTRuRzNkd1BMRTY2QmdqeldLMG5uL0cr?=
 =?utf-8?B?ZDB4SHpSZGYzVGtWSXhsVUc4TDM2NUtiL1ZJNTk0dXAyd1V0RGVLUjdlNjFj?=
 =?utf-8?B?Qmc4cXNCcldvUFA3TWdQK2QxaVJETjJaQVFoLy9jYW9ick5DUkZmRG5UN2FK?=
 =?utf-8?B?ZEFOdzVYQTFWQmkwT2tTdm9PRkp5NENLYUU2eHBFWW9scitZMWl1a3hSQ2cv?=
 =?utf-8?B?RVpCaXhNMXp2ODcrM3hQdHgxcm5NYWZEMTkxenZrYnZBVVJHcjZFOE56VmE2?=
 =?utf-8?B?bjdENGJrdnVYdjlLUHdGRVlrQk0zZk9SRStVSnlnME0xa2ZSS21GUWh6a2Q5?=
 =?utf-8?B?b0JPejVaQS80VHJhWS85VmQ5MkoyYWprMHlPdHZTUndSNStCRkJ5UFdHc2ZR?=
 =?utf-8?B?U2p6V2xZR3I4b1R6ektzVHQyd1JKQ24zRUQyZDQrTzc5d29nK0M3UTVJdkNn?=
 =?utf-8?B?d3N1Y201UjFuRmlweGE5WnV5TzhDVzFRUElIZUZCNVNEWm5zRGM3T0N4cnNP?=
 =?utf-8?B?ZTF0VmZvcnhmVk14QTN1OGFYcG8xYjRMV1RTNUcwVXBIdGZ4a3doRTNhUDBU?=
 =?utf-8?B?M3FiQ3NRRWtzRXlqSTdoekhKN25tcVFVb0YzZ05MUnZsT1Ewb0t4NzhPTkdw?=
 =?utf-8?B?MStFai9YSWYxamJHdG1VWjQ3MzhSV24xQk4zTDU3a1VMTjN0SHdMMlE0bjV6?=
 =?utf-8?B?VzZZYVR3YnloWVA5NEVXaG4yVkZ4UjhteUtMS201SUpEY3JwcEh6aEZMUHVZ?=
 =?utf-8?B?Y0FGdERVZHgvQnNHK2E4Nm9CVHA3V1hreGNtczVqVDBaN25CWDdzcHFnZyt6?=
 =?utf-8?B?R3h1UXVGemJXUlB5Sk5pdFBLdTV2SEdLWnpNUDJCcnl2dGo0YUFmUCttTGtB?=
 =?utf-8?Q?0QQ7PtphOlYkTX2ujapFNizwf?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c196635f-eb99-4292-d7aa-08dc84989281
X-MS-Exchange-CrossTenant-AuthSource: PH7PR12MB6588.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jun 2024 13:16:41.0905
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: WwkoL2iHb5rBB2OJD4/BiL4w0dW0jlrr1upKSRSRnMJQMSVD18H/YDMv+F3Fn5aqGNkgAP10FEvikqlmJHvrkg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB4317

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

Thanks,
Ravi