netdev - Re: [PATCH net] ionic: remove WARN_ON to prevent panic_on

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: <7e2acc56-e8cb-14b6-be45-a6cc09ad9ae1@intel.com>
Date: Thu, 29 Jun 2023 10:25:38 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Shannon Nelson <shannon.nelson@....com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "davem@...emloft.net" <davem@...emloft.net>,
	"kuba@...nel.org" <kuba@...nel.org>
CC: "brett.creeley@....com" <brett.creeley@....com>, "drivers@...sando.io"
	<drivers@...sando.io>, "nitya.sunkad@....com" <nitya.sunkad@....com>
Subject: Re: [PATCH net] ionic: remove WARN_ON to prevent panic_on_warn



On 6/28/2023 11:26 AM, Shannon Nelson wrote:
> On 6/28/23 10:57 AM, Keller, Jacob E wrote:
>>
>>> -----Original Message-----
>>> From: Shannon Nelson <shannon.nelson@....com>
>>> Sent: Wednesday, June 28, 2023 10:01 AM
>>> To: netdev@...r.kernel.org; davem@...emloft.net; kuba@...nel.org
>>> Cc: brett.creeley@....com; drivers@...sando.io; nitya.sunkad@....com;
>>> Shannon Nelson <shannon.nelson@....com>
>>> Subject: [PATCH net] ionic: remove WARN_ON to prevent panic_on_warn
>>>
>>> From: Nitya Sunkad <nitya.sunkad@....com>
>>>
>>> Remove instances of WARN_ON to prevent problematic panic_on_warn use
>>> resulting in kernel panics.
>>>
>>
>> This message could potentially use a bit more explanation since it doesn't look like you removed all the WARN_ONs in the driver, and it might help to explain why this particular WARN_ON was problematic. I don't think that would be worth a re-roll on its own though.
> 
> There has been recent mention of not using WARNxxx macros because so 
> many folks have been setting panic_on_warn [1].  This is intended to 
> help mitigate the possibility of unnecessarily killing a machine when we 
> can adjust and continue.
> [1]: https://lore.kernel.org/netdev/2023060820-atom-doorstep-9442@gregkh/
> 
> I believe the only other WARNxxx in this driver is a WARN_ON_ONCE in 
> ionic_regs.h which can be addressed in a separate patch.
> 
> Neither of these are ever expected to be hit, but also neither should 
> ever kill a machine.
> 

Ok. Makes sense enough to me.

>>> -     if (WARN_ON(n_qcq->flags & IONIC_QCQ_F_INTR)) {
>>> +     if (n_qcq->flags & IONIC_QCQ_F_INTR) {
>>> +             dev_warn(n_qcq->q.dev, "%s: n_qcq->flags and
>>> IONIC_QCQ_F_INTR set\n",
>>> +                      __func__);
>>
>> What calls this function? It feels a bit weird that the only action this code takes was in a WARN_ON state. Definitely agree this shouldn't be WARN_ON.
> 
> This isn't the only action in this function - after this 'if' is a bit 
> of code to link the queues onto the same interrupt.
> 

Yea, I think I missed some context, and saw a '}' that I thought was the
end of the function.