Re: [RFC PATCH 5/7] audit: allow audit_cmd_mutex holders to use reserves
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 <rgb(a)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