On Thu, Feb 22, 2018 at 3:40 AM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2018-02-20 11:55, Paul Moore wrote:
>> From: Paul Moore <p...@paul-moore.com>
>>
>> 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 <pet...@infradead.org>
>> Signed-off-by: Paul Moore <p...@paul-moore.com>
>
> 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 <r...@redhat.com>
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(&audit_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(&audit_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(&audit_cmd_mutex);
>> - mutex_unlock(&audit_cmd_mutex);
>> + audit_ctl_lock();
>> + audit_ctl_unlock();
>>
>> while ((skb = __skb_dequeue(&dest->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(&audit_cmd_mutex);
>> - mutex_unlock(&audit_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(&audit_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, &len);
>> }
>> - mutex_unlock(&audit_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(&audit_inode_hash[i]);
>>
>> + mutex_init(&audit_cmd_mutex.lock);
>> + audit_cmd_mutex.owner = NULL;
>> +
>> pr_info("initializing netlink subsys (%s)\n",
>> audit_default ? "enabled" : "disabled");
>> register_pernet_subsys(&audit_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. generator holding the audit_cmd_mutex - we don't want to block
>> * while holding the mutex */
>> - if (!(auditd_test_task(current) ||
>> - (current == __mutex_owner(&audit_cmd_mutex)))) {
>> + if (!(auditd_test_task(current) || audit_ctl_owner_current())) {
>> long stime = audit_backlog_wait_time;
>>
>> while (audit_backlog_limit &&
>> diff --git a/kernel/audit.h b/kernel/audit.h
>> index af5bc59487ed..214e14948370 100644
>> --- a/kernel/audit.h
>> +++ b/kernel/audit.h
>> @@ -341,4 +341,5 @@ extern struct list_head *audit_killed_trees(void);
>> #define audit_filter_inodes(t,c) AUDIT_DISABLED
>> #endif
>>
>> -extern struct mutex audit_cmd_mutex;
>> +extern void audit_ctl_lock(void);
>> +extern void audit_ctl_unlock(void);
>> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
>> index fd353120e0d9..67e6956c0b61 100644
>> --- a/kernel/audit_tree.c
>> +++ b/kernel/audit_tree.c
>> @@ -709,7 +709,7 @@ static int prune_tree_thread(void *unused)
>> schedule();
>> }
>>
>> - mutex_lock(&audit_cmd_mutex);
>> + audit_ctl_lock();
>> mutex_lock(&audit_filter_mutex);
>>
>> while (!list_empty(&prune_list)) {
>> @@ -727,7 +727,7 @@ static int prune_tree_thread(void *unused)
>> }
>>
>> mutex_unlock(&audit_filter_mutex);
>> - mutex_unlock(&audit_cmd_mutex);
>> + audit_ctl_unlock();
>> }
>> return 0;
>> }
>> @@ -924,7 +924,7 @@ static void audit_schedule_prune(void)
>> */
>> void audit_kill_trees(struct list_head *list)
>> {
>> - mutex_lock(&audit_cmd_mutex);
>> + audit_ctl_lock();
>> mutex_lock(&audit_filter_mutex);
>>
>> while (!list_empty(list)) {
>> @@ -942,7 +942,7 @@ void audit_kill_trees(struct list_head *list)
>> }
>>
>> mutex_unlock(&audit_filter_mutex);
>> - mutex_unlock(&audit_cmd_mutex);
>> + audit_ctl_unlock();
>> }
>>
>> /*
>>
>> --
>> Linux-audit mailing list
>> Linux-audit@redhat.com
>> https://www.redhat.com/mailman/listinfo/linux-audit
>
> - RGB
>
> --
> Richard Guy Briggs <r...@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
