On Tue, Feb 02, 2021 at 02:52:52PM +1000, David Gwynne wrote:
> this is part of a high level discussion about when pf runs against a
> packet. the options are:
>
> 1. pf runs when a packet goes over an interface
> or
> 2. pf runs when a packet enters or leaves the network stack.
>
> for normal packet handling there isn't a difference between these
> options. in the routing case a packet comes in on an interface, pf tests
> it, then the stack processes it and decides to send it out another
> interface, pf tests it again on the way out, the packet goes on the
> wire. for packets handled by the local system, a packet comes in on an
> interface, pf tests it, the stack processes it locally, something
> generates a reply, the stack decides to route that out an interface, pf
> tests it on the way out, the reply packet ends up on the wire.
>
> in both situations, you get the same sequence of events if you think
> that pf runs when a packet goes over an interface or wether pf runs when
> a packet enters or leaves the stack.
>
> however, there is a difference if route-to gets involved. if route-to is
> applied on an outbound rule/state, it could change which interface the
> packet should be going over.
>
> currently the code implements option 1. this means that if route-to
> changes the interface, it reruns pf test for the packet going over the
> new interface. i would like to change it to option 2.
>
> the main reason i want to change it is that option 1 creates confusion
> for the state table. by default, pf states are floating, meaning that
> packets are matched to states regardless of which interface they're
> going over. if a packet leaving on em0 is rerouted out em1, both
> traversals will end up using the same state, which at best will make the
> accounting look weird, or at worst fail some checks in the state and get
> dropped.
>
> another reason i want to change this is to make it consistent with
> other changes that are made to packet. eg, when nat is applied to
> a packet, we don't run pf_test again with the new addresses.
>
> the downside to this change is that the pf_test rerun may have been used
> to do things like push a packet out another interface with the first run
> through pf, and pick up a broad "nat all packets leaving this interface"
> rule on the second one.
>
> however, like most things relating to route-to/reply-to/dup-to, im
> pretty sure at this point it's not used a lot, so the impact is minimal.
> a lot of changes in this space have already been made, so adding another
> simplification is justifiable. if this does remove functionality that
> people need, i believe sashan@ has agreed to help me implement route-to
> on match rules to give more flexibility and composability of rules.
>
> i've canvassed a few people, and their responses have varied from "i
> don't care, route-to is the worst" to "i thought we did option 2
> anyway". anyone else want to chime in?
>
> this keeps the behaviour where route-to on a packet coming into the
> stack is pushed past it and immediately forwarded to the output
> interface. the condition for that is greatly simplified now though.
>
> ok?
For me pf(4) should behave like 2. That was the initial design (pf_test in
ip_input and in ip_output) and with this a forwarded packet is tested when
received and when sent. route-to, reply-to, dup-to where added later and
the change of behaviour using these special shortcuts was never fully
considered. (I'm not even sure if you could use route-to on out rules in
old versions of pf). The state matching was built on the behaviour in 2
and retesting the same packet again results in a lot of state confusion.
The result is erratic behaviour and this is why I think that you're right
and route-to, reply-to should avoid rerunning pf_test() for the same
direction.
So in my opinion this diff is OK.
> Index: pf.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf.c,v
> retrieving revision 1.1106
> diff -u -p -r1.1106 pf.c
> --- pf.c 1 Feb 2021 00:31:05 -0000 1.1106
> +++ pf.c 2 Feb 2021 03:44:51 -0000
> @@ -6033,7 +6033,7 @@ pf_route(struct pf_pdesc *pd, struct pf_
> (ifp->if_flags & IFF_LOOPBACK) == 0)
> ip->ip_src = ifatoia(rt->rt_ifa)->ia_addr.sin_addr;
>
> - if (s->rt != PF_DUPTO && pd->kif->pfik_ifp != ifp) {
> + if (s->rt != PF_DUPTO && pd->dir == PF_IN) {
> if (pf_test(AF_INET, PF_OUT, ifp, &m0) != PF_PASS)
> goto bad;
> else if (m0 == NULL)
> @@ -6178,7 +6178,7 @@ pf_route6(struct pf_pdesc *pd, struct pf
> (ifp->if_flags & IFF_LOOPBACK) == 0)
> ip6->ip6_src = ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>
> - if (s->rt != PF_DUPTO && pd->kif->pfik_ifp != ifp) {
> + if (s->rt != PF_DUPTO && pd->dir == PF_IN) {
> if (pf_test(AF_INET6, PF_OUT, ifp, &m0) != PF_PASS)
> goto bad;
> else if (m0 == NULL)
>
>
--
:wq Claudio
