Kernel panic on FreeBSD 9.0-beta2

[Mikolaj Golub]

On Wed, 12 Oct 2011 09:53:34 +0800 dave jones wrote:

 dj> On Fri, Oct 7, 2011 at 9:12 AM, dave jones  wrote:
 >> 2011/10/4 Mikolaj Golub :
 >>>
 >>> On Sat, 1 Oct 2011 14:15:45 +0800 dave jones wrote:
 >>>
 >>>  dj> On Fri, Sep 30, 2011 at 9:41 PM, Robert Watson wrote:
 >>>  >>
 >>>  >> On Wed, 28 Sep 2011, Mikolaj Golub wrote:
 >>>  >>
 >>>  >>> On Mon, 26 Sep 2011 16:12:55 +0200 K. Macy wrote:
 >>>  >>>
 >>>  >>> KM> Sorry, didn't look at the images (limited bw), I've seen something KM>
 >>>  >>> like this before in timewait. This "can't happen" with UDP so will be KM>
 >>>  >>> interested in learning more about the bug.
 >>>  >>>
 >>>  >>> The panic can be easily triggered by this:
 >>>  >>
 >>>  >> Hi:
 >>>  >>
 >>>  >> Just catching up on this thread.  I think the analysis here is generally
 >>>  >> right: in 9.0, you're much more likely to see an inpcb with its in_socket
 >>>  >> pointer cleared in the hash list than in prior releases, and
 >>>  >> in_pcbbind_setup() trips over this.
 >>>  >>
 >>>  >> However, at least on first glance (and from the perspective of invariants
 >>>  >> here), I think the bug is actualy that in_pcbbind_setup() is asking
 >>>  >> in_pcblookup_local() for an inpcb and then access the returned inpcb's
 >>>  >> in_socket pointer without acquiring a lock on the inpcb.  Structurally, it
 >>>  >> can't acquire this lock for lock order reasons -- it already holds the lock
 >>>  >> on its own inpcb.  Therefore, we should only access fields that are safe to
 >>>  >> follow in an inpcb when you hold a reference via the hash lock and not a
 >>>  >> lock on the inpcb itself, which appears generally OK (+/-) for all the
 >>>  >> fields in that clause but the t->inp_socket->so_options dereference.
 >>>  >>
 >>>  >> A preferred fix would cache the SO_REUSEPORT flag in an inpcb-layer field,
 >>>  >> such as inp_flags2, giving us access to its value without having to walk
 >>>  >> into the attached (or not) socket.
 >>>  >>
 >>>  >> This raises another structural question, which is whether we need a new
 >>>  >> inp_foo flags field that is protected explicitly by the hash lock, and not
 >>>  >> by the inpcb lock, which could hold fields relevant to address binding.  I
 >>>  >> don't think we need to solve that problem in this context, as a slightly
 >>>  >> race on SO_REUSEPORT is likely acceptable.
 >>>  >>
 >>>  >> The suggested fix does perform the desired function of explicitly detaching
 >>>  >> the inpcb from the hash list before the socket is disconnected from the
 >>>  >> inpcb. However, it's incomplete in that the invariant that's being broken is
 >>>  >> also relied on for other protocols (such as raw sockets).  The correct
 >>>  >> invariant is that inp_socket is safe to follow unconditionally if an inpcb
 >>>  >> is locked and INP_DROPPED isn't set -- the bug is in "locked" not in
 >>>  >> "INP_DROPPED", which is why I think this is the wrong fix, even though it
 >>>  >> prevents a panic :-).
 >>>
 >>>  dj> Hello Robert,
 >>>
 >>>  dj> Thank you for taking your valuable time to find out the problem.
 >>>  dj> Since I don't have idea about network internals, would you have a patch
 >>>  dj> about this? I'd be glad to test it, thanks again.
 >>>
 >>> Here is the patch that implements what Robert suggests.
 >>>
 >>> Dave, could you test it?
 >>
 >> Sure. Thanks for cooking the patch.
 >> Machines have been running two days now without panic.

Thank you for testing it.

 dj> Is there any plan to commit your fix? Thank you.
 dj> I'd upgrade to 9.0-release from beta-2 once it's released.

I have an upgraded version of the patch, which is under review now. I have
been waiting for the response before asking you to test it, but it would be
great if you try it not waiting :-).

As it was pointed by Robert the previous version introduced a regression:
SO_REUSEPORT was ignored if setsockopt was called after bind (the old cached
value was still used). So the updated version fixes this and also contains
several other fixes, the most important among them is that it fixes the panic
for IPv6 bind case too.

-- 
Mikolaj Golub

-------------- next part --------------
A non-text attachment was scrubbed...
Name: INP_REUSEPORT.patch
Type: text/x-diff
Size: 7600 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-net/attachments/20111012/a4d5912b/INP_REUSEPORT.bin