Index: sys/netgraph/bluetooth/hci/ng_hci_misc.c =================================================================== --- sys/netgraph/bluetooth/hci/ng_hci_misc.c +++ sys/netgraph/bluetooth/hci/ng_hci_misc.c @@ -391,7 +391,7 @@ panic( "%s: %s - No command timeout!\n", __func__, NG_NODE_NAME(unit->node)); - if (ng_uncallout(&unit->cmd_timo, unit->node) == 0) + if (ng_uncallout(&unit->cmd_timo, unit->node) < 1) return (ETIMEDOUT); unit->state &= ~NG_HCI_UNIT_COMMAND_PENDING; @@ -432,7 +432,7 @@ panic( "%s: %s - No connection timeout!\n", __func__, NG_NODE_NAME(con->unit->node)); - if (ng_uncallout(&con->con_timo, con->unit->node) == 0) + if (ng_uncallout(&con->con_timo, con->unit->node) < 1) return (ETIMEDOUT); con->flags &= ~NG_HCI_CON_TIMEOUT_PENDING; Index: sys/netgraph/bluetooth/l2cap/ng_l2cap_misc.c =================================================================== --- sys/netgraph/bluetooth/l2cap/ng_l2cap_misc.c +++ sys/netgraph/bluetooth/l2cap/ng_l2cap_misc.c @@ -235,7 +235,7 @@ __func__, NG_NODE_NAME(con->l2cap->node), con->state, con->flags); - if (ng_uncallout(&con->con_timo, con->l2cap->node) == 0) + if (ng_uncallout(&con->con_timo, con->l2cap->node) < 1) return (ETIMEDOUT); con->flags &= ~NG_L2CAP_CON_AUTO_DISCON_TIMO; @@ -534,7 +534,7 @@ __func__, NG_NODE_NAME(con->l2cap->node), con->state, con->flags); - if (ng_uncallout(&con->con_timo, con->l2cap->node) == 0) + if (ng_uncallout(&con->con_timo, con->l2cap->node) < 1) return (ETIMEDOUT); con->flags &= ~NG_L2CAP_CON_LP_TIMO; @@ -579,7 +579,7 @@ __func__, NG_NODE_NAME(cmd->con->l2cap->node), cmd->code, cmd->flags); - if (ng_uncallout(&cmd->timo, cmd->con->l2cap->node) == 0) + if (ng_uncallout(&cmd->timo, cmd->con->l2cap->node) < 1) return (ETIMEDOUT); cmd->flags &= ~NG_L2CAP_CMD_PENDING; Index: sys/netgraph/netgraph.h =================================================================== --- sys/netgraph/netgraph.h +++ sys/netgraph/netgraph.h @@ -1162,6 +1162,7 @@ int ng_send_fn2(node_p node, hook_p hook, item_p pitem, ng_item_fn2 *fn, void *arg1, int arg2, int flags); int ng_uncallout(struct callout *c, node_p node); +int ng_uncallout_drain(struct callout *c, node_p node); int ng_callout(struct callout *c, node_p node, hook_p hook, int ticks, ng_item_fn *fn, void * arg1, int arg2); #define ng_callout_init(c) callout_init(c, 1) Index: sys/netgraph/ng_base.c =================================================================== --- sys/netgraph/ng_base.c +++ sys/netgraph/ng_base.c @@ -3816,20 +3816,16 @@ return (0); } -/* A special modified version of callout_stop() */ -int -ng_uncallout(struct callout *c, node_p node) +/* + * Free references and item if callout_stop/callout_drain returned 1. + */ +static void +ng_uncallout_internal(struct callout *c, node_p node) { item_p item; - int rval; - - KASSERT(c != NULL, ("ng_uncallout: NULL callout")); - KASSERT(node != NULL, ("ng_uncallout: NULL node")); - rval = callout_stop(c); item = c->c_arg; - /* Do an extra check */ - if ((rval > 0) && (c->c_func == &ng_callout_trampoline) && + if ((c->c_func == &ng_callout_trampoline) && (item != NULL) && (NGI_NODE(item) == node)) { /* * We successfully removed it from the queue before it ran @@ -3839,12 +3835,40 @@ NG_FREE_ITEM(item); } c->c_arg = NULL; +} - /* - * Callers only want to know if the callout was cancelled and - * not draining or stopped. - */ - return (rval > 0); + +/* A special modified version of callout_stop() */ +int +ng_uncallout(struct callout *c, node_p node) +{ + int rval; + + rval = callout_stop(c); + if (rval > 0) + /* + * XXXGL: in case if callout is already running and next + * invocation is scheduled at the same time, callout_stop() + * returns 0. See d153eeee97d. In this case netgraph(4) would + * leak resources. However, no nodes are known to induce such + * behavior. + */ + ng_uncallout_internal(c, node); + + return (rval); +} + +/* A special modified version of callout_drain() */ +int +ng_uncallout_drain(struct callout *c, node_p node) +{ + int rval; + + rval = callout_drain(c); + if (rval > 0) + ng_uncallout_internal(c, node); + + return (rval); } /* Index: sys/netgraph/ng_l2tp.c =================================================================== --- sys/netgraph/ng_l2tp.c +++ sys/netgraph/ng_l2tp.c @@ -188,10 +188,6 @@ static hookpriv_p ng_l2tp_find_session(priv_p privp, u_int16_t sid); static ng_fn_eachhook ng_l2tp_reset_session; -#ifdef INVARIANTS -static void ng_l2tp_seq_check(struct l2tp_seq *seq); -#endif - /* Parse type for struct ng_l2tp_seq_config. */ static const struct ng_parse_struct_field ng_l2tp_seq_config_fields[] = NG_L2TP_SEQ_CONFIG_TYPE_INFO; @@ -335,12 +331,22 @@ }; NETGRAPH_INIT(l2tp, &ng_l2tp_typestruct); -/* Sequence number state sanity checking */ +/* Sequence number state locking & sanity checking */ #ifdef INVARIANTS -#define L2TP_SEQ_CHECK(seq) ng_l2tp_seq_check(seq) +static void ng_l2tp_seq_check(struct l2tp_seq *seq); +#define SEQ_LOCK(seq) do { \ + mtx_lock(&(seq)->mtx); \ + ng_l2tp_seq_check(seq); \ +} while (0) +#define SEQ_UNLOCK(seq) do { \ + ng_l2tp_seq_check(seq); \ + mtx_unlock(&(seq)->mtx); \ +} while (0) #else -#define L2TP_SEQ_CHECK(x) do { } while (0) +#define SEQ_LOCK(seq) mtx_lock(&(seq)->mtx) +#define SEQ_UNLOCK(seq) mtx_unlock(&(seq)->mtx) #endif +#define SEQ_LOCK_ASSERT(seq) mtx_assert(&(seq)->mtx, MA_OWNED) /* Whether to use m_copypacket() or m_dup() */ #define L2TP_COPY_MBUF m_copypacket @@ -650,15 +656,14 @@ const priv_p priv = NG_NODE_PRIVATE(node); struct l2tp_seq *const seq = &priv->seq; - /* Sanity check */ - L2TP_SEQ_CHECK(seq); - /* Reset sequence number state */ + SEQ_LOCK(seq); ng_l2tp_seq_reset(priv); + SEQ_UNLOCK(seq); /* Free private data if neither timer is running */ - ng_uncallout(&seq->rack_timer, node); - ng_uncallout(&seq->xack_timer, node); + ng_uncallout_drain(&seq->rack_timer, node); + ng_uncallout_drain(&seq->xack_timer, node); mtx_destroy(&seq->mtx); @@ -757,9 +762,6 @@ int error; int len, plen; - /* Sanity check */ - L2TP_SEQ_CHECK(&priv->seq); - /* If not configured, reject */ if (!priv->conf.enabled) { NG_FREE_ITEM(item); @@ -899,18 +901,20 @@ if ((hdr & L2TP_HDR_CTRL) != 0) { struct l2tp_seq *const seq = &priv->seq; + SEQ_LOCK(seq); + /* Handle receive ack sequence number Nr */ ng_l2tp_seq_recv_nr(priv, nr); /* Discard ZLB packets */ if (m->m_pkthdr.len == 0) { + SEQ_UNLOCK(seq); priv->stats.recvZLBs++; NG_FREE_ITEM(item); NG_FREE_M(m); ERROUT(0); } - mtx_lock(&seq->mtx); /* * If not what we expect or we are busy, drop packet and * send an immediate ZLB ack. @@ -920,23 +924,16 @@ priv->stats.recvDuplicates++; else priv->stats.recvOutOfOrder++; - mtx_unlock(&seq->mtx); ng_l2tp_xmit_ctrl(priv, NULL, seq->ns); NG_FREE_ITEM(item); NG_FREE_M(m); ERROUT(0); } - /* - * Until we deliver this packet we can't receive next one as - * we have no information for sending ack. - */ - seq->inproc = 1; - mtx_unlock(&seq->mtx); /* Prepend session ID to packet. */ M_PREPEND(m, 2, M_NOWAIT); if (m == NULL) { - seq->inproc = 0; + SEQ_UNLOCK(seq); priv->stats.memoryFailures++; NG_FREE_ITEM(item); ERROUT(ENOBUFS); @@ -944,10 +941,17 @@ mtod(m, u_int8_t *)[0] = sid >> 8; mtod(m, u_int8_t *)[1] = sid & 0xff; + /* + * Until we deliver this packet we can't receive next one as + * we have no information for sending ack. + */ + seq->inproc = 1; + SEQ_UNLOCK(seq); + /* Deliver packet to upper layers */ NG_FWD_NEW_DATA(error, item, priv->ctrl, m); - mtx_lock(&seq->mtx); + SEQ_LOCK(seq); /* Ready to process next packet. */ seq->inproc = 0; @@ -962,7 +966,7 @@ NULL, 0); } } - mtx_unlock(&seq->mtx); + SEQ_UNLOCK(seq); ERROUT(error); } @@ -997,8 +1001,6 @@ /* Deliver data */ NG_FWD_NEW_DATA(error, item, hook, m); done: - /* Done */ - L2TP_SEQ_CHECK(&priv->seq); return (error); } @@ -1016,9 +1018,6 @@ int i; u_int16_t ns; - /* Sanity check */ - L2TP_SEQ_CHECK(&priv->seq); - /* If not configured, reject */ if (!priv->conf.enabled) { NG_FREE_ITEM(item); @@ -1043,12 +1042,12 @@ ERROUT(EOVERFLOW); } - mtx_lock(&seq->mtx); + SEQ_LOCK(seq); /* Find next empty slot in transmit queue */ for (i = 0; i < L2TP_MAX_XWIN && seq->xwin[i] != NULL; i++); if (i == L2TP_MAX_XWIN) { - mtx_unlock(&seq->mtx); + SEQ_UNLOCK(seq); priv->stats.xmitDrops++; m_freem(m); ERROUT(ENOBUFS); @@ -1057,7 +1056,7 @@ /* If peer's receive window is already full, nothing else to do */ if (i >= seq->cwnd) { - mtx_unlock(&seq->mtx); + SEQ_UNLOCK(seq); ERROUT(0); } @@ -1068,10 +1067,9 @@ ns = seq->ns++; - mtx_unlock(&seq->mtx); - /* Copy packet */ if ((m = L2TP_COPY_MBUF(m, M_NOWAIT)) == NULL) { + SEQ_UNLOCK(seq); priv->stats.memoryFailures++; ERROUT(ENOBUFS); } @@ -1079,8 +1077,6 @@ /* Send packet and increment xmit sequence number */ error = ng_l2tp_xmit_ctrl(priv, m, ns); done: - /* Done */ - L2TP_SEQ_CHECK(&priv->seq); return (error); } @@ -1098,9 +1094,6 @@ int error; int i = 2; - /* Sanity check */ - L2TP_SEQ_CHECK(&priv->seq); - /* If not configured, reject */ if (!priv->conf.enabled) { NG_FREE_ITEM(item); @@ -1161,8 +1154,6 @@ /* Send packet */ NG_FWD_NEW_DATA(error, item, priv->lower, m); done: - /* Done */ - L2TP_SEQ_CHECK(&priv->seq); return (error); } @@ -1201,10 +1192,9 @@ if (seq->wmax > L2TP_MAX_XWIN) seq->wmax = L2TP_MAX_XWIN; seq->ssth = seq->wmax; - ng_callout_init(&seq->rack_timer); - ng_callout_init(&seq->xack_timer); mtx_init(&seq->mtx, "ng_l2tp", NULL, MTX_DEF); - L2TP_SEQ_CHECK(seq); + callout_init_mtx(&seq->rack_timer, &seq->mtx, CALLOUT_RETURNUNLOCKED); + callout_init_mtx(&seq->xack_timer, &seq->mtx, CALLOUT_RETURNUNLOCKED); } /* @@ -1241,9 +1231,11 @@ struct l2tp_seq *const seq = &priv->seq; u_int16_t new_wmax; + SEQ_LOCK(seq); /* If disabling node, reset state sequence number */ if (!conf->enabled) { ng_l2tp_seq_reset(priv); + SEQ_UNLOCK(seq); return (0); } @@ -1271,8 +1263,7 @@ hook_p hook; int i; - /* Sanity check */ - L2TP_SEQ_CHECK(seq); + SEQ_LOCK_ASSERT(seq); /* Stop timers */ ng_uncallout(&seq->rack_timer, priv->node); @@ -1299,9 +1290,6 @@ seq->acks = 0; seq->rexmits = 0; bzero(seq->xwin, sizeof(seq->xwin)); - - /* Done */ - L2TP_SEQ_CHECK(seq); } /* @@ -1316,15 +1304,12 @@ int i, j; uint16_t ns; - mtx_lock(&seq->mtx); + SEQ_LOCK_ASSERT(seq); /* Verify peer's ACK is in range */ - if ((nack = L2TP_SEQ_DIFF(nr, seq->rack)) <= 0) { - mtx_unlock(&seq->mtx); + if ((nack = L2TP_SEQ_DIFF(nr, seq->rack)) <= 0) return; /* duplicate ack */ - } if (L2TP_SEQ_DIFF(nr, seq->ns) > 0) { - mtx_unlock(&seq->mtx); priv->stats.recvBadAcks++; /* ack for packet not sent */ return; } @@ -1375,10 +1360,8 @@ ng_uncallout(&seq->rack_timer, priv->node); /* If transmit queue is empty, we're done for now */ - if (seq->xwin[0] == NULL) { - mtx_unlock(&seq->mtx); + if (seq->xwin[0] == NULL) return; - } /* Start restransmit timer again */ ng_callout(&seq->rack_timer, priv->node, NULL, @@ -1396,8 +1379,6 @@ seq->ns++; } - mtx_unlock(&seq->mtx); - /* * Send prepared. * If there is a memory error, pretend packet was sent, as it @@ -1407,8 +1388,10 @@ struct mbuf *m; if ((m = L2TP_COPY_MBUF(xwin[i], M_NOWAIT)) == NULL) priv->stats.memoryFailures++; - else + else { ng_l2tp_xmit_ctrl(priv, m, ns); + SEQ_LOCK(seq); + } ns++; } } @@ -1423,22 +1406,15 @@ const priv_p priv = NG_NODE_PRIVATE(node); struct l2tp_seq *const seq = &priv->seq; - /* Make sure callout is still active before doing anything */ - if (callout_pending(&seq->xack_timer) || - (!callout_active(&seq->xack_timer))) - return; - - /* Sanity check */ - L2TP_SEQ_CHECK(seq); + SEQ_LOCK_ASSERT(seq); + MPASS(!callout_pending(&seq->xack_timer)); + MPASS(callout_active(&seq->xack_timer)); /* Send a ZLB */ ng_l2tp_xmit_ctrl(priv, NULL, seq->ns); /* callout_deactivate() is not needed here as ng_uncallout() was called by ng_l2tp_xmit_ctrl() */ - - /* Sanity check */ - L2TP_SEQ_CHECK(seq); } /* @@ -1453,16 +1429,9 @@ struct mbuf *m; u_int delay; - /* Sanity check */ - L2TP_SEQ_CHECK(seq); - - mtx_lock(&seq->mtx); - /* Make sure callout is still active before doing anything */ - if (callout_pending(&seq->rack_timer) || - !callout_active(&seq->rack_timer)) { - mtx_unlock(&seq->mtx); - return; - } + SEQ_LOCK_ASSERT(seq); + MPASS(!callout_pending(&seq->rack_timer)); + MPASS(callout_active(&seq->rack_timer)); priv->stats.xmitRetransmits++; @@ -1485,41 +1454,39 @@ /* Retransmit oldest unack'd packet */ m = L2TP_COPY_MBUF(seq->xwin[0], M_NOWAIT); - mtx_unlock(&seq->mtx); - if (m == NULL) + if (m == NULL) { + SEQ_UNLOCK(seq); priv->stats.memoryFailures++; - else + } else ng_l2tp_xmit_ctrl(priv, m, seq->ns++); /* callout_deactivate() is not needed here as ng_callout() is getting called each time */ - - /* Sanity check */ - L2TP_SEQ_CHECK(seq); } /* * Transmit a control stream packet, payload optional. * The transmit sequence number is not incremented. + * Requires seq lock, returns unlocked. */ static int ng_l2tp_xmit_ctrl(priv_p priv, struct mbuf *m, u_int16_t ns) { struct l2tp_seq *const seq = &priv->seq; uint8_t *p; - u_int16_t session_id = 0; + uint16_t nr, session_id = 0; int error; - mtx_lock(&seq->mtx); + SEQ_LOCK_ASSERT(seq); /* Stop ack timer: we're sending an ack with this packet. Doing this before to keep state predictable after error. */ if (callout_active(&seq->xack_timer)) ng_uncallout(&seq->xack_timer, priv->node); - seq->xack = seq->nr; + nr = seq->xack = seq->nr; - mtx_unlock(&seq->mtx); + SEQ_UNLOCK(seq); /* If no mbuf passed, send an empty packet (ZLB) */ if (m == NULL) { @@ -1570,8 +1537,8 @@ p[7] = session_id & 0xff; p[8] = ns >> 8; p[9] = ns & 0xff; - p[10] = seq->nr >> 8; - p[11] = seq->nr & 0xff; + p[10] = nr >> 8; + p[11] = nr & 0xff; /* Update sequence number info and stats */ priv->stats.xmitPackets++; @@ -1594,7 +1561,7 @@ #define CHECK(p) KASSERT((p), ("%s: not: %s", __func__, #p)) - mtx_lock(&seq->mtx); + SEQ_LOCK_ASSERT(seq); self_unack = L2TP_SEQ_DIFF(seq->nr, seq->xack); peer_unack = L2TP_SEQ_DIFF(seq->ns, seq->rack); @@ -1617,8 +1584,6 @@ for ( ; i < seq->cwnd; i++) /* verify peer's recv window full */ CHECK(seq->xwin[i] == NULL); - mtx_unlock(&seq->mtx); - #undef CHECK } #endif /* INVARIANTS */