Module Name: src
Committed By: riastradh
Date: Mon Jul 29 02:33:58 UTC 2024
Modified Files:
src/sys/net: if_wg.c
Log Message:
wg(4): No need for atomic access to wgs_time_established in tx/rx.
This is stable while the session is visible to the tx/rx paths -- it
is initialized before the session is exposed to tx/rx, and doesn't
change until the session is no longer used by any tx/rx path and has
been recycled.
When I sprinkled atomic access to wgs_time_established in if_wg.c
rev. 1.104, it was a vestige of an uncommitted draft that did the
transition from INIT_PASSIVE to ESTABLISHED in the tx path itself, in
an attempt to enable prompter tx on the new session as soon as it is
established. This turned out to be unnecessary, so I reverted most
of it, but forgot that wgs_time_established no longer needed atomic
treatment.
We could go back to using time_t and time_uptime, now that there's no
need to do atomic loads and stores on these quantities. But there's
no point in 64-bit arithmetic when the time differences are all
guaranteed bounded by a few minutes, so keeping it 32-bit is probably
a slight performance improvement on 32-bit systems.
(In contrast, wgs_time_last_data_sent is both written and read in the
tx path, which may run in parallel on multiple CPUs, so it still
requires the atomic treatment.)
Tidying up for:
PR kern/55729: net/if_wg/t_misc:wg_rekey test case fails
PR kern/56252: wg(4) state machine has race conditions
PR kern/58463: if_wg does not work when idle.
To generate a diff of this commit:
cvs rdiff -u -r1.116 -r1.117 src/sys/net/if_wg.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/net/if_wg.c
diff -u src/sys/net/if_wg.c:1.116 src/sys/net/if_wg.c:1.117
--- src/sys/net/if_wg.c:1.116 Mon Jul 29 02:33:44 2024
+++ src/sys/net/if_wg.c Mon Jul 29 02:33:58 2024
@@ -1,4 +1,4 @@
-/* $NetBSD: if_wg.c,v 1.116 2024/07/29 02:33:44 riastradh Exp $ */
+/* $NetBSD: if_wg.c,v 1.117 2024/07/29 02:33:58 riastradh Exp $ */
/*
* Copyright (C) Ryota Ozaki <ozaki.ry...@gmail.com>
@@ -41,7 +41,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_wg.c,v 1.116 2024/07/29 02:33:44 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_wg.c,v 1.117 2024/07/29 02:33:58 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_altq_enabled.h"
@@ -518,8 +518,7 @@ struct wg_session {
#define WGS_STATE_ESTABLISHED 3
#define WGS_STATE_DESTROYING 4
- volatile uint32_t
- wgs_time_established;
+ uint32_t wgs_time_established;
volatile uint32_t
wgs_time_last_data_sent;
volatile bool wgs_force_rekey;
@@ -1693,14 +1692,12 @@ wg_handle_msg_init(struct wg_softc *wg,
wg_update_endpoint_if_necessary(wgp, src);
/*
- * Count the time of the INIT message as the time of
- * establishment -- this is used to decide when to erase keys,
- * and we want to start counting as soon as we have generated
- * keys.
- *
- * No need for atomic store because the session can't be used
- * in the rx or tx paths yet -- not until we transition to
- * INTI_PASSIVE.
+ * Even though we don't transition from INIT_PASSIVE to
+ * ESTABLISHED until we receive the first data packet from the
+ * initiator, we count the time of the INIT message as the time
+ * of establishment -- this is used to decide when to erase
+ * keys, and we want to start counting as soon as we have
+ * generated keys.
*/
wgs->wgs_time_established = time_uptime32;
wg_schedule_session_dtor_timer(wgp);
@@ -2530,8 +2527,7 @@ wg_need_to_send_init_message(struct wg_s
*/
return wgs->wgs_is_initiator &&
atomic_load_relaxed(&wgs->wgs_time_last_data_sent) == 0 &&
- ((time_uptime32 -
- atomic_load_relaxed(&wgs->wgs_time_established)) >=
+ (time_uptime32 - wgs->wgs_time_established >=
(wg_reject_after_time - wg_keepalive_timeout -
wg_rekey_timeout));
}
@@ -2814,7 +2810,7 @@ wg_handle_msg_data(struct wg_softc *wg,
/*
* Reject if the session is too old.
*/
- age = time_uptime32 - atomic_load_relaxed(&wgs->wgs_time_established);
+ age = time_uptime32 - wgs->wgs_time_established;
if (__predict_false(age >= wg_reject_after_time)) {
WG_DLOG("session %"PRIx32" too old, %"PRIu32" sec\n",
wgmd->wgmd_receiver, age);
@@ -3428,9 +3424,6 @@ wg_task_destroy_prev_session(struct wg_s
* was ESTABLISHED and is now DESTROYING, older than
* reject-after-time, destroy it. Upcoming sessions are still
* in INIT_ACTIVE or INIT_PASSIVE -- we don't touch those here.
- *
- * No atomic for access to wgs_time_established because it is
- * only updated under wgp_lock.
*/
wgs = wgp->wgp_session_unstable;
KASSERT(wgs->wgs_state != WGS_STATE_ESTABLISHED);
@@ -3663,8 +3656,6 @@ wg_socreate(struct wg_softc *wg, int af,
static bool
wg_session_hit_limits(struct wg_session *wgs)
{
- uint32_t time_established =
- atomic_load_relaxed(&wgs->wgs_time_established);
/*
* [W] 6.2: Transport Message Limits
@@ -3673,8 +3664,8 @@ wg_session_hit_limits(struct wg_session
* comes first, WireGuard will refuse to send or receive any more
* transport data messages using the current secure session, ..."
*/
- KASSERT(time_established != 0 || time_uptime > UINT32_MAX);
- if ((time_uptime32 - time_established) > wg_reject_after_time) {
+ KASSERT(wgs->wgs_time_established != 0 || time_uptime > UINT32_MAX);
+ if (time_uptime32 - wgs->wgs_time_established > wg_reject_after_time) {
WG_DLOG("The session hits REJECT_AFTER_TIME\n");
return true;
} else if (wg_session_get_send_counter(wgs) >
@@ -4430,9 +4421,7 @@ wg_send_data_msg(struct wg_peer *wgp, st
* Check rekey-after-time.
*/
if (wgs->wgs_is_initiator &&
- ((time_uptime32 -
- atomic_load_relaxed(&wgs->wgs_time_established)) >=
- wg_rekey_after_time)) {
+ now - wgs->wgs_time_established >= wg_rekey_after_time) {
/*
* [W] 6.2 Transport Message Limits
* "if a peer is the initiator of a current secure
