netdev - Re: [PATCH] Fix deadlock between boomerang_interrupt and boomerang_start

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: <20100823234006.GA8976@hmsreliant.think-freely.org>
Date:	Mon, 23 Aug 2010 19:41:44 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH] Fix deadlock between boomerang_interrupt and
 boomerang_start_tx in 3c59x

On Mon, Aug 23, 2010 at 10:48:58PM +0200, Eric Dumazet wrote:
> Le lundi 23 août 2010 à 16:24 -0400, Neil Horman a écrit :
> 
> > 
> > Faster I agree with, although I'm not sure if speed is really a big issue here,
> > given that this is a ancient (but fairly well used) 10/100 adapter.  And we
> > still have space in the octet that that bitfield is living in, so I figured I'd
> > use that anyway.
> > 
> > As for safe, I'm not sure I follow you on that point.  Is there something
> > inherently dangerous about using a bitfield in this case?
> > 
> 
> A bitfield is not SMP safe.
> 
> Are you sure another cpu is not changing another bit, using a non atomic
> RMW sequence, and your bit change is lost ?
> 
> Quite tricky to check I suppose, so just add an "int" ;)
> 
> 
> 
> 

Ah, I see what your saying.  Since bitfield access is not a single instruction
you get races when multiple accesses take place at once, since the assignment is
non-atomic, nor is the read.

In this case, thats ok.  I say that because in the only time we really care what
the value of this bitfield is, is when we've recursed from the interrupt handler
to the netconsole module, back into the transmit path on the same cpu.  In that
case the read is guaranteed to be ordered after the write, since its a linear
code path.  In the case where cpu 0 is in the interrupt handler and cpu1 is
going into the transmit method for this driver, we don't really care what the
value of the bitfield is, its a don't care.  If we read it as a zero, thats ok,
we have the driver-internal sempahore still protecting us (the one that
deadlocks if you recurse via netconsole on the same cpu).  And if we read it as
a 1, thats ok too, because we simply cause the network scheduler to queue the
frame and send it again as soon as we're out of the interrupt handler.

Theres still the size vs. speed issue with the conversion to int though.  I
figured since it was already using bitfields in lots of places, there wouldn't
be much performance impact.  But if you feel really strongly about it, let me
know, and I'll make the conversion.

Neil

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