This change looks fine, but we should figure out why some of the pf dummynet tests broke with the previous step before we go further down this path: https://ci.freebsd.org/view/Test/job/FreeBSD-main-amd64-test/23651/#showFailuresLink

We do already activate this new hook for pf in the common tests, but not in the netpfil/pf tests. I'm rather worried that a lot more tests would start failing if we did that.

In D40373#919092, @kp wrote:

This change looks fine, but we should figure out why some of the pf dummynet tests broke with the previous step before we go further down this path: https://ci.freebsd.org/view/Test/job/FreeBSD-main-amd64-test/23651/#showFailuresLink

We do already activate this new hook for pf in the common tests, but not in the netpfil/pf tests. I'm rather worried that a lot more tests would start failing if we did that.

This is a very good point. I will debug the dummynet tests and take a closer look at the netpfil/pf tests

In D40373#919110, @dfr wrote:

In D40373#919092, @kp wrote:

This change looks fine, but we should figure out why some of the pf dummynet tests broke with the previous step before we go further down this path: https://ci.freebsd.org/view/Test/job/FreeBSD-main-amd64-test/23651/#showFailuresLink

We do already activate this new hook for pf in the common tests, but not in the netpfil/pf tests. I'm rather worried that a lot more tests would start failing if we did that.

This is a very good point. I will debug the dummynet tests and take a closer look at the netpfil/pf tests

I think the dummynet test failures are caused by the new filter head adding a second PFIL_OUT for the ping packet. The code allows for dummynet's re-injection but only once:

if (ip_dn_io_ptr != NULL && pd.pf_mtag != NULL &&
    pd.pf_mtag->flags & PF_TAG_DUMMYNET) {
        /* Dummynet re-injects packets after they've
         * completed their delay. We've already
         * processed them, so pass unconditionally. */

        /* But only once. We may see the packet multiple times (e.g.
         * PFIL_IN/PFIL_OUT). */
        pd.pf_mtag->flags &= ~PF_TAG_DUMMYNET;
        PF_RULES_RUNLOCK();

        return (PF_PASS);
}

It turned out that the dummynet test failures were just caused by the pf rules matching packets delivered to the local IP stack. Narrowing the rules to exclude that fixed them, details in D40393

Just getting back to this now - I'll spend some time running the netpfil/pf tests and see what breaks.

I did not test the ALTQ tests but all the other pf tests pass except sys/netpfil/pf/pfsync:defer which seems to be a known failing test.

@kp I plan to merge this to current this week - do you have any concerns or comments before I do that?

In D40373#924407, @dfr wrote:

@kp I plan to merge this to current this week - do you have any concerns or comments before I do that?

Really only that I don't think we should enable this by default. It'll break existing configurations and we cannot rely on users reading and fully understanding UPDATING.

Ok, I'm happy with that - I will change the default for net.pf.filter_local back to false and adjust the UPDATING text to reflect that. I'll leave the test changes in so that tests will pass on machines where this is enabled. I'll also squash the two commits - there is no need to separate if we are keeping filter_local disabled by default.