[RFC PATCH 0/7] audit: clean up audit queue handling

[RFC PATCH 0/7] audit: clean up audit queue handling

[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 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 0/7] audit: clean up audit queue handling

[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 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

[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 <rgb@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

[RFC PATCH 1/7] audit: don't needlessly reset valid wait time

[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 <rgb@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

Re: [RFC PATCH 1/7] audit: don't needlessly reset valid wait time

[Paul Moore]

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@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

Re: [RFC PATCH 1/7] audit: don't needlessly reset valid wait time

[Richard Guy Briggs]

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@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@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

Re: [RFC PATCH 1/7] audit: don't needlessly reset valid wait time

[Paul Moore]

On Wed, Nov 4, 2015 at 10:13 PM, Richard Guy Briggs <rgb@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@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

[RFC PATCH 2/7] audit: include auditd's threads in audit_log_start() wait exception

[Richard Guy Briggs]

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@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

Re: [RFC PATCH 2/7] audit: include auditd's threads in audit_log_start() wait exception

[Paul Moore]

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@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

[RFC PATCH 3/7] audit: allow systemd to use queue reserves

[Richard Guy Briggs]

Treat systemd the same way as auditd, allowing it to overrun the queue to avoid
blocking.

Signed-off-by: Richard Guy Briggs <rgb@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

[RFC PATCH 3/7] audit: allow systemd to use queue reserves

[Richard Guy Briggs]

Treat systemd the same way as auditd, allowing it to overrun the queue to avoid
blocking.

Signed-off-by: Richard Guy Briggs <rgb@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

Re: [RFC PATCH 3/7] audit: allow systemd to use queue reserves

[Steve Grubb]

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@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;

Re: [RFC PATCH 3/7] audit: allow systemd to use queue reserves

[Richard Guy Briggs]

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@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@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

Re: [RFC PATCH 3/7] audit: allow systemd to use queue reserves

[Paul Moore]

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@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@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@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 3/7] audit: allow systemd to use queue reserves

[Richard Guy Briggs]

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@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@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

[RFC PATCH 4/7] audit: wake up threads if queue switched from limited to unlimited

[Richard Guy Briggs]

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@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

[RFC PATCH 4/7] audit: wake up threads if queue switched from limited to unlimited

[Richard Guy Briggs]

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@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

Re: [RFC PATCH 4/7] audit: wake up threads if queue switched from limited to unlimited

[Paul Moore]

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@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

[RFC PATCH 5/7] audit: allow audit_cmd_mutex holders to use reserves

[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 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@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

[RFC PATCH 5/7] audit: allow audit_cmd_mutex holders to use reserves

[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 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@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

Re: [RFC PATCH 5/7] audit: allow audit_cmd_mutex holders to use reserves

[Paul Moore]

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@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

[RFC PATCH 6/7] audit: wake up audit_backlog_wait queue when auditd goes away.

[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_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@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

[RFC PATCH 6/7] audit: wake up audit_backlog_wait queue when auditd goes away.

[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_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@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

Re: [RFC PATCH 6/7] audit: wake up audit_backlog_wait queue when auditd goes away.

[Paul Moore]

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@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

[RFC PATCH 7/7] audit: wake up kauditd_thread after auditd registers

[Richard Guy Briggs]

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@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

Re: [RFC PATCH 7/7] audit: wake up kauditd_thread after auditd registers

[Paul Moore]

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@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

Re: [RFC PATCH 0/7] audit: clean up audit queue handling

[Paul Moore]

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

Re: [RFC PATCH 0/7] audit: clean up audit queue handling

[Richard Guy Briggs]

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@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

Re: [RFC PATCH 0/7] audit: clean up audit queue handling

[Paul Moore]

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

Re: [RFC PATCH 0/7] audit: clean up audit queue handling

[Paul Moore]

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