Page MenuHomeFreeBSD

Add per-process flag to disable logsigexit
Needs ReviewPublic

Authored by kevans on Fri, Oct 4, 11:06 PM.

Details

Reviewers
kib
jilles
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
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 26921

Event Timeline

kevans created this revision.Fri, Oct 4, 11:06 PM
kevans added inline comments.Fri, Oct 4, 11:09 PM
sys/kern/kern_procctl.c
605

That's backwards

kib added a comment.Sat, Oct 5, 9:56 AM

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.

kevans added a comment.Sat, Oct 5, 4:26 PM
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."

jilles added a subscriber: jilles.Sat, Oct 5, 5:37 PM
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.

kevans added a comment.Sat, Oct 5, 5:44 PM
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.

kevans edited reviewers, added: jilles; removed: manpages.Sat, Oct 5, 5:45 PM

(roping Jilles in, too)

kevans updated this revision to Diff 62947.Sat, Oct 5, 10:44 PM

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.

kib accepted this revision.Sun, Oct 6, 12:11 PM

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.Sun, Oct 6, 12:11 PM
kevans added a comment.Sun, Oct 6, 3:33 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.

jilles added inline comments.Sun, Oct 6, 3:48 PM
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 updated this revision to Diff 63021.Tue, Oct 8, 3:24 AM
kevans marked 2 inline comments as done.

General wordsmithing/relabelling for consistency; no functional change.

This revision now requires review to proceed.Tue, Oct 8, 3:24 AM