git: 358c5f5c0899 - main - pf: fix cleanup deadlock
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Mon, 16 Dec 2024 22:34:12 UTC
The branch main has been updated by kp: URL: https://cgit.FreeBSD.org/src/commit/?id=358c5f5c0899339a005ef2c68ef166601bb9dca9 commit 358c5f5c0899339a005ef2c68ef166601bb9dca9 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-12-10 14:02:47 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-12-16 22:33:55 +0000 pf: fix cleanup deadlock We can get to pfi_kkif_remove_if_unref() via at least two distinct paths: - when the struct ifnet is removed, via pfi_detach_ifnet_event() - when a rule referencing us is removed, via pfi_kkif_unref(). These two events can race against each other, leading us to free this kif twice. That leads to loop in V_pfi_unlinked_kifs, and an eventual deadlock. Avoid this by making sure we only ever insert the kif into V_pfi_unlinked_kifs once. If we don't find it in V_pfi_ifs it's already been removed. Check that it exists in V_pfi_unlinked_kifs (for INVARIANTS). Sponsored by: Rubicon Communications, LLC ("Netgate") Differential Revision: https://reviews.freebsd.org/D48082 --- sys/netpfil/pf/pf_if.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/sys/netpfil/pf/pf_if.c b/sys/netpfil/pf/pf_if.c index 650a7e4db799..d2b1b6a781f4 100644 --- a/sys/netpfil/pf/pf_if.c +++ b/sys/netpfil/pf/pf_if.c @@ -274,6 +274,13 @@ pf_kkif_free(struct pfi_kkif *kif) if (! kif) return; +#ifdef INVARIANTS + if (kif->pfik_ifp) { + struct ifnet *ifp = kif->pfik_ifp; + MPASS(ifp->if_pf_kif == NULL || ifp->if_pf_kif == kif); + } +#endif + #ifdef PF_WANT_32_TO_64_COUNTER wowned = PF_RULES_WOWNED(); if (!wowned) @@ -378,6 +385,35 @@ pfi_kkif_remove_if_unref(struct pfi_kkif *kif) kif == V_pfi_all || kif->pfik_flags != 0) return; + /* + * We can get here in at least two distinct paths: + * - when the struct ifnet is removed, via pfi_detach_ifnet_event() + * - when a rule referencing us is removed, via pfi_kkif_unref(). + * These two events can race against each other, leading us to free this kif + * twice. That leads to a loop in V_pfi_unlinked_kifs, and an eventual + * deadlock. + * + * Avoid this by making sure we only ever insert the kif into + * V_pfi_unlinked_kifs once. + * If we don't find it in V_pfi_ifs it's already been removed. Check that it + * exists in V_pfi_unlinked_kifs. + */ + if (! RB_FIND(pfi_ifhead, &V_pfi_ifs, kif)) { +#ifdef INVARIANTS + struct pfi_kkif *tmp; + bool found = false; + mtx_lock(&pfi_unlnkdkifs_mtx); + LIST_FOREACH(tmp, &V_pfi_unlinked_kifs, pfik_list) { + if (tmp == kif) { + found = true; + break; + } + } + mtx_unlock(&pfi_unlnkdkifs_mtx); + MPASS(found); +#endif + return; + } RB_REMOVE(pfi_ifhead, &V_pfi_ifs, kif); kif->pfik_flags |= PFI_IFLAG_REFS;