Skip site navigation (1)Skip section navigation (2) 
Date:      Fri, 10 Sep 2021 18:28:01 GMT
From:      Gleb Smirnoff <glebius@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org
Subject:   git: 89042ff77668 - main - ng_l2tp: improve callout locking.
Message-ID:  <202109101828.18AIS13U077270@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
The branch main has been updated by glebius:

URL: https://cgit.FreeBSD.org/src/commit/?id=89042ff77668555e77c88549e6ba697088ee72f9

commit 89042ff77668555e77c88549e6ba697088ee72f9
Author:     Gleb Smirnoff <glebius@FreeBSD.org>
AuthorDate: 2021-08-06 23:04:31 +0000
Commit:     Gleb Smirnoff <glebius@FreeBSD.org>
CommitDate: 2021-09-10 18:27:19 +0000

    ng_l2tp: improve callout locking.
    
    Apparently e62e4b85942 wasn't enough to close the race between
    a queue being flushed by a packet and callout executing, because
    the callouts used without a lock aren't 100% bulletproof. To close
    the race use callout_init_mtx() for L2TP timers, and make sure that
    all calls to ng_callout()/ng_uncallout() are done under the seq lock.
    
    If used properly, a locked callout can be used transparently with
    old netgraph KPI of ng_callout/ng_uncallout which predates locked
    callouts.
    
    While here, utilize ng_uncallout_drain() instead of ng_uncallout()
    on the node shutdown.
    
    PR:                     241133
    Reviewed by:            mjg, markj
    Differential Revision:  https://reviews.freebsd.org/D31476
---
 sys/netgraph/ng_l2tp.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/sys/netgraph/ng_l2tp.c b/sys/netgraph/ng_l2tp.c
index c62bde0c54f7..916f5db6c2ec 100644
--- a/sys/netgraph/ng_l2tp.c
+++ b/sys/netgraph/ng_l2tp.c
@@ -662,8 +662,8 @@ ng_l2tp_shutdown(node_p node)
 	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);
 
@@ -1192,9 +1192,9 @@ ng_l2tp_seq_init(priv_p priv)
 	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);
+	callout_init_mtx(&seq->rack_timer, &seq->mtx, CALLOUT_RETURNUNLOCKED);
+	callout_init_mtx(&seq->xack_timer, &seq->mtx, CALLOUT_RETURNUNLOCKED);
 }
 
 /*
@@ -1408,12 +1408,9 @@ ng_l2tp_seq_xack_timeout(node_p node, hook_p hook, void *arg1, int arg2)
 	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;
-
-	SEQ_LOCK(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);
@@ -1434,14 +1431,10 @@ ng_l2tp_seq_rack_timeout(node_p node, hook_p hook, void *arg1, int arg2)
 	struct mbuf *m;
 	u_int delay;
 
-	SEQ_LOCK(seq);
-
-	/* Make sure callout is still active before doing anything */
-	if (callout_pending(&seq->rack_timer) ||
-	    !callout_active(&seq->rack_timer)) {
-		SEQ_UNLOCK(seq);
-		return;
-	}
+	SEQ_LOCK_ASSERT(seq);
+	MPASS(seq->xwin[0]);
+	MPASS(!callout_pending(&seq->rack_timer));
+	MPASS(callout_active(&seq->rack_timer));
 
 	priv->stats.xmitRetransmits++;

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202109101828.18AIS13U077270>