netdev - Re: [PATCH net-next 1/3] r8169: replace BUG_ON with WARN in _rtl_eri

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: <eaa484aa-5033-d635-6292-e512534cb168@gmail.com>
Date:   Thu, 7 Jan 2021 00:02:05 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Realtek linux nic maintainers <nic_swsd@...ltek.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/3] r8169: replace BUG_ON with WARN in
 _rtl_eri_write

On 06.01.2021 23:18, Alexander Duyck wrote:
> On Wed, Jan 6, 2021 at 5:32 AM Heiner Kallweit <hkallweit1@...il.com> wrote:
>>
>> Use WARN here to avoid stopping the system. In addition print the addr
>> and mask values that triggered the warning.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>> ---
>>  drivers/net/ethernet/realtek/r8169_main.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
>> index 024042f37..9af048ad0 100644
>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>> @@ -763,7 +763,7 @@ static void _rtl_eri_write(struct rtl8169_private *tp, int addr, u32 mask,
>>  {
>>         u32 cmd = ERIAR_WRITE_CMD | type | mask | addr;
>>
>> -       BUG_ON((addr & 3) || (mask == 0));
>> +       WARN(addr & 3 || !mask, "addr: 0x%x, mask: 0x%08x\n", addr, mask);
>>         RTL_W32(tp, ERIDR, val);
>>         r8168fp_adjust_ocp_cmd(tp, &cmd, type);
>>         RTL_W32(tp, ERIAR, cmd);
> 
> Would it make more sense to perhaps just catch the case via an if
> statement, display the warning, and then return instead of proceeding
> with the write with the bad values?
> 
> I'm just wondering if this could make things worse by putting the
> device in a bad state in some way resulting in either a timeout
> waiting for a response or a hang.
> 
I tend to agree. Typically I'd expect that this warning is triggered
during development only, but yes: Returning instead of writing to a
wrong register is better.