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*