Date: Sat, 7 Apr 2012 20:02:46 +1200 From: Andrew Thompson <thompsa@FreeBSD.org> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-net@freebsd.org, Andrew Boyer <aboyer@averesystems.com> Subject: Re: LACP kernel panics: /* unlocking is safe here */ Message-ID: <CAFAOGNSbc05zLwWeatHRvmfWhCc1=UtCLFwmooHaoW1vBd4mwg@mail.gmail.com> In-Reply-To: <201204020835.00357.jhb@freebsd.org> References: <D02B1265-C1F4-4F6A-979D-E141565E813F@averesystems.com> <201204020835.00357.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 3 April 2012 00:35, John Baldwin <jhb@freebsd.org> wrote:
> On Friday, March 30, 2012 6:04:24 pm Andrew Boyer wrote:
>> While investigating a LACP issue, I turned on LACP_DEBUG on a debug kern=
el.
> In this configuration it's easy to panic the kernel - just run 'ifconfig =
lagg0
> laggproto lacp' on a lagg that's already in LACP mode and receiving LACP
> messages.
>>
>> The problem is that lagg_lacp_detach() drops the lagg wlock (with the
> comment in the title), which allows incoming LACP messages to get through
> lagg_input() while the structure is being destroyed in lacp_detach().
>>
>> There's a very simple fix, but I don't know if it's the best way to fix =
it.
> Resetting the protocol before calling sc_detach causes any further incomi=
ng
> packets to be dropped until the lagg gets reconfigured. =A0Thoughts?
>
> This looks sensible.
Changing the order also needs an additional check as LAGG_PROTO_NONE
no longer means the detach is finished. If one ioctl sleeps then we
may nullify all the pointers upon wake that have already been set by
the other ioctl.
Does this look ok?
Index: if_lagg.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- if_lagg.c (revision 233252)
+++ if_lagg.c (working copy)
@@ -950,11 +950,11 @@ lagg_ioctl(struct ifnet *ifp, u_long cmd, caddr_t
error =3D EPROTONOSUPPORT;
break;
}
+ LAGG_WLOCK(sc);
if (sc->sc_proto !=3D LAGG_PROTO_NONE) {
- LAGG_WLOCK(sc);
+ /* Reset protocol first in case detach unlocks */
+ sc->sc_proto =3D LAGG_PROTO_NONE;
error =3D sc->sc_detach(sc);
- /* Reset protocol and pointers */
- sc->sc_proto =3D LAGG_PROTO_NONE;
sc->sc_detach =3D NULL;
sc->sc_start =3D NULL;
sc->sc_input =3D NULL;
@@ -966,8 +966,11 @@ lagg_ioctl(struct ifnet *ifp, u_long cmd, caddr_t
sc->sc_lladdr =3D NULL;
sc->sc_req =3D NULL;
sc->sc_portreq =3D NULL;
- LAGG_WUNLOCK(sc);
+ } else if (sc->sc_input !=3D NULL) {
+ /* Still detaching */
+ error =3D EBUSY;
}
+ LAGG_WUNLOCK(sc);
if (error !=3D 0)
break;
for (int i =3D 0; i < (sizeof(lagg_protos) /
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFAOGNSbc05zLwWeatHRvmfWhCc1=UtCLFwmooHaoW1vBd4mwg>