On Tue, Jul 06, 2021 at 04:02:58PM +0200, Alexandr Nedvedicky wrote:
> diff below moves call to pfsync_undefer() to pf_test(). The
> pfsync_undefer() will get called _after_ we drop PF_LOCK().
> diff below still needs OKs to get committed.
Could you please replace the void pointer with struct pfsync_deferral ?
This avoids a cast, see below.
otherwise OK bluhm@
diff --git net/pf.c net/pf.c
index 0672f5f6..3f9dd999 100644
--- net/pf.c
+++ net/pf.c
@@ -195,7 +195,7 @@ void pf_rule_to_actions(struct
pf_rule *,
int pf_test_rule(struct pf_pdesc *, struct pf_rule **,
struct pf_state **, struct pf_rule **,
struct pf_ruleset **, u_short *,
- void **);
+ 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 **,
@@ -3810,7 +3810,7 @@ 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,
- void **pdeferral)
+ struct pfsync_deferral **pdeferral)
{
struct pf_rule *r = NULL;
struct pf_rule *a = NULL;
@@ -4043,8 +4043,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,
- (struct pfsync_deferral **)pdeferral))
+ if (pfsync_defer(*sm, pd->m, pdeferral))
return (PF_DEFER);
}
#endif /* NPFSYNC > 0 */
@@ -6893,7 +6892,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;
- void *deferral = NULL;
+ struct pfsync_deferral *deferral = NULL;
if (!pf_status.running)
return (PF_PASS);
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sys/net/if_pfsync.c b/sys/net/if_pfsync.c
> index 744a7ca9dbc..11b4e3f1f2f 100644
> --- a/sys/net/if_pfsync.c
> +++ b/sys/net/if_pfsync.c
> @@ -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..037219537e8 100644
> --- a/sys/net/if_pfsync.h
> +++ b/sys/net/if_pfsync.h
> @@ -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..0672f5f6992 100644
> --- a/sys/net/pf.c
> +++ b/sys/net/pf.c
> @@ -194,7 +194,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 *,
> + void **);
> 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 +3809,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,
> + void **pdeferral)
> {
> struct pf_rule *r = NULL;
> struct pf_rule *a = NULL;
> @@ -4041,7 +4043,8 @@ 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,
> + (struct pfsync_deferral **)pdeferral))
> return (PF_DEFER);
> }
> #endif /* NPFSYNC > 0 */
> @@ -6890,6 +6893,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;
> + void *deferral = NULL;
>
> if (!pf_status.running)
> return (PF_PASS);
> @@ -6992,7 +6996,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 +7029,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 +7061,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 +7137,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 +7273,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;
