From 52ade9e76be23b8910d4e6b91c3840ea68ea5382 Mon Sep 17 00:00:00 2001 From: sashan Date: Wed, 7 Jul 2021 18:38:25 +0000 Subject: [PATCH] pfsync_undefer() must be called outside of PF_LOCK OK @bluhm --- sys/net/if_pfsync.c | 33 +++++++++++++++++++++------------ sys/net/if_pfsync.h | 8 ++++++-- sys/net/pf.c | 30 ++++++++++++++++++++++-------- 3 files changed, 49 insertions(+), 22 deletions(-) diff --git a/sys/net/if_pfsync.c b/sys/net/if_pfsync.c index 744a7ca9dbc..7d0d6b59ff9 100644 --- a/sys/net/if_pfsync.c +++ b/sys/net/if_pfsync.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if_pfsync.c,v 1.296 2021/06/25 23:48:30 dlg Exp $ */ +/* $OpenBSD: if_pfsync.c,v 1.297 2021/07/07 18:38:25 sashan Exp $ */ /* * Copyright (c) 2002 Michael Shalayeff @@ -1931,7 +1931,7 @@ pfsync_insert_state(struct pf_state *st) } int -pfsync_defer(struct pf_state *st, struct mbuf *m) +pfsync_defer(struct pf_state *st, struct mbuf *m, struct pfsync_deferral **ppd) { struct pfsync_softc *sc = pfsyncif; struct pfsync_deferral *pd; @@ -1944,21 +1944,30 @@ pfsync_defer(struct pf_state *st, struct mbuf *m) m->m_flags & (M_BCAST|M_MCAST)) return (0); + pd = pool_get(&sc->sc_pool, M_NOWAIT); + if (pd == NULL) + return (0); + + /* + * deferral queue grows faster, than timeout can consume, + * we have to ask packet (caller) to help timer and dispatch + * one deferral for us. + * + * We wish to call pfsync_undefer() here. Unfortunately we can't, + * because pfsync_undefer() will be calling to ip_output(), + * which in turn will call to pf_test(), which would then attempt + * to grab PF_LOCK() we currently hold. + */ if (sc->sc_deferred >= 128) { mtx_enter(&sc->sc_deferrals_mtx); - pd = TAILQ_FIRST(&sc->sc_deferrals); - if (pd != NULL) { - TAILQ_REMOVE(&sc->sc_deferrals, pd, pd_entry); + *ppd = TAILQ_FIRST(&sc->sc_deferrals); + if (*ppd != NULL) { + TAILQ_REMOVE(&sc->sc_deferrals, *ppd, pd_entry); sc->sc_deferred--; } mtx_leave(&sc->sc_deferrals_mtx); - if (pd != NULL) - pfsync_undefer(pd, 0); - } - - pd = pool_get(&sc->sc_pool, M_NOWAIT); - if (pd == NULL) - return (0); + } else + *ppd = NULL; m->m_pkthdr.pf.flags |= PF_TAG_GENERATED; SET(st->state_flags, PFSTATE_ACK); diff --git a/sys/net/if_pfsync.h b/sys/net/if_pfsync.h index 2520c87a36c..bee6c77f228 100644 --- a/sys/net/if_pfsync.h +++ b/sys/net/if_pfsync.h @@ -1,4 +1,4 @@ -/* $OpenBSD: if_pfsync.h,v 1.56 2021/03/10 10:21:48 jsg Exp $ */ +/* $OpenBSD: if_pfsync.h,v 1.57 2021/07/07 18:38:25 sashan Exp $ */ /* * Copyright (c) 2001 Michael Shalayeff @@ -297,6 +297,8 @@ enum pfsync_counters { extern struct cpumem *pfsynccounters; +struct pfsync_deferral; + static inline void pfsyncstat_inc(enum pfsync_counters c) { @@ -335,7 +337,9 @@ void pfsync_clear_states(u_int32_t, const char *); void pfsync_update_tdb(struct tdb *, int); void pfsync_delete_tdb(struct tdb *); -int pfsync_defer(struct pf_state *, struct mbuf *); +int pfsync_defer(struct pf_state *, struct mbuf *, + struct pfsync_deferral **); +void pfsync_undefer(struct pfsync_deferral *, int); int pfsync_up(void); int pfsync_state_in_use(struct pf_state *); diff --git a/sys/net/pf.c b/sys/net/pf.c index 672f4159719..4d497212304 100644 --- a/sys/net/pf.c +++ b/sys/net/pf.c @@ -1,4 +1,4 @@ -/* $OpenBSD: pf.c,v 1.1121 2021/06/23 06:53:52 dlg Exp $ */ +/* $OpenBSD: pf.c,v 1.1122 2021/07/07 18:38:25 sashan Exp $ */ /* * Copyright (c) 2001 Daniel Hartmeier @@ -99,6 +99,8 @@ #if NPFSYNC > 0 #include +#else +struct pfsync_deferral; #endif /* NPFSYNC > 0 */ #ifdef DDB @@ -194,7 +196,8 @@ void pf_rule_to_actions(struct pf_rule *, struct pf_rule_actions *); int pf_test_rule(struct pf_pdesc *, struct pf_rule **, struct pf_state **, struct pf_rule **, - struct pf_ruleset **, u_short *); + struct pf_ruleset **, u_short *, + struct pfsync_deferral **); static __inline int pf_create_state(struct pf_pdesc *, struct pf_rule *, struct pf_rule *, struct pf_rule *, struct pf_state_key **, struct pf_state_key **, @@ -3808,7 +3811,8 @@ pf_match_rule(struct pf_test_ctx *ctx, struct pf_ruleset *ruleset) int pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, struct pf_state **sm, - struct pf_rule **am, struct pf_ruleset **rsm, u_short *reason) + struct pf_rule **am, struct pf_ruleset **rsm, u_short *reason, + struct pfsync_deferral **pdeferral) { struct pf_rule *r = NULL; struct pf_rule *a = NULL; @@ -4041,7 +4045,7 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, struct pf_state **sm, * firewall has to know about it to allow * replies through it. */ - if (pfsync_defer(*sm, pd->m)) + if (pfsync_defer(*sm, pd->m, pdeferral)) return (PF_DEFER); } #endif /* NPFSYNC > 0 */ @@ -6890,6 +6894,7 @@ pf_test(sa_family_t af, int fwdir, struct ifnet *ifp, struct mbuf **m0) int dir = (fwdir == PF_FWD) ? PF_OUT : fwdir; u_int32_t qid, pqid = 0; int have_pf_lock = 0; + struct pfsync_deferral *deferral = NULL; if (!pf_status.running) return (PF_PASS); @@ -6992,7 +6997,8 @@ pf_test(sa_family_t af, int fwdir, struct ifnet *ifp, struct mbuf **m0) */ PF_LOCK(); have_pf_lock = 1; - action = pf_test_rule(&pd, &r, &s, &a, &ruleset, &reason); + action = pf_test_rule(&pd, &r, &s, &a, &ruleset, &reason, + &deferral); s = pf_state_ref(s); if (action != PF_PASS) REASON_SET(&reason, PFRES_FRAG); @@ -7024,7 +7030,7 @@ pf_test(sa_family_t af, int fwdir, struct ifnet *ifp, struct mbuf **m0) PF_LOCK(); have_pf_lock = 1; action = pf_test_rule(&pd, &r, &s, &a, &ruleset, - &reason); + &reason, &deferral); s = pf_state_ref(s); } break; @@ -7056,7 +7062,7 @@ pf_test(sa_family_t af, int fwdir, struct ifnet *ifp, struct mbuf **m0) PF_LOCK(); have_pf_lock = 1; action = pf_test_rule(&pd, &r, &s, &a, &ruleset, - &reason); + &reason, &deferral); s = pf_state_ref(s); } break; @@ -7132,7 +7138,7 @@ pf_test(sa_family_t af, int fwdir, struct ifnet *ifp, struct mbuf **m0) PF_LOCK(); have_pf_lock = 1; action = pf_test_rule(&pd, &r, &s, &a, &ruleset, - &reason); + &reason, &deferral); s = pf_state_ref(s); } @@ -7268,6 +7274,14 @@ done: m_freem(pd.m); /* FALLTHROUGH */ case PF_DEFER: +#if NPFSYNC > 0 + /* + * We no longer hold PF_LOCK() here, so we can dispatch + * deferral if we are asked to do so. + */ + if (deferral != NULL) + pfsync_undefer(deferral, 0); +#endif /* NPFSYNC > 0 */ pd.m = NULL; action = PF_PASS; break; -- 2.20.1