netdev - Re: [PATCH 0/3] URGENT for 3.9: net: fec: revert NAPI introduction

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:	Thu, 25 Apr 2013 14:31:25 +0200
From:	Lucas Stach <l.stach@...gutronix.de>
To:	Frank Li <lznuaa@...il.com>
Cc:	Fabio Estevam <festevam@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>,
	Frank Li <Frank.Li@...escale.com>,
	Shawn Guo <shawn.guo@...aro.org>
Subject: Re: [PATCH 0/3] URGENT for 3.9: net: fec: revert NAPI introduction

Am Montag, den 22.04.2013, 17:17 +0800 schrieb Frank Li:
> 2013/4/22 Lucas Stach <l.stach@...gutronix.de>:
> > Hi all,
> >
> > Am Samstag, den 20.04.2013, 20:35 +0800 schrieb Frank Li:
> >> 2013/4/20 Fabio Estevam <festevam@...il.com>
> >> >
> >> > Lucas,
> >> >
> >> > On Fri, Apr 19, 2013 at 11:36 AM, Lucas Stach <l.stach@...gutronix.de> wrote:
> >> > > Those patches introduce instability to the point of kernel OOPSes with
> >> > > NULL-ptr dereferences.
> >> > >
> >> > > The patches drop locks from the code without justifying why this would
> >> > > be safe at all. In fact it isn't safe as now the controller restart can
> >> > > happily free the RX and TX ring buffers while the NAPI poll function is
> >> > > still accessing them. So with a heavily loaded but slightly instable
> >>
> >> I think a possible solution is disable NAPI in restart function.
> >> So only one thread can reset BD queue.
> >>
> >> BD queue is nolock design.
> >>
> > It doesn't matter at all that the hardware BD queue is designed to be
> > operated lockless, you still have to synchronize the driver functions to
> > each other and explicit locks are a far better way to achieve this than
> > some implicit tunneling through a single thread or other such things.
> 
> Not hardware BD queue.
> I redesign software BD queue as lockless queue.
> 
> After put actual queue process work to NAPI,  interrupt handle will
> not interrupt xmit and NAPI function again.
> 
> There are just one entry xmit to push new data to bd queue.
> One entry fec_enet_tx to pull old data from bd queue.
> 
>                        HARD_TX_LOCK(dev, txq, cpu);
> 
>                         if (!netif_xmit_stopped(txq)) {
>                                 __this_cpu_inc(xmit_recursion);
>                                 rc = dev_hard_start_xmit(skb, dev, txq);
>                                 __this_cpu_dec(xmit_recursion);
>                                 if (dev_xmit_complete(rc)) {
>                                         HARD_TX_UNLOCK(dev, txq);
>                                         goto out;
>                                 }
>                         }
>                         HARD_TX_UNLOCK(dev, txq);
> 
> Restart function will only called at suspend/resume, init, and speed change.
> So risk should not in heave loading.
> 
> The other reason of remove lock is that fix deadlock detected by kernel.

While I agree that lockless queues and NAPI disable while doing FEC
restart is the way to go for further development, I tried to implement
that yesterday and it needs some bigger changes to the driver to split
things up properly, otherwise we need a lot of ad-hoc hackery to check
if NAPI is enabled, which seems really error prone. 

think such a change in the driver is not acceptable in the current state
of the cycle. I for one will not submit this change, as I'm not sure at
all that it won't regress in other situations.

Regards,
Lucas
-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html