On Fri, Oct 30, 2015 at 12:56 +0100, Reyk Floeter wrote: > On Fri, Oct 30, 2015 at 12:29:27PM +0100, Mike Belopuhov wrote: > > On Fri, Oct 30, 2015 at 12:29 +0100, Mike Belopuhov wrote: > > > On Fri, Oct 30, 2015 at 12:19 +0100, Reyk Floeter wrote: > > > > 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 > > > > > > > > > > As I've told Reyk privately, since this function removes all > > > mbufs including IPsec ones, etc. it should not be part of pf, > > > but part of mbuf source code. > > > > mbuf *tags* > > I think pf does handle packet mangling in various ways, which would > include the intended removal of other tags, but I don't care. > > Here is the updated diff. > > OK? >
This looks much better. We could potentially reset some other fields as well, but this doesn't have to happen right now. OK mikeb > Index: sys/kern/uipc_mbuf.c > =================================================================== > RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v > retrieving revision 1.208 > diff -u -p -r1.208 uipc_mbuf.c > --- sys/kern/uipc_mbuf.c 22 Oct 2015 05:26:06 -0000 1.208 > +++ sys/kern/uipc_mbuf.c 30 Oct 2015 11:30:32 -0000 > @@ -250,6 +250,18 @@ m_inithdr(struct mbuf *m) > return (m); > } > > +void > +m_resethdr(struct mbuf *m) > +{ > + /* like the previous, but keep any associated data and mbufs */ > + m->m_flags = M_PKTHDR; > + memset(&m->m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf)); > + m->m_pkthdr.pf.prio = IFQ_DEFPRIO; > + > + /* also delete all mbuf tags to reset the state */ > + m_tag_delete_chain(m); > +} > + > struct mbuf * > m_getclr(int nowait, int type) > { > Index: sys/sys/mbuf.h > =================================================================== > RCS file: /cvs/src/sys/sys/mbuf.h,v > retrieving revision 1.198 > diff -u -p -r1.198 mbuf.h > --- sys/sys/mbuf.h 22 Oct 2015 05:26:06 -0000 1.198 > +++ sys/sys/mbuf.h 30 Oct 2015 11:30:33 -0000 > @@ -410,6 +410,7 @@ struct mbuf *m_get(int, int); > struct mbuf *m_getclr(int, int); > struct mbuf *m_gethdr(int, int); > struct mbuf *m_inithdr(struct mbuf *); > +void m_resethdr(struct mbuf *); > int m_defrag(struct mbuf *, int); > struct mbuf *m_prepend(struct mbuf *, int, int); > struct mbuf *m_pulldown(struct mbuf *, int, int, int *); > Index: sys/net/if_pair.c > =================================================================== > RCS file: /cvs/src/sys/net/if_pair.c,v > retrieving revision 1.4 > diff -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 11:30:33 -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) > + m_resethdr(m); > +#endif > ml_enqueue(&ml, m); > - else > + } else > m_freem(m); > } >