This probably also wants this:

And this is wrong in firewall_init():

I seem to run into issues running the ipfw_basic test:

I think I'm happy with this.
I'll give Tom a bit of time to add any more remarks he might have, but I think we can commit this soon.

Full context.

In D21086#457948, @ae wrote:

Does this mean that you concluded in the IETF mailing list to drop this support?

Remove more bits, as suggested by thj

I'd want Tom to have a look too, but I think we're pretty close to something ready to commit.

Also, I get 'install: /usr/tests/sys/netpfil/common/pass_block: No such file or directory' trying to install world.
This patch is missing this:

I think I now understand better how the problem happens. It's specific to epair.
It's indeed a race between if_vmove() moving the interface back into its original vnet, and the epair interface getting destroyed. Destruction of epairs is special, because we remove two interfaces at once.
See epair_clone_destroy(). There we if_detach() (through ether_ifdetach()) the other interface as well. If that interface has been moved out of the vnet by then the if_detach() will fail, but this does not return an error (if_detach_internal() does). So we end up freeing the ifp, while if_vmove() is reinserting it in another vnet, leading to the described panic.

Good point. While this does make things a lot better (as in, nearly perfectly reliable panic running the pf tests, to being able to loop the tests all night, when combined with D20869), I think you're right that the race is still there.

Given that it's possible to use blacklistd with ipfw as well (at least, I believe it is), it should probably get its own chapter, with pf and ipfw subchapters.

In D20616#445627, @jhb wrote:

Is this routine used for transmit or only for receive? In the current patch, unmapped mbufs are only used for transmit. The comments in pf_check_proto_cksum imply it might only apply to receive in which case it should be fine as-is.

In D20616#445618, @jhb wrote:

In D20616#445537, @kristof wrote:

What happens if a firewall is enabled and an unmapped mbuf is passed through pfil(9)?
I suspect that, if a pfil hook is hit, we'd also have to copy it in, just like when a checksum needs to be updated.

Only if the firewall needs to read/write actual packet data. Protocol headers (TCP, IP, etc.) are always stored in a normal mbuf at the start of a packet's mbuf chain. Unmapped mbufs only hold payload data that is stored in a socket buffer, so most of the filters I can think of off the top of my head as well as things like NAT should only operate on the normal mbuf holding the headers.

Okay, thanks. That should indeed just work. The 'pf_check_proto_cksum()' flow, assuming there's no hardware assist, might break. I suspect that hardware which uses unmapped mbufs is always going to have checksum offload, so that's probably not an issue either.

What happens if a firewall is enabled and an unmapped mbuf is passed through pfil(9)?
I suspect that, if a pfil hook is hit, we'd also have to copy it in, just like when a checksum needs to be updated.

There are sadly still a lot of panics triggered by these tests.

That seems like a good idea. I'll try to bring it up during the BSDCan devsummit.

Burn it with fire! This code is not used, and pretty much can't be used, so let's get rid of it.

Rather annoyingly things like ezjail can't directly use the devfsrules_vnet_jail name. They must specify the number. See https://svnweb.freebsd.org/base/head/libexec/rc/rc.d/jail?view=markup#l238
I have no idea how to address that, but this at the very least will hint users in the direction of what they may want for vnet jails.

cc bapt who knows more about this code than I do.

LGTM. It may be useful to point at 122773 in the commit message for extra context.

It's not clear to me how this leaks memory. Can you go into a bit more detail about how you found the memory leak, and the codepath it follows?

I don't see any obvious problems, but I don't think I know this part of the code well enough to say for sure.

panic: Assertion in_epoch(net_epoch_preempt) failed at /usr/src/sys/netinet6/nd6_rtr.c:874
cpuid = 3
time = 1552657485
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe0091c86300
vpanic() at vpanic+0x19d/frame 0xfffffe0091c86350
panic() at panic+0x43/frame 0xfffffe0091c863b0
defrouter_select_fib() at defrouter_select_fib+0x52a/frame 0xfffffe0091c86460
defrouter_del() at defrouter_del+0x15f/frame 0xfffffe0091c864a0
nd6_purge() at nd6_purge+0x1af/frame 0xfffffe0091c864f0
if_detach_internal() at if_detach_internal+0x7f6/frame 0xfffffe0091c86570
if_detach() at if_detach+0x3d/frame 0xfffffe0091c86590
epair_clone_destroy() at epair_clone_destroy+0x98/frame 0xfffffe0091c865e0
if_clone_destroyif() at if_clone_destroyif+0x175/frame 0xfffffe0091c86630
if_clone_destroy() at if_clone_destroy+0x205/frame 0xfffffe0091c86680
ifioctl() at ifioctl+0x3de/frame 0xfffffe0091c86750
kern_ioctl() at kern_ioctl+0x28a/frame 0xfffffe0091c867c0
sys_ioctl() at sys_ioctl+0x15d/frame 0xfffffe0091c86890
amd64_syscall() at amd64_syscall+0x276/frame 0xfffffe0091c869b0
fast_syscall_common() at fast_syscall_common+0x101/frame 0xfffffe0091c869b0
--- syscall (54, FreeBSD ELF64, sys_ioctl), rip = 0x8004852ca, rsp = 0x7fffffffe228, rbp = 0x7fffffffe240 ---
KDB: enter: panic
[ thread pid 93197 tid 100466 ]
Stopped at      kdb_enter+0x3b: movq    $0,kdb_why
db>

During the sys/netinet/fibs_test:slaac_on_nondefault_fib6 test, I believe.

Oh yes, of course.

Do we need to virtualise the bridge_rtnode_zone? Doesn't the cleanup call to bridge_rtflush() take care of all of the allocations already?

I'll try to take a more in-depth look later today.

LGTM.

Add comment

In D19459#417542, @vangyzen wrote:

@bdrewery and I both independently had the same idea. He mentioned it in D19426.

Ah, I hadn't looked at that review yet.

https://bugs.freebsd.org/12345 redirects to https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=12345. Wouldn't the first form be better? It's shorter and likely easier to map to whatever bug tracker we might move to after bugzilla (if we ever do change). It also looks more like the phabricator links.

'\0'

If there are no objections I'm going to commit this soon.

Fix issues pointed out by hrs.

Fix tun_clone_match() to not match on e.g. tunnel.

Remove stray debug lines

Ensure that unit numbers are system-unique.

The pf bit is good.
Everything else looks good to me as well, modulo the remark karels had about the XXX comment.

New panic, I suspect while an interface is being moved into or out of a vnet jail.
The pf tests seem to trigger a lot of edge cases, so you may want to see if you can get them to run too. They don't need much setup (basically, pkg install scapy kyua, then kldload pfsync ; cd /usr/tests/sys/netpfil/pf ; sudo kyua test).

Thanks. The original reporter of the bug discovered that too.

This one boots, but panics when I kldload pfsync:

I have no objections to the patch, but I don't know enough about HFSC to meaningfully review this, I'm afraid.

I'm pretty sure that ipv6_activate_all_interfaces="YES" in /etc/rc.conf is the trigger for the previous panic.
That also matches nicely with the backtrace. Presumably nd6_dad_timer() doesn't enter the NET_EPOCH.

This panics my test vm:

LGTM

Did you happen to do any benchmarking on this? I'd have expected "Sync pfil hooks epoch(9)" to give us a (small) performance improvement, but my initial test shows a small reduction in forwarding performance (with pf loaded).

This panics during the netipsec and pf regression tests:

To be clear: I don't object to your change. It's just that ifconfig is a bit of a mess and any opportunity for cleanup is tempting. Especially if I don't have to do it.

Yeah, looking at my original commit I did manage to lose that flag. I’ve not tested yet, but this patch is almost certainly correct.

Also, I’d really, really like some basic tests for altq. It should be reasonably straightforward to do something based on the existing pf tests. At least, if altq can work on top of epairs.

LGTM.

I’ll try to review asap, but I’m on holiday wirh limited internet access right now.

In D18373#392613, @eri wrote:

Not a blocker but:

• It would also be nice to have measure if the other side can keep up with the blast of state updates now?
• Even better, provide the same bucket mechanism on reception so it can be distributed on the various cores

In D18373#392610, @eri wrote:

Can you please measure the latency of syncing states with this change against previous latency?

Fix build with ! VIMAGE

Address man page remark.

Minor improvements, style and such.
Small man page addition.