Hi,

Instead of passing an extra mbuf pointer to pf_route(), it should
just use pd->m.  Then pf_test() can also operate on pd.m and set
the *m0 value in the caller just before it returns.  Note that the
goto done after ip6_output() fixes a mbuf leak.

ok?

bluhm

Index: net/if_pfsync.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_pfsync.c,v
retrieving revision 1.236
diff -u -p -r1.236 if_pfsync.c
--- net/if_pfsync.c     27 Oct 2016 21:41:20 -0000      1.236
+++ net/if_pfsync.c     28 Oct 2016 11:53:20 -0000
@@ -1754,16 +1754,17 @@ pfsync_undefer(struct pfsync_deferral *p
                        }
                        switch (pd->pd_st->key[PF_SK_WIRE]->af) {
                        case AF_INET:
-                               pf_route(&pd->pd_m, &pdesc,
+                               pf_route(&pdesc,
                                    pd->pd_st->rule.ptr, pd->pd_st);
                                break;
 #ifdef INET6
                        case AF_INET6:
-                               pf_route6(&pd->pd_m, &pdesc,
+                               pf_route6(&pdesc,
                                    pd->pd_st->rule.ptr, pd->pd_st);
                                break;
 #endif /* INET6 */
                        }
+                       pd->pd_m = pdesc.m;
                } else {
                        switch (pd->pd_st->key[PF_SK_WIRE]->af) {
                        case AF_INET:
Index: net/pf.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.996
diff -u -p -r1.996 pf.c
--- net/pf.c    28 Oct 2016 07:54:19 -0000      1.996
+++ net/pf.c    28 Oct 2016 11:57:27 -0000
@@ -2196,6 +2196,7 @@ pf_translate_a(struct pf_pdesc *pd, stru
 }
 
 #if INET6
+/* pf_translate_af() may change pd->m, adjust local copies after calling */
 int
 pf_translate_af(struct pf_pdesc *pd)
 {
@@ -5775,9 +5776,9 @@ pf_rtlabel_match(struct pf_addr *addr, s
        return (ret);
 }
 
+/* pf_route() may change pd->m, adjust local copies after calling */
 void
-pf_route(struct mbuf **m, struct pf_pdesc *pd, struct pf_rule *r,
-    struct pf_state *s)
+pf_route(struct pf_pdesc *pd, struct pf_rule *r, struct pf_state *s)
 {
        struct mbuf             *m0, *m1;
        struct sockaddr_in      *dst, sin;
@@ -5789,22 +5790,19 @@ pf_route(struct mbuf **m, struct pf_pdes
        int                      error = 0;
        unsigned int             rtableid;
 
-       if (m == NULL || *m == NULL || r == NULL)
-               panic("pf_route: invalid parameters");
-
-       if ((*m)->m_pkthdr.pf.routed++ > 3) {
-               m0 = *m;
-               *m = NULL;
-               goto bad;
+       if (pd->m->m_pkthdr.pf.routed++ > 3) {
+               m_freem(pd->m);
+               pd->m = NULL;
+               return;
        }
 
        if (r->rt == PF_DUPTO) {
-               if ((m0 = m_dup_pkt(*m, max_linkhdr, M_NOWAIT)) == NULL)
+               if ((m0 = m_dup_pkt(pd->m, max_linkhdr, M_NOWAIT)) == NULL)
                        return;
        } else {
                if ((r->rt == PF_REPLYTO) == (r->direction == pd->dir))
                        return;
-               m0 = *m;
+               m0 = pd->m;
        }
 
        if (m0->m_len < sizeof(struct ip)) {
@@ -5929,7 +5927,7 @@ pf_route(struct mbuf **m, struct pf_pdes
 
 done:
        if (r->rt != PF_DUPTO)
-               *m = NULL;
+               pd->m = NULL;
        if (!r->rt)
                if_put(ifp);
        rtfree(rt);
@@ -5941,9 +5939,9 @@ bad:
 }
 
 #ifdef INET6
+/* pf_route6() may change pd->m, adjust local copies after calling */
 void
-pf_route6(struct mbuf **m, struct pf_pdesc *pd, struct pf_rule *r,
-    struct pf_state *s)
+pf_route6(struct pf_pdesc *pd, struct pf_rule *r, struct pf_state *s)
 {
        struct mbuf             *m0;
        struct sockaddr_in6     *dst, sin6;
@@ -5955,22 +5953,19 @@ pf_route6(struct mbuf **m, struct pf_pde
        struct m_tag            *mtag;
        unsigned int             rtableid;
 
-       if (m == NULL || *m == NULL || r == NULL)
-               panic("pf_route6: invalid parameters");
-
-       if ((*m)->m_pkthdr.pf.routed++ > 3) {
-               m0 = *m;
-               *m = NULL;
-               goto bad;
+       if (pd->m->m_pkthdr.pf.routed++ > 3) {
+               m_freem(pd->m);
+               pd->m = NULL;
+               return;
        }
 
        if (r->rt == PF_DUPTO) {
-               if ((m0 = m_dup_pkt(*m, max_linkhdr, M_NOWAIT)) == NULL)
+               if ((m0 = m_dup_pkt(pd->m, max_linkhdr, M_NOWAIT)) == NULL)
                        return;
        } else {
                if ((r->rt == PF_REPLYTO) == (r->direction == pd->dir))
                        return;
-               m0 = *m;
+               m0 = pd->m;
        }
 
        if (m0->m_len < sizeof(struct ip6_hdr)) {
@@ -5990,7 +5985,7 @@ pf_route6(struct mbuf **m, struct pf_pde
        if (!r->rt) {
                m0->m_pkthdr.pf.flags |= PF_TAG_GENERATED;
                ip6_output(m0, NULL, NULL, 0, NULL, NULL);
-               return;
+               goto done;
        }
 
        if (s == NULL) {
@@ -6051,7 +6046,7 @@ pf_route6(struct mbuf **m, struct pf_pde
 
 done:
        if (r->rt != PF_DUPTO)
-               *m = NULL;
+               pd->m = NULL;
        return;
 
 bad:
@@ -6659,7 +6654,7 @@ pf_test(sa_family_t af, int fwdir, struc
 
        /* if packet has been reassembled, update packet description */
        if (pf_status.reass && pd.virtual_proto == PF_VPROTO_FRAGMENT) {
-               action = pf_setup_pdesc(&pd, &pdhdrs, af, dir, kif, *m0,
+               action = pf_setup_pdesc(&pd, &pdhdrs, af, dir, kif, pd.m,
                    &reason);
                if (action != PF_PASS) {
 #if NPFLOG > 0
@@ -6885,22 +6880,23 @@ done:
 
        switch (action) {
        case PF_SYNPROXY_DROP:
-               m_freem(*m0);
+               m_freem(pd.m);
+               /* FALLTHROUGH */
        case PF_DEFER:
-               *m0 = NULL;
+               pd.m = NULL;
                action = PF_PASS;
                break;
        case PF_DIVERT:
                switch (pd.af) {
                case AF_INET:
                        if (!divert_packet(pd.m, pd.dir, r->divert_packet.port))
-                               *m0 = NULL;
+                               pd.m = NULL;
                        break;
 #ifdef INET6
                case AF_INET6:
                        if (!divert6_packet(pd.m, pd.dir,
                            r->divert_packet.port))
-                               *m0 = NULL;
+                               pd.m = NULL;
                        break;
 #endif /* INET6 */
                }
@@ -6909,33 +6905,29 @@ done:
 #ifdef INET6
        case PF_AFRT:
                if (pf_translate_af(&pd)) {
-                       if (!pd.m)
-                               *m0 = NULL;
                        action = PF_DROP;
                        break;
                }
                if (pd.naf == AF_INET)
-                       pf_route(&pd.m, &pd, r, s);
+                       pf_route(&pd, r, s);
                if (pd.naf == AF_INET6)
-                       pf_route6(&pd.m, &pd, r, s);
-               *m0 = NULL;
+                       pf_route6(&pd, r, s);
                action = PF_PASS;
                break;
 #endif /* INET6 */
        case PF_DROP:
-               m_freem(*m0);
-               *m0 = NULL;
+               m_freem(pd.m);
+               pd.m = NULL;
                break;
        default:
-               /* pf_route can free the mbuf causing *m0 to become NULL */
                if (r->rt) {
                        switch (pd.af) {
                        case AF_INET:
-                               pf_route(m0, &pd, r, s);
+                               pf_route(&pd, r, s);
                                break;
 #ifdef INET6
                        case AF_INET6:
-                               pf_route6(m0, &pd, r, s);
+                               pf_route6(&pd, r, s);
                                break;
 #endif /* INET6 */
                        }
@@ -6945,11 +6937,11 @@ done:
 
 #ifdef INET6
        /* if reassembled packet passed, create new fragments */
-       if (pf_status.reass && action == PF_PASS && *m0 && fwdir == PF_FWD) {
+       if (pf_status.reass && action == PF_PASS && pd.m && fwdir == PF_FWD) {
                struct m_tag    *mtag;
 
-               if ((mtag = m_tag_find(*m0, PACKET_TAG_PF_REASSEMBLED, NULL)))
-                       action = pf_refragment6(m0, mtag, NULL, NULL);
+               if ((mtag = m_tag_find(pd.m, PACKET_TAG_PF_REASSEMBLED, NULL)))
+                       action = pf_refragment6(&pd.m, mtag, NULL, NULL);
        }
 #endif /* INET6 */
        if (s && action != PF_DROP) {
@@ -6959,6 +6951,7 @@ done:
                        s->if_index_out = ifp->if_index;
        }
 
+       *m0 = pd.m;
        return (action);
 }
 
Index: net/pfvar.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pfvar.h,v
retrieving revision 1.443
diff -u -p -r1.443 pfvar.h
--- net/pfvar.h 27 Oct 2016 21:41:20 -0000      1.443
+++ net/pfvar.h 28 Oct 2016 11:48:51 -0000
@@ -1710,10 +1710,8 @@ int      pf_state_key_attach(struct pf_state_
 int    pf_translate(struct pf_pdesc *, struct pf_addr *, u_int16_t,
            struct pf_addr *, u_int16_t, u_int16_t, int);
 int    pf_translate_af(struct pf_pdesc *);
-void   pf_route(struct mbuf **, struct pf_pdesc *, struct pf_rule *,
-           struct pf_state *);
-void   pf_route6(struct mbuf **, struct pf_pdesc *, struct pf_rule *,
-           struct pf_state *);
+void   pf_route(struct pf_pdesc *, struct pf_rule *, struct pf_state *);
+void   pf_route6(struct pf_pdesc *, struct pf_rule *, struct pf_state *);
 
 void   pfr_initialize(void);
 int    pfr_match_addr(struct pfr_ktable *, struct pf_addr *, sa_family_t);

Reply via email to