1:53 p.m.
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(-)
1:53 p.m.
New subject: [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.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index a72ad37..daefd81 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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);
--
1.7.1
5:03 p.m.
New subject: [RFC PATCH 1/7] audit: don't needlessly reset valid wait time
On Thursday, October 22, 2015 02:53:14 PM Richard Guy Briggs wrote:
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 <rgb(a)redhat.com>
---
kernel/audit.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index a72ad37..daefd81 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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.
--
paul moore
www.paul-moore.com
9:13 p.m.
New subject: [RFC PATCH 1/7] audit: don't needlessly reset valid wait time
On 15/11/04, Paul Moore wrote:
On Thursday, October 22, 2015 02:53:14 PM Richard Guy Briggs wrote:
> 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 <rgb(a)redhat.com>
> ---
> kernel/audit.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index a72ad37..daefd81 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -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.
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.
paul moore
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
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
9:17 a.m.
New subject: [RFC PATCH 1/7] audit: don't needlessly reset valid wait time
On Wed, Nov 4, 2015 at 10:13 PM, Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 15/11/04, Paul Moore wrote:
> On Thursday, October 22, 2015 02:53:14 PM Richard Guy Briggs wrote:
> > 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 <rgb(a)redhat.com>
> > ---
> > kernel/audit.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index a72ad37..daefd81 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -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.
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.
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.
--
paul moore
www.paul-moore.com
1:53 p.m.
New subject: [RFC PATCH 2/7] audit: include auditd's threads in audit_log_start() wait exception
Should auditd spawn threads, allow all members of its thread group to
use the audit_backlog_limit reserves to bypass the queue limits too.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index daefd81..3917aad 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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;
--
1.7.1
5:08 p.m.
New subject: [RFC PATCH 2/7] audit: include auditd's threads in audit_log_start() wait exception
On Thursday, October 22, 2015 02:53:15 PM Richard Guy Briggs wrote:
Should auditd spawn threads, allow all members of its thread group
to
use the audit_backlog_limit reserves to bypass the queue limits too.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
Looks good, applied to audit#next-queue.
diff --git a/kernel/audit.c b/kernel/audit.c
index daefd81..3917aad 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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;
--
paul moore
www.paul-moore.com
1:53 p.m.
New subject: [RFC PATCH 3/7] audit: allow systemd to use queue reserves
Treat systemd the same way as auditd, allowing it to overrun the queue to avoid
blocking.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 3917aad..384a1a1 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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;
--
1.7.1
2:26 p.m.
New subject: [RFC PATCH 3/7] audit: allow systemd to use queue reserves
On Thursday, October 22, 2015 02:53:16 PM Richard Guy Briggs wrote:
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
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 3917aad..384a1a1 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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;
2:51 p.m.
New subject: [RFC PATCH 3/7] audit: allow systemd to use queue reserves
On 15/10/22, Steve Grubb wrote:
On Thursday, October 22, 2015 02:53:16 PM Richard Guy Briggs wrote:
> 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.
-Steve
> Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> ---
> kernel/audit.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 3917aad..384a1a1 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -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;
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
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
4:38 p.m.
New subject: [RFC PATCH 3/7] audit: allow systemd to use queue reserves
On Thursday, October 22, 2015 03:51:59 PM Richard Guy Briggs wrote:
On 15/10/22, Steve Grubb wrote:
> On Thursday, October 22, 2015 02:53:16 PM Richard Guy Briggs wrote:
> > 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.
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).
> -Steve
>
> > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> > ---
> >
> > kernel/audit.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 3917aad..384a1a1 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -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;
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
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
Linux-audit(a)redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
12:35 p.m.
New subject: [RFC PATCH 3/7] audit: allow systemd to use queue reserves
On 15/11/05, Paul Moore wrote:
On Thursday, October 22, 2015 03:51:59 PM Richard Guy Briggs wrote:
> On 15/10/22, Steve Grubb wrote:
> > On Thursday, October 22, 2015 02:53:16 PM Richard Guy Briggs wrote:
> > > 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.
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
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).
> > -Steve
> >
> > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> > > ---
> >
> > > kernel/audit.c | 2 +-
> > > 1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index 3917aad..384a1a1 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -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;
>
> - RGB
paul moore
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
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
1:53 p.m.
New subject: [RFC PATCH 4/7] audit: wake up threads if queue switched from limited to unlimited
If 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.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 384a1a1..02a5ec0 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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);
--
1.7.1
6:05 p.m.
New subject: [RFC PATCH 4/7] audit: wake up threads if queue switched from limited to unlimited
On Thursday, October 22, 2015 02:53:17 PM Richard Guy Briggs wrote:
If 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.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
Looks like the right thing to do, merged to audit#next-queue.
diff --git a/kernel/audit.c b/kernel/audit.c
index 384a1a1..02a5ec0 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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);
--
paul moore
www.paul-moore.com
1:53 p.m.
New subject: [RFC PATCH 5/7] audit: allow audit_cmd_mutex holders to use reserves
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 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.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 02a5ec0..34411af 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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) {
--
1.7.1
6:48 p.m.
New subject: [RFC PATCH 5/7] audit: allow audit_cmd_mutex holders to use reserves
On Thursday, October 22, 2015 02:53:18 PM Richard Guy Briggs wrote:
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 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.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
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?
diff --git a/kernel/audit.c b/kernel/audit.c
index 02a5ec0..34411af 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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) {
--
paul moore
www.paul-moore.com
1:53 p.m.
New subject: [RFC PATCH 6/7] audit: wake up audit_backlog_wait queue when auditd goes away.
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_wait queue when auditd
is no longer present so they can be sent instead to the hold queue.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 34411af..688fa1e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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;
}
}
--
1.7.1
7:21 p.m.
New subject: [RFC PATCH 6/7] audit: wake up audit_backlog_wait queue when auditd goes away.
On Thursday, October 22, 2015 02:53:19 PM Richard Guy Briggs wrote:
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_wait queue when auditd
is no longer present so they can be sent instead to the hold queue.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 34411af..688fa1e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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);
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.
@@ -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;
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.
--
paul moore
www.paul-moore.com
1:53 p.m.
New subject: [RFC PATCH 7/7] audit: wake up kauditd_thread after auditd registers
When 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.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 688fa1e..369cfcc 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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);
--
1.7.1
7:23 p.m.
New subject: [RFC PATCH 7/7] audit: wake up kauditd_thread after auditd registers
On Thursday, October 22, 2015 02:53:20 PM Richard Guy Briggs wrote:
When 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.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
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.
diff --git a/kernel/audit.c b/kernel/audit.c
index 688fa1e..369cfcc 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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);
--
paul moore
www.paul-moore.com
1:44 p.m.
On Thursday, October 22, 2015 02:53:13 PM Richard Guy Briggs wrote:
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(-)
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.
--
paul moore
security @ redhat
1:43 p.m.
On 15/10/27, Paul Moore wrote:
On Thursday, October 22, 2015 02:53:13 PM Richard Guy Briggs wrote:
> 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(-)
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. :)
paul moore
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
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
1:58 p.m.
On Wednesday, October 28, 2015 02:43:18 PM Richard Guy Briggs wrote:
On 15/10/27, Paul Moore wrote:
> On Thursday, October 22, 2015 02:53:13 PM Richard Guy Briggs wrote:
> > 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(-)
>
> 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).
--
paul moore
security @ redhat
3111
days inactive
3348
days old
linux-audit@lists.linux-audit.osci.io
22 comments
4 participants
participants (4)
-
Paul Moore -
Paul Moore -
Richard Guy Briggs -
Steve Grubb