Overall, I think this approach has value. See my in-line comments for suggestions on things to review further.

Thanks! I like this approach. I've added a few comments about potential enhancements.

In D45411#1036082, @tuexen wrote:

In D45411#1035980, @jtl wrote:

Should we check for TCP_FUNC_BEING_REMOVED?

I thought about this and did again after you raised the question. The original code didn't.
...
However, since we don't hold the tcp_function_lock, the tfb can be removed any time before or after we set the TCP function block in tcp_newtcpcb. It cannot go away, since we hold a reference count. I guess that is the reason why there was no check...

Can you give a brief explanation of the problem this is supposed to solve?

Should we check for TCP_FUNC_BEING_REMOVED?

In D42664#973383, @glebius wrote:

I actually planned to make it a loadable module. Given that it creates extra threads that aren't going to be used by default makes its presence questionable.

In D42407#967685, @imp wrote:

I might also note in the commit message you used a here document with the shell output expanded in the commit message... it is a bit of a surprising change based on the commit message.

For what its worth...

Incorporate two suggestions from @jhb.

Updated the diff to upgrade the delay field from u_short to u_int in struct lock_delay_arg. Due to alignment requirements, this probably won't actually change the size or in-memory layout of the structure at all.

In D38853#884786, @mjg wrote:

Quite frankly I'm confused how come people keep claiming atomic ops are cheap, when it seemed it is common knowledge they are very expensive, especially so on amd64. Maybe I should do a writeup with real numbers(tm).

In D38853#884557, @mjg wrote:

atomics already make it probably too expensive to use

By the way, I should clarify that I am not opposed to this change; rather, my previous comment was merely intended to help explain what I view as the implications of the change and advocate that we make sure there is sufficiently broad agreement to proceed.

The rationale for the present state is found here.

Functionally committed in D36716

In D36716#833679, @tuexen wrote:

How does this relate to the TCP User Timeout from RFC 793/9293?

In D36595#832812, @kib wrote:

I still do not like the extra verbosity from kernel. If insisting on adding more kernel messages, you could make logging for SIGTRAP with interesting si_codes, covering both wxorx and capsicum in one shot.

Fix the man page description of the vm.enforce_wxorx sysctl/tunable to indicate that a value of 2 both denies mappings and sends a signal.

Addressed review feedback.
Restored defaults.

Added a section to the security(7) man page in an attempt to clarify the controls which will be available if this change is merged.

In D36595#831110, @jtl wrote:

In D36595#831026, @kib wrote:

IMO there is too much controls now for wxorx. There is proccontrol -m wxmap, and essentially adding yet another knob breaks it.

I'll look into this a little more.

In D36595#831026, @kib wrote:

IMO there is too much controls now for wxorx. There is proccontrol -m wxmap, and essentially adding yet another knob breaks it.

Made changes based on review:

• Switch from syslog to uprintf() for reporting ELF W^X failure.
• Separate W^X logging and enforcement.
• Change test for which processes to signal. (Now, system processes, processes which are still starting, and kernel threads are exempt.)
• Switch from using SIGSEGV to SIGTRAP with a descriptive code.

Changes suggested in the review:

• Added entry to UPDATING.
• Updated security(7) man page.

In D36275#826020, @markj wrote:

The diff seems ok to me, but I don't really understand why we need one callout per hash bucket. It's an extra 64KB per VNET, which isn't exactly cheap. I can see why it's conceptually simpler to implement this way, since you have a per-bucket mutex to synchronize the callout, but it still feels like overkill. I don't have any other comments on the patch itself.

In D29519#666595, @tuexen wrote:

In D29519#666585, @markj wrote:

Ping? Any comments on the overall approach, or on the details of the change?

Hi Mark,

we discussed this at the last transport call. We agreed that that handling should be consistent but wanted to check until the next transport call (about two weeks from now) what the consistent way would be. My understanding is:

Thanks for doing this! It looks like a very positive change, and I'm sure there was a lot of effort put into finding the right way to clean up the code.

In D24316#639938, @brd wrote:

@jtl Please MFC this to stable/12 along with D24272

Thanks for doing this so quickly!

FWIW, I disagree with this change. I think we should instead use atomic operations here.

In D29225#654397, @kib wrote:

It is strange indeed, and it sounds more like a self-inflicting action from userspace. Code in rtld or libthr should not leak sigfastblock block, but of course bugs are possible.

This is the change I am planning to commit once the regression tests finish running.

This does not fix the regression I am experiencing in my test setup. I am testing with a machine which uses a LAGG interface to communicate with the outside world. Shutting this interface down still makes my SSH sessions hang.

I just saw the discussion on the committers mailing list. First, it shows that @cy already has a proposed fix. Secondly, it shows that this is a larger issue (for example, netboot), which probably needs a different solution.

In D27464#613656, @tuexen wrote:

When committing, please add a reference to https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=251540

In D27246#609375, @markj wrote:

I'd suggest running the DTrace test suite with this change if you haven't. make -C cddl/usr.sbin/dtrace WITH_DTRACE_TESTS= all install should install them to /usr/tests/cddl/usr.sbin/dtrace.

In D27246#608560, @markj wrote:

Could you please re-upload with context?

Updating the diff to include context.

While here, update the code in ctf_add_generic() to encode empty type names with index 0. This fixes the analogous case for type names.

In D27213#608228, @jtl wrote:

In D27213#608181, @markj wrote:

I can't reproduce this problem at all on head. The script appears to work properly. I remember that the use of anonymous unions in struct mbuf caused some problems, at least one of which was fixed by r305055, but that was a long time ago.

I can reproduce this problem locally. Some others can't reproduce the problem I reported in the main description. I'm not sure why, but there seems to be some non-determinism in the way symbols are loaded/resolved?

In D27213#608269, @ae wrote:

It seems I found how to reproduce it on test system:

1. Load systemt without any unneeded modules
2. kldload dtraceall
3. Run

# dtrace -n 'fbt::ip_input:entry { printf("%s", stringof(args[0]->m_pkthdr.rcvif->if_xname)); }'
dtrace: description 'fbt::ip_input:entry ' matched 1 probe
CPU     ID                    FUNCTION:NAME
  2  49220                   ip_input:entry ix0
  2  49220                   ip_input:entry ix0
  6  49220                   ip_input:entry ix0
^C
# kldunload dtraceall
# kldload ipfw
# kldload dtraceall
# dtrace -n 'fbt::ip_input:entry { printf("%s", stringof(args[0]->m_pkthdr.rcvif->if_xname)); }'
dtrace: invalid probe specifier fbt::ip_input:entry { printf("%s", stringof(args[0]->m_pkthdr.rcvif->if_xname)); }: in action list: m_pkthdr is not a member of struct mbuf

In D27213#608181, @markj wrote:

In D27213#608180, @jtl wrote:

In D27213#608081, @ae wrote:

Recently I faced with this problem on some machines:

# dtrace -n 'fbt::ip_input:entry { printf("%s", stringof(args[0]->m_pkthdr.rcvif->if_xname)); }'
dtrace: invalid probe specifier fbt::ip_input:entry { printf("%s", stringof(args[0]->m_pkthdr.rcvif->if_xname)); }: in action list: m_pkthdr is not a member of struct mbuf

And it seems it is exactly related.

It does appear that problem is likely related to type resolution. However, for what its worth, this patch does not solve that problem on my test machine. So, that problem may have a different cause.

I can't reproduce this problem at all on head. The script appears to work properly. I remember that the use of anonymous unions in struct mbuf caused some problems, at least one of which was fixed by r305055, but that was a long time ago.

In D27213#608081, @ae wrote:

Recently I faced with this problem on some machines:

# dtrace -n 'fbt::ip_input:entry { printf("%s", stringof(args[0]->m_pkthdr.rcvif->if_xname)); }'
dtrace: invalid probe specifier fbt::ip_input:entry { printf("%s", stringof(args[0]->m_pkthdr.rcvif->if_xname)); }: in action list: m_pkthdr is not a member of struct mbuf

And it seems it is exactly related.