[PATCH 1/2] audit: don't lose set wait time on first successful call to audit_log_start()

[PATCH 1/2] audit: don't lose set wait time on first successful call to audit_log_start()

[Richard Guy Briggs]

Copy the set wait time to a working value to avoid losing the set value if the
queue overflows.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/audit.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 53bb39b..b333f03 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -107,6 +107,7 @@ static u32	audit_rate_limit;
  * When set to zero, this means unlimited. */
 static u32	audit_backlog_limit = 64;
 #define AUDIT_BACKLOG_WAIT_TIME (60 * HZ)
+static u32	audit_backlog_wait_time_master = AUDIT_BACKLOG_WAIT_TIME;
 static u32	audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
 static u32	audit_backlog_wait_overflow = 0;
 
@@ -338,7 +339,7 @@ static int audit_set_backlog_limit(u32 limit)
 static int audit_set_backlog_wait_time(u32 timeout)
 {
 	return audit_do_config_change("audit_backlog_wait_time",
-				      &audit_backlog_wait_time, timeout);
+				      &audit_backlog_wait_time_master, timeout);
 }
 
 static int audit_set_enabled(u32 state)
@@ -843,7 +844,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		s.lost			= atomic_read(&audit_lost);
 		s.backlog		= skb_queue_len(&audit_skb_queue);
 		s.version		= AUDIT_VERSION_LATEST;
-		s.backlog_wait_time	= audit_backlog_wait_time;
+		s.backlog_wait_time	= audit_backlog_wait_time_master;
 		audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &s, sizeof(s));
 		break;
 	}
@@ -1394,7 +1395,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
 		return NULL;
 	}
 
-	audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
+	audit_backlog_wait_time = audit_backlog_wait_time_master;
 
 	ab = audit_buffer_alloc(ctx, gfp_mask, type);
 	if (!ab) {
-- 
1.7.1

[PATCH 2/2] audit: don't reset working wait time accidentally with auditd

[Richard Guy Briggs]

During a queue overflow condition while we are waiting for auditd to drain the
queue to make room for regular messages, we don't want a successful auditd that
has bypassed the queue check to reset the backlog wait time.

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 b333f03..73293ea 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1395,7 +1395,8 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
 		return NULL;
 	}
 
-	audit_backlog_wait_time = audit_backlog_wait_time_master;
+	if (!reserve)
+		audit_backlog_wait_time = audit_backlog_wait_time_master;
 
 	ab = audit_buffer_alloc(ctx, gfp_mask, type);
 	if (!ab) {
-- 
1.7.1

Re: [PATCH 1/2] audit: don't lose set wait time on first successful call to audit_log_start()

[Paul Moore]

On Tuesday, January 27, 2015 07:34:01 PM Richard Guy Briggs wrote:
> Copy the set wait time to a working value to avoid losing the set value if
> the queue overflows.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/audit.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)

Just so I'm understanding this patch correctly, you create a the new 
audit_backlog_wait_time_master because the existing audit_backlog_wait_time 
can be overwritten by the code in audit_log_start() when the audit record 
backlog overflows (it is set to audit_backlog_wait_overflow), yes?

Further, if the queue overflows the audit_backlog_wait_time will remain set to 
audit_backlog_wait_overflow until the queue is drained, yes?  Is that what we 
want?

> diff --git a/kernel/audit.c b/kernel/audit.c
> index 53bb39b..b333f03 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -107,6 +107,7 @@ static u32	audit_rate_limit;
>   * When set to zero, this means unlimited. */
>  static u32	audit_backlog_limit = 64;
>  #define AUDIT_BACKLOG_WAIT_TIME (60 * HZ)
> +static u32	audit_backlog_wait_time_master = AUDIT_BACKLOG_WAIT_TIME;
>  static u32	audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
>  static u32	audit_backlog_wait_overflow = 0;
> 
> @@ -338,7 +339,7 @@ static int audit_set_backlog_limit(u32 limit)
>  static int audit_set_backlog_wait_time(u32 timeout)
>  {
>  	return audit_do_config_change("audit_backlog_wait_time",
> -				      &audit_backlog_wait_time, timeout);
> +				      &audit_backlog_wait_time_master, timeout);
>  }
> 
>  static int audit_set_enabled(u32 state)
> @@ -843,7 +844,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct
> nlmsghdr *nlh) s.lost			= atomic_read(&audit_lost);
>  		s.backlog		= skb_queue_len(&audit_skb_queue);
>  		s.version		= AUDIT_VERSION_LATEST;
> -		s.backlog_wait_time	= audit_backlog_wait_time;
> +		s.backlog_wait_time	= audit_backlog_wait_time_master;
>  		audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &s, sizeof(s));
>  		break;
>  	}
> @@ -1394,7 +1395,7 @@ struct audit_buffer *audit_log_start(struct
> audit_context *ctx, gfp_t gfp_mask, return NULL;
>  	}
> 
> -	audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
> +	audit_backlog_wait_time = audit_backlog_wait_time_master;
> 
>  	ab = audit_buffer_alloc(ctx, gfp_mask, type);
>  	if (!ab) {

-- 
paul moore
security @ redhat

Re: [PATCH 2/2] audit: don't reset working wait time accidentally with auditd

[Paul Moore]

On Tuesday, January 27, 2015 07:34:02 PM Richard Guy Briggs wrote:
> During a queue overflow condition while we are waiting for auditd to drain
> the queue to make room for regular messages, we don't want a successful
> auditd that has bypassed the queue check to reset the backlog wait time.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/audit.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)

I'm still wondering why we ever change audit_backlog_wait_time, it is only so 
we don't end up calling wait_for_auditd() multiple times while we are waiting 
for the queue to drain?

As a general comment, not directed at anyone in particular, the audit 
backlog/queue handling looks a little odd ...

> diff --git a/kernel/audit.c b/kernel/audit.c
> index b333f03..73293ea 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1395,7 +1395,8 @@ struct audit_buffer *audit_log_start(struct
> audit_context *ctx, gfp_t gfp_mask, return NULL;
>  	}
> 
> -	audit_backlog_wait_time = audit_backlog_wait_time_master;
> +	if (!reserve)
> +		audit_backlog_wait_time = audit_backlog_wait_time_master;
> 
>  	ab = audit_buffer_alloc(ctx, gfp_mask, type);
>  	if (!ab) {

-- 
paul moore
security @ redhat

Re: [PATCH 1/2] audit: don't lose set wait time on first successful call to audit_log_start()

[Richard Guy Briggs]

On 15/01/29, Paul Moore wrote:
> On Tuesday, January 27, 2015 07:34:01 PM Richard Guy Briggs wrote:
> > Copy the set wait time to a working value to avoid losing the set value if
> > the queue overflows.
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  kernel/audit.c |    7 ++++---
> >  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> Just so I'm understanding this patch correctly, you create a the new 
> audit_backlog_wait_time_master because the existing audit_backlog_wait_time 
> can be overwritten by the code in audit_log_start() when the audit record 
> backlog overflows (it is set to audit_backlog_wait_overflow), yes?

Correct.

> Further, if the queue overflows the audit_backlog_wait_time will remain set to 
> audit_backlog_wait_overflow until the queue is drained, yes?  Is that what we 
> want?

Drained sufficiently to be able to allocate audit log buffers to regular
processes, yes.

This was the intent and original functioning until the logic was
disrupted by the "negative sleep durations" fix in commit 8291991.

Several attempts were made to fix it since (e789e56, ae887e0, 51cc83f, c81825d).

> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 53bb39b..b333f03 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -107,6 +107,7 @@ static u32	audit_rate_limit;
> >   * When set to zero, this means unlimited. */
> >  static u32	audit_backlog_limit = 64;
> >  #define AUDIT_BACKLOG_WAIT_TIME (60 * HZ)
> > +static u32	audit_backlog_wait_time_master = AUDIT_BACKLOG_WAIT_TIME;
> >  static u32	audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
> >  static u32	audit_backlog_wait_overflow = 0;
> > 
> > @@ -338,7 +339,7 @@ static int audit_set_backlog_limit(u32 limit)
> >  static int audit_set_backlog_wait_time(u32 timeout)
> >  {
> >  	return audit_do_config_change("audit_backlog_wait_time",
> > -				      &audit_backlog_wait_time, timeout);
> > +				      &audit_backlog_wait_time_master, timeout);
> >  }
> > 
> >  static int audit_set_enabled(u32 state)
> > @@ -843,7 +844,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct
> > nlmsghdr *nlh) s.lost			= atomic_read(&audit_lost);
> >  		s.backlog		= skb_queue_len(&audit_skb_queue);
> >  		s.version		= AUDIT_VERSION_LATEST;
> > -		s.backlog_wait_time	= audit_backlog_wait_time;
> > +		s.backlog_wait_time	= audit_backlog_wait_time_master;
> >  		audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &s, sizeof(s));
> >  		break;
> >  	}
> > @@ -1394,7 +1395,7 @@ struct audit_buffer *audit_log_start(struct
> > audit_context *ctx, gfp_t gfp_mask, return NULL;
> >  	}
> > 
> > -	audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
> > +	audit_backlog_wait_time = audit_backlog_wait_time_master;
> > 
> >  	ab = audit_buffer_alloc(ctx, gfp_mask, type);
> >  	if (!ab) {
> 
> -- 
> paul moore
> security @ redhat
> 

- 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: [PATCH 2/2] audit: don't reset working wait time accidentally with auditd

[Richard Guy Briggs]

On 15/01/29, Paul Moore wrote:
> On Tuesday, January 27, 2015 07:34:02 PM Richard Guy Briggs wrote:
> > During a queue overflow condition while we are waiting for auditd to drain
> > the queue to make room for regular messages, we don't want a successful
> > auditd that has bypassed the queue check to reset the backlog wait time.
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  kernel/audit.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> I'm still wondering why we ever change audit_backlog_wait_time, it is only so 
> we don't end up calling wait_for_auditd() multiple times while we are waiting 
> for the queue to drain?

Not exactly.  Up to the timeout, all subsequent callers will wait for
auditd as well.  It is so that if wait_for_auditd() does time out, we
don't make new callers after that timeout wait, but return an error
immediately.  If/when auditd does manage to succeed and recover after
that wait time, it will reset the wait time and resume normal operation.

> As a general comment, not directed at anyone in particular, the audit 
> backlog/queue handling looks a little odd ...

Indeed...

> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index b333f03..73293ea 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1395,7 +1395,8 @@ struct audit_buffer *audit_log_start(struct
> > audit_context *ctx, gfp_t gfp_mask, return NULL;
> >  	}
> > 
> > -	audit_backlog_wait_time = audit_backlog_wait_time_master;
> > +	if (!reserve)
> > +		audit_backlog_wait_time = audit_backlog_wait_time_master;
> > 
> >  	ab = audit_buffer_alloc(ctx, gfp_mask, type);
> >  	if (!ab) {
> 
> -- 
> 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: [PATCH 2/2] audit: don't reset working wait time accidentally with auditd

[Paul Moore]

On Friday, January 30, 2015 04:10:44 PM Richard Guy Briggs wrote:
> On 15/01/29, Paul Moore wrote:
> > On Tuesday, January 27, 2015 07:34:02 PM Richard Guy Briggs wrote:
> > > During a queue overflow condition while we are waiting for auditd to
> > > drain
> > > the queue to make room for regular messages, we don't want a successful
> > > auditd that has bypassed the queue check to reset the backlog wait time.
> > > 
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > > 
> > >  kernel/audit.c |    3 ++-
> > >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > I'm still wondering why we ever change audit_backlog_wait_time, it is only
> > so we don't end up calling wait_for_auditd() multiple times while we are
> > waiting for the queue to drain?
> 
> Not exactly.  Up to the timeout, all subsequent callers will wait for
> auditd as well.  It is so that if wait_for_auditd() does time out, we
> don't make new callers after that timeout wait, but return an error
> immediately.  If/when auditd does manage to succeed and recover after
> that wait time, it will reset the wait time and resume normal operation.

Okay, thanks for the clarification on both patches.  If I have one nit, it 
would be that you could have merged both patches into a single patch; just 
something to remember for future submissions.

Like the tree pruning thread patch, I'm going to queue this up for after this 
upcoming merge window.

> > As a general comment, not directed at anyone in particular, the audit
> > backlog/queue handling looks a little odd ...
> 
> Indeed...

I suspect we'll need to look closer at this code in the future, or rather I'll 
*want* to look closer at this in the future but for right now I think we've 
got enough to deal with.

-- 
paul moore
security @ redhat