Re: [PATCH] audit: track the owner of the command mutex ourselves
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(&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 bi
Re: [PATCH] audit: track the owner of the command mutex ourselves
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(&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. generat