diff options
author | glebius <glebius@FreeBSD.org> | 2011-10-23 14:59:54 +0000 |
---|---|---|
committer | glebius <glebius@FreeBSD.org> | 2011-10-23 14:59:54 +0000 |
commit | aaf1597b1d9ef18f687ca892c430b613a9b69c3a (patch) | |
tree | d2f0052ca46bb39945f809fd26b143b54b64a6cd /sys/contrib | |
parent | 87ac018dc516587d5a8c018db162b3a0be5ae797 (diff) | |
download | FreeBSD-src-aaf1597b1d9ef18f687ca892c430b613a9b69c3a.zip FreeBSD-src-aaf1597b1d9ef18f687ca892c430b613a9b69c3a.tar.gz |
Fix from r226623 is not sufficient to close all races in pfsync(4).
The root of problem is re-locking at the end of pfsync_sendout().
Several functions are calling pfsync_sendout() holding pointers
to pf data on stack, and these functions expect this data to be
consistent.
To fix this, the following approach was taken:
- The pfsync_sendout() doesn't call ip_output() directly, but
enqueues the mbuf on sc->sc_ifp's interfaces queue, that
is currently unused. Then pfsync netisr is scheduled. PF_LOCK
isn't dropped in pfsync_sendout().
- The netisr runs through queue and ip_output()s packets
on it.
Apart from fixing race, this also decouples stack, fixing
potential issues, that may happen, when sending pfsync(4)
packets on input path.
Reviewed by: eri (a quick review)
Diffstat (limited to 'sys/contrib')
-rw-r--r-- | sys/contrib/pf/net/if_pfsync.c | 99 |
1 files changed, 59 insertions, 40 deletions
diff --git a/sys/contrib/pf/net/if_pfsync.c b/sys/contrib/pf/net/if_pfsync.c index a1f5630..2a7c434 100644 --- a/sys/contrib/pf/net/if_pfsync.c +++ b/sys/contrib/pf/net/if_pfsync.c @@ -856,7 +856,11 @@ pfsync_state_import(struct pfsync_state *sp, u_int8_t flags) CLR(st->state_flags, PFSTATE_NOSYNC); if (ISSET(st->state_flags, PFSTATE_ACK)) { pfsync_q_ins(st, PFSYNC_S_IACK); +#ifdef __FreeBSD__ + pfsync_sendout(); +#else schednetisr(NETISR_PFSYNC); +#endif } } CLR(st->state_flags, PFSTATE_ACK); @@ -1312,7 +1316,11 @@ pfsync_in_upd(struct pfsync_pkt *pkt, struct mbuf *m, int offset, int count) V_pfsyncstats.pfsyncs_stale++; pfsync_update_state(st); +#ifdef __FreeBSD__ + pfsync_sendout(); +#else schednetisr(NETISR_PFSYNC); +#endif continue; } pfsync_alloc_scrub_memory(&sp->dst, &st->dst); @@ -1418,7 +1426,11 @@ pfsync_in_upd_c(struct pfsync_pkt *pkt, struct mbuf *m, int offset, int count) V_pfsyncstats.pfsyncs_stale++; pfsync_update_state(st); +#ifdef __FreeBSD__ + pfsync_sendout(); +#else schednetisr(NETISR_PFSYNC); +#endif continue; } pfsync_alloc_scrub_memory(&up->dst, &st->dst); @@ -2146,6 +2158,7 @@ pfsync_sendout(void) #endif #ifdef __FreeBSD__ size_t pktlen; + int dummy_error; #endif int offset; int q, count = 0; @@ -2349,32 +2362,21 @@ pfsync_sendout(void) #ifdef __FreeBSD__ sc->sc_ifp->if_opackets++; sc->sc_ifp->if_obytes += m->m_pkthdr.len; + sc->sc_len = PFSYNC_MINPKT; + + IFQ_ENQUEUE(&sc->sc_ifp->if_snd, m, dummy_error); + schednetisr(NETISR_PFSYNC); #else sc->sc_if.if_opackets++; sc->sc_if.if_obytes += m->m_pkthdr.len; -#endif - sc->sc_len = PFSYNC_MINPKT; -#ifdef __FreeBSD__ - PF_UNLOCK(); -#endif if (ip_output(m, NULL, NULL, IP_RAWOUTPUT, &sc->sc_imo, NULL) == 0) -#ifdef __FreeBSD__ - { - PF_LOCK(); -#endif - V_pfsyncstats.pfsyncs_opackets++; -#ifdef __FreeBSD__ - } -#endif + pfsyncstats.pfsyncs_opackets++; else -#ifdef __FreeBSD__ - { - PF_LOCK(); -#endif - V_pfsyncstats.pfsyncs_oerrors++; -#ifdef __FreeBSD__ - } + pfsyncstats.pfsyncs_oerrors++; + + /* start again */ + sc->sc_len = PFSYNC_MINPKT; #endif } @@ -2422,7 +2424,11 @@ pfsync_insert_state(struct pf_state *st) pfsync_q_ins(st, PFSYNC_S_INS); if (ISSET(st->state_flags, PFSTATE_ACK)) +#ifdef __FreeBSD__ + pfsync_sendout(); +#else schednetisr(NETISR_PFSYNC); +#endif else st->sync_updates = 0; } @@ -2619,7 +2625,11 @@ pfsync_update_state(struct pf_state *st) if (sync || (time_second - st->pfsync_time) < 2) { pfsync_upds++; +#ifdef __FreeBSD__ + pfsync_sendout(); +#else schednetisr(NETISR_PFSYNC); +#endif } } @@ -2670,7 +2680,11 @@ pfsync_request_update(u_int32_t creatorid, u_int64_t id) TAILQ_INSERT_TAIL(&sc->sc_upd_req_list, item, ur_entry); sc->sc_len += nlen; +#ifdef __FreeBSD__ + pfsync_sendout(); +#else schednetisr(NETISR_PFSYNC); +#endif } void @@ -2699,7 +2713,11 @@ pfsync_update_state_req(struct pf_state *st) pfsync_q_del(st); case PFSYNC_S_NONE: pfsync_q_ins(st, PFSYNC_S_UPD); +#ifdef __FreeBSD__ + pfsync_sendout(); +#else schednetisr(NETISR_PFSYNC); +#endif return; case PFSYNC_S_INS: @@ -3253,37 +3271,38 @@ pfsync_timeout(void *arg) void #ifdef __FreeBSD__ pfsyncintr(void *arg) +{ + struct pfsync_softc *sc = arg; + struct mbuf *m; + + CURVNET_SET(sc->sc_ifp->if_vnet); + pfsync_ints++; + + for (;;) { + IF_DEQUEUE(&sc->sc_ifp->if_snd, m); + if (m == 0) + break; + + if (ip_output(m, NULL, NULL, IP_RAWOUTPUT, &sc->sc_imo, NULL) + == 0) + V_pfsyncstats.pfsyncs_opackets++; + else + V_pfsyncstats.pfsyncs_oerrors++; + } + CURVNET_RESTORE(); +} #else pfsyncintr(void) -#endif { -#ifdef __FreeBSD__ - struct pfsync_softc *sc = arg; -#endif int s; -#ifdef __FreeBSD__ - if (sc == NULL) - return; - - CURVNET_SET(sc->sc_ifp->if_vnet); -#endif pfsync_ints++; s = splnet(); -#ifdef __FreeBSD__ - PF_LOCK(); -#endif pfsync_sendout(); -#ifdef __FreeBSD__ - PF_UNLOCK(); -#endif splx(s); - -#ifdef __FreeBSD__ - CURVNET_RESTORE(); -#endif } +#endif int pfsync_sysctl(int *name, u_int namelen, void *oldp, size_t *oldlenp, void *newp, |