Does this need an MFC tag too?

LGTM. Approved regardless of how you choose to deal with the remarks I've raised.

Approved.

Review remarks

Approved (without the 'draft' in the commit header line, of course).

review remarks

I need to have a closer look again tomorrow, but this looks like a good idea.
It expects to be applied on top of D47770, right?

rename PF_RT_RPOOL/PF_RT_NAT

Still approved :)

In D47106#1090312, @rscheff wrote:

@kp can you please review the PF-related changes in this Diff?

Approved.

Okay, mostly cosmetic issues. I'd like to give Tom a chance to comment on the udp endpoint thing too, but we can fix the open remarks while we wait for that.

In D47777#1089452, @franco_opnsense.org wrote:

I changed the summary.

Please do try to *explain* the problem. "Fixes a regression" does not do this.

In D47658#1086731, @franco_opnsense.org wrote:

In D47658#1086724, @kp wrote:

The entire backtrace would have been nice.

Just let me know what you need? Just "bt" or more?

Let's start with the full backtrace, yes.

In D47777#1089450, @franco_opnsense.org wrote:

D47658 is on hold as I struggle to understand why the code discussed there is not covered by existing tests and how to actually trigger it (it should easily panic after all but the code is never hit).

Shall we finish D47658 before we move on to other issues?

Approved.

Change the approach. Tell pf_refragment6() what interface to use. If unspecified
fall back to the previous ip6_forward/ip6_output calls.
This is basically the same approach OpenBSD took for this issue, and it's a
smaller change than splitting pf_refragment6() into two functions.

I'd love to see https://cgit.freebsd.org/src/tree/tests/sys/netpfil/pf/table.sh#n119 extended to also check for this.

I have the same patch (along with a test case) running a final test build now.

Same deal as D47679, send me the format-patch. Probably the last ones we'll do this dance for.

In D47321#1087326, @vegeta_tuxpowered.net wrote:

This code suffers from very old OpenBSD idea of (ab)using the same data structure for in-kernel storage and communication with userspace over ioctl.

More a note to self: looking at structure pf_src_node, the only other field we don't set is *kif, but given that it's a pointer that's not really something we can do in the ioctl. I'll make a note of it to revisit this once the netlink migration is complete. We ought to be able to return the interface name then.

Good catch. Given that it also needs to be fixed on stable/14 you should add an 'MFC after' tag to the commit message.

In D47658#1086624, @franco_opnsense.org wrote:

Sure. For context:

<snip>
The entire backtrace would have been nice.
Still, that's not the most important thing right now. Concentrate on the test case, which will mean we can trivially reproduce backtraces and we can circle back to improving the commit message once we have both the test case and an actual fix.

The backtrace appears to have gotten mangled here.
Also, a gdb backtrace would be more useful as it'll decode to line numbers rather than to addresses.

In D47543#1085347, @franco_opnsense.org wrote:

So regardless of why I already stated this is a technical issue that is by no means "pointless", what do you suggest to improve this particular test to make it more robust? Uncontrolled creation of processes that inherit file descriptors isn't exactly clean design but I can see why you do not want to apply this mere bandaid with that larger issue at hand. I'm happy to do the work since a lot of people were asking for test cases and here I am offering work on test cases to get started. :)

As for wider discussions about how to test let's just focus on this particular test case. Kyua is not a magic remedy for the problems the testing framework has like a lack of test coverage e.g. in the pflog department. Running atf-sh as "unsupported" and having a test pass just fine is no reason to not pursue this, especially since it says "you may get unexpected failures" and we talk about a passing test case. I appreciate the heads up but I still believe allowing test to run in a zero-config fashion from any running system is an actual benefit to the problem of not having enough people work on test cases. It really does not matter how you might see it differently because the test case needs work first.

Update the counter in ip_forward(), so we only set 'cantroute' in the forwarding case.
Extend the tests to test both slow and fast path forwarding.

In D47543#1084990, @franco_opnsense.org wrote:

Thanks, just to be clear you imply the change is wrong even though the test still works?

In D47543#1084854, @franco_opnsense.org wrote:

Consider running tests from the src tree using atf-sh (I'm using devel/atf but the base one also works with the full path I think):

sh -c 'echo $(atf-sh /usr/src/tests/sys/netpfil/pf/killstate.sh match)'

This hangs until you kill the stray nc processes. It works fine for the build of tests where nothing is forked into the background which seems to be the general issue. Another way would be to kill the nc processes when the test body ends (not in the cleanup as that would still cause it to hang), but a single line approach is likely easier although if you run the test a number of times these processes just keep building up in the system. I'm happy to improve this in any other way.

That's kind of expected to not work. We don't run tests that way. We always run them via kyua because it does a lot of setup work for us. This starts the process midway, and stops before it's entirely done.

What do you mean by "On an atf-sh script capture"?