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

[Andrew Boyer]

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 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) /

--------------------------------------------------
Andrew Boyer	aboyer at averesystems.com