From nobody Fri Jun 23 17:01:14 2023 X-Original-To: dev-commits-src-all@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 4Qnk6222Nmz4h2QR; Fri, 23 Jun 2023 17:01:14 +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 4Qnk621dMfz4JWJ; Fri, 23 Jun 2023 17:01:14 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1687539674; 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=X0hOtFWZtAwP30OW5xitV2QSbH5Ho7NfZZG+McfYIAo=; b=lO10m6K0e8xxs7oB0s+vKFF94uEMX5/6Y8WJbDzyNF6JmkEmI8YjSlHfNSirWDUrhbygsL MViz7f8YoS9rW7o+VL9dAbUr2HZyAwrnLw6u+1U4z/sKJJrvxRWAaCa12ZuAtQMeeJ2NNj fVTMjfwZS0wqMZZRnMFN7AUd6Tnj5GCcLt0GPvq0hNglP93UwmH6WY8cjxsY+U0RZUFGWM 1h3nzixSidY4tIgNcJjRw9ZAGJgxN5WPhp9m7ivntnDx55tFnQvwxuEYhW39oBy9MPy0I7 wlmlUAdlFE0gYWOl5YQHqsVqnxjw+zyv4GEz0u1OU1P9lVjcpdZKoJUiy3e6uA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1687539674; 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=X0hOtFWZtAwP30OW5xitV2QSbH5Ho7NfZZG+McfYIAo=; b=PKGu0+Hzmx5UmDeN8HRYwd71osxiMZismrU6gMmWeJMCLKJliaMA4L0rRcr17FRFly6X0d JvZJt25OdTge3q/CZEcEG+SHrcuZf2NmSH8B58ElxUuE2RzNFsPdFB6vsByeE5lPEBsAVZ Wq7GObF3ymPB1Ihzcmy32W5+9V417jgDxjdW1IBxWeTpGBDI4QN+PrKG8I1NiqjcC+8YSX nzggIYTwACN7hNJGE9V6OXjc/KsGV18fZRDDSsuudWgtTSXvZMGlmfEjdt8BIFdARvuRD5 Oyhi01Ln2l+uX0rAi7nUH0TYtfTeS00K8m+MTVpcrl9td6Z8EQx4tKcOut896A== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1687539674; a=rsa-sha256; cv=none; b=G2ivm/WjJfxZqxEePA5PwShFkaKFQOYLH720VtGNw9YVPPGJwUh7twzZB3xwW6BdE9i7IC HnowBIhRYdPfhWm9WJxSwVJEZdsR73oWASuh4h0DNmc9n+a6eWJ5NtYJhkSYvRJhBVbwPr og7tcpar6M1TL9Fc2OVcNN0MtuvmEY9TaHuKJfaETueBdUhjycB4ScQR21LxiWn1DsQ+Ea QSdcYE/Ok2+Nfb8HF7N3RMuXRMP6X6XKH6mQhmrK/krcDsxhJPdVHAIEMrwMe1nagY+Noh iCXiOM247dzOVyg9V57eE3HW6LOGtV+PaEk7R5XKeb300X7Px2PCgT78fVdhPw== 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 4Qnk620jrrz1BpS; Fri, 23 Jun 2023 17:01:14 +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 35NH1EBw029164; Fri, 23 Jun 2023 17:01:14 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 35NH1Edb029163; Fri, 23 Jun 2023 17:01:14 GMT (envelope-from git) Date: Fri, 23 Jun 2023 17:01:14 GMT Message-Id: <202306231701.35NH1Edb029163@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Kyle Evans Subject: git: b08ee10c0646 - main - wg: fix a number of issues with module load failure handling List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: kevans X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: b08ee10c0646e683cd03c9e28f537d9a7ba306af Auto-Submitted: auto-generated X-ThisMailContainsUnwantedMimeParts: N The branch main has been updated by kevans: URL: https://cgit.FreeBSD.org/src/commit/?id=b08ee10c0646e683cd03c9e28f537d9a7ba306af commit b08ee10c0646e683cd03c9e28f537d9a7ba306af Author: Kyle Evans AuthorDate: 2023-06-21 18:56:58 +0000 Commit: Kyle Evans 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