git: 829a69db855b - main - pf: change pf_route so pf only runs when packets enter and leave the stack.

[Kristof Provost]

The branch main has been updated by kp:

URL: https://cgit.FreeBSD.org/src/commit/?id=829a69db855b48ff7e8242b95e193a0783c489d9

commit 829a69db855b48ff7e8242b95e193a0783c489d9
Author: Kristof Provost <k...@FreeBSD.org>
AuthorDate: 2021-04-02 10:23:42 +0000
Commit: Kristof Provost <k...@FreeBSD.org>
CommitDate: 2021-04-05 07:57:06 +0000

pf: change pf_route so pf only runs when packets enter and leave the stack.

before this change pf_route operated on the semantic that pf runs
when packets go over an interface, so when pf_route changed which
interface the packet was on it would run pf_test again. this change
changes (restores) the semantic that pf is only supposed to run
when packets go in or out of the network stack, even if route-to
is responsibly for short circuiting past the network stack.

just to be clear, for normal packets (ie, those not touched by
route-to/reply-to/dup-to), there isn't a difference between running
pf when packets enter or leave the stack, or having pf run when a
packet goes over an interface.

the main reason for this change is that running the same packet
through pf multiple times 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 for this commit is is to make handling of the changes
that route-to makes 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 main caveat with this diff is you can't have one rule that
pushes a packet out a different interface, and then have a rule on
that second interface that NATs the packet. i'm not convinced this
ever worked reliably or was used much anyway, so we don't think
it's a big concern.

discussed with many, with special thanks to bluhm@, sashan@ and
sthen@ for weathering most of that pain.
ok claudio@ sashan@ jmatthew@

Obtained from: OpenBSD
MFC after: 2 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D29554
---
sys/netpfil/pf/pf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index 50bf4b3871c5..5b41be4ad683 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -5549,7 +5549,7 @@ pf_route(struct mbuf **m, struct pf_krule *r, int dir, struct ifnet *oifp,
if (ifp == NULL)
goto bad;

- if (oifp != ifp) {
+ if (dir == PF_IN) {
if (pf_test(PF_OUT, 0, ifp, &m0, inp) != PF_PASS)
goto bad;
else if (m0 == NULL)
@@ -5738,7 +5738,7 @@ pf_route6(struct mbuf **m, struct pf_krule *r, int dir, struct ifnet *oifp,
if (ifp == NULL)
goto bad;

- if (oifp != ifp) {
+ if (dir == PF_IN) {
if (pf_test6(PF_OUT, PFIL_FWD, ifp, &m0, inp) != PF_PASS)
goto bad;
else if (m0 == NULL)
_______________________________________________
dev-commi...@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/dev-commits-src-all
To unsubscribe, send any mail to "dev-commits-src...@freebsd.org"

[Kubilay Kocak]

Relnotes: Yes

For the rule semantics change?

> ---
> sys/netpfil/pf/pf.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
> index 50bf4b3871c5..5b41be4ad683 100644
> --- a/sys/netpfil/pf/pf.c
> +++ b/sys/netpfil/pf/pf.c
> @@ -5549,7 +5549,7 @@ pf_route(struct mbuf **m, struct pf_krule *r, int dir, struct ifnet *oifp,
> if (ifp == NULL)
> goto bad;
>
> - if (oifp != ifp) {
> + if (dir == PF_IN) {
> if (pf_test(PF_OUT, 0, ifp, &m0, inp) != PF_PASS)
> goto bad;
> else if (m0 == NULL)
> @@ -5738,7 +5738,7 @@ pf_route6(struct mbuf **m, struct pf_krule *r, int dir, struct ifnet *oifp,
> if (ifp == NULL)
> goto bad;
>
> - if (oifp != ifp) {
> + if (dir == PF_IN) {
> if (pf_test6(PF_OUT, PFIL_FWD, ifp, &m0, inp) != PF_PASS)
> goto bad;
> else if (m0 == NULL)
> _______________________________________________

> dev-commit...@freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/dev-commits-src-main
> To unsubscribe, send any mail to "dev-commits-src-...@freebsd.org"

[Kristof Provost]

On 6 Apr 2021, at 2:01, Kubilay Kocak wrote:
> On 5/04/2021 9:44 pm, Kristof Provost wrote:
>> The branch main has been updated by kp:
>>
>> URL:
>> https://cgit.FreeBSD.org/src/commit/?id=829a69db855b48ff7e8242b95e193a0783c489d9
>>
>> commit 829a69db855b48ff7e8242b95e193a0783c489d9
>> Author: Kristof Provost <k...@FreeBSD.org>
>> AuthorDate: 2021-04-02 10:23:42 +0000
>> Commit: Kristof Provost <k...@FreeBSD.org>
>> CommitDate: 2021-04-05 07:57:06 +0000
>>
>> pf: change pf_route so pf only runs when packets enter and leave
>> the stack.
>

> Relnotes: Yes
>
> For the rule semantics change?
>

I wouldn’t. This is an extremely subtle change, and in all likelihood
won’t impact anyone other than those suffering from the bug this
fixes. It’s really more of a bug fix than behaviour change.
Listing it in the release notes is going to generate more confusion than
enlightenment.

Best regards,
Kristof

[Kubilay Kocak]

On 6/04/2021 5:09 pm, Kristof Provost wrote:
> On 6 Apr 2021, at 2:01, Kubilay Kocak wrote:
>> On 5/04/2021 9:44 pm, Kristof Provost wrote:
>>> The branch main has been updated by kp:
>>>
>>> URL:
>>> https://cgit.FreeBSD.org/src/commit/?id=829a69db855b48ff7e8242b95e193a0783c489d9
>>>
>>>
>>> commit 829a69db855b48ff7e8242b95e193a0783c489d9
>>> Author:     Kristof Provost <k...@FreeBSD.org>
>>> AuthorDate: 2021-04-02 10:23:42 +0000
>>> Commit:     Kristof Provost <k...@FreeBSD.org>
>>> CommitDate: 2021-04-05 07:57:06 +0000
>>>
>>>      pf: change pf_route so pf only runs when packets enter and leave
>>> the stack.
>>
>> Relnotes: Yes
>>
>> For the rule semantics change?
>>
> I wouldn’t. This is an extremely subtle change, and in all likelihood
> won’t impact anyone other than those suffering from the bug this fixes.
> It’s really more of a bug fix than behaviour change.
> Listing it in the release notes is going to generate more confusion than
> enlightenment.

Wasn't sure of impact, thought it best to ask. Thanks Kristof!