git: b08ee10c0646 - main - wg: fix a number of issues with module load failure handling
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
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