pfsync_undefer() must be called outside of PF_LOCK

OK @bluhm

diff --git a/sys/net/if_pfsync.c b/sys/net/if_pfsync.c
index 744a7ca..7d0d6b5 100644 (file)
--- 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 2520c87..bee6c77 100644 (file)
--- 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 672f415..4d49721 100644 (file)
--- 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 <net/if_pfsync.h>
+#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;