I'm not sure if I object to this change or not, but it's worth noting that SSIDs are not necessarily UTF-8 strings. Unless the SSIDEncoding is set it is 0-32 octets. Having 0 bytes in the middle of the SSID is valid (though I'd be very surprised if that actually worked on more than a handful of devices). If SSIDEncoding is set it is indeed a UTF-8 string.
For additional fun Microsoft got this wrong and several Windows versions interpret the SSID as being Latin1 encoded.

I'd prefer the sysctl to reject values that are out of range.
What do you think about this: https://people.freebsd.org/~kp/patches/D14536.patch ?

Looks good at first glance.
I like the move of the sys/rmlock.h include into pfvar.h, so the header becomes more self-contained (i.e. you don't need extra includes to make your include of pfvar.h work). I'm running test builds for that, because I thought it broke when I tried. Perhaps I messed something else up though.

In D14536#316763, @darkfiberiru_gmail.com wrote:

Sorry I thought I had already replied to this. I have issues with keeping that as the default value as it has been deprecated since 1998. As such it's not really a sane default and not compatible with much modern routing gear. The setting I choose comes directly our of the RFC for this type of traffic.

Well, as I said, I have no views on what the value should be. I defaulted to keeping the old value, but if you've got a good reason to change it that's fine by me.

In D15018#316088, @cem wrote:

Looks good in general. It seems that some cases use WOULD_OVERFLOW as a secondary check and some don't. Is there any reason for that?

Good catch, I missed it in the first two cases because that used to be a fixed 65k value. Now that it's user configurable it needs the check too.

Fix static / extern mismatch

Add missing WOULD_OVERFLOW checks.

Committed as r332101

Committed as r332102

What problem does this fix?

Keep the old hooks so other pfil users don't need to change. Allow pf to use the new style of hook, with the flags argument.

Looks sane to me.

I have no specific views on what the value should be. The remark mostly comes from the desire to avoid having the kernel enforce policy. It's only a small extra step here for more flexibility.
I think I'd make net.inet.carp.dscp be an integer, with a default value of (the value of) IPTOS_LOWDELAY.

Would it make sense to set the DSCP value to the value configured in 'net.inet.carp.dscp' rather than a hardcoded value?

In D11137#297170, @olivier wrote:

Any news about pushing this change to head ?

In D13715#296796, @ae wrote:

I think someone from pfSense or openSense already reworked PF to work "inplace" like ipfw does, i.e. it returns mbuf back to the function from where pfil was invoked.

In D13715#296702, @eri wrote:

In D13715#295452, @kristof wrote:

In D13715#295449, @eri wrote:

While i have not much time lately to spend on this, i still think this is the wrong way of doing things since it just creates confusion.

I'm not sure I see how this would create confusion. This merely presents more information about the packet, and where the netpfil hook being called from.

pf(4) already has a loop detection mechanism that i created to support divert(9) and dummunet(9), not sure if the later ever made it into FreeBSD.

Now I'm confused. This isn't about loop detection. This is about detecting if a PFIL_OUT packet is being forwarded or output.

Good catch.

In D13715#295449, @eri wrote:

While i have not much time lately to spend on this, i still think this is the wrong way of doing things since it just creates confusion.

I'm not sure I see how this would create confusion. This merely presents more information about the packet, and where the netpfil hook being called from.

Based around a suggestion from Kyle Evans (who also did all of the work), introduce a flags variable to the pfil callbacks. Keep using PFIL_OUT for forwarded packets, but set the PFIL_FWD flag for them. This allows pf to work out if a packet is being forwarded or not, with essentially no changes to other netpfil consumers.

In D13766#290617, @erj wrote:

Is this going to be MFC'd into stable/10 and/or stable/11?

ae@ is probably the best person to talk to about ipfw, so you may want to cc him too.

In D13715#288702, @eri wrote:

While this is needed i do not agree that the modifications on the stack and packet filters should be so hackish.

More context. No changes to the diff.

Use the OpenBSD mallocarray implementation.

Add __alloc_size attributes.

Update malloc.9 man page.

I have to admit I initially wanted to call it calloc(), but it turns out ZFS already has calloc(size_t, size_t) in sys/cddl/compat/opensolaris/sys/kmem.h, so that failed to build.

Removed incorrect __alloc_size attribute.

I'll try again once I see a commit go by that looks like it'd fix that. Do feel free to remind me if I manage to miss it (or forget).

It looks like something's still wrong:

Well, I suppose this is good, in that it shows why these tests are useful:

Oh, also: if some of the tests are known to fail we should mark them as such until the issue is fixed.
See 'atf_expect_fail'

How to add the case of with and without AESNI without rewriting all these tests?

I suspect this will also address PR 222647.

This is similar to how I fixed this problem for pf.

Similar results for v6:

With a larger file (102400 bytes) the difference indeed shrinks:

I found some crusty old hardware to run a test on. This is nginx serving its default index page (612 bytes of data).
Test client is wrk. I played around with the number of connections and threads briefly, but didn't see a major difference (in the non-vimage performance, I've not compared vimage there).

In D12639#262598, @bz wrote:

@jtl I am not aware of anyone done a end-host, e.g., webserver kind of performance measurement. No one wanted to volunteer so far.

Thumbs up. Let's do this.

Sorry, I missed that remark.

This should address all of the review remarks.

This builds on r323390, right?

In D11401#254776, @paggas1_yandex.com wrote:

I've made a change which I'm testing currently, running pkg with the -r
flag instead of chrooting. Will commit if it works.

Yes, that's even better.

While looking at this I also noticed that this file is (mostly) intended with spaces. FreeBSD style is to use tabs.

In D12011#248956, @bcr wrote:

Adding Kristof for confirming that this is the actual address in the code.

In D3272#247495, @adrian wrote:

yes please!

I've done a bit more testing, and these debug traces tell the story:

Hmm, good question. I thought I understood the failure flow fully, but now I'm not so sure.

In D11782#244283, @bz wrote:

Is this a consequence of https://svnweb.freebsd.org/base?view=revision&revision=297816 or independent of it?

I've fixed that style remark and commit it. Thanks for the review!

Update patch after mkioctls was changed.

Ideally, yes, but it looks like the headers from sys/net (pfvar.h and if_pfsync.h) do get installed, so including those files leads to build failures.

If WITHOUT_PF is set we don't install pf.h (from sys/netpfil/pf), which means the generated file won't compile.
I suppose we could install it, but it's a little odd having pf.h installed if the user explicitly asked not to have pf.

I ran into issues with 'install world' with this patch. I think you also want the following:

I think you're also missing the connection in the pfctl makefile. I've been looking at sbin/mdconfig as an example and it has this:

This also appears to be missing the integration with the other tests. It can only be used by running it directly from the tests/sbin/pfctl directory.

In D11322#238488, @paggas1_yandex.com wrote:

Cleaned-up version, with improved test descriptions.

We're intermingling two sets of changes here.

It'd also be good to include the changes required to hook up the sbin/pfctl/Kyuafile to the tests Kyuafile.
Your GitHub repo has a symlink to get recursive behaviour out of Kyua, but there's basically no other symlinks in the repo, so let's avoid that. I'd add a simple include Kyuafile, like ./contrib/atf/Kyuafile does.

It might also be worth seeing if you can add a couple of tests for ALTQ.
That's also interesting to consider because ALTQ is an optional feature and might not be available in the running kernel.
That'd mean you'd have to have test metadata to figure out if you can sensibly run the test on this system or not (presumably kyua understands the concept of a skipped test).

Damjan has updated the patch to fix the style issues and address a panic with non-udp traffic.

I've just done a quick test. The box no longer panics with this patch.

I think this is correct, but I think you should skip the {}. The rest of the function does not use them for single-line if statements either.