netdev - Re: [PATCH net 2/2] r8169: disable interrupts also for GRO-scheduled NAPI

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]

Message-ID: <78fb284b-f78a-4dde-8398-d4f175e49723@gmail.com>
Date: Tue, 14 May 2024 18:35:46 +0200
From: Heiner Kallweit <hkallweit1@...il.com>
To: Jakub Kicinski <kuba@...nel.org>,
 Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
 David Miller <davem@...emloft.net>,
 Realtek linux nic maintainers <nic_swsd@...ltek.com>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 Ken Milmore <ken.milmore@...il.com>
Subject: Re: [PATCH net 2/2] r8169: disable interrupts also for GRO-scheduled
 NAPI

On 14.05.2024 16:11, Jakub Kicinski wrote:
> On Tue, 14 May 2024 13:05:55 +0200 Eric Dumazet wrote:
>>> napi_schedule()         // we disabled interrupts
>>> napi_poll()             // we polled < budget frames
>>> napi_complete_done()    // reenable the interrupts, no repoll
>>>   hrtimer_start()       // GRO flush is queued
>>>     napi_schedule()
>>>       napi_poll()       // GRO flush, BUT interrupts are enabled
> 
> I thought the bug is because of a race with disable.

No, the second napi_poll() in this scenario is executed with device
interrupts enabled, what triggers a (supposedly) hw bug under heavy
load. So the fix is to disable device interrupts also in the case
that NAPI is already scheduled when entering the interrupt handler.

> But there's already a synchronize_net() after disable, so NAPI poll
> must fully exit before we mask in rtl8169_cleanup().
> 
> If the bug is double-enable you describe the fix is just making 
> the race window smaller. But I don't think that's the bug.
> 
> BTW why are events only acked in rtl8169_interrupt() and not
> rtl8169_poll()? 

You mean clearing the rx/tx-related interrupt status bits only
after napi_complete_done(), as an alternative to disabling
device interrupts?