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]

Date: Tue, 14 May 2024 13:05:55 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: Heiner Kallweit <hkallweit1@...il.com>, Paolo Abeni <pabeni@...hat.com>, 
	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>, 
	Ken Milmore <ken.milmore@...il.com>
Subject: Re: [PATCH net 2/2] r8169: disable interrupts also for GRO-scheduled NAPI

On Tue, May 14, 2024 at 12:53 PM Alexander Lobakin
<aleksander.lobakin@...el.com> wrote:
>
> From: Eric Dumazet <edumazet@...gle.com>
> Date: Tue, 14 May 2024 11:45:05 +0200
>
> > On Tue, May 14, 2024 at 8:52 AM Heiner Kallweit <hkallweit1@...il.com> wrote:
> >>
> >> Ken reported that RTL8125b can lock up if gro_flush_timeout has the
> >> default value of 20000 and napi_defer_hard_irqs is set to 0.
> >> In this scenario device interrupts aren't disabled, what seems to
> >> trigger some silicon bug under heavy load. I was able to reproduce this
> >> behavior on RTL8168h.
> >> Disabling device interrupts if NAPI is scheduled from a place other than
> >> the driver's interrupt handler is a necessity in r8169, for other
> >> drivers it may still be a performance optimization.
> >>
> >> Fixes: 7274c4147afb ("r8169: don't try to disable interrupts if NAPI is scheduled already")
> >> Reported-by: Ken Milmore <ken.milmore@...il.com>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
> >> ---
> >>  drivers/net/ethernet/realtek/r8169_main.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> >> index e5ea827a2..01f0ca53d 100644
> >> --- a/drivers/net/ethernet/realtek/r8169_main.c
> >> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> >> @@ -4639,6 +4639,7 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
> >>  {
> >>         struct rtl8169_private *tp = dev_instance;
> >>         u32 status = rtl_get_events(tp);
> >> +       int ret;
> >>
> >>         if ((status & 0xffff) == 0xffff || !(status & tp->irq_mask))
> >>                 return IRQ_NONE;
> >> @@ -4657,10 +4658,11 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
> >>                 rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING);
> >>         }
> >>
> >> -       if (napi_schedule_prep(&tp->napi)) {
> >> +       ret = __napi_schedule_prep(&tp->napi);
> >> +       if (ret >= 0)
> >>                 rtl_irq_disable(tp);
> >> +       if (ret > 0)
> >>                 __napi_schedule(&tp->napi);
> >> -       }
> >>  out:
> >>         rtl_ack_events(tp, status);
> >>
> >
> > I do not understand this patch.
> >
> > __napi_schedule_prep() would only return -1 if NAPIF_STATE_DISABLE was set,
> > but this should not happen under normal operations ?
>
> Without this patch, napi_schedule_prep() returns false if it's either
> scheduled already OR it's disabled. Drivers disable interrupts only if
> it returns true, which means they don't do that if it's already scheduled.
> With this patch, __napi_schedule_prep() returns -1 if it's disabled and
> 0 if it was already scheduled. Which means we can disable interrupts
> when the result is >= 0, i.e. regardless if it was scheduled before the
> call or within the call.
>
> IIUC, this addresses such situations:
>
> 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
>
> On r8169, this seems to cause issues. On other drivers, it seems to be
> okay, but with this new helper, you can save some cycles.
>
> Reviewed-by: Alexander Lobakin <aleksander.lobakin@...el.com>

Rephrasing the changelog is not really helping.

Consider myself as a network maintainer, not as a casual patch reviewer.

"This seems to cause issues" is rather weak.

I would simply revert the faulty commit, because the interrupts are
going to be disabled no matter what.

Old logic was very simple and rock solid. A revert is a clear stable candidate.

rtl_irq_disable(tp);
napi_schedule(&tp->napi);

If this is still broken, we might have similar issues in old/legacy drivers.