Date: Fri, 14 Oct 2011 16:49:43 +0800 From: dave jones <s.dave.jones@gmail.com> To: Mikolaj Golub <trociny@freebsd.org> Cc: Adrian Chadd <adrian@freebsd.org>, "K. Macy" <kmacy@freebsd.org>, "freebsd-net@freebsd.org" <freebsd-net@freebsd.org>, bz@freebsd.org, Robert Watson <rwatson@freebsd.org>, Arnaud Lacombe <lacombar@gmail.com> Subject: Re: Kernel panic on FreeBSD 9.0-beta2 Message-ID: <CANf5e8at2fxU8V=jie-g6ifeGpH1gsaJRqEoQjGs=iniW-Sqgg@mail.gmail.com> In-Reply-To: <86fwiy91ty.fsf@in138.ua3> References: <CANf5e8aG4go4M_vsRExUsJB_sjaN5x-QK-TCDAhSH64JSo0mdQ@mail.gmail.com> <CACqU3MXStMMEoppvDtZS6hV4WGttbdJiF8E-ORwJ%2BQSmnTy-Yg@mail.gmail.com> <CACqU3MV-t4Va6VWUoXy1Y9FYnNJTUw1X%2BE7ik-2%2BtMVuVOV3RA@mail.gmail.com> <CAJ-Vmom-177OkdUXjz%2BZLqbaqn=p%2BuTGypiVuMqdeXgdOgb4hQ@mail.gmail.com> <CAHM0Q_Mmn3z1V6AtZHQMpgbdY7oQqOChiNt=8NJrZQDnravb7A@mail.gmail.com> <CACqU3MU9ZZtOsdBOa%2BF3SqUaYgO%2BEo0v1ACjY0S4rY4fRQyv5Q@mail.gmail.com> <CAHM0Q_PZD9_0ZkELZ5XL8Ebh8eD-uFuSjXWKKVpGDeM_JDaqMA@mail.gmail.com> <8662kcigif.fsf@kopusha.home.net> <alpine.BSF.2.00.1109301432570.65269@fledge.watson.org> <CANf5e8ab=mUw-AJuRXZy1T6%2BZcryxjKfuCOsakPPfqatuA3HdA@mail.gmail.com> <86y5x0ooik.fsf@in138.ua3> <CANf5e8YtQ5P2euF7E-D6Wt7U38UuLc8KVU-NCehq74XV_WTvBg@mail.gmail.com> <CANf5e8bLcxYDe%2BmHssUndOqh2B0j-V28Ox2dZCfy6%2Bo7aURw=w@mail.gmail.com> <86fwiy91ty.fsf@in138.ua3>
next in thread | previous in thread | raw e-mail | index | archive | help
2011/10/12 Mikolaj Golub wrote: > > 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. Thanks for cooking the patch. Machines have been running up days without any panic. > -- > Mikolaj Golub Regards, Dave.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANf5e8at2fxU8V=jie-g6ifeGpH1gsaJRqEoQjGs=iniW-Sqgg>