git: b08ee10c0646 - main - wg: fix a number of issues with module load failure handling

From: Kyle Evans <kevans_at_FreeBSD.org>
Date: Fri, 23 Jun 2023 17:01:14 UTC
The branch main has been updated by kevans:

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

commit b08ee10c0646e683cd03c9e28f537d9a7ba306af
Author:     Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2023-06-21 18:56:58 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2023-06-23 17:00:09 +0000

    wg: fix a number of issues with module load failure handling
    
    If MOD_LOAD fails, then MOD_UNLOAD will be called to unwind module
    state, but wg_module_init() will have already deinitialized everything
    it needs to in a manner that renders it unsafe to call MOD_UNLOAD
    after (e.g., freed zone not reset to NULL, wg_osd_jail_slot not reset
    to 0).  Let's simply stop trying to handle freeing everything in
    wg_module_init() to simplify it; let the subsequent MOD_UNLOAD deal with
    it, and let's make that robust against partially-constructed state.
    
    jhb@ notes that MOD_UNLOAD being called if MOD_LOAD fails is kind of an
    anomaly that doesn't match other paradigms in the kernel; e.g., if
    device_attach() fails, we don't invoke device_detach().  It's likely
    that a future commit will revert this and instead stop calling
    MOD_UNLOAD if MOD_LOAD fails, expecting modules to clean up after
    themselves in MOD_LOAD upon failure.  Some other modules already do this
    and may see similar problems to the wg module (see: carp).  The proper
    fix is decidedly a bit too invasive to do this close to 14 branching,
    and it requires auditing all kmods (base + ports) for potential leaks.
    
    PR:             272089
    Reviewed by:    emaste
    MFC after:      3 days
    Differential Revision:  https://reviews.freebsd.org/D40708
---
 sys/dev/wg/if_wg.c     | 28 +++++++++-------------------
 sys/dev/wg/wg_cookie.c |  9 ++++++++-
 2 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/sys/dev/wg/if_wg.c b/sys/dev/wg/if_wg.c
index 77aed2621c9a..4a21afe82eb5 100644
--- a/sys/dev/wg/if_wg.c
+++ b/sys/dev/wg/if_wg.c
@@ -2984,39 +2984,27 @@ static inline bool wg_run_selftests(void) { return true; }
 static int
 wg_module_init(void)
 {
-	int ret = ENOMEM;
-
+	int ret;
 	osd_method_t methods[PR_MAXMETHOD] = {
 		[PR_METHOD_REMOVE] = wg_prison_remove,
 	};
 
 	if ((wg_packet_zone = uma_zcreate("wg packet", sizeof(struct wg_packet),
 	     NULL, NULL, NULL, NULL, 0, 0)) == NULL)
-		goto free_none;
+		return (ENOMEM);
 	ret = crypto_init();
 	if (ret != 0)
-		goto free_zone;
+		return (ret);
 	ret = cookie_init();
 	if (ret != 0)
-		goto free_crypto;
+		return (ret);
 
 	wg_osd_jail_slot = osd_jail_register(NULL, methods);
 
-	ret = ENOTRECOVERABLE;
 	if (!wg_run_selftests())
-		goto free_all;
+		return (ENOTRECOVERABLE);
 
 	return (0);
-
-free_all:
-	osd_jail_deregister(wg_osd_jail_slot);
-	cookie_deinit();
-free_crypto:
-	crypto_deinit();
-free_zone:
-	uma_zdestroy(wg_packet_zone);
-free_none:
-	return (ret);
 }
 
 static void
@@ -3034,10 +3022,12 @@ wg_module_deinit(void)
 	VNET_LIST_RUNLOCK();
 	NET_EPOCH_WAIT();
 	MPASS(LIST_EMPTY(&wg_list));
-	osd_jail_deregister(wg_osd_jail_slot);
+	if (wg_osd_jail_slot != 0)
+		osd_jail_deregister(wg_osd_jail_slot);
 	cookie_deinit();
 	crypto_deinit();
-	uma_zdestroy(wg_packet_zone);
+	if (wg_packet_zone != NULL)
+		uma_zdestroy(wg_packet_zone);
 }
 
 static int
diff --git a/sys/dev/wg/wg_cookie.c b/sys/dev/wg/wg_cookie.c
index 16fa5d7fb52d..6ff9325c6613 100644
--- a/sys/dev/wg/wg_cookie.c
+++ b/sys/dev/wg/wg_cookie.c
@@ -55,6 +55,7 @@ struct ratelimit {
 	struct callout			rl_gc;
 	LIST_HEAD(, ratelimit_entry)	rl_table[RATELIMIT_SIZE];
 	size_t				rl_table_num;
+	bool				rl_initialized;
 };
 
 static void	precompute_key(uint8_t *,
@@ -102,7 +103,8 @@ cookie_deinit(void)
 #ifdef INET6
 	ratelimit_deinit(&ratelimit_v6);
 #endif
-	uma_zdestroy(ratelimit_zone);
+	if (ratelimit_zone != NULL)
+		uma_zdestroy(ratelimit_zone);
 }
 
 void
@@ -350,16 +352,21 @@ ratelimit_init(struct ratelimit *rl)
 	for (i = 0; i < RATELIMIT_SIZE; i++)
 		LIST_INIT(&rl->rl_table[i]);
 	rl->rl_table_num = 0;
+	rl->rl_initialized = true;
 }
 
 static void
 ratelimit_deinit(struct ratelimit *rl)
 {
+	if (!rl->rl_initialized)
+		return;
 	mtx_lock(&rl->rl_mtx);
 	callout_stop(&rl->rl_gc);
 	ratelimit_gc(rl, true);
 	mtx_unlock(&rl->rl_mtx);
 	mtx_destroy(&rl->rl_mtx);
+
+	rl->rl_initialized = false;
 }
 
 static void