On Sun, Jan 03, 2021 at 06:56:20PM +0100, Alexander Bluhm wrote: > On Sun, Jan 03, 2021 at 02:00:00PM +1000, David Gwynne wrote: > > On Tue, Oct 20, 2020 at 09:27:09AM +1000, David Gwynne wrote: > > We've been running this diff in production for the last couple of > > months, and it's been solid for us so far. Ignoring the fixes for > > crashes, I personally find it a lot more usable than the current > > route-to rules too. > > > > Can I commit it? > > The diff is quite large and does multiple things at a time.
In hindsight I agree. It was hard to see while being so close to it. > In general I also did not understand why I have to say em0@10.0.0.1 > for routing and it took me a while to figure out what to put into > pf.conf. I use this syntax in /usr/src/regress/sys/net/pf_forward/pf.conf. > This has to be fixed after this goes in. I will care about regress > as this test is quite complex an needs several machines to setup. > I am currently running a full regress to find more fallout. > > I do not use pfsync, so I cannot say what the consequences of the > change are in this area. Also I don't know why pf-route interfaces > were designed in such a strange way. we do use pfsync, and not being able to use it with route-to has been a point of friction for us for years. as for the design, i think it was copied (imperfectly) from ipfilter. look for "Policy Based Routing" in https://www.freebsd.org/cgi/man.cgi?query=ipf&sektion=5. > From a user perspective it is not clear, why route-to should not > work together with no-state. So we should either fix it or document > it and add a check in the parser. Is fixing hard? pf_route only takes a state now, so i'd say it is non-trivial. for now i'll go with documentation and a check in the parser.. > Are we losing any other features apart from this strange arp reuse > you described in your mail? i wouldn't say the arp reuse is a feature. > There is some refactoring in your diff. I splitted it to make > review easier. I think this should go in first. Note that the > pf_state variable is called st in if_pfsync.c. Can we be consistent > here? Is the pfsync_state properly aligned? During import it comes > from an mbuf. the stack should provide it on a 4 byte boundary, but it has uint64_t members. however, it is also __packed, so the compiler makes no assumptions about alignment. > Is there anything else that can be split out easily? let's put this in and then i'll have a look. ok by me. > > bluhm > > Index: net/if_pfsync.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_pfsync.c,v > retrieving revision 1.279 > diff -u -p -r1.279 if_pfsync.c > --- net/if_pfsync.c 12 Dec 2020 11:49:02 -0000 1.279 > +++ net/if_pfsync.c 3 Jan 2021 17:16:55 -0000 > @@ -612,7 +612,7 @@ pfsync_state_import(struct pfsync_state > st->rtableid[PF_SK_STACK] = ntohl(sp->rtableid[PF_SK_STACK]); > > /* copy to state */ > - bcopy(&sp->rt_addr, &st->rt_addr, sizeof(st->rt_addr)); > + st->rt_addr = sp->rt_addr; > st->creation = getuptime() - ntohl(sp->creation); > st->expire = getuptime(); > if (ntohl(sp->expire)) { > @@ -1843,6 +1843,7 @@ pfsync_undefer(struct pfsync_deferral *p > { > struct pfsync_softc *sc = pfsyncif; > struct pf_pdesc pdesc; > + struct pf_state *st = pd->pd_st; > > NET_ASSERT_LOCKED(); > > @@ -1852,35 +1853,32 @@ pfsync_undefer(struct pfsync_deferral *p > TAILQ_REMOVE(&sc->sc_deferrals, pd, pd_entry); > sc->sc_deferred--; > > - CLR(pd->pd_st->state_flags, PFSTATE_ACK); > + CLR(st->state_flags, PFSTATE_ACK); > if (drop) > m_freem(pd->pd_m); > else { > - if (pd->pd_st->rule.ptr->rt == PF_ROUTETO) { > - if (pf_setup_pdesc(&pdesc, > - pd->pd_st->key[PF_SK_WIRE]->af, > - pd->pd_st->direction, pd->pd_st->rt_kif, > - pd->pd_m, NULL) != PF_PASS) { > + if (st->rule.ptr->rt == PF_ROUTETO) { > + if (pf_setup_pdesc(&pdesc, st->key[PF_SK_WIRE]->af, > + st->direction, st->kif, pd->pd_m, NULL) != > + PF_PASS) { > m_freem(pd->pd_m); > goto out; > } > - switch (pd->pd_st->key[PF_SK_WIRE]->af) { > + switch (st->key[PF_SK_WIRE]->af) { > case AF_INET: > - pf_route(&pdesc, > - pd->pd_st->rule.ptr, pd->pd_st); > + pf_route(&pdesc, st->rule.ptr, st); > break; > #ifdef INET6 > case AF_INET6: > - pf_route6(&pdesc, > - pd->pd_st->rule.ptr, pd->pd_st); > + pf_route6(&pdesc, st->rule.ptr, st); > break; > #endif /* INET6 */ > default: > - unhandled_af(pd->pd_st->key[PF_SK_WIRE]->af); > + unhandled_af(st->key[PF_SK_WIRE]->af); > } > pd->pd_m = pdesc.m; > } else { > - switch (pd->pd_st->key[PF_SK_WIRE]->af) { > + switch (st->key[PF_SK_WIRE]->af) { > case AF_INET: > ip_output(pd->pd_m, NULL, NULL, 0, NULL, NULL, > 0); > @@ -1892,12 +1890,12 @@ pfsync_undefer(struct pfsync_deferral *p > break; > #endif /* INET6 */ > default: > - unhandled_af(pd->pd_st->key[PF_SK_WIRE]->af); > + unhandled_af(st->key[PF_SK_WIRE]->af); > } > } > } > out: > - pf_state_unref(pd->pd_st); > + pf_state_unref(st); > 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.1096 > diff -u -p -r1.1096 pf.c > --- net/pf.c 10 Dec 2020 06:40:22 -0000 1.1096 > +++ net/pf.c 3 Jan 2021 17:10:28 -0000 > @@ -1186,7 +1186,7 @@ pf_state_export(struct pfsync_state *sp, > > /* copy from state */ > strlcpy(sp->ifname, st->kif->pfik_name, sizeof(sp->ifname)); > - memcpy(&sp->rt_addr, &st->rt_addr, sizeof(sp->rt_addr)); > + sp->rt_addr = st->rt_addr; > sp->creation = htonl(getuptime() - st->creation); > expire = pf_state_expires(st); > if (expire <= getuptime()) > @@ -3437,21 +3437,8 @@ pf_set_rt_ifp(struct pf_state *s, struct > if (!r->rt) > return (0); > > - switch (af) { > - case AF_INET: > - rv = pf_map_addr(AF_INET, r, saddr, &s->rt_addr, NULL, sns, > - &r->route, PF_SN_ROUTE); > - break; > -#ifdef INET6 > - case AF_INET6: > - rv = pf_map_addr(AF_INET6, r, saddr, &s->rt_addr, NULL, sns, > - &r->route, PF_SN_ROUTE); > - break; > -#endif /* INET6 */ > - default: > - rv = 1; > - } > - > + rv = pf_map_addr(af, r, saddr, &s->rt_addr, NULL, sns, > + &r->route, PF_SN_ROUTE); > if (rv == 0) { > s->rt_kif = r->route.kif; > s->natrule.ptr = r; > @@ -6270,7 +6257,6 @@ bad: > goto done; > } > #endif /* INET6 */ > - > > /* > * check TCP checksum and set mbuf flag
