Author: glebius
Date: Sun Oct 23 14:59:54 2011
New Revision: 226660
URL: http://svn.freebsd.org/changeset/base/226660
Log:
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)
Modified:
head/sys/contrib/pf/net/if_pfsync.c
Modified: head/sys/contrib/pf/net/if_pfsync.c
==============================================================================
--- head/sys/contrib/pf/net/if_pfsync.c Sun Oct 23 13:33:10 2011
(r226659)
+++ head/sys/contrib/pf/net/if_pfsync.c Sun Oct 23 14:59:54 2011
(r226660)
@@ -856,7 +856,11 @@ pfsync_state_import(struct pfsync_state
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, st
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,
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 creatori
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
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,
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
