netdev - Re: [PATCH 2/2] mv643xx_eth: fix race in trasmit path.

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: <1308590505.2658.8.camel@edumazet-laptop>
Date:	Mon, 20 Jun 2011 19:21:45 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Lennert Buytenhek <buytenh@...tstofly.org>
Cc:	Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org,
	David Miller <davem@...emloft.net>, stable@...nel.org
Subject: Re: [PATCH 2/2] mv643xx_eth: fix race in trasmit path.

Le lundi 20 juin 2011 à 19:19 +0200, Eric Dumazet a écrit :
> Le lundi 20 juin 2011 à 18:33 +0200, Lennert Buytenhek a écrit :
> > On Mon, Jun 20, 2011 at 09:48:07AM +0200, Richard Cochran wrote:
> > 
> > > Because the socket buffer is freed in the completion interrupt, it
> > > is not safe to access it after submitting it to the hardware.
> > 
> > Maybe I'm missing something here, but mv643xx_eth TX reclaim is done
> > from NAPI poll, under __netif_tx_lock(), while mv643xx_eth_xmit() also
> > runs under __netif_tx_lock().
> 
> See my previous answer. Its true this driver _currently_ holds tx queue
> lock in its TX completion. But that might/should change.
> 
> Goal is to make tx completion not use tx queue lock in fast path, like
> its done in tg3, bnx2, bnx2x ... and other recent drivers.
> 
> Its obviously correct to move skb->len access in start_xmit() before
> starting the IO, even if not a bug fix, it makes all drivers behave the
> same : When reviewing them, its easier not to worry about these possible
> use after free.
> 
> 

One random example found in drivers/staging/hv/netvsc_drv.c

Is the "net->stats.tx_bytes += skb->len;" found in line 188 is correct ?

Who knows ?

I believe its not correct ;)



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