Re: [RFC PATCH] audit: fix a race condition with the auditd tracking code

On 2017-06-15 11:28, Paul Moore wrote: > From: Paul Moore <paul(a)paul-moore.com&gt; > > Originally reported by Adam and Dusty, it appears we have a small > race window in kauditd_thread(), as documented in the Fedora BZ: > > * https://bugzilla.redhat.com/show_bug.cgi?id=1459326#c35 > > "This issue is partly due to the read-copy nature of RCU, and > partly due to how we sync the auditd_connection state across > kauditd_thread and the audit control channel. The kauditd_thread > thread is always running so it can service the record queues and > emit the multicast messages, if it happens to be just past the > "main_queue" label, but before the "if (sk == NULL || ...)" > if-statement which calls auditd_reset() when the new auditd > connection is registered it could end up resetting the auditd > connection, regardless of if it is valid or not. This is a rather > small window and the variable nature of multi-core scheduling > explains why this is proving rather difficult to reproduce." > > The fix is to have functions only call auditd_reset() when they > believe that the kernel/auditd connection is still valid, e.g. > non-NULL, and to have these callers pass their local copy of the > auditd_connection pointer to auditd_reset() where it can be compared > with the current connection state before resetting. If the caller > has a stale state tracking pointer then the reset is ignored. > > We also make a small change to kauditd_thread() so that if the > kernel/auditd connection is dead we skip the retry queue and send the > records straight to the hold queue. This is necessary as we used to > rely on auditd_reset() to occasionally purge the retry queue but we > are going to be calling the reset function much less now and we want > to make sure the retry queue doesn't grow unbounded. > > Reported-by: Adam Williamson <awilliam(a)redhat.com&gt; > Reported-by: Dusty Mabe <dustymabe(a)redhat.com&gt; > Signed-off-by: Paul Moore <paul(a)paul-moore.com&gt; This looks reasonable. Reviewed-by: Richard Guy Briggs <rgb(a)redhat.com&gt;