On 21/10/2016, at 2:03 AM, Alexander Bluhm wrote:
> On Thu, Oct 20, 2016 at 10:53:17AM +0200, Claudio Jeker wrote:
>>> Unfortunately pf_route() is called from pfsync which has no idea
>>> of packet descriptors. As I do not want to rewrite pfsync, I create
>>> a temporary pf_pdesc on the stack.
>>
>> I'm OK with the direction but am wondering if setting only that little
>> information in the pf_pdesc is save in the future when that pd is passed
>> further down.
>
> Henning hat the same concern. Now I fill the pf_pdesc completely
> with pf_setup_pdesc().
I'm sympathetic to the direction. However the patch raises to three the
number of definitions of union pf_headers (the others in pf.c and
if_pflog.c). At minimum the comment on pf_headers in pf.c:130 should be
updated to reflect this.
best,
Richard.
>
> ok?
>
> bluhm
>
> Index: net/if_pfsync.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_pfsync.c,v
> retrieving revision 1.235
> diff -u -p -u -p -r1.235 if_pfsync.c
> --- net/if_pfsync.c 4 Oct 2016 13:54:32 -0000 1.235
> +++ net/if_pfsync.c 20 Oct 2016 12:19:31 -0000
> @@ -61,9 +61,13 @@
> #include <net/netisr.h>
> #include <netinet/in.h>
> #include <netinet/if_ether.h>
> +#include <netinet/ip.h>
> +#include <netinet/ip_icmp.h>
> +#include <netinet/icmp6.h>
> #include <netinet/tcp.h>
> #include <netinet/tcp_seq.h>
> #include <netinet/tcp_fsm.h>
> +#include <netinet/udp.h>
>
> #include <netinet/in_var.h>
> #include <netinet/ip.h>
> @@ -1732,6 +1736,17 @@ void
> pfsync_undefer(struct pfsync_deferral *pd, int drop)
> {
> struct pfsync_softc *sc = pfsyncif;
> + struct pf_pdesc pdesc;
> + union pf_headers {
> + struct tcphdr tcp;
> + struct udphdr udp;
> + struct icmp icmp;
> +#ifdef INET6
> + struct icmp6_hdr icmp6;
> + struct mld_hdr mld;
> + struct nd_neighbor_solicit nd_ns;
> +#endif /* INET6 */
> + } pdhdrs;
>
> splsoftassert(IPL_SOFTNET);
>
> @@ -1743,17 +1758,22 @@ pfsync_undefer(struct pfsync_deferral *p
> m_freem(pd->pd_m);
> else {
> if (pd->pd_st->rule.ptr->rt == PF_ROUTETO) {
> + if (pf_setup_pdesc(&pdesc, &pdhdrs,
> + pd->pd_st->key[PF_SK_WIRE]->af,
> + pd->pd_st->direction, pd->pd_st->rt_kif,
> + pd->pd_m, NULL) != PF_PASS) {
> + m_freem(pd->pd_m);
> + goto out;
> + }
> switch (pd->pd_st->key[PF_SK_WIRE]->af) {
> case AF_INET:
> - pf_route(&pd->pd_m, pd->pd_st->rule.ptr,
> - pd->pd_st->direction,
> - pd->pd_st->rt_kif->pfik_ifp, pd->pd_st);
> + pf_route(&pd->pd_m, &pdesc,
> + pd->pd_st->rule.ptr, pd->pd_st);
> break;
> #ifdef INET6
> case AF_INET6:
> - pf_route6(&pd->pd_m, pd->pd_st->rule.ptr,
> - pd->pd_st->direction,
> - pd->pd_st->rt_kif->pfik_ifp, pd->pd_st);
> + pf_route6(&pd->pd_m, &pdesc,
> + pd->pd_st->rule.ptr, pd->pd_st);
> break;
> #endif /* INET6 */
> }
> @@ -1772,7 +1792,7 @@ pfsync_undefer(struct pfsync_deferral *p
> }
> }
> }
> -
> + out:
> pool_put(&sc->sc_pool, pd);
> }
>
> Index: net/pf.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
> retrieving revision 1.992
> diff -u -p -u -p -r1.992 pf.c
> --- net/pf.c 18 Oct 2016 13:28:01 -0000 1.992
> +++ net/pf.c 20 Oct 2016 11:29:54 -0000
> @@ -5764,7 +5764,7 @@ pf_rtlabel_match(struct pf_addr *addr, s
> }
>
> void
> -pf_route(struct mbuf **m, struct pf_rule *r, int dir, struct ifnet *oifp,
> +pf_route(struct mbuf **m, struct pf_pdesc *pd, struct pf_rule *r,
> struct pf_state *s)
> {
> struct mbuf *m0, *m1;
> @@ -5777,8 +5777,7 @@ pf_route(struct mbuf **m, struct pf_rule
> int error = 0;
> unsigned int rtableid;
>
> - if (m == NULL || *m == NULL || r == NULL ||
> - (dir != PF_IN && dir != PF_OUT) || oifp == NULL)
> + if (m == NULL || *m == NULL || r == NULL)
> panic("pf_route: invalid parameters");
>
> if ((*m)->m_pkthdr.pf.routed++ > 3) {
> @@ -5791,7 +5790,7 @@ pf_route(struct mbuf **m, struct pf_rule
> if ((m0 = m_dup_pkt(*m, max_linkhdr, M_NOWAIT)) == NULL)
> return;
> } else {
> - if ((r->rt == PF_REPLYTO) == (r->direction == dir))
> + if ((r->rt == PF_REPLYTO) == (r->direction == pd->dir))
> return;
> m0 = *m;
> }
> @@ -5856,7 +5855,7 @@ pf_route(struct mbuf **m, struct pf_rule
> goto bad;
>
>
> - if (oifp != ifp) {
> + if (pd->kif->pfik_ifp != ifp) {
> if (pf_test(AF_INET, PF_OUT, ifp, &m0) != PF_PASS)
> goto bad;
> else if (m0 == NULL)
> @@ -5931,7 +5930,7 @@ bad:
>
> #ifdef INET6
> void
> -pf_route6(struct mbuf **m, struct pf_rule *r, int dir, struct ifnet *oifp,
> +pf_route6(struct mbuf **m, struct pf_pdesc *pd, struct pf_rule *r,
> struct pf_state *s)
> {
> struct mbuf *m0;
> @@ -5944,8 +5943,7 @@ pf_route6(struct mbuf **m, struct pf_rul
> struct m_tag *mtag;
> unsigned int rtableid;
>
> - if (m == NULL || *m == NULL || r == NULL ||
> - (dir != PF_IN && dir != PF_OUT) || oifp == NULL)
> + if (m == NULL || *m == NULL || r == NULL)
> panic("pf_route6: invalid parameters");
>
> if ((*m)->m_pkthdr.pf.routed++ > 3) {
> @@ -5958,7 +5956,7 @@ pf_route6(struct mbuf **m, struct pf_rul
> if ((m0 = m_dup_pkt(*m, max_linkhdr, M_NOWAIT)) == NULL)
> return;
> } else {
> - if ((r->rt == PF_REPLYTO) == (r->direction == dir))
> + if ((r->rt == PF_REPLYTO) == (r->direction == pd->dir))
> return;
> m0 = *m;
> }
> @@ -6004,7 +6002,7 @@ pf_route6(struct mbuf **m, struct pf_rul
> if (ifp == NULL)
> goto bad;
>
> - if (oifp != ifp) {
> + if (pd->kif->pfik_ifp != ifp) {
> if (pf_test(AF_INET6, PF_OUT, ifp, &m0) != PF_PASS)
> goto bad;
> else if (m0 == NULL)
> @@ -6905,9 +6903,9 @@ done:
> break;
> }
> if (pd.naf == AF_INET)
> - pf_route(&pd.m, r, dir, kif->pfik_ifp, s);
> + pf_route(&pd.m, &pd, r, s);
> if (pd.naf == AF_INET6)
> - pf_route6(&pd.m, r, dir, kif->pfik_ifp, s);
> + pf_route6(&pd.m, &pd, r, s);
> *m0 = NULL;
> action = PF_PASS;
> break;
> @@ -6921,11 +6919,11 @@ done:
> if (r->rt) {
> switch (pd.af) {
> case AF_INET:
> - pf_route(m0, r, pd.dir, pd.kif->pfik_ifp, s);
> + pf_route(m0, &pd, r, s);
> break;
> #ifdef INET6
> case AF_INET6:
> - pf_route6(m0, r, pd.dir, pd.kif->pfik_ifp, s);
> + pf_route6(m0, &pd, r, s);
> break;
> #endif /* INET6 */
> }
> Index: net/pfvar.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/pfvar.h,v
> retrieving revision 1.441
> diff -u -p -u -p -r1.441 pfvar.h
> --- net/pfvar.h 18 Oct 2016 13:28:01 -0000 1.441
> +++ net/pfvar.h 20 Oct 2016 11:29:54 -0000
> @@ -1765,10 +1765,10 @@ 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_rule *, int,
> - struct ifnet *, struct pf_state *);
> -void pf_route6(struct mbuf **, struct pf_rule *, int,
> - struct ifnet *, struct pf_state *);
> +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 pfr_initialize(void);
> int pfr_match_addr(struct pfr_ktable *, struct pf_addr *, sa_family_t);
>
