[PATCH 0/8] Audit backlog queue fixes related to soft lockup

[PATCH 0/8] Audit backlog queue fixes related to soft lockup

[Richard Guy Briggs]

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(-)

[PATCH 1/8] audit: avoid soft lockup due to audit_log_start() incorrect loop termination

[Richard Guy Briggs]

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 <lcapitulino@redhat.com>
Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
Signed-off-by: Dan Duval <dan.duval@oracle.com>
Signed-off-by: Chuck Anderson <chuck.anderson@oracle.com>
Signed-off-by: Richard Guy Briggs <rgb@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

[PATCH 2/8] audit: reset audit backlog wait time after error recovery

[Richard Guy Briggs]

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 <lcapitulino@redhat.com>
Signed-off-by: Dan Duval <dan.duval@oracle.com>
Signed-off-by: Chuck Anderson <chuck.anderson@oracle.com>
Signed-off-by: Richard Guy Briggs <rgb@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

[PATCH 3/8] audit: make use of remaining sleep time from wait_for_auditd

[Richard Guy Briggs]

If wait_for_auditd() times out, go immediately to the error function rather
than retesting the loop conditions.

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

[PATCH 4/8] audit: efficiency fix 1: only wake up if queue shorter than backlog limit

[Richard Guy Briggs]

author: Dan Duval <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 <dan.duval@oracle.com>
Signed-off-by: Chuck Anderson <chuck.anderson@oracle.com>
Signed-off-by: Richard Guy Briggs <rgb@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

[PATCH 5/8] audit: efficiency fix 2: request exclusive wait since all need same resource

[Richard Guy Briggs]

author: Dan Duval <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 <dan.duval@oracle.com>
Signed-off-by: Chuck Anderson <chuck.anderson@oracle.com>
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 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

[PATCH 6/8] audit: add boot option to override default backlog limit

[Richard Guy Briggs]

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

[PATCH 7/8] audit: clean up AUDIT_GET/SET local variables and future-proof API

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

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

Re: [PATCH 7/8] audit: clean up AUDIT_GET/SET local variables and future-proof API

[Steve Grubb]

On Wednesday, September 18, 2013 03:06:52 PM Richard Guy Briggs wrote:
> 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


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

Re: [PATCH 7/8] audit: clean up AUDIT_GET/SET local variables and future-proof API

[Eric Paris]

On Thu, 2013-09-19 at 17:18 -0400, Steve Grubb wrote:
> On Wednesday, September 18, 2013 03:06:52 PM Richard Guy Briggs wrote:
> > 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.

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

> , 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)

Re: [PATCH 7/8] audit: clean up AUDIT_GET/SET local variables and future-proof API

[Richard Guy Briggs]

On Fri, Sep 20, 2013 at 10:47:50AM -0400, Eric Paris wrote:
> On Thu, 2013-09-19 at 17:18 -0400, Steve Grubb wrote:
> > On Wednesday, September 18, 2013 03:06:52 PM Richard Guy Briggs wrote:
> > > 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 <rbriggs@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

[PATCH 8/8] audit: add audit_backlog_wait_time configuration option

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

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

Re: [PATCH 8/8] audit: add audit_backlog_wait_time configuration option

[Eric Paris]

On Wed, 2013-09-18 at 15:06 -0400, Richard Guy Briggs wrote:
> 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 <rgb@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;

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

> +			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:

Re: [PATCH 8/8] audit: add audit_backlog_wait_time configuration option

[Richard Guy Briggs]

On Wed, Sep 18, 2013 at 04:33:25PM -0400, Eric Paris wrote:
> On Wed, 2013-09-18 at 15:06 -0400, Richard Guy Briggs wrote:
> > 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.

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

Re: [PATCH 8/8] audit: add audit_backlog_wait_time configuration option

[Eric Paris]

On Wed, 2013-09-18 at 16:49 -0400, Richard Guy Briggs wrote:
> On Wed, Sep 18, 2013 at 04:33:25PM -0400, Eric Paris wrote:
> > On Wed, 2013-09-18 at 15:06 -0400, Richard Guy Briggs wrote:
> > > 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.
> 
> > > +			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