LACP kernel panics: /* unlocking is safe here */

[Andrew Thompson]

On 3 April 2012 00:35, John Baldwin <jhb at 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) /