Page MenuHomeFreeBSD

Add per-process flag to disable logsigexit
AcceptedPublic

Authored by kevans on Oct 4 2019, 11:06 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 20 2023, 1:57 AM
Unknown Object (File)
Nov 15 2023, 10:35 AM
Unknown Object (File)
Nov 8 2023, 4:25 AM
Unknown Object (File)
Nov 8 2023, 3:41 AM
Unknown Object (File)
Nov 6 2023, 12:56 PM
Unknown Object (File)
Nov 6 2023, 6:55 AM
Unknown Object (File)
Oct 27 2023, 6:15 AM
Unknown Object (File)
Oct 14 2023, 9:33 AM
Subscribers

Details

Reviewers
kib
jilles
pstef
Summary

I added a third value for kern.logsigexit to mean 'auto' as an abundance of caution, but I don't know how much it matters -- that can be easily consolidated back to boolean-ish. I'm also unsure of who else to add to this review, other than kib.

This is primarily targeted towards people running test suites under CI (e.g. buildbot, jenkins). Oftentimes tests entail segfaults that are expected, and logs get spammed -- this can be particularly high volume depending on the application. Per-process control of this behavior is desirable because they may still want to be logging legitimate segfaults, so the system-wide atomic bomb kern.logsigexit=0 is not a great option.

This adds a process flag to disable it, controllable via procctl(2)/proccontrol(1); the latter knows it as "sigexitlog" due to its length, but it's referred to almost everywhere else as "sigexit_log."

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 26890

Event Timeline

sys/kern/kern_procctl.c
605

That's backwards

You need to handle compat32.

I find it somewhat strange that you require global control to enable local setting. Less cryptic, I would expect to see per-process setting of 3 cases: 1. default/use sysctl, 2. enabled, 3. disabled.

In D21903#478452, @kib wrote:

You need to handle compat32.

I find it somewhat strange that you require global control to enable local setting. Less cryptic, I would expect to see per-process setting of 3 cases: 1. default/use sysctl, 2. enabled, 3. disabled.

I wasn't entirely sure how important the global control is. This doesn't gate a highly important feature in the first place, so I suppose the answer is "not really, let proccontrol override it all the time."

In D21903#478452, @kib wrote:

I find it somewhat strange that you require global control to enable local setting. Less cryptic, I would expect to see per-process setting of 3 cases: 1. default/use sysctl, 2. enabled, 3. disabled.

I wasn't entirely sure how important the global control is. This doesn't gate a highly important feature in the first place, so I suppose the answer is "not really, let proccontrol override it all the time."

An application can already override the global control (in most cases) by adding signal handlers. So making it hard for proccontrol to override the global control seems inappropriate.

In D21903#478452, @kib wrote:

I find it somewhat strange that you require global control to enable local setting. Less cryptic, I would expect to see per-process setting of 3 cases: 1. default/use sysctl, 2. enabled, 3. disabled.

I wasn't entirely sure how important the global control is. This doesn't gate a highly important feature in the first place, so I suppose the answer is "not really, let proccontrol override it all the time."

An application can already override the global control (in most cases) by adding signal handlers. So making it hard for proccontrol to override the global control seems inappropriate.

I hadn't thought of it that way- my new local version stops touching the sysctl and just adds one more p2 flag to indicate that the process intends to override it with the other p2 flag (that's getting flipped from DISABLE to ENABLE).

I'll be uploading that after some testing.

(roping Jilles in, too)

Some renames to be consistent with the sysctl:

  • sigexitlog -> logsigexit
  • SIGEXIT_LOG -> LOGSIGEXIT

Other news:

  • Add DEFAULT value, for "respect sysctl"
  • Add another p_flag2 to indicate the process is exercising control over logsigexit; flip sense of the DISABLE flag to ENABLE
  • Add compat32 bits
  • Maintain logsigexit behavior across fork

I was unsure how verbose we can/should be for proccontrol -m query, and whether or not that should have a pointer to the global control.

It is impossible to turn logsigexit back to default with proccontrol(1), but this is arguably a wart in the current structure of the arguments parser.

I recommend to commit the kernel/libc part separately of usr.bin/proccontrol.

This revision is now accepted and ready to land.Oct 6 2019, 12:11 PM
In D21903#478626, @kib wrote:

It is impossible to turn logsigexit back to default with proccontrol(1), but this is arguably a wart in the current structure of the arguments parser.

Yeah, I went to add a "default" value to proccontrol(1), but then decided that I'd really want to restructure it somehow to also allow, e.g., PROTMAX_NOFORCE/ASLR_NOFORCE to be indicated. Then I realized I didn't really want to build a whitelist of those that have a 'default'/'noforce' value, I'd rather have more mapping tables to indicate valid commands (represented as bitmasks)... at that point I decided I'm a little too struct array/data table happy, and halted.

I'll likely rename the procctl(2) flags pre-commit to match PROTMAX/ASLR verbiage of FORCE_ENABLE/FORCE_DISABLE/NOFORCE now that this is tri-state and similar semantics.

lib/libc/sys/procctl.2
127

Perhaps make this "signals that would normally cause a core dump"?

129–130

Of course this uses log(9) but the syslog facility is "kern", right?

sys/kern/kern_procctl.c
580

Incomplete sentence?

usr.bin/proccontrol/proccontrol.1
75

If you change the description in procctl(2) then please change it here as well.

kevans marked 2 inline comments as done.

General wordsmithing/relabelling for consistency; no functional change.

This revision now requires review to proceed.Oct 8 2019, 3:24 AM
pstef added a subscriber: pstef.

I haven't tested it, but I imagine many people would like to use this, me included.

This revision is now accepted and ready to land.Sep 24 2021, 8:51 PM