Discussion:
[PATCH 0/8] Audit backlog queue fixes related to soft lockup
(too old to reply)
Richard Guy Briggs
2013-09-18 19:06:45 UTC
Permalink
This patchset covers a number of issues surrounding the audit backlog queue.
The original trigger was a bug in a patch to fix code that produced negative
sleep times (commit 82919919). This bug caused soft lockups. There were a
number of other issues raised surrounding this bug which have been attempted
to be addressed in this patchset.

The first patch fixes this obvious bug.

The 2nd fixes the lack of reset to the original wait time that was set to
zero under error conditions should the system recover from the conditions that
tickled the first bug.

The 3rd makes use of the remaining timeout to avoid re-checking the loop
conditions on error.

The 4th and 5th are efficiency fixes for waking up waiting processes only when
necessary.

The 6th adds a boot parameter to temporarily override the backlog queue length
in case more buffers are needed before auditd is available.

The 7th and 8th are to add a config option to make the backlog wait time
configurable from the hard-coded default.


Richard Guy Briggs (8):
audit: avoid soft lockup due to audit_log_start() incorrect loop
termination
audit: reset audit backlog wait time after error recovery
audit: make use of remaining sleep time from wait_for_auditd
audit: efficiency fix 1: only wake up if queue shorter than backlog
limit
audit: efficiency fix 2: request exclusive wait since all need same
resource
audit: add boot option to override default backlog limit
audit: clean up AUDIT_GET/SET local variables and future-proof API
audit: add audit_backlog_wait_time configuration option

include/uapi/linux/audit.h | 2 +
kernel/audit.c | 111 +++++++++++++++++++++++++++++++-------------
2 files changed, 80 insertions(+), 33 deletions(-)
Richard Guy Briggs
2013-09-18 19:06:46 UTC
Permalink
Commit 82919919 caused the wait for auditd timeout condition to loop endlessly
rather than fall through to the error recovery code.

BUG: soft lockup - CPU#0 stuck for 22s! [preload:785]
RIP: 0010:[<ffffffff810fb240>] [<ffffffff810fb240>] audit_log_start+0xf0/0x460
Call Trace:
[<ffffffff810aca40>] ? try_to_wake_up+0x310/0x310
[<ffffffff81100fd1>] audit_log_exit+0x51/0xcb0
[<ffffffff811020b5>] __audit_syscall_exit+0x275/0x2d0
[<ffffffff816ec540>] sysret_audit+0x17/0x21

If the timeout condition goes negative, the loop continues endlessly instead of
breaking out and going to the failure code and allow other processes to run
when auditd is unable to drain the queue fast enough.

This can easily be triggered by readahead-collector, in particular during
installations. The readahead-collector (ab)uses the audit interface and
sometimes gets stuck in a 'stopped' state.

To trigger:
readahead-collector -f &
pkill -SIGSTOP readahead-collector
top

See:
https://lkml.org/lkml/2013/8/28/626
https://lkml.org/lkml/2013/9/2/471
https://lkml.org/lkml/2013/9/3/4

Signed-off-by: Luiz Capitulino <***@redhat.com>
Signed-off-by: Konstantin Khlebnikov <***@openvz.org>
Signed-off-by: Dan Duval <***@oracle.com>
Signed-off-by: Chuck Anderson <***@oracle.com>
Signed-off-by: Richard Guy Briggs <***@redhat.com>
---
kernel/audit.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 91e53d0..7b0e23a 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1117,9 +1117,10 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,

sleep_time = timeout_start + audit_backlog_wait_time -
jiffies;
- if ((long)sleep_time > 0)
+ if ((long)sleep_time > 0) {
wait_for_auditd(sleep_time);
- continue;
+ continue;
+ }
}
if (audit_rate_check() && printk_ratelimit())
printk(KERN_WARNING
--
1.7.1
Richard Guy Briggs
2013-09-18 19:06:47 UTC
Permalink
When the audit queue overflows and times out (audit_backlog_wait_time), the
audit queue overflow timeout is set to zero. Once the audit queue overflow
timeout condition recovers, the timeout should be reset to the original value.

See also:
https://lkml.org/lkml/2013/9/2/473

Signed-off-by: Luiz Capitulino <***@redhat.com>
Signed-off-by: Dan Duval <***@oracle.com>
Signed-off-by: Chuck Anderson <***@oracle.com>
Signed-off-by: Richard Guy Briggs <***@redhat.com>
---
kernel/audit.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 7b0e23a..772725e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -103,7 +103,8 @@ static int audit_rate_limit;

/* Number of outstanding audit_buffers allowed. */
static int audit_backlog_limit = 64;
-static int audit_backlog_wait_time = 60 * HZ;
+#define AUDIT_BACKLOG_WAIT_TIME (60 * HZ)
+static int audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
static int audit_backlog_wait_overflow = 0;

/* The identity of the user shutting down the audit system. */
@@ -1134,6 +1135,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;
+
ab = audit_buffer_alloc(ctx, gfp_mask, type);
if (!ab) {
audit_log_lost("out of memory in audit_log_start");
--
1.7.1
Richard Guy Briggs
2013-09-18 19:06:48 UTC
Permalink
If wait_for_auditd() times out, go immediately to the error function rather
than retesting the loop conditions.

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

diff --git a/kernel/audit.c b/kernel/audit.c
index 772725e..42c68db 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1054,18 +1054,21 @@ static inline void audit_get_stamp(struct audit_context *ctx,
/*
* Wait for auditd to drain the queue a little
*/
-static void wait_for_auditd(unsigned long sleep_time)
+static unsigned long wait_for_auditd(unsigned long sleep_time)
{
+ unsigned long timeout = sleep_time;
DECLARE_WAITQUEUE(wait, current);
set_current_state(TASK_UNINTERRUPTIBLE);
add_wait_queue(&audit_backlog_wait, &wait);

if (audit_backlog_limit &&
skb_queue_len(&audit_skb_queue) > audit_backlog_limit)
- schedule_timeout(sleep_time);
+ timeout = schedule_timeout(sleep_time);

__set_current_state(TASK_RUNNING);
remove_wait_queue(&audit_backlog_wait, &wait);
+
+ return timeout;
}

/* Obtain an audit buffer. This routine does locking to obtain the
@@ -1119,8 +1122,9 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
sleep_time = timeout_start + audit_backlog_wait_time -
jiffies;
if ((long)sleep_time > 0) {
- wait_for_auditd(sleep_time);
- continue;
+ sleep_time = wait_for_auditd(sleep_time);
+ if ((long)sleep_time > 0)
+ continue;
}
}
if (audit_rate_check() && printk_ratelimit())
--
1.7.1
Richard Guy Briggs
2013-09-18 19:06:49 UTC
Permalink
author: Dan Duval <***@oracle.com>

These and similar errors were seen on a patched 3.8 kernel when the
audit subsystem was overrun during boot:

udevd[876]: worker [887] unexpectedly returned with status 0x0100
udevd[876]: worker [887] failed while handling
'/devices/pci0000:00/0000:00:03.0/0000:40:00.0'
udevd[876]: worker [880] unexpectedly returned with status 0x0100
udevd[876]: worker [880] failed while handling
'/devices/LNXSYSTM:00/LNXPWRBN:00/input/input1/event1'

udevadm settle - timeout of 180 seconds reached, the event queue
contains:
/sys/devices/LNXSYSTM:00/LNXPWRBN:00/input/input1/event1 (3995)
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT3F0D:00 (4034)

audit: audit_backlog=258 > audit_backlog_limit=256
audit: audit_lost=1 audit_rate_limit=0 audit_backlog_limit=256

The change below increases the efficiency of the audit code and prevents it
from being overrun:

Only issue a wake_up in kauditd if the length of the skb queue is less than the
backlog limit. Otherwise, threads waiting in wait_for_auditd() will simply
wake up, discover that the queue is still too long for them to proceed, and go
back to sleep. This results in wasted context switches and machine cycles.
kauditd_thread() is the only function that removes buffers from audit_skb_queue
so we can't race. If we did, the timeout in wait_for_auditd() would expire and
the waiting thread would continue.

See: https://lkml.org/lkml/2013/9/2/479

Signed-off-by: Dan Duval <***@oracle.com>
Signed-off-by: Chuck Anderson <***@oracle.com>
Signed-off-by: Richard Guy Briggs <***@redhat.com>
---
kernel/audit.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 42c68db..25fab2d 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -448,8 +448,10 @@ static int kauditd_thread(void *dummy)
flush_hold_queue();

skb = skb_dequeue(&audit_skb_queue);
- wake_up(&audit_backlog_wait);
+
if (skb) {
+ if(skb_queue_len(&audit_skb_queue) <= audit_backlog_limit)
+ wake_up(&audit_backlog_wait);
if (audit_pid)
kauditd_send_skb(skb);
else
--
1.7.1
Richard Guy Briggs
2013-09-18 19:06:50 UTC
Permalink
author: Dan Duval <***@oracle.com>

These and similar errors were seen on a patched 3.8 kernel when the
audit subsystem was overrun during boot:

udevd[876]: worker [887] unexpectedly returned with status 0x0100
udevd[876]: worker [887] failed while handling
'/devices/pci0000:00/0000:00:03.0/0000:40:00.0'
udevd[876]: worker [880] unexpectedly returned with status 0x0100
udevd[876]: worker [880] failed while handling
'/devices/LNXSYSTM:00/LNXPWRBN:00/input/input1/event1'

udevadm settle - timeout of 180 seconds reached, the event queue
contains:
/sys/devices/LNXSYSTM:00/LNXPWRBN:00/input/input1/event1 (3995)
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT3F0D:00 (4034)

audit: audit_backlog=258 > audit_backlog_limit=256
audit: audit_lost=1 audit_rate_limit=0 audit_backlog_limit=256

The change below increases the efficiency of the audit code and prevents it
from being overrun:

Use add_wait_queue_exclusive() in wait_for_auditd() to put the
thread on the wait queue. When kauditd dequeues an skb, all
of the waiting threads are waiting for the same resource, but
only one is going to get it, so there's no need to wake up
more than one waiter.

See: https://lkml.org/lkml/2013/9/2/479

Signed-off-by: Dan Duval <***@oracle.com>
Signed-off-by: Chuck Anderson <***@oracle.com>
Signed-off-by: Richard Guy Briggs <***@redhat.com>
---
kernel/audit.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 25fab2d..990d02f 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1061,7 +1061,7 @@ static unsigned long wait_for_auditd(unsigned long sleep_time)
unsigned long timeout = sleep_time;
DECLARE_WAITQUEUE(wait, current);
set_current_state(TASK_UNINTERRUPTIBLE);
- add_wait_queue(&audit_backlog_wait, &wait);
+ add_wait_queue_exclusive(&audit_backlog_wait, &wait);

if (audit_backlog_limit &&
skb_queue_len(&audit_skb_queue) > audit_backlog_limit)
--
1.7.1
Richard Guy Briggs
2013-09-18 19:06:51 UTC
Permalink
The default audit_backlog_limit is 64. This was a reasonable limit at one time.

systemd causes so much audit queue activity on startup that auditd doesn't
start before the backlog queue has already overflowed by more than a factor of
2. On a system with audit= not set on the kernel command line, this isn't an
issue since that history isn't kept for auditd when it is available. On a
system with audit=1 set on the kernel command line, kaudit tries to keep that
history until auditd is able to drain the queue.

This default can be changed by the "-b" option in audit.rules once the system
has booted, but won't help with lost messages on boot.

One way to solve this would be to increase the default backlog queue size to
avoid losing any messages before auditd is able to consume them. This would
be overkill to the embedded community and insufficient for some servers.

Another way to solve it might be to add a kconfig option to set the default
based on the system type. An embedded system would get the current (or
smaller) default, while Workstations might get more than now and servers might
get more.

None of these solutions helps if a system's compiled default is too small to
see the lost messages without compiling a new kernel.

This patch adds a boot option (audit already has one to enable/disable it)
"audit_backlog_limit=<n>" that overrides the default to allow the system
administrator to set the backlog limit.

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

diff --git a/kernel/audit.c b/kernel/audit.c
index 990d02f..acfa7a9 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -944,9 +944,21 @@ static int __init audit_enable(char *str)

return 1;
}
-
__setup("audit=", audit_enable);

+/* Process kernel command-line parameter at boot time. audit_backlog_limit=<n> */
+static int __init audit_backlog_limit_set(char *str)
+{
+ int audit_backlog_limit_arg = simple_strtol(str, NULL, 0);
+ if ((audit_backlog_limit_arg >= 0) && (audit_backlog_limit_arg < 8192))
+ audit_backlog_limit = audit_backlog_limit_arg;
+
+ printk(KERN_INFO "audit_backlog_limit: %d\n", audit_backlog_limit);
+
+ return 1;
+}
+__setup("audit_backlog_limit=", audit_backlog_limit_set);
+
static void audit_buffer_free(struct audit_buffer *ab)
{
unsigned long flags;
--
1.7.1
Richard Guy Briggs
2013-09-18 19:06:52 UTC
Permalink
Re-named confusing local variable names (status_set and status_get didn't agree
with their command type name) and reduced their scope.

Future-proof API changes by not depending on the exact size of the audit_status
struct.

Signed-off-by: Richard Guy Briggs <***@redhat.com>
---
kernel/audit.c | 51 +++++++++++++++++++++++++++------------------------
1 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index acfa7a9..3d17670 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -635,7 +635,6 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
{
u32 seq;
void *data;
- struct audit_status *status_get, status_set;
int err;
struct audit_buffer *ab;
u16 msg_type = nlh->nlmsg_type;
@@ -661,47 +660,51 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
data = nlmsg_data(nlh);

switch (msg_type) {
- case AUDIT_GET:
- status_set.enabled = audit_enabled;
- status_set.failure = audit_failure;
- status_set.pid = audit_pid;
- status_set.rate_limit = audit_rate_limit;
- status_set.backlog_limit = audit_backlog_limit;
- status_set.lost = atomic_read(&audit_lost);
- status_set.backlog = skb_queue_len(&audit_skb_queue);
+ case AUDIT_GET: {
+ struct audit_status s;
+ s.enabled = audit_enabled;
+ s.failure = audit_failure;
+ s.pid = audit_pid;
+ s.rate_limit = audit_rate_limit;
+ s.backlog_limit = audit_backlog_limit;
+ s.lost = atomic_read(&audit_lost);
+ s.backlog = skb_queue_len(&audit_skb_queue);
audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_GET, 0, 0,
- &status_set, sizeof(status_set));
+ &s, sizeof(s));
break;
- case AUDIT_SET:
- if (nlh->nlmsg_len < sizeof(struct audit_status))
- return -EINVAL;
- status_get = (struct audit_status *)data;
- if (status_get->mask & AUDIT_STATUS_ENABLED) {
- err = audit_set_enabled(status_get->enabled);
+ }
+ case AUDIT_SET: {
+ struct audit_status s;
+ memset(&s, 0, sizeof(s));
+ /* guard against past and future API changes */
+ memcpy(&s, data, min(sizeof(s), (size_t)nlh->nlmsg_len));
+ if (s.mask & AUDIT_STATUS_ENABLED) {
+ err = audit_set_enabled(s.enabled);
if (err < 0)
return err;
}
- if (status_get->mask & AUDIT_STATUS_FAILURE) {
- err = audit_set_failure(status_get->failure);
+ if (s.mask & AUDIT_STATUS_FAILURE) {
+ err = audit_set_failure(s.failure);
if (err < 0)
return err;
}
- if (status_get->mask & AUDIT_STATUS_PID) {
- int new_pid = status_get->pid;
+ if (s.mask & AUDIT_STATUS_PID) {
+ int new_pid = s.pid;

if (audit_enabled != AUDIT_OFF)
audit_log_config_change("audit_pid", new_pid, audit_pid, 1);
audit_pid = new_pid;
audit_nlk_portid = NETLINK_CB(skb).portid;
}
- if (status_get->mask & AUDIT_STATUS_RATE_LIMIT) {
- err = audit_set_rate_limit(status_get->rate_limit);
+ if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
+ err = audit_set_rate_limit(s.rate_limit);
if (err < 0)
return err;
}
- if (status_get->mask & AUDIT_STATUS_BACKLOG_LIMIT)
- err = audit_set_backlog_limit(status_get->backlog_limit);
+ if (s.mask & AUDIT_STATUS_BACKLOG_LIMIT)
+ err = audit_set_backlog_limit(s.backlog_limit);
break;
+ }
case AUDIT_USER:
case AUDIT_FIRST_USER_MSG ... AUDIT_LAST_USER_MSG:
case AUDIT_FIRST_USER_MSG2 ... AUDIT_LAST_USER_MSG2:
--
1.7.1
Steve Grubb
2013-09-19 21:18:55 UTC
Permalink
Post by Richard Guy Briggs
Re-named confusing local variable names (status_set and status_get didn't
agree with their command type name) and reduced their scope.
Future-proof API changes by not depending on the exact size of the
audit_status struct.
I wished things like this were coordinated before being written. We had some
discussion of this back in July under a topic, "audit: implement generic
feature setting and retrieving". Maybe that API can be fixed so its not just
set/unset but can take a number as well.

Also, because there is no way to query the kernel to see what kind of things
it supports, we've typically defined a new message type and put into it exactly
what we need. In other words, if you want something expandable, the define a
new message type like AUDIT_GET_EXT and AUDIT_SET_EXT and build it to be
expandable.

Then in a future version of auditctl it will try to use the new command and
fall back to the old one if it gets EINVAL. Then some years later the old GET
and SET can be deprecated. But the audit code base has to support a wide
variety of kernels and suddenly making on resizable might break old code on
new kernel. A new message type is a safer migration path.

-Steve
Post by Richard Guy Briggs
---
kernel/audit.c | 51 +++++++++++++++++++++++++++------------------------
1 files changed, 27 insertions(+), 24 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index acfa7a9..3d17670 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -635,7 +635,6 @@ static int audit_receive_msg(struct sk_buff *skb, struct
nlmsghdr *nlh) {
u32 seq;
void *data;
- struct audit_status *status_get, status_set;
int err;
struct audit_buffer *ab;
u16 msg_type = nlh->nlmsg_type;
@@ -661,47 +660,51 @@ static int audit_receive_msg(struct sk_buff *skb,
struct nlmsghdr *nlh) data = nlmsg_data(nlh);
switch (msg_type) {
- status_set.enabled = audit_enabled;
- status_set.failure = audit_failure;
- status_set.pid = audit_pid;
- status_set.rate_limit = audit_rate_limit;
- status_set.backlog_limit = audit_backlog_limit;
- status_set.lost = atomic_read(&audit_lost);
- status_set.backlog = skb_queue_len(&audit_skb_queue);
+ case AUDIT_GET: {
+ struct audit_status s;
+ s.enabled = audit_enabled;
+ s.failure = audit_failure;
+ s.pid = audit_pid;
+ s.rate_limit = audit_rate_limit;
+ s.backlog_limit = audit_backlog_limit;
+ s.lost = atomic_read(&audit_lost);
+ s.backlog = skb_queue_len(&audit_skb_queue);
audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_GET, 0, 0,
- &status_set, sizeof(status_set));
+ &s, sizeof(s));
break;
- if (nlh->nlmsg_len < sizeof(struct audit_status))
- return -EINVAL;
- status_get = (struct audit_status *)data;
- if (status_get->mask & AUDIT_STATUS_ENABLED) {
- err = audit_set_enabled(status_get->enabled);
+ }
+ case AUDIT_SET: {
+ struct audit_status s;
+ memset(&s, 0, sizeof(s));
+ /* guard against past and future API changes */
+ memcpy(&s, data, min(sizeof(s), (size_t)nlh->nlmsg_len));
+ if (s.mask & AUDIT_STATUS_ENABLED) {
+ err = audit_set_enabled(s.enabled);
if (err < 0)
return err;
}
- if (status_get->mask & AUDIT_STATUS_FAILURE) {
- err = audit_set_failure(status_get->failure);
+ if (s.mask & AUDIT_STATUS_FAILURE) {
+ err = audit_set_failure(s.failure);
if (err < 0)
return err;
}
- if (status_get->mask & AUDIT_STATUS_PID) {
- int new_pid = status_get->pid;
+ if (s.mask & AUDIT_STATUS_PID) {
+ int new_pid = s.pid;
if (audit_enabled != AUDIT_OFF)
audit_log_config_change("audit_pid", new_pid, audit_pid,
1);
Post by Richard Guy Briggs
audit_pid = new_pid;
audit_nlk_portid = NETLINK_CB(skb).portid;
}
- if (status_get->mask & AUDIT_STATUS_RATE_LIMIT) {
- err = audit_set_rate_limit(status_get->rate_limit);
+ if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
+ err = audit_set_rate_limit(s.rate_limit);
if (err < 0)
return err;
}
- if (status_get->mask & AUDIT_STATUS_BACKLOG_LIMIT)
- err = audit_set_backlog_limit(status_get->backlog_limit);
+ if (s.mask & AUDIT_STATUS_BACKLOG_LIMIT)
+ err = audit_set_backlog_limit(s.backlog_limit);
break;
+ }
Eric Paris
2013-09-20 14:47:50 UTC
Permalink
Post by Steve Grubb
Post by Richard Guy Briggs
Re-named confusing local variable names (status_set and status_get didn't
agree with their command type name) and reduced their scope.
Future-proof API changes by not depending on the exact size of the
audit_status struct.
I wished things like this were coordinated before being written. We had some
discussion of this back in July under a topic, "audit: implement generic
feature setting and retrieving". Maybe that API can be fixed so its not just
set/unset but can take a number as well.
Your right, we did talk about it. Though it seems we talked past each
other. What was implemented was an on off extensible interface. The
status interface already fits for setting numbers. And because of how
it is used has been extended is is extensible for setting numbers.
Post by Steve Grubb
Also, because there is no way to query the kernel to see what kind of things
it supports
I'll agree. Richard, can you please add a version field to the status?
Start at version 1. Any time we add a new audit feature we'll bump it.
Post by Steve Grubb
, we've typically defined a new message type and put into it exactly
what we need. In other words, if you want something expandable, the define a
new message type like AUDIT_GET_EXT and AUDIT_SET_EXT and build it to be
expandable.
There is no reason we can't use what we have. (As we've done it before)
Richard Guy Briggs
2013-09-23 16:38:28 UTC
Permalink
Post by Eric Paris
Post by Steve Grubb
Post by Richard Guy Briggs
Re-named confusing local variable names (status_set and status_get didn't
agree with their command type name) and reduced their scope.
Future-proof API changes by not depending on the exact size of the
audit_status struct.
Also, because there is no way to query the kernel to see what kind of things
it supports
I'll agree. Richard, can you please add a version field to the status?
Start at version 1. Any time we add a new audit feature we'll bump it.
Done. Adding the interface and version field is version 1, so without,
it is zero. Adding the backlog timeout is version 2.

- RGB

--
Richard Guy Briggs <***@redhat.com>
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

Richard Guy Briggs
2013-09-18 19:06:53 UTC
Permalink
reaahead-collector abuses the audit logging facility to discover which files
are accessed at boot time to make a pre-load list

Add a tuning option to audit_backlog_wait_time so that if auditd can't keep up,
or gets blocked, the callers won't be blocked.

Signed-off-by: Richard Guy Briggs <***@redhat.com>
---
include/uapi/linux/audit.h | 2 ++
kernel/audit.c | 22 +++++++++++++++++++++-
2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 75cef3f..493a66e 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -316,6 +316,7 @@ enum {
#define AUDIT_STATUS_PID 0x0004
#define AUDIT_STATUS_RATE_LIMIT 0x0008
#define AUDIT_STATUS_BACKLOG_LIMIT 0x0010
+#define AUDIT_STATUS_BACKLOG_WAIT_TIME 0x0020
/* Failure-to-log actions */
#define AUDIT_FAIL_SILENT 0
#define AUDIT_FAIL_PRINTK 1
@@ -367,6 +368,7 @@ struct audit_status {
__u32 backlog_limit; /* waiting messages limit */
__u32 lost; /* messages lost */
__u32 backlog; /* messages waiting in queue */
+ __u32 backlog_wait_time;/* message queue wait timeout */
};

struct audit_tty_status {
diff --git a/kernel/audit.c b/kernel/audit.c
index 3d17670..fc535b6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -321,6 +321,12 @@ static int audit_set_backlog_limit(int limit)
return audit_do_config_change("audit_backlog_limit", &audit_backlog_limit, limit);
}

+static int audit_set_backlog_wait_time(int timeout)
+{
+ return audit_do_config_change("audit_backlog_wait_time",
+ &audit_backlog_wait_time, timeout);
+}
+
static int audit_set_enabled(int state)
{
int rc;
@@ -669,6 +675,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
s.backlog_limit = audit_backlog_limit;
s.lost = atomic_read(&audit_lost);
s.backlog = skb_queue_len(&audit_skb_queue);
+ s.backlog_wait_time = audit_backlog_wait_time;
audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_GET, 0, 0,
&s, sizeof(s));
break;
@@ -701,8 +708,21 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
if (err < 0)
return err;
}
- if (s.mask & AUDIT_STATUS_BACKLOG_LIMIT)
+ if (s.mask & AUDIT_STATUS_BACKLOG_LIMIT) {
err = audit_set_backlog_limit(s.backlog_limit);
+ if (err < 0)
+ return err;
+ }
+ if (s.mask & AUDIT_STATUS_BACKLOG_WAIT_TIME) {
+ if (sizeof(s) > (size_t)nlh->nlmsg_len)
+ break;
+ if (s.backlog_wait_time < 0 ||
+ s.backlog_wait_time > 10*AUDIT_BACKLOG_WAIT_TIME)
+ return -EINVAL;
+ err = audit_set_backlog_wait_time(s.backlog_wait_time);
+ if (err < 0)
+ return err;
+ }
break;
}
case AUDIT_USER:
--
1.7.1
Eric Paris
2013-09-18 20:33:25 UTC
Permalink
Post by Richard Guy Briggs
reaahead-collector abuses the audit logging facility to discover which files
are accessed at boot time to make a pre-load list
Add a tuning option to audit_backlog_wait_time so that if auditd can't keep up,
or gets blocked, the callers won't be blocked.
---
include/uapi/linux/audit.h | 2 ++
kernel/audit.c | 22 +++++++++++++++++++++-
2 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 75cef3f..493a66e 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -316,6 +316,7 @@ enum {
#define AUDIT_STATUS_PID 0x0004
#define AUDIT_STATUS_RATE_LIMIT 0x0008
#define AUDIT_STATUS_BACKLOG_LIMIT 0x0010
+#define AUDIT_STATUS_BACKLOG_WAIT_TIME 0x0020
/* Failure-to-log actions */
#define AUDIT_FAIL_SILENT 0
#define AUDIT_FAIL_PRINTK 1
@@ -367,6 +368,7 @@ struct audit_status {
__u32 backlog_limit; /* waiting messages limit */
__u32 lost; /* messages lost */
__u32 backlog; /* messages waiting in queue */
+ __u32 backlog_wait_time;/* message queue wait timeout */
};
struct audit_tty_status {
diff --git a/kernel/audit.c b/kernel/audit.c
index 3d17670..fc535b6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -321,6 +321,12 @@ static int audit_set_backlog_limit(int limit)
return audit_do_config_change("audit_backlog_limit", &audit_backlog_limit, limit);
}
+static int audit_set_backlog_wait_time(int timeout)
+{
+ return audit_do_config_change("audit_backlog_wait_time",
+ &audit_backlog_wait_time, timeout);
+}
+
static int audit_set_enabled(int state)
{
int rc;
@@ -669,6 +675,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
s.backlog_limit = audit_backlog_limit;
s.lost = atomic_read(&audit_lost);
s.backlog = skb_queue_len(&audit_skb_queue);
+ s.backlog_wait_time = audit_backlog_wait_time;
audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_GET, 0, 0,
&s, sizeof(s));
break;
@@ -701,8 +708,21 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
if (err < 0)
return err;
}
- if (s.mask & AUDIT_STATUS_BACKLOG_LIMIT)
+ if (s.mask & AUDIT_STATUS_BACKLOG_LIMIT) {
err = audit_set_backlog_limit(s.backlog_limit);
+ if (err < 0)
+ return err;
+ }
+ if (s.mask & AUDIT_STATUS_BACKLOG_WAIT_TIME) {
+ if (sizeof(s) > (size_t)nlh->nlmsg_len)
+ break;
What gets returned here? I think err has a value of 0, but it doesn't
seem to have been clearly intentional. If they know about the
AUDIT_STATUS_BACKLOG_WAIT_TIME flag, but they didn't send a long enough
skb? That seems like an error condition....
Post by Richard Guy Briggs
+ if (s.backlog_wait_time < 0 ||
+ s.backlog_wait_time > 10*AUDIT_BACKLOG_WAIT_TIME)
+ return -EINVAL;
+ err = audit_set_backlog_wait_time(s.backlog_wait_time);
+ if (err < 0)
+ return err;
+ }
break;
}
Richard Guy Briggs
2013-09-18 20:49:49 UTC
Permalink
Post by Eric Paris
Post by Richard Guy Briggs
reaahead-collector abuses the audit logging facility to discover which files
are accessed at boot time to make a pre-load list
Add a tuning option to audit_backlog_wait_time so that if auditd can't keep up,
or gets blocked, the callers won't be blocked.
diff --git a/kernel/audit.c b/kernel/audit.c
index 3d17670..fc535b6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -701,8 +708,21 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
if (err < 0)
return err;
}
- if (s.mask & AUDIT_STATUS_BACKLOG_LIMIT)
+ if (s.mask & AUDIT_STATUS_BACKLOG_LIMIT) {
err = audit_set_backlog_limit(s.backlog_limit);
+ if (err < 0)
+ return err;
+ }
+ if (s.mask & AUDIT_STATUS_BACKLOG_WAIT_TIME) {
+ if (sizeof(s) > (size_t)nlh->nlmsg_len)
+ break;
What gets returned here? I think err has a value of 0, but it doesn't
seem to have been clearly intentional. If they know about the
AUDIT_STATUS_BACKLOG_WAIT_TIME flag, but they didn't send a long enough
skb? That seems like an error condition....
The intent was that it is a NOP, since err would have a value of zero,
but I see your point, that if that flag is present, the struct member
should be too. My original intent was that if the structure member
wasn't present, it would default to zero, unintentionally setting the
wait time to zero. It was part of my paranoia in the absence of an API
version indicator. No harm done, but I agree it should return an error.

Thanks for the catch.
Post by Eric Paris
Post by Richard Guy Briggs
+ if (s.backlog_wait_time < 0 ||
+ s.backlog_wait_time > 10*AUDIT_BACKLOG_WAIT_TIME)
+ return -EINVAL;
I assume values less than zero or larger than 10 times the current
default of one minute are errors or unreasonable.

Any argument for more than 10 minutes?


- RGB

--
Richard Guy Briggs <***@redhat.com>
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545
Eric Paris
2013-09-18 20:54:02 UTC
Permalink
Post by Richard Guy Briggs
Post by Eric Paris
Post by Richard Guy Briggs
reaahead-collector abuses the audit logging facility to discover which files
are accessed at boot time to make a pre-load list
Add a tuning option to audit_backlog_wait_time so that if auditd can't keep up,
or gets blocked, the callers won't be blocked.
diff --git a/kernel/audit.c b/kernel/audit.c
index 3d17670..fc535b6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -701,8 +708,21 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
if (err < 0)
return err;
}
- if (s.mask & AUDIT_STATUS_BACKLOG_LIMIT)
+ if (s.mask & AUDIT_STATUS_BACKLOG_LIMIT) {
err = audit_set_backlog_limit(s.backlog_limit);
+ if (err < 0)
+ return err;
+ }
+ if (s.mask & AUDIT_STATUS_BACKLOG_WAIT_TIME) {
+ if (sizeof(s) > (size_t)nlh->nlmsg_len)
+ break;
What gets returned here? I think err has a value of 0, but it doesn't
seem to have been clearly intentional. If they know about the
AUDIT_STATUS_BACKLOG_WAIT_TIME flag, but they didn't send a long enough
skb? That seems like an error condition....
The intent was that it is a NOP, since err would have a value of zero,
but I see your point, that if that flag is present, the struct member
should be too. My original intent was that if the structure member
wasn't present, it would default to zero, unintentionally setting the
wait time to zero. It was part of my paranoia in the absence of an API
version indicator. No harm done, but I agree it should return an error.
Thanks for the catch.
Post by Eric Paris
Post by Richard Guy Briggs
+ if (s.backlog_wait_time < 0 ||
+ s.backlog_wait_time > 10*AUDIT_BACKLOG_WAIT_TIME)
+ return -EINVAL;
I assume values less than zero or larger than 10 times the current
default of one minute are errors or unreasonable.
Any argument for more than 10 minutes?
10 Minutes already seems nuts. If someone has a reason to go higher, we
can change this sanity check then.

-Eric
Loading...