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.
