Re: [RFC PATCH 5/7] audit: allow audit_cmd_mutex holders to use reserves

[Paul Moore <paul@paul-moore.com>]

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@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