Message-ID: <fcf5ee96-178d-061a-ccbe-9be925b1b708@amd.com>
Date:   Tue, 31 Jan 2023 13:55:35 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH 1/1] perf: Fix warning from concurrent read/write of
 perf_event_pmu_context
Content-Language: en-US
To:     James Clark <james.clark@arm.com>,
        linux-perf-users@vger.kernel.org, peterz@infradead.org
Cc:     syzbot+697196bc0265049822bd@syzkaller.appspotmail.com,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        linux-kernel@vger.kernel.org,
        Thomas Richter <tmricht@linux.ibm.com>,
        Ravi Bangoria <ravi.bangoria@amd.com>
References: <20230127143141.1782804-1-james.clark@arm.com>
 <20230127143141.1782804-2-james.clark@arm.com>
 <2cb6bb4b-fd04-556e-0790-524bf174cf89@amd.com>
From:   Ravi Bangoria <ravi.bangoria@amd.com>
In-Reply-To: <2cb6bb4b-fd04-556e-0790-524bf174cf89@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SlhhK1J5YWMxdVFNTEt3QWptSlNJaGFJcFZrbFRSVHdXLzhTMDZYbHIvTk8x?=
 =?utf-8?B?aVl0NTMrQjBZUVhsTi9DSXpINkRsY0M2QjJ1cmFibVExUXV4MWJkOU82SzFz?=
 =?utf-8?B?YmMzMU9pSlYxTkVKM05PSXFqdE1hU2JoQld4aWxTK3dNaThERFdpY1pQUUY5?=
 =?utf-8?B?WUhFTXIyNloxWGNqN0RMSHFXZ0NJMnc2cXl1WE5DckM3KzhXaFBqZElIMGIx?=
 =?utf-8?B?RkFFa2o5SjVWNEhoRXhhQS91YlUvMVdHYTFkL3BjcmIrWUVkQ0NWc09NcWN4?=
 =?utf-8?B?MXI5YUpsTkRQbnYxUFhqY0ZtNm0xVlovcTFISTl1SnExeTZ6ZEdOeDlydzdl?=
 =?utf-8?B?Z2tnMFpsRE0ra29Ec2RFL0dyYlVsem9zMVAvVmNZQktwc3g2SXlLMkVWYWd2?=
 =?utf-8?B?NjNpK1psckVFSU8vNExWN0hVWHFMRzA0WllITGcrekozSjhGWExBelRwS1ho?=
 =?utf-8?B?TzZpZU9CWHZKbHk4QUF5TFhaMkFiTTJWSnppUXlmQ1RTaVQxQmlmQ2ZJRjN1?=
 =?utf-8?B?VW43bFM0a3dNbjdWWXBUUlh6WUR4S2dnK0xHU25XUlE0dGFDY1VKeW1xajlR?=
 =?utf-8?B?ZWo1eE15RzVOWFZhcTdnNTVXWVpETkJ4d25acEtTY0FTcWZaTXRza0pyY0pl?=
 =?utf-8?B?VktjeEc1TGQ0T1Z2cG8zZ0d6L3lOMXBveTMrRDg3d3NZeVYvMUptRW1KK01j?=
 =?utf-8?B?b3pBOUdIblFsMmpyM1RuUlE2QWdiVk5YUDBmcjVjK3BXY2c2NlJjTTJrWFV0?=
 =?utf-8?B?VkxsSWo3azlIbUVNYXBCU1FGb25WZDdVVHd2REltUnZRdG5rOWZCZk5MS0hI?=
 =?utf-8?B?TmxlbGZzc3pYY2RVOEZuMjhwcUtIck53NlFtS3JTSkV6bTcvRGxWSmR5dmZv?=
 =?utf-8?B?WjVpdWcyWUFEVkFOWWM2U3lGdThHR2F0ZFJ4V3VWUDk1R3E2WGx3YTRMZjR3?=
 =?utf-8?B?MnhEZUNGTkdxY3FJTnRWSmkxVmRNMFlDT0hGZnU0RWpZQVpWV1hFeW5lSUZK?=
 =?utf-8?B?bURqMUVUc2o5R2djKzg3dnd5QUIwNXNEcHVkTmsrTE93ZkVvN2dBNHQyWHRp?=
 =?utf-8?B?UEdqZjBHTm9kRzVJUFR2U1NtOU0rU254a2c2NUFUdlBIOUFtdGVTU0hLbHNq?=
 =?utf-8?B?b0syVWZTRnh6N2RhMmxsZGZYS1ZoclpRcXhJZGxvTld4bllzZjgzZ1RLdEpN?=
 =?utf-8?B?cTJvWTIxTGVuQTdPVFc0SkdLMFdCZFRBVFlQYXdzaXFDL1NrTmgxUWR4VWpE?=
 =?utf-8?B?TTE2SE1GOVJHQzQrazdxbGwrMEpvS1dzZ1dKRUE3TE1BWEJMNHg5NE5QZmJL?=
 =?utf-8?B?QmlrTWc5VEpad2VpNWh4VmtPYnMzWlcxQ3crU2NGWXdaMlRyUXdYMXg2Qmdu?=
 =?utf-8?B?QVQ4MjVLYnR0MkxqZk1PQjhnZFJCQmFqakdtMitOOExtSEdzVEFrSDRWNlZU?=
 =?utf-8?B?a1FOdVVrc2ZMZXhsdjFTYVh1WmRNb1RnZ3UwWkJ3WHdJa0crRjVmWHQ2cTJv?=
 =?utf-8?B?TUhncndRdmpkcmhnMmxHdGU1dnpWWVdUbVdpSFl1bmFqNU1xUlAwWW5xYStV?=
 =?utf-8?B?RDhBdWpaSFJMVlNEZnZBZlNoYm9DdVJZNUR2RCt3WkJ1WnFmRjZXMGU2VVMr?=
 =?utf-8?B?Vzl3YUhXcnZPMENoaVFzYVZkVnZPR1N6TGZhUndkc0ZGVSt6S0FuQTE4SVNk?=
 =?utf-8?B?cG5HdElkU3ZwamdVMmd5V0ZxNEZqcURGcEFQZTI3WXM2aVZTV2JFWkJUMHhr?=
 =?utf-8?B?WHlmZHhBMWp1ZlRwQTVXdmErRVE0UGVTZUdUWEE0SG1LdXVGTFVMZllzbDZ4?=
 =?utf-8?B?ckUrSU1iSlRRbWNKZ2JaS0gyNW5Bb0JmUWhsUDNVcmJKZ2FlaTVyUnNvMUIw?=
 =?utf-8?B?aDBCV2NuM3gxNlFFejE4RkJtZjMxdXp1ZUhkdUhiZ2J1QWt0ZW44alc5NGI1?=
 =?utf-8?B?WUFya3MwQmNpZ1ViT1ZScytmU0I4UU1RQUtNdXcxOHg5enJiR2tBSDZLdnBL?=
 =?utf-8?B?VGtYNGVoY201eGRTY1YvSmdjb3duZE56a3JjVlFPekRjMC8vcDRaVkJIZk9m?=
 =?utf-8?B?U0pGYjRYQTdDYzVXYmtFVXFmL1VJR09LbTVpejEwVCtBNU5qY1pRT2ZUYy93?=
 =?utf-8?Q?d5FTYGMTyL8swKXE+H4wjzEMb?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d54ccb56-8f20-4500-d533-08db0364c23e
X-MS-Exchange-CrossTenant-AuthSource: PH7PR12MB6588.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2023 08:25:49.4817
 (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: ga+qTFSA3M7cs15tmBcMtmCeTKDr9zsuZe3/ZAhd5pQZhBIJnGSfEtk6+rwf80CzioKrtD92fxqnivR7aleZ1A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR12MB6780
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 30-Jan-23 11:19 AM, Ravi Bangoria wrote:
> Hi James,
> 
> On 27-Jan-23 8:01 PM, James Clark wrote:
>> When running two Perf sessions, the following warning can appear:
>>
>>   WARNING: CPU: 1 PID: 2245 at kernel/events/core.c:4925 put_pmu_ctx+0x1f0/0x278
>>   Modules linked in: xt_CHECKSUM xt_MASQUERADE xt_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp ip6table_mangle ip6table_nat iptable_mangle iptable_nat nf_nat nf_conntrack libcrc32c nf_defrag_ipv6 nf_defrag_ipv4 ip6table_filter ip6_tables iptable_filter bridge stp llc coresight_stm stm_core coresight_etm4x coresight_tmc coresight_replicator coresight_funnel coresight_tpiu coresight arm_spe_pmu ip_tables x_tables ipv6 xhci_pci xhci_pci_renesas r8169
>>   CPU: 1 PID: 2245 Comm: perf Not tainted 6.2.0-rc4+ #1
>>   pstate: 20400009 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>   pc : put_pmu_ctx+0x1f0/0x278
>>   lr : put_pmu_ctx+0x1b4/0x278
>>   sp : ffff80000dfcbc20
>>   x29: ffff80000dfcbca0 x28: ffff008004f00000 x27: ffff00800763a928
>>   x26: ffff00800763a928 x25: 00000000000000c0 x24: 0000000000000000
>>   x23: 00000000000a0003 x22: ffff00837df74088 x21: ffff80000dfcbd18
>>   x20: 0000000000000000 x19: ffff00800763a6c0 x18: 0000000000000000
>>   x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
>>   x14: 0000000000000000 x13: ffff80000dfc8000 x12: ffff80000dfcc000
>>   x11: be58ab6d2939e700 x10: be58ab6d2939e700 x9 : 0000000000000000
>>   x8 : 0000000000000001 x7 : 0000000000000000 x6 : 0000000000000000
>>   x5 : ffff00800093c9c0 x4 : 0000000000000000 x3 : ffff80000dfcbca0
>>   x2 : ffff008004f00000 x1 : ffff8000082403c4 x0 : 0000000000000000
>>   Call trace:
>>    put_pmu_ctx+0x1f0/0x278
>>    _free_event+0x2bc/0x3d0
>>    perf_event_release_kernel+0x444/0x4bc
>>    perf_release+0x20/0x30
>>    __fput+0xe4/0x25c
>>    ____fput+0x1c/0x28
>>    task_work_run+0xc4/0xe8
>>    do_notify_resume+0x10c/0x164
>>    el0_svc+0xb4/0xdc
>>    el0t_64_sync_handler+0x84/0xf0
>>    el0t_64_sync+0x190/0x194
>>
>> This is because there is no locking around the access of "if
>> (!epc->ctx)" in find_get_pmu_context() and when it is set to NULL in
>> put_pmu_ctx().
>>
>> The decrement of the reference count in put_pmu_ctx() also happens
>> outside of the spinlock, leading to the possibility of this order of
>> events, and the context being cleared in put_pmu_ctx(), after its
>> refcount is non zero:
>>
>>  CPU0                                   CPU1
>>  find_get_pmu_context()
>>    if (!epc->ctx) == false
>>                                         put_pmu_ctx()
>>                                         atomic_dec_and_test(&epc->refcount) == true
>>                                         epc->refcount == 0
>>      atomic_inc(&epc->refcount);
>>      epc->refcount == 1
>>                                         list_del_init(&epc->pmu_ctx_entry);
>> 	                                      epc->ctx = NULL;
>>
>> Another issue is that WARN_ON for no active PMU events in put_pmu_ctx()
>> is outside of the lock. If the perf_event_pmu_context is an embedded
>> one, even after clearing it, it won't be deleted and can be re-used. So
>> the warning can trigger. For this reason it also needs to be moved
>> inside the lock.
>>
>> The above warning is very quick to trigger on Arm by running these two
>> commands at the same time:
>>
>>   while true; do perf record -- ls; done
>>   while true; do perf record -- ls; done
> 
> These dose not trigger WARN_ON on my x86 machine, however, the C reproducer
> provided by syzbot[1] does trigger it.
> 
> [1]: https://syzkaller.appspot.com/text?tag=ReproC&x=17beacbc480000

Unless I'm missing some subtle scenario, the patch looks fine to me.

Reviewed-by: Ravi Bangoria <ravi.bangoria@amd.com>

Thanks,
Ravi