commit 38063d0e19e0fdd2077952265b6acdf7e5fc9349 Author: Ryota Ozaki Date: Tue Feb 16 15:35:41 2016 +0900 Fix race condition on if_link_state_change softint diff --git a/sys/net/if.c b/sys/net/if.c index f047c00..edaa1bd 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -201,6 +201,7 @@ static int ifconf(u_long, void *); static int if_clone_create(const char *); static int if_clone_destroy(const char *); static void if_link_state_change_si(void *); +static void if_link_state_change0(struct ifnet *, int, bool); struct if_percpuq { struct ifnet *ipq_ifp; @@ -208,6 +209,15 @@ struct if_percpuq { struct percpu *ipq_ifqs; /* struct ifqueue */ }; +struct link_state_change { + SIMPLEQ_ENTRY(link_state_change) lsc_entry; + int lsc_state; + bool lsc_lost; +}; + +static pool_cache_t link_state_change_pc __read_mostly; +static int max_link_state_changes = 10; + static struct mbuf *if_percpuq_dequeue(struct if_percpuq *); #if defined(INET) || defined(INET6) @@ -256,6 +266,9 @@ ifinit(void) /* interfaces are available, inform socket code */ ifioctl = doifioctl; + + link_state_change_pc = pool_cache_init(sizeof(struct link_state_change), + 0, 0, 0, "if_link_state_change", NULL, IPL_NET, NULL, NULL, NULL); } /* @@ -603,7 +616,10 @@ if_initialize(ifnet_t *ifp) ifp->if_broadcastaddr = 0; /* reliably crash if used uninitialized */ ifp->if_link_state = LINK_STATE_UNKNOWN; - ifp->if_old_link_state = LINK_STATE_UNKNOWN; + ifp->if_link_state_pending = LINK_STATE_UNKNOWN; + ifp->if_link_state_lost = false; + ifp->if_link_state_change_count = 0; + SIMPLEQ_INIT(&ifp->if_link_state_changes); ifp->if_capenable = 0; ifp->if_csum_flags_tx = 0; @@ -629,8 +645,9 @@ if_initialize(ifnet_t *ifp) IF_AFDATA_LOCK_INIT(ifp); - ifp->if_link_si = softint_establish(SOFTINT_NET, if_link_state_change_si, ifp); - if (ifp->if_link_si == NULL) + ifp->if_link_state_si = softint_establish(SOFTINT_NET, + if_link_state_change_si, ifp); + if (ifp->if_link_state_si == NULL) panic("%s: softint_establish() failed", __func__); if_getindex(ifp); @@ -1051,8 +1068,14 @@ again: IF_AFDATA_LOCK_DESTROY(ifp); - softint_disestablish(ifp->if_link_si); - ifp->if_link_si = NULL; + softint_disestablish(ifp->if_link_state_si); + ifp->if_link_state_si = NULL; + while (!SIMPLEQ_EMPTY(&ifp->if_link_state_changes)) { + struct link_state_change *lsc; + lsc = SIMPLEQ_FIRST(&ifp->if_link_state_changes); + SIMPLEQ_REMOVE_HEAD(&ifp->if_link_state_changes, lsc_entry); + pool_cache_put(link_state_change_pc, lsc); + } /* * remove packets that came from ifp, from software interrupt queues. @@ -1573,40 +1596,78 @@ if_link_state_change(struct ifnet *ifp, int link_state) int s; s = splnet(); - if (ifp->if_link_state == link_state) { - splx(s); - return; + /* + * Skip redundant change events. If there is no pending events, + * if_link_state_pending equals to the current if_link_state. + * If events are pending, if_link_state_pending indicates + * a state of the latest event. + */ + if (ifp->if_link_state_pending == link_state) + goto out; + + /* ??? */ + if (ifp->if_link_state_pending != ifp->if_link_state) { + struct link_state_change *lsc; + + /* We don't keep too many events to avoid DoS */ + if (ifp->if_link_state_change_count > + max_link_state_changes) { + ifp->if_link_state_lost = true; + goto change; + } + + lsc = pool_cache_get(link_state_change_pc, PR_NOWAIT); + if (lsc == NULL) { + ifp->if_link_state_lost = true; + goto change; + } + + lsc->lsc_state = link_state; + lsc->lsc_lost = ifp->if_link_state_lost; + SIMPLEQ_INSERT_TAIL(&ifp->if_link_state_changes, lsc, + lsc_entry); + ifp->if_link_state_change_count++; } - ifp->if_link_state = link_state; - softint_schedule(ifp->if_link_si); - + ifp->if_link_state_lost = false; +change: + ifp->if_link_state_pending = link_state; + softint_schedule(ifp->if_link_state_si); +out: splx(s); } - static void if_link_state_change_si(void *arg) { struct ifnet *ifp = arg; + struct link_state_change *lsc; int s; - int link_state, old_link_state; - struct domain *dp; s = splnet(); - link_state = ifp->if_link_state; - old_link_state = ifp->if_old_link_state; - ifp->if_old_link_state = ifp->if_link_state; + while (!SIMPLEQ_EMPTY(&ifp->if_link_state_changes)) { + lsc = SIMPLEQ_FIRST(&ifp->if_link_state_changes); + SIMPLEQ_REMOVE_HEAD(&ifp->if_link_state_changes, lsc_entry); + if_link_state_change0(ifp, lsc->lsc_state, lsc->lsc_lost); + pool_cache_put(link_state_change_pc, lsc); + ifp->if_link_state_change_count--; + } -#ifdef DEBUG - log(LOG_DEBUG, "%s: link state %s (was %s)\n", ifp->if_xname, - link_state == LINK_STATE_UP ? "UP" : - link_state == LINK_STATE_DOWN ? "DOWN" : - "UNKNOWN", - old_link_state == LINK_STATE_UP ? "UP" : - old_link_state == LINK_STATE_DOWN ? "DOWN" : - "UNKNOWN"); -#endif + /* Make sure the last state is reflected */ + if (ifp->if_link_state_pending != ifp->if_link_state) + if_link_state_change0(ifp, ifp->if_link_state_pending, + ifp->if_link_state_lost); + KASSERT(ifp->if_link_state == ifp->if_link_state_pending); + + ifp->if_link_state_lost = false; + splx(s); +} + +static void +if_link_state_change0(struct ifnet *ifp, int link_state, bool lost) +{ + int old_link_state __debugused; + struct domain *dp; /* * When going from UNKNOWN to UP, we need to mark existing @@ -1617,9 +1678,8 @@ if_link_state_change_si(void *arg) * listeners would have an address and expect it to work right * away. */ - if (link_state == LINK_STATE_UP && - old_link_state == LINK_STATE_UNKNOWN) - { + if (lost || (ifp->if_link_state == LINK_STATE_UP && + link_state == LINK_STATE_UNKNOWN)) { DOMAIN_FOREACH(dp) { if (dp->dom_if_link_state_change != NULL) dp->dom_if_link_state_change(ifp, @@ -1627,6 +1687,19 @@ if_link_state_change_si(void *arg) } } + old_link_state = ifp->if_link_state; + ifp->if_link_state = link_state; + +#ifdef DEBUG + log(LOG_DEBUG, "%s: link state %s (was %s)\n", ifp->if_xname, + link_state == LINK_STATE_UP ? "UP" : + link_state == LINK_STATE_DOWN ? "DOWN" : + "UNKNOWN", + old_link_state == LINK_STATE_UP ? "UP" : + old_link_state == LINK_STATE_DOWN ? "DOWN" : + "UNKNOWN"); +#endif + /* Notify that the link state has changed. */ rt_ifmsg(ifp); @@ -1639,8 +1712,6 @@ if_link_state_change_si(void *arg) if (dp->dom_if_link_state_change != NULL) dp->dom_if_link_state_change(ifp, link_state); } - - splx(s); } /* diff --git a/sys/net/if.h b/sys/net/if.h index c9a0fe6..d39f794 100644 --- a/sys/net/if.h +++ b/sys/net/if.h @@ -248,6 +248,7 @@ struct bridge_iflist; struct callout; struct krwlock; struct if_percpuq; +struct link_state_change; typedef struct ifnet { void *if_softc; /* lower-level data for this if */ @@ -350,8 +351,12 @@ typedef struct ifnet { struct callout *if_slowtimo_ch; struct krwlock *if_afdata_lock; struct if_percpuq *if_percpuq; /* We should remove it in the future */ - void *if_link_si; /* softint to handle link state changes */ - int if_old_link_state; /* previous link state */ + void *if_link_state_si; /* softint to handle link state changes */ + int if_link_state_pending; /* latest pending link state */ + /* events of link state changes to the softint */ + SIMPLEQ_HEAD(,link_state_change) if_link_state_changes; + int if_link_state_change_count; /* the number of pending events */ + bool if_link_state_lost; /* if event(s) is lost or not */ #endif } ifnet_t;