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 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.
>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?
Are we losing any other features apart from this strange arp reuse
you described in your mail?
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.
Is there anything else that can be split out easily?
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
