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.
>>>
>>> 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 incoming
>> packets to be dropped until the lagg gets reconfigured. Thoughts?
>>
>> 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
> ===================================================================
> --- 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 = EPROTONOSUPPORT;
> break;
> }
> + LAGG_WLOCK(sc);
> if (sc->sc_proto != LAGG_PROTO_NONE) {
> - LAGG_WLOCK(sc);
> + /* Reset protocol first in case detach unlocks */
> + sc->sc_proto = LAGG_PROTO_NONE;
> error = sc->sc_detach(sc);
> - /* Reset protocol and pointers */
> - sc->sc_proto = LAGG_PROTO_NONE;
> sc->sc_detach = NULL;
> sc->sc_start = NULL;
> sc->sc_input = NULL;
> @@ -966,8 +966,11 @@ lagg_ioctl(struct ifnet *ifp, u_long cmd, caddr_t
> sc->sc_lladdr = NULL;
> sc->sc_req = NULL;
> sc->sc_portreq = NULL;
> - LAGG_WUNLOCK(sc);
> + } else if (sc->sc_input != NULL) {
> + /* Still detaching */
> + error = EBUSY;
> }
> + LAGG_WUNLOCK(sc);
> if (error != 0)
> break;
> for (int i = 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>