On Thursday, October 22, 2015 02:53:18 PM Richard Guy Briggs wrote: > If we hold the audit_cmd_mutex, we should never sleep waiting for auditd > to drain the queue since auditd may need the mutex to shut down. > > This was first implemented with mutex_trylock(), but since > audit_log_start() can be called in softirq context, that won't work. > Next, owner_running() was used to check audit_cmd_mutex but another > process could have this locked on another cpu. Use rcu_read_lock() and > ACCESS_ONCE() to check audit_cmd_mutex. > > Signed-off-by: Richard Guy Briggs <r...@redhat.com> > --- > kernel/audit.c | 7 +++++-- > 1 files changed, 5 insertions(+), 2 deletions(-)
Ungh. This is painful ... and I'm talking about the problem, not necessarily the solution your proposing here. I'm going to pass on this patch for now because I'd like to see us step back and reexamine our approach here. When it comes down to it, audit_cmd_mutex is really just there because we don't have proper, granular locking in audit_receive_msg(), right? Looking quickly at it, it appears that AUDIT_GET/SET could be dealt with via a spinlock (we could add RCU if GET is frequent) ... similar could be done with AUDIT_GET/SET_FEATURE ... AUDIT_USER is a little more complex and not immediately obvious, but it looks like most of the pain points (audit_filter_user() and tty_audit_push_current() are already safe ... AUDIT_ADD/DEL_RULE look to be already protected via the audit_filter_mutex ... same with AUDIT_LIST_RULES ... same with AUDIT_TRIM ... same with AUDIT_MAKE_EQUIV ... AUDIT_SIGNAL_INFO shouldn't be a problem ... AUDIT_TTY_GET/SET already have spinlocks. Am I missing something? > diff --git a/kernel/audit.c b/kernel/audit.c > index 02a5ec0..34411af 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -1376,12 +1376,15 @@ struct audit_buffer *audit_log_start(struct > audit_context *ctx, gfp_t gfp_mask, return NULL; > > if (gfp_mask & __GFP_WAIT) { > - if (current->tgid == 1 || (audit_pid && audit_pid == > current->tgid)) > + rcu_read_lock(); > + if (ACCESS_ONCE(audit_cmd_mutex.owner) == current || > + current->tgid == 1 || > + (audit_pid && audit_pid == current->tgid)) > gfp_mask &= ~__GFP_WAIT; > else > reserve = 0; > + rcu_read_unlock(); > } > - > while (audit_backlog_limit > && skb_queue_len(&audit_skb_queue) > audit_backlog_limit + > reserve) > { if (gfp_mask & __GFP_WAIT && audit_backlog_wait_time) { -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit