6:34 p.m.
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(a)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
6:34 p.m.
New subject: [PATCH 2/2] audit: don't reset working wait time accidentally with auditd
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(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 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
5:16 p.m.
New subject: [PATCH 2/2] audit: don't reset working wait time accidentally with auditd
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(a)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
3:10 p.m.
New subject: [PATCH 2/2] audit: don't reset working wait time accidentally with auditd
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(a)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(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
3:16 p.m.
New subject: [PATCH 2/2] audit: don't reset working wait time accidentally with auditd
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(a)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
5:11 p.m.
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(a)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
3:03 p.m.
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(a)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(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
3619
days inactive
3624
days old
linux-audit@lists.linux-audit.osci.io
6 comments
2 participants
participants (2)
-
Paul Moore -
Richard Guy Briggs