Re: apparently missing locking in if_bnx.c

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Mon, Mar 05, 2012 at 03:45:10PM -0500, Greg Troxel wrote:
> 
> My colleague Beverly Schwartz has found a problem in if_bnx.c where
> bnx_start takes packets off ifp->if_snd without a mutex.  bnx_start can
> be called from multiple places (the output routine, the tx complete
> routine, and the allocate-more-tx-buffers routine).  We have in fact
> seen calls to bnx_start while it is running, resulting in the packet
> being queued twice and a double free.  With pure netbsd-6, we can lock
> up a machine (i386) with multiple bnx interfaces by running multiple
> transfers in both directions on multiple interfaces at once.  With the
> following patch, it is solid.  (I am pretty sure we are running with
> only 1 core.)
> 
> I'm sending this as early as I can, because whatever change we converge
> on will need to be pulled up to netbsd-6 and I'd like some more eyes on
> it.
> 
> There are a splnet calls around ifq_enqueue.  So perhaps the bug is
> instead that bnx_start is somehow called not at splnet.  But bnx_intr
> should be running at splnet, which should serialize access.

Yes, bnx_start() should be called at splnet(), or raise to splnet() itself.
This driver runs under the KERNEL_LOCK() anyway, so no mutex is required,
ony splnet().

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--