auditd_stop: wait_for_pids instead of sleeping

[rlibby]

Summary

It's faster and more reliable to wait_for_pids than to sleep 1.

This greatly reduces the time to run the /usr/tests/sys/audit tests.
audit has hundreds of tests, and each slept for one second due to
auditd_stop.

Test Plan

time kyua test -k /usr/tests/sys/audit/Kyuafile

Before:

418/418 passed (0 failed)
      533.65 real        15.86 user        29.62 sys

vs after:

418/418 passed (0 failed)
      104.93 real        15.35 user        30.30 sys

[cem]

Seems like there is a possible race condition here where $rc_pid exits and is recycled before we check for exit.

[cem]

I don't see any reason auditd cannot just use the default stop action, which sends SIGTERM. auditd handles sigterm and performs more or less the same shutdown actions as those triggered by audit -t (AUDIT_TRIGGER_CLOSE_AND_DIE). I would suggest just removing the auditd_stop() function entirely.

[rlibby]

In D23223#515931, @cem wrote:

Seems like there is a possible race condition here where $rc_pid exits and is recycled before we check for exit.

There totally is. I thought just to do it this way because it's what everybody else does, including the built-in stop command in rc.subr.

However, I think it's unlikely that this race will be expressed, unless we aggressively recycle pids, in which case I think a lot of stuff will break. Also, I wasn't sure what kind of job control might be relied upon in this context.

IOW, I think you're right, and it's a bigger problem.

[rlibby]

In D23223#515942, @cem wrote:

I don't see any reason auditd cannot just use the default stop action, which sends SIGTERM. auditd handles sigterm and performs more or less the same shutdown actions as those triggered by audit -t (AUDIT_TRIGGER_CLOSE_AND_DIE). I would suggest just removing the auditd_stop() function entirely.

This sounds reasonable to me, I just wasn't personally confident enough that it was true. Is there a maintainer who can speak to this? There seems to be a difference, e.g. auditd_reap_children(), and whatever is going on with launchd (but doesn't apply to us?).

[cem]

In D23223#515944, @rlibby wrote:

This sounds reasonable to me, I just wasn't personally confident enough that it was true. Is there a maintainer who can speak to this? There seems to be a difference, e.g. auditd_reap_children(), and whatever is going on with launchd (but doesn't apply to us?).

I've CCed folks who have been involved with OpenBSM. I don't believe the launchd stuff applies to us, and I'm not sure why the existing path doesn't reap children.

[asomers]

Good catch! I don't see any reason not to do it your way. In fact, @rwatson anticipated that in his original commit message.

r162605 | rwatson | 2006-09-24 11:31:04 -0600 (Sun, 24 Sep 2006) | 13 lines

Sleep for one second after calling audit -t to give the audit daemon a
chance to actually terminate the audit service and exit.  Otherwise, on
an rc.d/auditd restart, the new audit daemon instance may try to start
auditing while the previous session is still running.  Likewise, this
ensures a chance for auditd to terminate the audit trail at system
shutdown.

Perhaps more ideally, the script would wait synchronously for auditd to
exit rather than for an arbitrary but short period of time.

MFC after:	3 days
Obtained from:	TrustedBSD Project