On Fri, Oct 30, 2015 at 12:45:31PM +0100, Mike Belopuhov wrote:
> 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.
>
I agree, I left them out for now as they need more thoughts and don't
do much harm. The candidates are:
- ph_cookie (seems to be used by pipex)
- flowid (only for a hack in trunk?!)
- ether_vtag
Everything else is either set by m_resethdr or by if_input().
btw., it looks somewhat ugly that some members in the pkthdr are
prefixed with ph_ while others are not.
Reyk
> 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);
> > }
> >
--
