Re: git: 0a76c63dd498 - main - ng_l2tp: improve seq structure locking.
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Mon, 18 Oct 2021 16:50:23 UTC
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 <glebius@FreeBSD.org>
T> AuthorDate: 2021-08-06 22:49:51 +0000
T> Commit: Gleb Smirnoff <glebius@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-all@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-unsubscribe@freebsd.org"
--
Gleb Smirnoff