Replying to my own commit. Looks like either this commit or following 89042ff77668 has a bug that leaks locked mutex to the kernel interrupt thread. It is really hard to reproduce it, happened just once for me. I failed to find the leak just reviewing the code.
If you are using ng_l2tp and experience a bug when some network-associated program (usually ifconfig) blocks forever in kernel, please get in touch with me. On Fri, Sep 10, 2021 at 06:28:00PM +0000, Gleb Smirnoff wrote: T> The branch main has been updated by glebius: T> T> URL: https://cgit.FreeBSD.org/src/commit/?id=0a76c63dd4987d8f7af37fe93569ce8a020cf43e T> T> commit 0a76c63dd4987d8f7af37fe93569ce8a020cf43e T> Author: Gleb Smirnoff <gleb...@freebsd.org> T> AuthorDate: 2021-08-06 22:49:51 +0000 T> Commit: Gleb Smirnoff <gleb...@freebsd.org> T> CommitDate: 2021-09-10 18:27:13 +0000 T> T> ng_l2tp: improve seq structure locking. T> T> Cover few cases of access to seq without lock missed in 702f98951d5. T> There are no known bugs fixed with this change, however. With INVARIANTS T> embed ng_l2tp_seq_check() into lock/unlock macros. Slightly reduce number T> of locks/unlocks per packet keeping the lock between functions. T> T> Reviewed by: mjg, markj T> Differential Revision: https://reviews.freebsd.org/D31476 T> --- T> sys/netgraph/ng_l2tp.c | 147 ++++++++++++++++++++----------------------------- T> 1 file changed, 61 insertions(+), 86 deletions(-) T> T> diff --git a/sys/netgraph/ng_l2tp.c b/sys/netgraph/ng_l2tp.c T> index 9b6f19f9f0ad..c62bde0c54f7 100644 T> --- a/sys/netgraph/ng_l2tp.c T> +++ b/sys/netgraph/ng_l2tp.c T> @@ -188,10 +188,6 @@ static void ng_l2tp_seq_rack_timeout(node_p node, hook_p hook, T> static hookpriv_p ng_l2tp_find_session(priv_p privp, u_int16_t sid); T> static ng_fn_eachhook ng_l2tp_reset_session; T> T> -#ifdef INVARIANTS T> -static void ng_l2tp_seq_check(struct l2tp_seq *seq); T> -#endif T> - T> /* Parse type for struct ng_l2tp_seq_config. */ T> static const struct ng_parse_struct_field T> ng_l2tp_seq_config_fields[] = NG_L2TP_SEQ_CONFIG_TYPE_INFO; T> @@ -335,12 +331,22 @@ static struct ng_type ng_l2tp_typestruct = { T> }; T> NETGRAPH_INIT(l2tp, &ng_l2tp_typestruct); T> T> -/* Sequence number state sanity checking */ T> +/* Sequence number state locking & sanity checking */ T> #ifdef INVARIANTS T> -#define L2TP_SEQ_CHECK(seq) ng_l2tp_seq_check(seq) T> +static void ng_l2tp_seq_check(struct l2tp_seq *seq); T> +#define SEQ_LOCK(seq) do { \ T> + mtx_lock(&(seq)->mtx); \ T> + ng_l2tp_seq_check(seq); \ T> +} while (0) T> +#define SEQ_UNLOCK(seq) do { \ T> + ng_l2tp_seq_check(seq); \ T> + mtx_unlock(&(seq)->mtx); \ T> +} while (0) T> #else T> -#define L2TP_SEQ_CHECK(x) do { } while (0) T> +#define SEQ_LOCK(seq) mtx_lock(&(seq)->mtx) T> +#define SEQ_UNLOCK(seq) mtx_unlock(&(seq)->mtx) T> #endif T> +#define SEQ_LOCK_ASSERT(seq) mtx_assert(&(seq)->mtx, MA_OWNED) T> T> /* Whether to use m_copypacket() or m_dup() */ T> #define L2TP_COPY_MBUF m_copypacket T> @@ -650,11 +656,10 @@ ng_l2tp_shutdown(node_p node) T> const priv_p priv = NG_NODE_PRIVATE(node); T> struct l2tp_seq *const seq = &priv->seq; T> T> - /* Sanity check */ T> - L2TP_SEQ_CHECK(seq); T> - T> /* Reset sequence number state */ T> + SEQ_LOCK(seq); T> ng_l2tp_seq_reset(priv); T> + SEQ_UNLOCK(seq); T> T> /* Free private data if neither timer is running */ T> ng_uncallout(&seq->rack_timer, node); T> @@ -757,9 +762,6 @@ ng_l2tp_rcvdata_lower(hook_p h, item_p item) T> int error; T> int len, plen; T> T> - /* Sanity check */ T> - L2TP_SEQ_CHECK(&priv->seq); T> - T> /* If not configured, reject */ T> if (!priv->conf.enabled) { T> NG_FREE_ITEM(item); T> @@ -899,18 +901,20 @@ ng_l2tp_rcvdata_lower(hook_p h, item_p item) T> if ((hdr & L2TP_HDR_CTRL) != 0) { T> struct l2tp_seq *const seq = &priv->seq; T> T> + SEQ_LOCK(seq); T> + T> /* Handle receive ack sequence number Nr */ T> ng_l2tp_seq_recv_nr(priv, nr); T> T> /* Discard ZLB packets */ T> if (m->m_pkthdr.len == 0) { T> + SEQ_UNLOCK(seq); T> priv->stats.recvZLBs++; T> NG_FREE_ITEM(item); T> NG_FREE_M(m); T> ERROUT(0); T> } T> T> - mtx_lock(&seq->mtx); T> /* T> * If not what we expect or we are busy, drop packet and T> * send an immediate ZLB ack. T> @@ -920,23 +924,16 @@ ng_l2tp_rcvdata_lower(hook_p h, item_p item) T> priv->stats.recvDuplicates++; T> else T> priv->stats.recvOutOfOrder++; T> - mtx_unlock(&seq->mtx); T> ng_l2tp_xmit_ctrl(priv, NULL, seq->ns); T> NG_FREE_ITEM(item); T> NG_FREE_M(m); T> ERROUT(0); T> } T> - /* T> - * Until we deliver this packet we can't receive next one as T> - * we have no information for sending ack. T> - */ T> - seq->inproc = 1; T> - mtx_unlock(&seq->mtx); T> T> /* Prepend session ID to packet. */ T> M_PREPEND(m, 2, M_NOWAIT); T> if (m == NULL) { T> - seq->inproc = 0; T> + SEQ_UNLOCK(seq); T> priv->stats.memoryFailures++; T> NG_FREE_ITEM(item); T> ERROUT(ENOBUFS); T> @@ -944,10 +941,17 @@ ng_l2tp_rcvdata_lower(hook_p h, item_p item) T> mtod(m, u_int8_t *)[0] = sid >> 8; T> mtod(m, u_int8_t *)[1] = sid & 0xff; T> T> + /* T> + * Until we deliver this packet we can't receive next one as T> + * we have no information for sending ack. T> + */ T> + seq->inproc = 1; T> + SEQ_UNLOCK(seq); T> + T> /* Deliver packet to upper layers */ T> NG_FWD_NEW_DATA(error, item, priv->ctrl, m); T> T> - mtx_lock(&seq->mtx); T> + SEQ_LOCK(seq); T> /* Ready to process next packet. */ T> seq->inproc = 0; T> T> @@ -962,7 +966,7 @@ ng_l2tp_rcvdata_lower(hook_p h, item_p item) T> NULL, 0); T> } T> } T> - mtx_unlock(&seq->mtx); T> + SEQ_UNLOCK(seq); T> T> ERROUT(error); T> } T> @@ -997,8 +1001,6 @@ ng_l2tp_rcvdata_lower(hook_p h, item_p item) T> /* Deliver data */ T> NG_FWD_NEW_DATA(error, item, hook, m); T> done: T> - /* Done */ T> - L2TP_SEQ_CHECK(&priv->seq); T> return (error); T> } T> T> @@ -1016,9 +1018,6 @@ ng_l2tp_rcvdata_ctrl(hook_p hook, item_p item) T> int i; T> u_int16_t ns; T> T> - /* Sanity check */ T> - L2TP_SEQ_CHECK(&priv->seq); T> - T> /* If not configured, reject */ T> if (!priv->conf.enabled) { T> NG_FREE_ITEM(item); T> @@ -1043,12 +1042,12 @@ ng_l2tp_rcvdata_ctrl(hook_p hook, item_p item) T> ERROUT(EOVERFLOW); T> } T> T> - mtx_lock(&seq->mtx); T> + SEQ_LOCK(seq); T> T> /* Find next empty slot in transmit queue */ T> for (i = 0; i < L2TP_MAX_XWIN && seq->xwin[i] != NULL; i++); T> if (i == L2TP_MAX_XWIN) { T> - mtx_unlock(&seq->mtx); T> + SEQ_UNLOCK(seq); T> priv->stats.xmitDrops++; T> m_freem(m); T> ERROUT(ENOBUFS); T> @@ -1057,7 +1056,7 @@ ng_l2tp_rcvdata_ctrl(hook_p hook, item_p item) T> T> /* If peer's receive window is already full, nothing else to do */ T> if (i >= seq->cwnd) { T> - mtx_unlock(&seq->mtx); T> + SEQ_UNLOCK(seq); T> ERROUT(0); T> } T> T> @@ -1068,10 +1067,9 @@ ng_l2tp_rcvdata_ctrl(hook_p hook, item_p item) T> T> ns = seq->ns++; T> T> - mtx_unlock(&seq->mtx); T> - T> /* Copy packet */ T> if ((m = L2TP_COPY_MBUF(m, M_NOWAIT)) == NULL) { T> + SEQ_UNLOCK(seq); T> priv->stats.memoryFailures++; T> ERROUT(ENOBUFS); T> } T> @@ -1079,8 +1077,6 @@ ng_l2tp_rcvdata_ctrl(hook_p hook, item_p item) T> /* Send packet and increment xmit sequence number */ T> error = ng_l2tp_xmit_ctrl(priv, m, ns); T> done: T> - /* Done */ T> - L2TP_SEQ_CHECK(&priv->seq); T> return (error); T> } T> T> @@ -1098,9 +1094,6 @@ ng_l2tp_rcvdata(hook_p hook, item_p item) T> int error; T> int i = 2; T> T> - /* Sanity check */ T> - L2TP_SEQ_CHECK(&priv->seq); T> - T> /* If not configured, reject */ T> if (!priv->conf.enabled) { T> NG_FREE_ITEM(item); T> @@ -1161,8 +1154,6 @@ ng_l2tp_rcvdata(hook_p hook, item_p item) T> /* Send packet */ T> NG_FWD_NEW_DATA(error, item, priv->lower, m); T> done: T> - /* Done */ T> - L2TP_SEQ_CHECK(&priv->seq); T> return (error); T> } T> T> @@ -1204,7 +1195,6 @@ ng_l2tp_seq_init(priv_p priv) T> ng_callout_init(&seq->rack_timer); T> ng_callout_init(&seq->xack_timer); T> mtx_init(&seq->mtx, "ng_l2tp", NULL, MTX_DEF); T> - L2TP_SEQ_CHECK(seq); T> } T> T> /* T> @@ -1240,10 +1230,13 @@ ng_l2tp_seq_adjust(priv_p priv, const struct ng_l2tp_config *conf) T> { T> struct l2tp_seq *const seq = &priv->seq; T> u_int16_t new_wmax; T> + int error = 0; T> T> + SEQ_LOCK(seq); T> /* If disabling node, reset state sequence number */ T> if (!conf->enabled) { T> ng_l2tp_seq_reset(priv); T> + SEQ_UNLOCK(seq); T> return (0); T> } T> T> @@ -1252,13 +1245,14 @@ ng_l2tp_seq_adjust(priv_p priv, const struct ng_l2tp_config *conf) T> if (new_wmax > L2TP_MAX_XWIN) T> new_wmax = L2TP_MAX_XWIN; T> if (new_wmax == 0) T> - return (EINVAL); T> + ERROUT(EINVAL); T> if (new_wmax < seq->wmax) T> - return (EBUSY); T> + ERROUT(EBUSY); T> seq->wmax = new_wmax; T> T> - /* Done */ T> - return (0); T> +done: T> + SEQ_UNLOCK(seq); T> + return (error); T> } T> T> /* T> @@ -1271,8 +1265,7 @@ ng_l2tp_seq_reset(priv_p priv) T> hook_p hook; T> int i; T> T> - /* Sanity check */ T> - L2TP_SEQ_CHECK(seq); T> + SEQ_LOCK_ASSERT(seq); T> T> /* Stop timers */ T> ng_uncallout(&seq->rack_timer, priv->node); T> @@ -1299,9 +1292,6 @@ ng_l2tp_seq_reset(priv_p priv) T> seq->acks = 0; T> seq->rexmits = 0; T> bzero(seq->xwin, sizeof(seq->xwin)); T> - T> - /* Done */ T> - L2TP_SEQ_CHECK(seq); T> } T> T> /* T> @@ -1316,15 +1306,12 @@ ng_l2tp_seq_recv_nr(priv_p priv, u_int16_t nr) T> int i, j; T> uint16_t ns; T> T> - mtx_lock(&seq->mtx); T> + SEQ_LOCK_ASSERT(seq); T> T> /* Verify peer's ACK is in range */ T> - if ((nack = L2TP_SEQ_DIFF(nr, seq->rack)) <= 0) { T> - mtx_unlock(&seq->mtx); T> + if ((nack = L2TP_SEQ_DIFF(nr, seq->rack)) <= 0) T> return; /* duplicate ack */ T> - } T> if (L2TP_SEQ_DIFF(nr, seq->ns) > 0) { T> - mtx_unlock(&seq->mtx); T> priv->stats.recvBadAcks++; /* ack for packet not sent */ T> return; T> } T> @@ -1375,10 +1362,8 @@ ng_l2tp_seq_recv_nr(priv_p priv, u_int16_t nr) T> ng_uncallout(&seq->rack_timer, priv->node); T> T> /* If transmit queue is empty, we're done for now */ T> - if (seq->xwin[0] == NULL) { T> - mtx_unlock(&seq->mtx); T> + if (seq->xwin[0] == NULL) T> return; T> - } T> T> /* Start restransmit timer again */ T> ng_callout(&seq->rack_timer, priv->node, NULL, T> @@ -1396,8 +1381,6 @@ ng_l2tp_seq_recv_nr(priv_p priv, u_int16_t nr) T> seq->ns++; T> } T> T> - mtx_unlock(&seq->mtx); T> - T> /* T> * Send prepared. T> * If there is a memory error, pretend packet was sent, as it T> @@ -1407,8 +1390,10 @@ ng_l2tp_seq_recv_nr(priv_p priv, u_int16_t nr) T> struct mbuf *m; T> if ((m = L2TP_COPY_MBUF(xwin[i], M_NOWAIT)) == NULL) T> priv->stats.memoryFailures++; T> - else T> + else { T> ng_l2tp_xmit_ctrl(priv, m, ns); T> + SEQ_LOCK(seq); T> + } T> ns++; T> } T> } T> @@ -1428,17 +1413,13 @@ ng_l2tp_seq_xack_timeout(node_p node, hook_p hook, void *arg1, int arg2) T> (!callout_active(&seq->xack_timer))) T> return; T> T> - /* Sanity check */ T> - L2TP_SEQ_CHECK(seq); T> + SEQ_LOCK(seq); T> T> /* Send a ZLB */ T> ng_l2tp_xmit_ctrl(priv, NULL, seq->ns); T> T> /* callout_deactivate() is not needed here T> as ng_uncallout() was called by ng_l2tp_xmit_ctrl() */ T> - T> - /* Sanity check */ T> - L2TP_SEQ_CHECK(seq); T> } T> T> /* T> @@ -1453,14 +1434,12 @@ ng_l2tp_seq_rack_timeout(node_p node, hook_p hook, void *arg1, int arg2) T> struct mbuf *m; T> u_int delay; T> T> - /* Sanity check */ T> - L2TP_SEQ_CHECK(seq); T> + SEQ_LOCK(seq); T> T> - mtx_lock(&seq->mtx); T> /* Make sure callout is still active before doing anything */ T> if (callout_pending(&seq->rack_timer) || T> !callout_active(&seq->rack_timer)) { T> - mtx_unlock(&seq->mtx); T> + SEQ_UNLOCK(seq); T> return; T> } T> T> @@ -1485,41 +1464,39 @@ ng_l2tp_seq_rack_timeout(node_p node, hook_p hook, void *arg1, int arg2) T> T> /* Retransmit oldest unack'd packet */ T> m = L2TP_COPY_MBUF(seq->xwin[0], M_NOWAIT); T> - mtx_unlock(&seq->mtx); T> - if (m == NULL) T> + if (m == NULL) { T> + SEQ_UNLOCK(seq); T> priv->stats.memoryFailures++; T> - else T> + } else T> ng_l2tp_xmit_ctrl(priv, m, seq->ns++); T> T> /* callout_deactivate() is not needed here T> as ng_callout() is getting called each time */ T> - T> - /* Sanity check */ T> - L2TP_SEQ_CHECK(seq); T> } T> T> /* T> * Transmit a control stream packet, payload optional. T> * The transmit sequence number is not incremented. T> + * Requires seq lock, returns unlocked. T> */ T> static int T> ng_l2tp_xmit_ctrl(priv_p priv, struct mbuf *m, u_int16_t ns) T> { T> struct l2tp_seq *const seq = &priv->seq; T> uint8_t *p; T> - u_int16_t session_id = 0; T> + uint16_t nr, session_id = 0; T> int error; T> T> - mtx_lock(&seq->mtx); T> + SEQ_LOCK_ASSERT(seq); T> T> /* Stop ack timer: we're sending an ack with this packet. T> Doing this before to keep state predictable after error. */ T> if (callout_active(&seq->xack_timer)) T> ng_uncallout(&seq->xack_timer, priv->node); T> T> - seq->xack = seq->nr; T> + nr = seq->xack = seq->nr; T> T> - mtx_unlock(&seq->mtx); T> + SEQ_UNLOCK(seq); T> T> /* If no mbuf passed, send an empty packet (ZLB) */ T> if (m == NULL) { T> @@ -1570,8 +1547,8 @@ ng_l2tp_xmit_ctrl(priv_p priv, struct mbuf *m, u_int16_t ns) T> p[7] = session_id & 0xff; T> p[8] = ns >> 8; T> p[9] = ns & 0xff; T> - p[10] = seq->nr >> 8; T> - p[11] = seq->nr & 0xff; T> + p[10] = nr >> 8; T> + p[11] = nr & 0xff; T> T> /* Update sequence number info and stats */ T> priv->stats.xmitPackets++; T> @@ -1594,7 +1571,7 @@ ng_l2tp_seq_check(struct l2tp_seq *seq) T> T> #define CHECK(p) KASSERT((p), ("%s: not: %s", __func__, #p)) T> T> - mtx_lock(&seq->mtx); T> + SEQ_LOCK_ASSERT(seq); T> T> self_unack = L2TP_SEQ_DIFF(seq->nr, seq->xack); T> peer_unack = L2TP_SEQ_DIFF(seq->ns, seq->rack); T> @@ -1617,8 +1594,6 @@ ng_l2tp_seq_check(struct l2tp_seq *seq) T> for ( ; i < seq->cwnd; i++) /* verify peer's recv window full */ T> CHECK(seq->xwin[i] == NULL); T> T> - mtx_unlock(&seq->mtx); T> - T> #undef CHECK T> } T> #endif /* INVARIANTS */ T> _______________________________________________ T> dev-commits-src-...@freebsd.org mailing list T> https://lists.freebsd.org/mailman/listinfo/dev-commits-src-all T> To unsubscribe, send any mail to "dev-commits-src-all-unsubscr...@freebsd.org" -- Gleb Smirnoff
