Date: Mon, 9 Apr 2012 13:06:30 -0400 From: Andrew Boyer <aboyer@averesystems.com> To: Andrew Thompson <thompsa@FreeBSD.org> Cc: freebsd-net@freebsd.org, John Baldwin <jhb@freebsd.org> Subject: Re: LACP kernel panics: /* unlocking is safe here */ Message-ID: <8B0D75C6-5918-4EFE-9929-4344D877B320@averesystems.com> In-Reply-To: <CAFAOGNSbc05zLwWeatHRvmfWhCc1=UtCLFwmooHaoW1vBd4mwg@mail.gmail.com> References: <D02B1265-C1F4-4F6A-979D-E141565E813F@averesystems.com> <201204020835.00357.jhb@freebsd.org> <CAFAOGNSbc05zLwWeatHRvmfWhCc1=UtCLFwmooHaoW1vBd4mwg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Makes sense to me.
-Andrew
On Apr 7, 2012, at 4:02 AM, Andrew Thompson wrote:
> 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 =
kernel.
>> 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.
>>>=20
>>> 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().
>>>=20
>>> 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 =
incoming
>> packets to be dropped until the lagg gets reconfigured. Thoughts?
>>=20
>> This looks sensible.
>=20
> 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.
>=20
> Does this look ok?
>=20
> 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) /
--------------------------------------------------
Andrew Boyer aboyer@averesystems.com
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8B0D75C6-5918-4EFE-9929-4344D877B320>