netdev - Re: [PATCH v5 2/2] Ethernet driver for the WIZnet W5100 chip

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: <20120331094014.GA7746@electric-eye.fr.zoreil.com>
Date:	Sat, 31 Mar 2012 11:40:14 +0200
From:	Francois Romieu <romieu@...zoreil.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Mike Sinkovsky <msink@...monline.ru>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/2] Ethernet driver for the WIZnet W5100 chip

Eric Dumazet <eric.dumazet@...il.com> :
> Le vendredi 30 mars 2012 ?? 13:00 +0600, Mike Sinkovsky a ??crit :
> > Tested and used in production with Blackfin BF531 embedded processor.
[...]
> > +static int w5100_start_tx(struct sk_buff *skb, struct net_device *ndev)
> > +{
> > +	struct w5100_priv *priv = netdev_priv(ndev);
> > +	u16 offset;
> > +
> > +	if (w5100_read16(priv, W5100_S0_TX_FSR) < skb->len) {
> 
> There is a race here.
> 
> Interrupt came come right here before you set the stop_queue()
> 
> 
> > +		netif_stop_queue(ndev);
> 
> So Here I suggest adding a test again
> 	
> 	if (w5100_read16(priv, W5100_S0_TX_FSR) >= skb->len)
> 		netif_wake_queue(ndev);
> 	else
> 		return NETDEV_TX_BUSY;

The start_tx logic seems broken anyway: it claims it always can take
packets until it is given one it can't process. The driver should rather
do the opposite and stop queueing as soon as there is no guarantee for
the next packet.

-- 
Ueimor
--
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