[PATCH] net: eth: altera: fix napi poll_list corruption

[PATCH] net: eth: altera: fix napi poll_list corruption

[Atsushi Nemoto]

tse_poll() calls __napi_complete() with irq enabled.  This leads napi
poll_list corruption and may stop all napi drivers working.
Use napi_complete() instead of __napi_complete().

Signed-off-by: Atsushi Nemoto <nemoto@toshiba-tops.co.jp>
---
 drivers/net/ethernet/altera/altera_tse_main.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c
index da48e66..8207877 100644
--- a/drivers/net/ethernet/altera/altera_tse_main.c
+++ b/drivers/net/ethernet/altera/altera_tse_main.c
@@ -511,8 +511,7 @@ static int tse_poll(struct napi_struct *napi, int budget)
 
 	if (rxcomplete < budget) {
 
-		napi_gro_flush(napi, false);
-		__napi_complete(napi);
+		napi_complete(napi);
 
 		netdev_dbg(priv->dev,
 			   "NAPI Complete, did %d packets with budget %d\n",
-- 

Re: [PATCH] net: eth: altera: fix napi poll_list corruption

[David Miller]

From: Atsushi Nemoto <nemoto@toshiba-tops.co.jp>
Date: Wed, 2 Sep 2015 17:49:29 +0900

> tse_poll() calls __napi_complete() with irq enabled.  This leads napi
> poll_list corruption and may stop all napi drivers working.
> Use napi_complete() instead of __napi_complete().
> 
> Signed-off-by: Atsushi Nemoto <nemoto@toshiba-tops.co.jp>

Two lines below this change you are disabling interrupts anyways,
so I would suggest just moving the spin_lock_irqsave() before the
napi_gro_flush() to fix this.

Many of the checks done by napi_complete_done() (invoked by
napi_complete()) are completely redundant in this context.  For
example, the direct __napi_complete() call is a really nice
optimization because we know we are on the poll list and therefore
it is not empty.

Re: [PATCH] net: eth: altera: fix napi poll_list corruption

[Atsushi Nemoto]

On Wed, 2 Sep 2015 11:25:00 -0700, David Miller <davem@davemloft.net> wrote:
> Two lines below this change you are disabling interrupts anyways,
> so I would suggest just moving the spin_lock_irqsave() before the
> napi_gro_flush() to fix this.
> 
> Many of the checks done by napi_complete_done() (invoked by
> napi_complete()) are completely redundant in this context.  For
> example, the direct __napi_complete() call is a really nice
> optimization because we know we are on the poll list and therefore
> it is not empty.

Thank you for your suggestion.

I think napi_gro_flush() can be called with irq enabled, so moving the
spin_lock_irqsave() just before the __napi_complete() (or moving the
__napi_complete() just after the spin_lock_irqsave()) would be better,
right?

Re: [PATCH] net: eth: altera: fix napi poll_list corruption

[Eric Dumazet]

On Thu, 2015-09-03 at 09:52 +0900, Atsushi Nemoto wrote:
> On Wed, 2 Sep 2015 11:25:00 -0700, David Miller <davem@davemloft.net> wrote:
> > Two lines below this change you are disabling interrupts anyways,
> > so I would suggest just moving the spin_lock_irqsave() before the
> > napi_gro_flush() to fix this.
> > 
> > Many of the checks done by napi_complete_done() (invoked by
> > napi_complete()) are completely redundant in this context.  For
> > example, the direct __napi_complete() call is a really nice
> > optimization because we know we are on the poll list and therefore
> > it is not empty.
> 
> Thank you for your suggestion.
> 
> I think napi_gro_flush() can be called with irq enabled, so moving the
> spin_lock_irqsave() just before the __napi_complete() (or moving the
> __napi_complete() just after the spin_lock_irqsave()) would be better,
> right?

Unless masking irqs are damn slow on hosts supporting this NIC,
I would rather use napi_complete_done() and add the possibility of
aggregating more frames per GRO packet, setting a non zero
gro_flush_timeout

Calling napi_gro_flush() and __napi_complete() looks error prone.

Re: [PATCH] net: eth: altera: fix napi poll_list corruption

[David Miller]

From: Atsushi Nemoto <nemoto@toshiba-tops.co.jp>
Date: Thu, 3 Sep 2015 09:52:57 +0900

> On Wed, 2 Sep 2015 11:25:00 -0700, David Miller <davem@davemloft.net> wrote:
>> Two lines below this change you are disabling interrupts anyways,
>> so I would suggest just moving the spin_lock_irqsave() before the
>> napi_gro_flush() to fix this.
>> 
>> Many of the checks done by napi_complete_done() (invoked by
>> napi_complete()) are completely redundant in this context.  For
>> example, the direct __napi_complete() call is a really nice
>> optimization because we know we are on the poll list and therefore
>> it is not empty.
> 
> Thank you for your suggestion.
> 
> I think napi_gro_flush() can be called with irq enabled, so moving the
> spin_lock_irqsave() just before the __napi_complete() (or moving the
> __napi_complete() just after the spin_lock_irqsave()) would be better,
> right?

It should work, yes.

Re: [PATCH] net: eth: altera: fix napi poll_list corruption

[Atsushi Nemoto]

On Wed, 2 Sep 2015 22:32:54 -0700, David Miller <davem@davemloft.net> wrote:
>> I think napi_gro_flush() can be called with irq enabled, so moving the
>> spin_lock_irqsave() just before the __napi_complete() (or moving the
>> __napi_complete() just after the spin_lock_irqsave()) would be better,
>> right?
> 
> It should work, yes.

Thank you.  But I agree with Eric's last comment ("Calling
napi_gro_flush() and __napi_complete() looks error prone."), and found
that napi_complete_done() also checks NAPI_STATE_NPSVC to support
NETPOLL.  These checks looks somewhat redundant but I like simple way
unless it is really critical to performance.

So, please take original fix as is.

Re: [PATCH] net: eth: altera: fix napi poll_list corruption

[David Miller]

 From: Atsushi Nemoto <nemoto@toshiba-tops.co.jp>
Date: Thu, 3 Sep 2015 15:01:02 +0900

> On Wed, 2 Sep 2015 22:32:54 -0700, David Miller <davem@davemloft.net> wrote:
>>> I think napi_gro_flush() can be called with irq enabled, so moving the
>>> spin_lock_irqsave() just before the __napi_complete() (or moving the
>>> __napi_complete() just after the spin_lock_irqsave()) would be better,
>>> right?
>> 
>> It should work, yes.
> 
> Thank you.  But I agree with Eric's last comment ("Calling
> napi_gro_flush() and __napi_complete() looks error prone."), and found
> that napi_complete_done() also checks NAPI_STATE_NPSVC to support
> NETPOLL.  These checks looks somewhat redundant but I like simple way
> unless it is really critical to performance.
> 
> So, please take original fix as is.

Fair enough, applied and queued up for -stable.