From nobody Fri Dec 03 16:58:59 2021 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 56E7918BA2A4; Fri, 3 Dec 2021 16:59:00 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4J5Jv80dYHz4cCJ; Fri, 3 Dec 2021 16:59:00 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id E9A31149A2; Fri, 3 Dec 2021 16:58:59 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 1B3Gwxp5043739; Fri, 3 Dec 2021 16:58:59 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 1B3GwxKt043738; Fri, 3 Dec 2021 16:58:59 GMT (envelope-from git) Date: Fri, 3 Dec 2021 16:58:59 GMT Message-Id: <202112031658.1B3GwxKt043738@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Gleb Smirnoff Subject: git: ae04d3045105 - main - ng_l2tp: use callout_reset() instead of ng_callout() List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: glebius X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: ae04d30451056f16096cba7d8debcb15dac275d7 Auto-Submitted: auto-generated ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1638550740; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=wr77omrD8aDBt1QwT9zg0mGgKg1xaD/hd0v9LP9gEy4=; b=TAcEaI3i/UnaFThRHZnsq/Xsh+GhmvLsg7bYu4Sw6pphfI/CihJYW8ttgPkH8k1D6vfYJX sRpQ6iN5AOA2xRhpbtshcDBFNU9dD0U1WcR3Qx6zWWIjk+/cAtz0HUV69bhurnL5BSzvE7 5MVMKU+A1oshxue1EGe1i83YmSPugjX3XyGwTvpu71F2RDHdCzfjwHlQHMRKFhkJmZ+1Ca cb08IWLdBfYCqazJGKw69H/3kAENzUL1Y4jDdKf0P4KMTP87HeR/Xhuii5nCURU3OpOAWl uzEp1l4V9WX8vBKcpwRvbmz0IAi/mWpJzfaXxQVeU4YfVa7VMO9T1gr7GJsNXA== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1638550740; a=rsa-sha256; cv=none; b=nHWQ75pu0U3tJKpXlvx6kZP01hH4klkVA0WNv+AGa9jbPRBOBbMvxGJm3GidYAdgIut97u oh+gN520eKnpb+t6t+kcfGSlQ4T6izlH1XzdCqmuFrDT1ROYEMEgm48cgiMpuR5Hxg12P7 DbWoBHXFLT7OoMDSA39dpFzNnflu+2mbEKcMQ9AxmrI4LsKXxiRyUUO8XDAp1xf+bJUXd/ OxNVEngZX6XUFfqm1qOrFtO9WnLz4bSu6JCU00Ptx6ik5H8fGh6Xr/dZoqyTlfOPaza4ED d0p8DC9v+l8co7avSM67mDglEwD+uGDbZPoJVXhrLm9lFeTOL6SM7gfpvf0o7A== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N The branch main has been updated by glebius: URL: https://cgit.FreeBSD.org/src/commit/?id=ae04d30451056f16096cba7d8debcb15dac275d7 commit ae04d30451056f16096cba7d8debcb15dac275d7 Author: Gleb Smirnoff AuthorDate: 2021-12-03 16:57:23 +0000 Commit: Gleb Smirnoff CommitDate: 2021-12-03 16:57:23 +0000 ng_l2tp: use callout_reset() instead of ng_callout() The previous commit to this node falsely stated that locked callouts are compatible with netgraph ng_callout KPI. They are not. An item can be queued instead of being applied to the node, which results in a mutex leak to the callout thread and later unlocked call into function that expects to be called locked. Potentially netgraph can be taught to handle locked callouts, but that would bring a lot of complexity in it. Instead lets question necessity of ng_callout() instead of callout_reset(). It protects against node going away while callout is scheduled. But a node that drains all callouts in the shutdown method (ng_l2tp does) is already protected. Fixes: 89042ff77668 --- sys/netgraph/ng_l2tp.c | 57 +++++++++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/sys/netgraph/ng_l2tp.c b/sys/netgraph/ng_l2tp.c index 916f5db6c2ec..94617fa4dad9 100644 --- a/sys/netgraph/ng_l2tp.c +++ b/sys/netgraph/ng_l2tp.c @@ -54,8 +54,11 @@ #include #include #include +#include #include +#include + #include #include #include @@ -180,10 +183,8 @@ static int ng_l2tp_seq_adjust(priv_p priv, static void ng_l2tp_seq_reset(priv_p priv); static void ng_l2tp_seq_failure(priv_p priv); static void ng_l2tp_seq_recv_nr(priv_p priv, u_int16_t nr); -static void ng_l2tp_seq_xack_timeout(node_p node, hook_p hook, - void *arg1, int arg2); -static void ng_l2tp_seq_rack_timeout(node_p node, hook_p hook, - void *arg1, int arg2); +static void ng_l2tp_seq_xack_timeout(void *); +static void ng_l2tp_seq_rack_timeout(void *); static hookpriv_p ng_l2tp_find_session(priv_p privp, u_int16_t sid); static ng_fn_eachhook ng_l2tp_reset_session; @@ -662,8 +663,8 @@ ng_l2tp_shutdown(node_p node) SEQ_UNLOCK(seq); /* Free private data if neither timer is running */ - ng_uncallout_drain(&seq->rack_timer, node); - ng_uncallout_drain(&seq->xack_timer, node); + callout_drain(&seq->rack_timer); + callout_drain(&seq->xack_timer); mtx_destroy(&seq->mtx); @@ -961,9 +962,9 @@ ng_l2tp_rcvdata_lower(hook_p h, item_p item) seq->nr++; /* Start receive ack timer, if not already running */ if (!callout_active(&seq->xack_timer)) { - ng_callout(&seq->xack_timer, priv->node, NULL, + callout_reset(&seq->xack_timer, L2TP_DELAYED_ACK, ng_l2tp_seq_xack_timeout, - NULL, 0); + node); } } SEQ_UNLOCK(seq); @@ -1062,8 +1063,8 @@ ng_l2tp_rcvdata_ctrl(hook_p hook, item_p item) /* Start retransmit timer if not already running */ if (!callout_active(&seq->rack_timer)) - ng_callout(&seq->rack_timer, node, NULL, - hz, ng_l2tp_seq_rack_timeout, NULL, 0); + callout_reset(&seq->rack_timer, hz, ng_l2tp_seq_rack_timeout, + node); ns = seq->ns++; @@ -1268,8 +1269,8 @@ ng_l2tp_seq_reset(priv_p priv) SEQ_LOCK_ASSERT(seq); /* Stop timers */ - ng_uncallout(&seq->rack_timer, priv->node); - ng_uncallout(&seq->xack_timer, priv->node); + (void )callout_stop(&seq->rack_timer); + (void )callout_stop(&seq->xack_timer); /* Free retransmit queue */ for (i = 0; i < L2TP_MAX_XWIN; i++) { @@ -1359,15 +1360,15 @@ ng_l2tp_seq_recv_nr(priv_p priv, u_int16_t nr) /* Stop xmit timer */ if (callout_active(&seq->rack_timer)) - ng_uncallout(&seq->rack_timer, priv->node); + (void )callout_stop(&seq->rack_timer); /* If transmit queue is empty, we're done for now */ if (seq->xwin[0] == NULL) return; /* Start restransmit timer again */ - ng_callout(&seq->rack_timer, priv->node, NULL, - hz, ng_l2tp_seq_rack_timeout, NULL, 0); + callout_reset(&seq->rack_timer, hz, ng_l2tp_seq_rack_timeout, + priv->node); /* * Send more packets, trying to keep peer's receive window full. @@ -1403,20 +1404,26 @@ ng_l2tp_seq_recv_nr(priv_p priv, u_int16_t nr) * were hoping to piggy-back, but haven't, so send a ZLB. */ static void -ng_l2tp_seq_xack_timeout(node_p node, hook_p hook, void *arg1, int arg2) +ng_l2tp_seq_xack_timeout(void *arg) { + const node_p node = arg; const priv_p priv = NG_NODE_PRIVATE(node); + struct epoch_tracker et; struct l2tp_seq *const seq = &priv->seq; SEQ_LOCK_ASSERT(seq); MPASS(!callout_pending(&seq->xack_timer)); MPASS(callout_active(&seq->xack_timer)); + NET_EPOCH_ENTER(et); + CURVNET_SET(node->nd_vnet); /* Send a ZLB */ ng_l2tp_xmit_ctrl(priv, NULL, seq->ns); + CURVNET_RESTORE(); + NET_EPOCH_EXIT(et); /* callout_deactivate() is not needed here - as ng_uncallout() was called by ng_l2tp_xmit_ctrl() */ + as callout_stop() was called by ng_l2tp_xmit_ctrl() */ } /* @@ -1424,9 +1431,11 @@ ng_l2tp_seq_xack_timeout(node_p node, hook_p hook, void *arg1, int arg2) * with an ack for our packet, so retransmit it. */ static void -ng_l2tp_seq_rack_timeout(node_p node, hook_p hook, void *arg1, int arg2) +ng_l2tp_seq_rack_timeout(void *arg) { + const node_p node = arg; const priv_p priv = NG_NODE_PRIVATE(node); + struct epoch_tracker et; struct l2tp_seq *const seq = &priv->seq; struct mbuf *m; u_int delay; @@ -1436,6 +1445,9 @@ ng_l2tp_seq_rack_timeout(node_p node, hook_p hook, void *arg1, int arg2) MPASS(!callout_pending(&seq->rack_timer)); MPASS(callout_active(&seq->rack_timer)); + NET_EPOCH_ENTER(et); + CURVNET_SET(node->nd_vnet); + priv->stats.xmitRetransmits++; /* Have we reached the retransmit limit? If so, notify owner. */ @@ -1446,8 +1458,8 @@ ng_l2tp_seq_rack_timeout(node_p node, hook_p hook, void *arg1, int arg2) delay = (seq->rexmits > 12) ? (1 << 12) : (1 << seq->rexmits); if (delay > priv->conf.rexmit_max_to) delay = priv->conf.rexmit_max_to; - ng_callout(&seq->rack_timer, node, NULL, - hz * delay, ng_l2tp_seq_rack_timeout, NULL, 0); + callout_reset(&seq->rack_timer, hz * delay, ng_l2tp_seq_rack_timeout, + node); /* Do slow-start/congestion algorithm windowing algorithm */ seq->ns = seq->rack; @@ -1463,6 +1475,9 @@ ng_l2tp_seq_rack_timeout(node_p node, hook_p hook, void *arg1, int arg2) } else ng_l2tp_xmit_ctrl(priv, m, seq->ns++); + CURVNET_RESTORE(); + NET_EPOCH_EXIT(et); + /* callout_deactivate() is not needed here as ng_callout() is getting called each time */ } @@ -1485,7 +1500,7 @@ ng_l2tp_xmit_ctrl(priv_p priv, struct mbuf *m, u_int16_t ns) /* 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); + (void )callout_stop(&seq->xack_timer); nr = seq->xack = seq->nr;