Yes, 1:1 alias name assignment would be desirable and sidestep constraints with interface name length for descriptive interfaces (QinQ is too much actually) as well as avoid renaming interfaces when a VLAN ID changes for example. Just change alias and done (possibly with more character support).

In D39689#903716, @melifaro wrote:

Sure. The only thing actually used in if_mib in base is exactly this IFDATA_DRIVERNAME functionality. It is undocumented (and wrong for tunap/epair/soem other devices). I'm going to deprecate the module by providing this missing piece of functionality via Netlink ( D39659 ).

I wonder why adding a new field is needed when IFDATA_DRIVERNAME via if_dname/if_dunit exists? It's also exposed via libifconfig but sadly not via ifconfig command. I only noticed recently by looking at ifinfo command which we use in OPNsense because it provides better interface overview than ifconfig, but is not built in the base system.

Could this be the same as https://reviews.freebsd.org/D38065#875109 eventually resulting in:

The use case: a number of VPN software solutions like OpenVPN use this driver so the idea was to be able to grab traffic off of the interface before encryption/after decryption. It looks like tun may not be worth the effort, but it could work for tap mode without further constraints?

In D38065#874905, @vmaffione wrote:

Can I ask what kind of tests you performed? I guess you have set sysctl dev.netmap.admode=2 (see netmap(4)) and tried on a vtnet0 interface.
If not done yet, could you please perform some tests on an em0 interface (e.g. emulated by qemu or bhyve)?

For this to make sense from the user perspective attaching to a bridge should capture all packets associated with the bridge as e.g. seen by bpf (although here for now bpf might be circumvented). The reason for that is we don't want to modify user programs and restart and instead simply reconfigure bridge device akin to how lagg netmap works now.

@oshogbo I've updated the documentation and also described the caveats with the current implementation of restart_precmd which is pretty dangerous when not using restart_cmd ... but running the setup there prior to running it again during start seems silly just to pass a potential failure of restart_precmd. A number of ports seem to be using this override but a "proper" config file should be present if we assume that start was ran successfully first?

• update documentation on internals and caveats

merge issue

change setup/precmd order so when precmd checks config file it won't fail

I don't have any intention to debug this as I don't have a setup at hand that causes this. It should, however, be considered to revert the commit before 13.2 or 14.0 is released with it for the sole purpose of fixing a theoretical issue vs. breaking existing setups.

From what we can tell if the other end prohibits auto-negotiation forcing a particular media setting the NIC ends up in "no carrier" status with this patch, see https://www.reddit.com/r/opnsense/comments/xw4oiz/comment/ir4mxb0/?utm_source=reddit&utm_medium=web2x&context=3 and https://forum.opnsense.org/index.php?topic=30274

rc: extend NAME_setup, redefining commands escapes all structure

I reverted the change in question although I don't agree with the rationale. NAME_prepend remains a fragile construct, not being used in visible code in ports/src and prepending command(s) would imply that either ";" or "&&" is being used by default to separate the argument, which the user will not know because the documentation is not complete or the concept involved is not well-designed.

• revert prepend wording

Updated revision to address requirement to only skip known modifiers. Minimal code change, but more convoluted with the cont pointer being passed down additionally.

• Revert "pf: stop resolving hosts as dns that use ":" modifier"
• pfctl: stop resolving hosts as DNS that use internal ":" modifiers

@oshogbo I don't have the means to commit so if you would pick that up when you have some time that'd be highly appreciated

D35117 looks reasonable, let me abandon this then :)

I don't have a crash core and this only happened once on a customer device in FreeBSD 12.

Something like "ovpnc0:network" is hardly a domain name as one user noted seeing these pop up and chasing it to lookups in pfctl. host_if() implements these special markers and we could argue that pfctl-specific markers have priority and shouldn't be handled elsewhere.

Since I don't have a commit bit... anyone willing to commit this? Thanks in advance.

LGTM, thanks!

update as mentioned

created new review instead of update

thanks a lot :)

In D31515#724915, @markj wrote:

Sorry for the delay. I have no objections to the change and the implementation looks fine. I am a little wary of committing it without some wider approval: could I ask you to post a short note to freebsd-net@ linking this diff and soliciting opinions? If there are no objections after a week or so I will commit this.

We have multiple reports that this causes throughput regressions when in use on 13-STABLE as opposed to 13.0-RELEASE where it is not present. We have had this commit reverted and speeds are back to normal for our OPNsense users. For more info see https://forum.opnsense.org/index.php?topic=26364.0

fqpie_callout_cleanup() should exhibit the same issue

In D33432#755856, @kp wrote:

In D33432#755851, @franco_opnsense.org wrote:

I was thinking the same at first but the locking introduced in https://cgit.freebsd.org/src/commit/sys/netpfil/ipfw/dn_aqm_pie.c?id=12be18c7d594 looks arbitrary and isn't anywhere else in those two files. It was added to "protect" the ref_count manipulation, but if you look at the other ref_count modification in that file these are also done without (obvious) locks.

Maybe these ref_count modifications should receive atomic updates without locks to avoid the locking overhead completely?

Perhaps, yes.

Although it looks like the ref_count is only read in unload_dn_aqm(), under the sched_mtx lock. That lock lives only in ip_dummynet.c, so I wonder if we shouldn't just move the updating of the reference count to dn_aqm_ref()/dn_aqm_unref() and protect it with the sched_mtx lock. That doesn't need vnet, so we don't have to worry about setting the context (because it's about a global setting, so using a vnet-ed lock is wrong anyway) and we actually clean the locking up a little.

I was thinking the same at first but the locking introduced in https://cgit.freebsd.org/src/commit/sys/netpfil/ipfw/dn_aqm_pie.c?id=12be18c7d594 looks arbitrary and isn't anywhere else in those two files. It was added to "protect" the ref_count manipulation, but if you look at the other ref_count modification in that file these are also done without (obvious) locks.

In D33432#755816, @kp wrote:

Do you have a description on how to trigger this panic?

In D8877#754326, @mjg wrote:

what's the fate of this patch?

Just as a comment: With all these ties to RSS defines I'm not sure where that leaves the RSS feature with regard to its multiple hardware/software use cases but there's no point in blocking this with no visible consumers. I'll make sure to give this a test once it hits main.

In D32585#738110, @glebius wrote:

In D32585#737897, @kbowling wrote:

In D32585#737015, @np wrote:

Note. This change requires PCBGROUP to be retired.

Have you circulated this proposal in the wider -net and -vendor community? I know of one downstream that uses this feature.

It looks like opnsense is trying to make use of pcbgroups/rss, might want to check with @franco_opnsense.org

I don't see much history here https://github.com/opnsense/src/commits/master/sys/netinet/in_pcbgroup.c
Anyway, adding Franco as reviewer.

Thanks for the find! Looks reasonable to bring in. I will try to get more test coverage from our users, though feedback was low on this particular hang ;(

Added motivation for checking for untagged priority to the filter program comments.

add comment about need to test for VID 0 presence

Not sure about omitting the match on a NAT rule, but doing it inside the log code was definitely wrong.

void REASON_SET by directly passing PFRES_MATCH

But to be fair both rules are matching accounting-wise unless we assume that only "pass" can account for "match".

There is an older discussion about it here https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=224961

• skip to end on vlanid != 0 and add comments

Correct. Here is the test:

• lease declaration skip fixes

Use EVL_VLID_MASK as suggested

OpenBSD added the single htons() while adding write filter, but nowhere else. I suspect the fragmentation check is mostly correct so this doesn't matter in the real world.

constify filters and avoid static length variables

Appreciate the reviews :) Unfortunately I'm not a committer so is someone willing to help out? Thanks in advance.

fixed partial length on tx

Another one refactored

That would be great, thanks!

The only other script is the DNS script and it looks like -u already does append the sender address to that script's data.