On Fri, Oct 30, 2015 at 11:30:56AM +0100, Alexander Bluhm wrote:
> On Fri, Oct 30, 2015 at 10:43:21AM +0100, Reyk Floeter wrote:
> > Question:
> > > How does pair(4) interact with pf? If a packet crosses a pair
> > > does it create a new state or does pf track the original state?
> > >
> >
> > Answer:
> > It does create a new state, you can filter between pair(4) without
> > problems and all features including nat work.
> > But it currently does not clear some of the extended packet settings -
> > like flags, tags, qid etc. - so you can filter on the tag, eg.
> >
> > # Assuming pair1 is patched to pair0
> > pass out on pair1 tag FOO
> > pass in on pair0 tagged FOO
> >
> > The attached diff _removes_ that and resets all pf settings, so the pf
> > rules above will not work anymore. I think it is better to assume
> > crossing barriers and receiving packets with pair(4) works like a
> > "normal" Ethernet packet. The following will still work:
> >
> > # Received packets on pair0 don't carry any existing pf states
> > pass out on pair1
> > pass in on pair0
> >
> > OK?
>
> The new semantics is better.
>
> > +void
> > +pf_pkt_reset(struct mbuf *m)
> > +{
> > + if (m->m_flags & M_PKTHDR)
> > + m_tag_delete_chain(m);
> > +
> > + /* resets state key, pcb reference, qid, tag, and all flags */
> > + memset(&m->m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> > +}
>
> You need a packet header mbuf to access m->m_pkthdr. So either
> assume that M_PKTHDR is set and don't check. Or put both actions
> into the if block.
>
> As pf_pkt_addr_changed() accesses the m->m_pkthdr without check, I
> would recomend to remove the "if (m->m_flags & M_PKTHDR)" here,
> too. You may also put an assert into both functions.
>
> The default for m->m_pkthdr.pf.prio is IFQ_DEFPRIO, not 0.
> Look at m_inithdr().
>
Thanks.
Adding asserts in in both functions makes sense, but I'll try it
separately - no caller of pf_pkt_addr_changed() checks for the pkthdr
at the moment and I fear potential fallout. Maybe it would also make
sense to rename both functions to pf_pkthdr_* to make it clear, but
this can also be done separately.
Here is the updated diff, OK?
Reyk
Index: sys/net/if_pair.c
===================================================================
RCS file: /cvs/src/sys/net/if_pair.c,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 if_pair.c
--- sys/net/if_pair.c 25 Oct 2015 12:59:57 -0000 1.4
+++ sys/net/if_pair.c 30 Oct 2015 10:57:10 -0000
@@ -30,6 +30,11 @@
#include <netinet/in.h>
#include <netinet/if_ether.h>
+#include "pf.h"
+#if NPF > 0
+#include <net/pfvar.h>
+#endif
+
#include "bpfilter.h"
#if NBPFILTER > 0
#include <net/bpf.h>
@@ -182,9 +187,13 @@ pairstart(struct ifnet *ifp)
#endif /* NBPFILTER > 0 */
ifp->if_opackets++;
- if (pairedifp != NULL)
+ if (pairedifp != NULL) {
+#if NPF > 0
+ if (m->m_flags & M_PKTHDR)
+ pf_pkt_reset(m);
+#endif
ml_enqueue(&ml, m);
- else
+ } else
m_freem(m);
}
Index: sys/net/pf.c
===================================================================
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.948
diff -u -p -u -p -r1.948 pf.c
--- sys/net/pf.c 27 Oct 2015 10:52:17 -0000 1.948
+++ sys/net/pf.c 30 Oct 2015 10:57:11 -0000
@@ -6701,6 +6701,19 @@ pf_pkt_addr_changed(struct mbuf *m)
m->m_pkthdr.pf.inp = NULL;
}
+/*
+ * should be called when reinjecting the packet with a clean state
+ */
+void
+pf_pkt_reset(struct mbuf *m)
+{
+ m_tag_delete_chain(m);
+
+ /* resets pf state key, pcb, qid, tag, and flags like m_inithdr() */
+ memset(&m->m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
+ m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
+}
+
#if NPFLOG > 0
void
pf_log_matches(struct pf_pdesc *pd, struct pf_rule *rm, struct pf_rule *am,
Index: sys/net/pfvar.h
===================================================================
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.421
diff -u -p -u -p -r1.421 pfvar.h
--- sys/net/pfvar.h 13 Oct 2015 19:32:32 -0000 1.421
+++ sys/net/pfvar.h 30 Oct 2015 10:57:11 -0000
@@ -1756,6 +1756,7 @@ int pf_rtlabel_match(struct pf_addr *, s
int pf_socket_lookup(struct pf_pdesc *);
struct pf_state_key *pf_alloc_state_key(int);
void pf_pkt_addr_changed(struct mbuf *);
+void pf_pkt_reset(struct mbuf *);
int pf_state_key_attach(struct pf_state_key *, struct pf_state *, int);
int pf_translate(struct pf_pdesc *, struct pf_addr *, u_int16_t,
struct pf_addr *, u_int16_t, u_int16_t, int);
