audit: clean up audit queue handling
cover audit: clean up audit queue handling
This set of patches cleans up a number of corner cases in the management of the audit queue.
Richard Guy Briggs (7):
audit: don't needlessly reset valid wait time
audit: include auditd's threads in audit_log_start() wait exception
audit: allow systemd to use queue reserves
audit: wake up threads if queue switched from limited to unlimited
audit: allow audit_cmd_mutex holders to use reserves
audit: wake up audit_backlog_wait queue when auditd goes away.
audit: wake up kauditd_thread after auditd registers
kernel/audit.c | 20 +++++++++++++++----- 1 files changed, 15 insertions(+), 5 deletions(-)
RFC PATCH 1/7 audit: don't needlessly reset valid wait time
After auditd has recovered from an overflowed queue, the first process that doesn't use reserves to make it through the queue checks should reset the audit backlog wait time to the configured value. After that, there is no need to keep resetting it.
--- kernel/audit.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/audit.c +1 -1
@@ -1403,7 +1403,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,return NULL ; } -if (!reserve) +if (!reserve && !audit_backlog_wait_time) audit_backlog_wait_time = audit_backlog_wait_time_master; ab =audit_buffer_alloc (ctx, gfp_mask, type);
Signature · 1 line
1.7.1
RFC PATCH 2/7 audit: include auditd's threads in audit_log_start() wait exception
In reply to Richard Guy BriggsShould auditd spawn threads, allow all members of its thread group to use the audit_backlog_limit reserves to bypass the queue limits too.
--- kernel/audit.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/audit.c +1 -1
@@ -1375,7 +1375,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,return NULL ;if (gfp_mask & __GFP_WAIT) { -if (audit_pid && audit_pid == current->pid) +if (audit_pid && audit_pid == current->tgid) gfp_mask &= ~__GFP_WAIT;else reserve =0 ;
Signature · 1 line
1.7.1
RFC PATCH 3/7 audit: allow systemd to use queue reserves
In reply to Richard Guy BriggsTreat systemd the same way as auditd, allowing it to overrun the queue to avoid blocking.
--- kernel/audit.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/audit.c +1 -1
@@ -1375,7 +1375,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,return NULL ;if (gfp_mask & __GFP_WAIT) { -if (audit_pid && audit_pid == current->tgid) +if (current->tgid ==1 || (audit_pid && audit_pid == current->tgid)) gfp_mask &= ~__GFP_WAIT;else reserve =0 ;
Signature · 1 line
1.7.1
RFC PATCH 4/7 audit: wake up threads if queue switched from limited to unlimited
In reply to Richard Guy BriggsIf the audit_backlog_limit is changed from a limited value to an unlimited value (zero) while the queue was overflowed, wake up the audit_backlog_wait queue to allow those processes to continue.
--- kernel/audit.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
kernel/audit.c +2 -1
@@ -523,7 +523,8 @@ static int kauditd_thread(void *dummy) skb =skb_dequeue (&audit_skb_queue);if (skb) { -if (skb_queue_len (&audit_skb_queue) <= audit_backlog_limit) +if (!audit_backlog_limit || + (skb_queue_len (&audit_skb_queue) <= audit_backlog_limit))wake_up (&audit_backlog_wait);if (audit_pid)kauditd_send_skb (skb);
Signature · 1 line
1.7.1
RFC PATCH 5/7 audit: allow audit_cmd_mutex holders to use reserves
In reply to Richard Guy BriggsIf 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.
--- kernel/audit.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
kernel/audit.c +5 -2
@@ -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) {
Signature · 1 line
1.7.1
RFC PATCH 6/7 audit: wake up audit_backlog_wait queue when auditd goes away.
In reply to Richard Guy BriggsWhen auditd goes away (died, killed or shutdown, or net namespace shut down), there is no point in sleeping waiting for auditd to drain the queue since that message would be distined for the hold queue after the timeout anyways. This will needlessly have those processes wait the full default timeout of 60 seconds (audit_backlog_wait_time).
Wake up the processes caught in the audit_backlog_wait queue when auditd is no longer present so they can be sent instead to the hold queue.
--- kernel/audit.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
kernel/audit.c +5 -1
@@ -425,6 +425,7 @@ restart:audit_log_lost (s); audit_pid =0 ; audit_sock =NULL ; +wake_up (&audit_backlog_wait); }else {pr_warn ("re-scheduling(#%d) write to audit_pid=%d\n" , attempts, audit_pid);
@@ -882,6 +883,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) audit_pid = new_pid; audit_nlk_portid =NETLINK_CB (skb).portid; audit_sock = skb->sk; +if (!audit_pid) +wake_up (&audit_backlog_wait); }if (s.mask &AUDIT_STATUS_RATE_LIMIT ) { err =audit_set_rate_limit (s.rate_limit);
@@ -1154,6 +1157,7 @@ static void __net_exit audit_net_exit(struct net *net)if (sock == audit_sock) { audit_pid =0 ; audit_sock =NULL ; +wake_up (&audit_backlog_wait); }RCU_INIT_POINTER (aunet->nlsk,NULL );
@@ -1393,7 +1397,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, sleep_time = timeout_start + audit_backlog_wait_time - jiffies;if (sleep_time >0 ) { sleep_time =wait_for_auditd (sleep_time); -if (sleep_time >0 ) +if (audit_pid && sleep_time >0 )continue ; } }
Signature · 1 line
1.7.1
RFC PATCH 7/7 audit: wake up kauditd_thread after auditd registers
In reply to Richard Guy BriggsWhen auditd is restarted, even though the kauditd_thread is present, it remains dormant until the next audit log message is queued.
Wake up the kauditd_thread in the kauditd_wait queue immediately when auditd registers its availability to drain the queue.
--- kernel/audit.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
kernel/audit.c +2 -0
@@ -885,6 +885,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) audit_sock = skb->sk;if (!audit_pid)wake_up (&audit_backlog_wait); +if (audit_pid) +wake_up_interruptible (&kauditd_wait); }if (s.mask &AUDIT_STATUS_RATE_LIMIT ) { err =audit_set_rate_limit (s.rate_limit);
Signature · 1 line
1.7.1
Re: [RFC PATCH 3/7] audit: allow systemd to use queue reserves
In reply to Richard Guy BriggsQuoted 2 lines · Richard Guy Briggs
Treat systemd the same way as auditd, allowing it to overrun the queue to avoid blocking.
Do you mind explaining this a little more? I'm having a hard time understanding how systemd is involved.
-Steve
Quoted 18 lines
Signed-off-by Richard Guy Briggs <[email protected]>--- kernel/audit.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)kernel/audit.c +1
-1@@ -1375,7 +1375,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, return NULL;if (gfp_mask & __GFP_WAIT) { -if (audit_pid && audit_pid == current->tgid) +if (current->tgid ==1 || (audit_pid && audit_pid == current->tgid)) gfp_mask &= ~__GFP_WAIT;else reserve =0 ;
Re: [RFC PATCH 3/7] audit: allow systemd to use queue reserves
Quoted 6 lines · Steve Grubb
Quoted 2 lines · Richard Guy Briggs
Treat systemd the same way as auditd, allowing it to overrun the queue to avoid blocking.
Do you mind explaining this a little more? I'm having a hard time understanding how systemd is involved.
systemd should only have CAP_AUDIT_READ for the multicast socket and otherwise behaves as a user client, sending AUDIT_USER_* messages. It starts and stops auditd and we don't want it blocking trying to allocate a buffer on the standard queue in audit_log_start() while it is tasked with telling auditd to start or stop.
Quoted 14 lines
-Steve
Quoted 12 lines
Signed-off-by Richard Guy Briggs <[email protected]>--- kernel/audit.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)kernel/audit.c +0
-0@@ -1375,7 +1375,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, return NULL;
Quoted 6 lines
kernel/audit.c struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, · line 1381
Quoted 6 lines
kernel/audit.c +1
-1if (gfp_mask & __GFP_WAIT) { -if (audit_pid && audit_pid == current->tgid) +if (current->tgid ==1 || (audit_pid && audit_pid == current->tgid)) gfp_mask &= ~__GFP_WAIT;else reserve =0 ;
- RGB
--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
Quoted 12 lines · Richard Guy Briggs
This set of patches cleans up a number of corner cases in the management of the audit queue.
Richard Guy Briggs (7):
audit: don't needlessly reset valid wait time
audit: include auditd's threads inaudit_log_start()wait exception
audit: allow systemd to use queue reserves
audit: wake up threads if queue switched from limited to unlimited
audit: allowaudit_cmd_mutexholders to use reserves
audit: wake upaudit_backlog_waitqueue when auditd goes away.
audit: wake upkauditd_threadafter auditd registers
Quoted 2 lines
kernel/audit.c | 20 +++++++++++++++----- 1 files changed, 15 insertions(+), 5 deletions(-)
Due to the fact that these patches were posted late in the 4.3-rcX cycle, I've decided not to merge these into linux-audit#next for the upcoming merge window. I still need to take a closer look and properly review these patches, but I wanted to let you know why I haven't acted on them yet.
Signature · 2 lines
paul moore security @ redhat
Quoted 13 lines · Paul Moore
Quoted 12 lines · Richard Guy Briggs
This set of patches cleans up a number of corner cases in the management of the audit queue.
Richard Guy Briggs (7):
audit: don't needlessly reset valid wait time
audit: include auditd's threads inaudit_log_start()wait exception
audit: allow systemd to use queue reserves
audit: wake up threads if queue switched from limited to unlimited
audit: allowaudit_cmd_mutexholders to use reserves
audit: wake upaudit_backlog_waitqueue when auditd goes away.
audit: wake upkauditd_threadafter auditd registers
Quoted 7 lines
Quoted 2 lines
kernel/audit.c | 20 +++++++++++++++----- 1 files changed, 15 insertions(+), 5 deletions(-)Due to the fact that these patches were posted late in the 4.3-rcX cycle, I've decided not to merge these into linux-audit#next for the upcoming merge window. I still need to take a closer look and properly review these patches, but I wanted to let you know why I haven't acted on them yet.
No problem, at least it is out of my queue, as long as we have enough time to hit the next one. :)
Quoted 1 line
paul moore
- RGB
--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
Quoted 17 lines · Richard Guy Briggs
Quoted 16 lines · Paul Moore
Quoted 14 lines · Richard Guy Briggs
This set of patches cleans up a number of corner cases in the management of the audit queue.
Richard Guy Briggs (7):
audit: don't needlessly reset valid wait time
audit: include auditd's threads inaudit_log_start()wait exception
audit: allow systemd to use queue reserves
audit: wake up threads if queue switched from limited to unlimited
audit: allowaudit_cmd_mutexholders to use reserves
audit: wake upaudit_backlog_waitqueue when auditd goes away.
audit: wake upkauditd_threadafter auditd registerskernel/audit.c | 20 +++++++++++++++----- 1 files changed, 15 insertions(+), 5 deletions(-)
Quoted 8 lines
Quoted 5 lines
Due to the fact that these patches were posted late in the 4.3-rcX cycle, I've decided not to merge these into linux-audit#next for the upcoming merge window. I still need to take a closer look and properly review these patches, but I wanted to let you know why I haven't acted on them yet.
No problem, at least it is out of my queue, as long as we have enough time to hit the next one. :)
Definitely. I just start getting twitchy about accepting non-trivial patches post -rc5(ish).
Signature · 2 lines
paul moore security @ redhat
Paul Moore
maintainerRe: [RFC PATCH 1/7] audit: don't needlessly reset valid wait time
In reply to Richard Guy BriggsQuoted 15 lines · Richard Guy Briggs
After auditd has recovered from an overflowed queue, the first process that doesn't use reserves to make it through the queue checks should reset the audit backlog wait time to the configured value. After that, there is no need to keep resetting it.
Signed-off-by Richard Guy Briggs <[email protected]>--- kernel/audit.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)kernel/audit.c +0
-0
kernel/audit.c struct audit_buffer *audit_log_start(struct · line 1409
Quoted 8 lines
kernel/audit.c +1
-1@@ -1403,7 +1403,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, return NULL; } -if (!reserve) +if (!reserve && !audit_backlog_wait_time) audit_backlog_wait_time = audit_backlog_wait_time_master; ab =audit_buffer_alloc (ctx, gfp_mask, type);
This looks fine to me, I'm going to add it to audit#next-queue.
Also, can you think of a good reason why "audit_backlog_wait_overflow" exists? I'm going to replace it with the simple "audit_backlog_wait_time = 0;" unless you can think of a solid reason not to do so. It seems much more obvious and readable to me.
Signature · 2 lines
paul moore www.paul-moore.com
Paul Moore
maintainerRe: [RFC PATCH 2/7] audit: include auditd's threads in audit_log_start() wait exception
In reply to Richard Guy BriggsQuoted 7 lines · Richard Guy Briggs
Should auditd spawn threads, allow all members of its thread group to use the
audit_backlog_limitreserves to bypass the queue limits too.Signed-off-by Richard Guy Briggs <[email protected]>--- kernel/audit.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
Looks good, applied to audit#next-queue.
Quoted 13 lines
kernel/audit.c +1
-1@@ -1375,7 +1375,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, return NULL;if (gfp_mask & __GFP_WAIT) { -if (audit_pid && audit_pid == current->pid) +if (audit_pid && audit_pid == current->tgid) gfp_mask &= ~__GFP_WAIT;else reserve =0 ;
Signature · 2 lines
paul moore www.paul-moore.com
Re: [RFC PATCH 1/7] audit: don't needlessly reset valid wait time
In reply to Paul MooreQuoted 23 lines · Paul Moore
Quoted 22 lines · Richard Guy Briggs
After auditd has recovered from an overflowed queue, the first process that doesn't use reserves to make it through the queue checks should reset the audit backlog wait time to the configured value. After that, there is no need to keep resetting it.
Signed-off-by Richard Guy Briggs <[email protected]>--- kernel/audit.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)kernel/audit.c +1
-1@@ -1403,7 +1403,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, return NULL; } -if (!reserve) +if (!reserve && !audit_backlog_wait_time) audit_backlog_wait_time = audit_backlog_wait_time_master;
Quoted 8 lines
kernel/audit.c struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, · line 1409
Quoted 1 line
kernel/audit.c +0
-0ab =audit_buffer_alloc (ctx, gfp_mask, type);This looks fine to me, I'm going to add it to audit#next-queue.
Also, can you think of a good reason why "
audit_backlog_wait_overflow" exists? I'm going to replace it with the simple "audit_backlog_wait_time= 0;" unless you can think of a solid reason not to do so. It seems much more obvious and readable to me.
That goes back to ac4cec44, DWMW, July 2005. Best answer I can come up with is that it labels magic values and puts them up front at the top of the file. I'd suggest instead replacing it with a macro. I don't have an significant objection to just assigning zero where you suggest.
Quoted 1 line
paul moore
- RGB
--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
Paul Moore
maintainerRe: [RFC PATCH 1/7] audit: don't needlessly reset valid wait time
Quoted 28 lines · Richard Guy Briggs
Quoted 27 lines · Paul Moore
Quoted 15 lines · Richard Guy Briggs
After auditd has recovered from an overflowed queue, the first process that doesn't use reserves to make it through the queue checks should reset the audit backlog wait time to the configured value. After that, there is no need to keep resetting it.
Signed-off-by Richard Guy Briggs <[email protected]>--- kernel/audit.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)kernel/audit.c +0
-0kernel/audit.c struct audit_buffer *audit_log_start(struct · line 1409
Quoted 8 lines
kernel/audit.c +1
-1@@ -1403,7 +1403,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, return NULL; } -if (!reserve) +if (!reserve && !audit_backlog_wait_time) audit_backlog_wait_time = audit_backlog_wait_time_master; ab =audit_buffer_alloc (ctx, gfp_mask, type);This looks fine to me, I'm going to add it to audit#next-queue.
Quoted 8 lines
Quoted 4 lines
Also, can you think of a good reason why "
audit_backlog_wait_overflow" exists? I'm going to replace it with the simple "audit_backlog_wait_time= 0;" unless you can think of a solid reason not to do so. It seems much more obvious and readable to me.That goes back to
ac4cec44, DWMW, July 2005. Best answer I can come up with is that it labels magic values and puts them up front at the top of the file.
Yeah, I can see that from git blame, I was hoping for some thread I may have missed. Oh well, not terribly important.
Quoted 2 lines
I'd suggest instead replacing it with a macro. I don't have an significant objection to just assigning zero where you suggest.
If it weren't zero I would agree with you, magic numbers in general are a bit scary. However, in this particular case I don't consider zero to be a magic number and its use seems pretty clear given the context.
Signature · 2 lines
paul moore www.paul-moore.com
Paul Moore
maintainerRe: [RFC PATCH 3/7] audit: allow systemd to use queue reserves
In reply to Richard Guy BriggsQuoted 5 lines · Richard Guy Briggs
Quoted 4 lines · Steve Grubb
Quoted 2 lines · Richard Guy Briggs
Treat systemd the same way as auditd, allowing it to overrun the queue to avoid blocking.
Quoted 8 lines
Quoted 2 lines
Do you mind explaining this a little more? I'm having a hard time understanding how systemd is involved.
systemd should only have
CAP_AUDIT_READfor the multicast socket and otherwise behaves as a user client, sending AUDIT_USER_* messages. It starts and stops auditd and we don't want it blocking trying to allocate a buffer on the standard queue inaudit_log_start()while it is tasked with telling auditd to start or stop.
Is this something we are hearing reports about? Starting and stopping auditd should be rare in normal use, and by rare I mean start it at boot and don't touch it again ... although I suspect you might update/patch it at some point if your system is long running.
If this is a common problem we can look at doing something like this, but if it isn't - and I don't think it is - I'd like to avoid special casing init (it's even more specialized since we are basically talking about just systemd, although others could have similar problems).
Quoted 39 lines
Quoted 15 lines
-Steve
Quoted 13 lines
Signed-off-by Richard Guy Briggs <[email protected]>---
kernel/audit.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)kernel/audit.c +0
-0@@ -1375,7 +1375,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, return NULL;Quoted 4 lines
kernel/audit.c struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
Quoted 4 lines
kernel/audit.c +1
-1if (gfp_mask & __GFP_WAIT) { -if (audit_pid && audit_pid == current->tgid) + if (current->tgid == 1 || (audit_pid && audit_pid == current-tgid))
Quoted 6 lines
kernel/audit.c struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, · line 1381
Quoted 6 lines
kernel/audit.c +0
-0gfp_mask &= ~__GFP_WAIT;kernel/audit.c +0
-0else reserve =0 ;- RGB
--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit
Signature · 2 lines
paul moore www.paul-moore.com
Paul Moore
maintainerRe: [RFC PATCH 4/7] audit: wake up threads if queue switched from limited to unlimited
In reply to Richard Guy BriggsQuoted 8 lines · Richard Guy Briggs
If the
audit_backlog_limitis changed from a limited value to an unlimited value (zero) while the queue was overflowed, wake up theaudit_backlog_waitqueue to allow those processes to continue.Signed-off-by Richard Guy Briggs <[email protected]>--- kernel/audit.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
Looks like the right thing to do, merged to audit#next-queue.
Quoted 14 lines
kernel/audit.c +2
-1@@ -523,7 +523,8 @@ static int kauditd_thread(void *dummy) skb =skb_dequeue (&audit_skb_queue);if (skb) { -if (skb_queue_len (&audit_skb_queue) <= audit_backlog_limit) +if (!audit_backlog_limit || + (skb_queue_len (&audit_skb_queue) <= audit_backlog_limit))wake_up (&audit_backlog_wait);if (audit_pid)kauditd_send_skb (skb);
Signature · 2 lines
paul moore www.paul-moore.com
Paul Moore
maintainerRe: [RFC PATCH 5/7] audit: allow audit_cmd_mutex holders to use reserves
In reply to Richard Guy BriggsQuoted 9 lines · Richard Guy Briggs
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 sinceaudit_log_start()can be called in softirq context, that won't work. Next,owner_running()was used to checkaudit_cmd_mutexbut another process could have this locked on another cpu. Usercu_read_lock()andACCESS_ONCE()to checkaudit_cmd_mutex.
Quoted 4 lines
Signed-off-by Richard Guy Briggs <[email protected]>--- 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?
Quoted 22 lines
kernel/audit.c +5
-2@@ -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) {
Signature · 2 lines
paul moore www.paul-moore.com
Paul Moore
maintainerRe: [RFC PATCH 6/7] audit: wake up audit_backlog_wait queue when auditd goes away.
In reply to Richard Guy BriggsQuoted 27 lines · Richard Guy Briggs
When auditd goes away (died, killed or shutdown, or net namespace shut down), there is no point in sleeping waiting for auditd to drain the queue since that message would be distined for the hold queue after the timeout anyways. This will needlessly have those processes wait the full default timeout of 60 seconds (
audit_backlog_wait_time).Wake up the processes caught in the
audit_backlog_waitqueue when auditd is no longer present so they can be sent instead to the hold queue.Signed-off-by Richard Guy Briggs <[email protected]>--- kernel/audit.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)kernel/audit.c +1
-0@@ -425,6 +425,7 @@ restart:audit_log_lost (s); audit_pid =0 ; audit_sock =NULL ; +wake_up (&audit_backlog_wait); }else {pr_warn ("re-scheduling(#%d) write to audit_pid=%d\n" , attempts, audit_pid);
kernel/audit.c static int audit_receive_msg(struct sk_buff *skb, struct · line 890
Quoted 8 lines
kernel/audit.c +2
-0@@ -882,6 +883,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) audit_pid = new_pid; audit_nlk_portid =NETLINK_CB (skb).portid; audit_sock = skb->sk; +if (!audit_pid) +wake_up (&audit_backlog_wait); }if (s.mask &AUDIT_STATUS_RATE_LIMIT ) { err =audit_set_rate_limit (s.rate_limit);
I'm thinking it might be time for two small, static helper functions, auditd_register() and auditd_unregister() (or similar, feel free to suggest other names), that set/reset the various auditd state variables and handle the wake_up() call. We're duplicating some code that is starting to get non- trivial.
I'd also add a comment about why you are calling wake_up() in the unregister function.
Quoted 11 lines
kernel/audit.c +1
-0@@ -1154,6 +1157,7 @@ static void __net_exit audit_net_exit(struct net *net)if (sock == audit_sock) { audit_pid =0 ; audit_sock =NULL ; +wake_up (&audit_backlog_wait); }RCU_INIT_POINTER (aunet->nlsk,NULL );@@ -1393,7 +1397,7 @@ struct audit_buffer *audit_log_start(struct
audit_context*ctx,gfp_tgfp_mask,sleep_time=timeout_start+audit_backlog_wait_time- jiffies; if (sleep_time> 0) {
kernel/audit.c struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, · line 1401
Quoted 4 lines
kernel/audit.c +1
-1sleep_time =wait_for_auditd (sleep_time); -if (sleep_time >0 ) +if (audit_pid && sleep_time >0 )continue ;
Perhaps handle this in wait_for_auditd()? Right now this is the only caller, but if we use it elsewhere it seems like we would want the same logic.
Signature · 2 lines
paul moore www.paul-moore.com
Paul Moore
maintainerRe: [RFC PATCH 7/7] audit: wake up kauditd_thread after auditd registers
In reply to Richard Guy BriggsQuoted 10 lines · Richard Guy Briggs
When auditd is restarted, even though the
kauditd_threadis present, it remains dormant until the next audit log message is queued.Wake up the
kauditd_threadin thekauditd_waitqueue immediately when auditd registers its availability to drain the queue.Signed-off-by Richard Guy Briggs <[email protected]>--- kernel/audit.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
See my 6/7 comment ... this could/should go in the auditd_register() function.
Quoted 13 lines
kernel/audit.c +2
-0@@ -885,6 +885,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) audit_sock = skb->sk;if (!audit_pid)wake_up (&audit_backlog_wait); +if (audit_pid) +wake_up_interruptible (&kauditd_wait); }if (s.mask &AUDIT_STATUS_RATE_LIMIT ) { err =audit_set_rate_limit (s.rate_limit);
Signature · 2 lines
paul moore www.paul-moore.com
Re: [RFC PATCH 3/7] audit: allow systemd to use queue reserves
In reply to Paul MooreQuoted 15 lines · Paul Moore
Quoted 5 lines · Richard Guy Briggs
Quoted 4 lines · Steve Grubb
Quoted 2 lines · Richard Guy Briggs
Treat systemd the same way as auditd, allowing it to overrun the queue to avoid blocking.
Quoted 8 lines
Quoted 2 lines
Do you mind explaining this a little more? I'm having a hard time understanding how systemd is involved.
systemd should only have
CAP_AUDIT_READfor the multicast socket and otherwise behaves as a user client, sending AUDIT_USER_* messages. It starts and stops auditd and we don't want it blocking trying to allocate a buffer on the standard queue inaudit_log_start()while it is tasked with telling auditd to start or stop.
Quoted 4 lines
Is this something we are hearing reports about? Starting and stopping auditd should be rare in normal use, and by rare I mean start it at boot and don't touch it again ... although I suspect you might update/patch it at some point if your system is long running.
(Sorry, this message has been on my queue for a while...)
Well, it is touched again, when shutting down the machine, or upgrading auditd. There also seem to be issues when the logs are rotated. This first case is the one I've been working on that caused all this examination and instrumentation. systemd was chucking in a couple dozen messages onto the queue on startup
Quoted 28 lines
If this is a common problem we can look at doing something like this, but if it isn't - and I don't think it is - I'd like to avoid special casing init (it's even more specialized since we are basically talking about just systemd, although others could have similar prblems).
Quoted 23 lines
Quoted 15 lines
-Steve
Quoted 13 lines
Signed-off-by Richard Guy Briggs <[email protected]>---
kernel/audit.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)kernel/audit.c +0
-0@@ -1375,7 +1375,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, return NULL;Quoted 4 lines
kernel/audit.c struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
Quoted 4 lines
kernel/audit.c +1
-1if (gfp_mask & __GFP_WAIT) { -if (audit_pid && audit_pid == current->tgid) + if (current->tgid == 1 || (audit_pid && audit_pid == current-tgid))
Quoted 3 lines
Quoted 3 lines
kernel/audit.c +0
-0gfp_mask &= ~__GFP_WAIT;
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
kernel/audit.c struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, · line 1381
Quoted 3 lines
kernel/audit.c +0
-0else reserve =0 ;- RGB
paul moore
- RGB
--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545