Re: [PATCH] audit: track the owner of the command mutex ourselves

[Paul Moore]

On Thu, Feb 22, 2018 at 3:40 AM, Richard Guy Briggs  wrote:
> On 2018-02-20 11:55, Paul Moore wrote:
>> From: Paul Moore 
>>
>> Evidently the __mutex_owner() function was never intended for use
>> outside the core mutex code, so build a thing locking wrapper around
>> the mutex code which allows us to track the mutex owner.
>>
>> One, arguably positive, side effect is that this allows us to hide
>> the audit_cmd_mutex inside of kernel/audit.c behind the lock/unlock
>> functions.
>>
>> Reported-by: Peter Zijlstra 
>> Signed-off-by: Paul Moore 
>
> This is what I was trying to accomplish here through several iterations,
> but your solution looks more graceful:
>
> https://www.redhat.com/archives/linux-audit/2015-October/msg00073.html

Yes, I credited you with the mutex owner idea in the patch from last
March that kickstarted the queue rework; while your patch didn't quite
work out I still think the idea is solid.

> /me has no dog...
>
> Reviewed-by: Richard Guy Briggs 

I haven't heard any objections over the past few days, just
misunderstanding on how the locking/queues work, so I'm going to go
ahead and merge this into audit/next.  At the very least it should
satisfy Peter's concerns with our usage of __mutex_owner().

>> ---
>>  kernel/audit.c  |   66 
>> +++
>>  kernel/audit.h  |3 ++
>>  kernel/audit_tree.c |8 +++---
>>  3 files changed, 61 insertions(+), 16 deletions(-)
>>
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index 5c2544984375..3c4f6f3d7041 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -181,9 +181,21 @@ static char *audit_feature_names[2] = {
>>   "loginuid_immutable",
>>  };
>>
>> -
>> -/* Serialize requests from userspace. */
>> -DEFINE_MUTEX(audit_cmd_mutex);
>> +/**
>> + * struct audit_ctl_mutex - serialize requests from userspace
>> + * @lock: the mutex used for locking
>> + * @owner: the task which owns the lock
>> + *
>> + * Description:
>> + * This is the lock struct used to ensure we only process userspace requests
>> + * in an orderly fashion.  We can't simply use a mutex/lock here because we
>> + * need to track lock ownership so we don't end up blocking the lock owner 
>> in
>> + * audit_log_start() or similar.
>> + */
>> +static struct audit_ctl_mutex {
>> + struct mutex lock;
>> + void *owner;
>> +} audit_cmd_mutex;
>>
>>  /* AUDIT_BUFSIZ is the size of the temporary buffer used for formatting
>>   * audit records.  Since printk uses a 1024 byte buffer, this buffer
>> @@ -227,6 +239,36 @@ int auditd_test_task(struct task_struct *task)
>>   return rc;
>>  }
>>
>> +/**
>> + * audit_ctl_lock - Take the audit control lock
>> + */
>> +void audit_ctl_lock(void)
>> +{
>> + mutex_lock(_cmd_mutex.lock);
>> + audit_cmd_mutex.owner = current;
>> +}
>> +
>> +/**
>> + * audit_ctl_unlock - Drop the audit control lock
>> + */
>> +void audit_ctl_unlock(void)
>> +{
>> + audit_cmd_mutex.owner = NULL;
>> + mutex_unlock(_cmd_mutex.lock);
>> +}
>> +
>> +/**
>> + * audit_ctl_owner_current - Test to see if the current task owns the lock
>> + *
>> + * Description:
>> + * Return true if the current task owns the audit control lock, false if it
>> + * doesn't own the lock.
>> + */
>> +static bool audit_ctl_owner_current(void)
>> +{
>> + return (current == audit_cmd_mutex.owner);
>> +}
>> +
>>  /**
>>   * auditd_pid_vnr - Return the auditd PID relative to the namespace
>>   *
>> @@ -861,8 +903,8 @@ int audit_send_list(void *_dest)
>>   struct sock *sk = audit_get_sk(dest->net);
>>
>>   /* wait for parent to finish and send an ACK */
>> - mutex_lock(_cmd_mutex);
>> - mutex_unlock(_cmd_mutex);
>> + audit_ctl_lock();
>> + audit_ctl_unlock();
>>
>>   while ((skb = __skb_dequeue(>q)) != NULL)
>>   netlink_unicast(sk, skb, dest->portid, 0);
>> @@ -903,8 +945,8 @@ static int audit_send_reply_thread(void *arg)
>>   struct audit_reply *reply = (struct audit_reply *)arg;
>>   struct sock *sk = audit_get_sk(reply->net);
>>
>> - mutex_lock(_cmd_mutex);
>> - mutex_unlock(_cmd_mutex);
>> + audit_ctl_lock();
>> + audit_ctl_unlock();
>>
>>   /* Ignore failure. It'll only happen if the sender goes away,
>>  because our timeout is set to infinite. */
>> @@ -1467,7 +1509,7 @@ static void audit_receive(struct sk_buff  *skb)
>>   nlh = nlmsg_hdr(skb);
>>   len = skb->len;
>>
>> - mutex_lock(_cmd_mutex);
>> + audit_ctl_lock();
>>   while (nlmsg_ok(nlh, len)) {
>>   err = audit_receive_msg(skb, nlh);
>>   /* if err or if this message says it wants a response */
>> @@ -1476,7 +1518,7 @@ static void audit_receive(struct sk_buff  *skb)
>>
>>   nlh = nlmsg_next(nlh, );
>>   }
>> - mutex_unlock(_cmd_mutex);
>> + audit_ctl_unlock();
>>  }
>>
>>  /* Run custom bind 

Re: [PATCH] audit: track the owner of the command mutex ourselves

[Richard Guy Briggs]

On 2018-02-20 11:55, Paul Moore wrote:
> From: Paul Moore 
> 
> Evidently the __mutex_owner() function was never intended for use
> outside the core mutex code, so build a thing locking wrapper around
> the mutex code which allows us to track the mutex owner.
> 
> One, arguably positive, side effect is that this allows us to hide
> the audit_cmd_mutex inside of kernel/audit.c behind the lock/unlock
> functions.
> 
> Reported-by: Peter Zijlstra 
> Signed-off-by: Paul Moore 

This is what I was trying to accomplish here through several iterations,
but your solution looks more graceful:

https://www.redhat.com/archives/linux-audit/2015-October/msg00073.html

/me has no dog...

Reviewed-by: Richard Guy Briggs 

> ---
>  kernel/audit.c  |   66 
> +++
>  kernel/audit.h  |3 ++
>  kernel/audit_tree.c |8 +++---
>  3 files changed, 61 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 5c2544984375..3c4f6f3d7041 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -181,9 +181,21 @@ static char *audit_feature_names[2] = {
>   "loginuid_immutable",
>  };
>  
> -
> -/* Serialize requests from userspace. */
> -DEFINE_MUTEX(audit_cmd_mutex);
> +/**
> + * struct audit_ctl_mutex - serialize requests from userspace
> + * @lock: the mutex used for locking
> + * @owner: the task which owns the lock
> + *
> + * Description:
> + * This is the lock struct used to ensure we only process userspace requests
> + * in an orderly fashion.  We can't simply use a mutex/lock here because we
> + * need to track lock ownership so we don't end up blocking the lock owner in
> + * audit_log_start() or similar.
> + */
> +static struct audit_ctl_mutex {
> + struct mutex lock;
> + void *owner;
> +} audit_cmd_mutex;
>  
>  /* AUDIT_BUFSIZ is the size of the temporary buffer used for formatting
>   * audit records.  Since printk uses a 1024 byte buffer, this buffer
> @@ -227,6 +239,36 @@ int auditd_test_task(struct task_struct *task)
>   return rc;
>  }
>  
> +/**
> + * audit_ctl_lock - Take the audit control lock
> + */
> +void audit_ctl_lock(void)
> +{
> + mutex_lock(_cmd_mutex.lock);
> + audit_cmd_mutex.owner = current;
> +}
> +
> +/**
> + * audit_ctl_unlock - Drop the audit control lock
> + */
> +void audit_ctl_unlock(void)
> +{
> + audit_cmd_mutex.owner = NULL;
> + mutex_unlock(_cmd_mutex.lock);
> +}
> +
> +/**
> + * audit_ctl_owner_current - Test to see if the current task owns the lock
> + *
> + * Description:
> + * Return true if the current task owns the audit control lock, false if it
> + * doesn't own the lock.
> + */
> +static bool audit_ctl_owner_current(void)
> +{
> + return (current == audit_cmd_mutex.owner);
> +}
> +
>  /**
>   * auditd_pid_vnr - Return the auditd PID relative to the namespace
>   *
> @@ -861,8 +903,8 @@ int audit_send_list(void *_dest)
>   struct sock *sk = audit_get_sk(dest->net);
>  
>   /* wait for parent to finish and send an ACK */
> - mutex_lock(_cmd_mutex);
> - mutex_unlock(_cmd_mutex);
> + audit_ctl_lock();
> + audit_ctl_unlock();
>  
>   while ((skb = __skb_dequeue(>q)) != NULL)
>   netlink_unicast(sk, skb, dest->portid, 0);
> @@ -903,8 +945,8 @@ static int audit_send_reply_thread(void *arg)
>   struct audit_reply *reply = (struct audit_reply *)arg;
>   struct sock *sk = audit_get_sk(reply->net);
>  
> - mutex_lock(_cmd_mutex);
> - mutex_unlock(_cmd_mutex);
> + audit_ctl_lock();
> + audit_ctl_unlock();
>  
>   /* Ignore failure. It'll only happen if the sender goes away,
>  because our timeout is set to infinite. */
> @@ -1467,7 +1509,7 @@ static void audit_receive(struct sk_buff  *skb)
>   nlh = nlmsg_hdr(skb);
>   len = skb->len;
>  
> - mutex_lock(_cmd_mutex);
> + audit_ctl_lock();
>   while (nlmsg_ok(nlh, len)) {
>   err = audit_receive_msg(skb, nlh);
>   /* if err or if this message says it wants a response */
> @@ -1476,7 +1518,7 @@ static void audit_receive(struct sk_buff  *skb)
>  
>   nlh = nlmsg_next(nlh, );
>   }
> - mutex_unlock(_cmd_mutex);
> + audit_ctl_unlock();
>  }
>  
>  /* Run custom bind function on netlink socket group connect or bind 
> requests. */
> @@ -1548,6 +1590,9 @@ static int __init audit_init(void)
>   for (i = 0; i < AUDIT_INODE_BUCKETS; i++)
>   INIT_LIST_HEAD(_inode_hash[i]);
>  
> + mutex_init(_cmd_mutex.lock);
> + audit_cmd_mutex.owner = NULL;
> +
>   pr_info("initializing netlink subsys (%s)\n",
>   audit_default ? "enabled" : "disabled");
>   register_pernet_subsys(_net_ops);
> @@ -1711,8 +1756,7 @@ struct audit_buffer *audit_log_start(struct 
> audit_context *ctx, gfp_t gfp_mask,
>*using a PID anchored in the caller's namespace
>* 2.