Makes sense to me.
-Andrew
On Apr 7, 2012, at 4:02 AM, Andrew Thompson wrote:
> On 3 April 2012 00:35, John Baldwin <j...@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 abo...@averesystems.com
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
