On 2018-02-20 11:55, Paul Moore wrote: > From: Paul Moore <paul(a)paul-moore.com&gt; > > 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 <peterz(a)infradead.org&gt; > Signed-off-by: Paul Moore <paul(a)paul-moore.com&gt; 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 <rgb(a)redhat.com&gt;

> --- > 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(a)redhat.com > https://www.redhat.com/mailman/listinfo/linux-audit - RGB -- Richard Guy Briggs <rgb(a)redhat.com&gt; 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(a)redhat.com https://www.redhat.com/mailman/listinfo/linux-audit