linux-kernel - Re: [PATCH] drivers/perf: Change WARN_ON() to dev_err() on irq_set

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

Date:   Tue, 16 Aug 2022 14:26:51 +0800
From:   Yicong Yang <yangyicong@...wei.com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     <yangyicong@...ilicon.com>, <will@...nel.org>,
        <mark.rutland@....com>, <Frank.li@....com>, <shawnguo@...nel.org>,
        <s.hauer@...gutronix.de>, <kernel@...gutronix.de>,
        <festevam@...il.com>, <linux-imx@....com>,
        <zhangshaokun@...ilicon.com>, <agross@...nel.org>,
        <bjorn.andersson@...aro.org>, <konrad.dybcio@...ainline.org>,
        <khuong@...amperecomputing.com>, <john.garry@...wei.com>,
        <jonathan.cameron@...wei.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH] drivers/perf: Change WARN_ON() to dev_err() on
 irq_set_affinity() failure

On 2022/8/15 18:47, Greg KH wrote:
> On Mon, Aug 15, 2022 at 05:28:15PM +0800, Yicong Yang wrote:
>> From: Yicong Yang <yangyicong@...ilicon.com>
>>
>> The WARN_ON() on irq_set_affinity() failure is misused according to the [1]
>> and may crash people's box unintentionally. This may also be redundant since
>> in the failure case we may also trigger the WARN and dump the stack in the
>> perf core[2] for a second time.
>>
>> So change the WARN_ON() to dev_err() to just print the failure message.
>>
>> [1] https://github.com/torvalds/linux/blob/master/include/asm-generic/bug.h#L74
>> [2] https://github.com/torvalds/linux/blob/master/kernel/events/core.c#L313
> 
> Please point to git.kernel.org links, we do not control github.com and
> it's random mirrors.
> 

Got it. Will update with a git.kernel.org link. Thanks for point it out!

>>
>> Suggested-by: Greg KH <gregkh@...uxfoundation.org>
>> [https://lore.kernel.org/lkml/YuOi3i0XHV++z1YI@kroah.com/]
>> Signed-off-by: Yicong Yang <yangyicong@...ilicon.com>
>> ---
>>  drivers/perf/arm-ccn.c                   | 5 +++--
>>  drivers/perf/arm_dmc620_pmu.c            | 3 ++-
>>  drivers/perf/arm_smmuv3_pmu.c            | 6 ++++--
>>  drivers/perf/fsl_imx8_ddr_perf.c         | 3 ++-
>>  drivers/perf/hisilicon/hisi_pcie_pmu.c   | 6 ++++--
>>  drivers/perf/hisilicon/hisi_uncore_pmu.c | 6 ++++--
>>  drivers/perf/qcom_l2_pmu.c               | 8 ++++++--
>>  drivers/perf/xgene_pmu.c                 | 6 ++++--
>>  8 files changed, 29 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/perf/arm-ccn.c b/drivers/perf/arm-ccn.c
>> index 728d13d8e98a..83abd909ba49 100644
>> --- a/drivers/perf/arm-ccn.c
>> +++ b/drivers/perf/arm-ccn.c
>> @@ -1210,8 +1210,9 @@ static int arm_ccn_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
>>  		return 0;
>>  	perf_pmu_migrate_context(&dt->pmu, cpu, target);
>>  	dt->cpu = target;
>> -	if (ccn->irq)
>> -		WARN_ON(irq_set_affinity(ccn->irq, cpumask_of(dt->cpu)));
>> +	if (ccn->irq && irq_set_affinity(ccn->irq, cpumask_of(dt->cpu)))
>> +		dev_err(ccn->dev, "Failed to set interrupt affinity\n");
>> +
>>  	return 0;
> 
> Why are you returning with no error, if an error happened?
> 
> Same everywhere else, you need to explain this in your changelog text.
> 

This patch intends no functional change but to switch the way on the error notification to avoid
crash the box. So just keep the current handling behaviour. Will mention this in the commit in v2.
I think whether we should actually reponse to the error should be according to the driver.

Thanks.