Testing concluded and with the two changes mentioned it works for multiple people who previously reported seeing continuous segfaults with e.g. Unbound. Original issue described in 291577 is fixed as well.

Looks like this fixes the reported issue but causes other instabilities documented here https://forum.opnsense.org/index.php?topic=53114.0

taking it back to bugzilla

I'm happy to admit I'm wrong, but I'm not going to indicate the user is wrong about the crashes and that this patch actually helps to avoid it. On top of this the $0 comment was tremendously insightful to me. Thanks!

I'm wondering if https://cgit.freebsd.org/src/commit/?id=38cc0bfa261 plays a role here. The move of s->timer assignment is reversed here. I'm happy to fix the locking issue in a next patch iteration. The fact remains that in an edge case the PF_STATE_UNLOCK() fails on an already freed state and this sidesteps the issue.

Sorry, I'm just confused: is INVARIANTS in GENERIC? I'm dealing with users and real world issues here and this definitely helps with the situation. If you agree the patch works for non-INVARIANTS build than that is a good step to address the code for INVARIANTS, maybe just hiding the locks in that case if we're sure this is air-tight.

I don't care about INVARIANTS and we don't use it for reasons of not being reliable.

To my understanding PF_HASHROW_LOCK() does not lock the state itself. The state is currently in use and locked by another thread which it acquired by pf_find_state() which basically does:

In D49359#1127313, @zlei wrote:

In D49359#1126467, @franco_opnsense.org wrote:

For 285129 this still crashes in the same place: https://github.com/opnsense/src/issues/207#issuecomment-2733080313

I tested the patch for days, it appears quite stable as far as now ( surely, for the input path only ).

Can you share the steps to repeat the Github issue 207 ? I do not understand the original reporter's steps.

In D49359#1126495, @zlei wrote:

In D49359#1126467, @franco_opnsense.org wrote:

For 285129 this still crashes in the same place: https://github.com/opnsense/src/issues/207#issuecomment-2733080313

Is that based on stable/14 ?

For 285129 this still crashes in the same place: https://github.com/opnsense/src/issues/207#issuecomment-2733080313

Merged manually.

I'll do a CFT on this change with OPNsense users next week. A few can trigger the if_afdata panics with PPPoE.

simplified this and marked as POC to keep the discussion as a reference

In D49212#1125353, @melifaro wrote:

The code cleanup in the diff LGTM - added a couple of comments.

Happy to see this progress. Do you think removing RT_LINK_IS_UP without adding the additional conditional would be a useful cleanup nonetheless? Half the code uses NH_IS_VALID, the other RT_LINK_IS_UP and all of it is within NH scope nowadays.

Something is filling the table while we read it with the lock held in pfr_get_addrs() is what this situation tells me, which points to a missing lock somewhere else, maybe introduced as far back as https://cgit.freebsd.org/src/commit/?id=890612bbeb69f

In my local testing KASAN allows me to push 100 mbit max, which is going to be a challenge asking the user to deploy this in production and to trigger the bug which only happens under traffic. I can try, but not optimistic.

I'm happy to pass an alternative patch on to the affected users, but I'm unable to reproduce this locally.

In D49214#1123225, @markj wrote:

In D49214#1122687, @franco_opnsense.org wrote:

I'm tracking down what looks to be a memory corruption of some sort:

Have you tried testing a GENERIC-KASAN kernel?

also remove the other assert

removed assert in tree traversal hotpath

I'm tracking down what looks to be a memory corruption of some sort:

Last bits in dmesg, perhaps unrelated:

Let me throw in the towel here. I've left two review comments.

First of all thanks for your work. I'm in the same boat as you wanting to see this progress.

Also confirmed now but the reporters have operational remarks when running with the fix, see https://github.com/opnsense/src/issues/239#issuecomment-2669497329

In D49053#1118526, @zlei wrote:

IN_ is short for inet but no doubt it is a little ambiguity esp. for newbies. That requires network domain knowledge but apparently IN6_ is much better.

Linux KPI has a pragmatic approach:

Did the same here https://github.com/opnsense/src/commit/8f86d0fdd37 but haven't heard from the user yet whether the provided test kernel works for them.

@des did you find the time to make a technical assessment? thanks!

The Common Criteria aspects are documented here https://docs.vmware.com/en/VMware-SD-WAN/6.0/VMware-SD-WAN-Administration-Guide/GUID-DF592A36-E680-44CE-ABFC-ACE19B55B448.html

change summary

@kp let me know how to proceed

We've tested https://github.com/opnsense/src/commit/7f55b75b successfully with the user in OPNsense.

Done, for reference: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=283137

Potentially fixed by c22c98798 ;)

This commit likely causes the following panic reported by multiple users:

Offered a test kernel to the user in the meantime. @glebius is probably busy (last I spoke with him) and doesn't mind if you commit?

Like this?

In D46301#1079978, @melifaro wrote:

I'm going to come up with a different version of this patch (likely using a new flag rtmsg->rtm_flags to signal RTM_F_FORCE) in a day or two. The current version allows all netlink customers to fully bypass PINNED route protection, which defeats its purpose.

For the record I have no stakes in SCTP and I'm not involved in the changes done here. This work with the user reporting it is a courtesy to the FreeBSD pf code and to triage panics which I only think is a good idea for production code.

What's the status now? From my point of you I've explained the previous behaviour, the change that caused this to exhibit the wrong behaviour and how it was fixed. I'd like to get to a more productive cooperation based on technical urgency. I don't think tests prevented this from happening and I don't think the issue gets any better discussing commit messages.

Here it is:

Feel free to make a different suggestion. I've long been under the impressions explaining one liners is not the scope of a commit message and the tests should make this rather clear as requested in previous discussions.

I changed the summary.

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).

In D47658#1086724, @kp wrote:

The entire backtrace would have been nice.

Sounds good to me, thanks :)

Meh, sorry for the misinformation. All of this is very difficult to trace.

(kyua depends on e.g. libexec/atf-sh for script execution via shebang in /usr/src/tests/sys/netpfil/pf/*.sh)

I have WITHOUT_TESTS=yes and WITH_TESTS_SUPPORT=yes which adds kyua and libatf, but not atf tools in libexec. On a build without WITHOUT_TESTS=yes the atf tools are placed in the base system as a side effect.

Sure. For context:

I'm trying to understand the entry barrier here. If kyua fixes everything that's alright and thanks for the pointers. From release engineering scope there are a number of kyua integration challenges irrelevant to your argumentation (which I can understand), but you both seem to see the situation too easy from an established development workflow ("just do it right"). Using a custom src.conf already makes kyua defunct, but it is what it is. Going to raise appropriate patches elsewhere then. Thanks!

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. :)

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

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):

• libfetch: shuffle SSL_CRL_VERIFY options