[PATCH] audit: improve robustness of the audit queue handling

[PATCH] audit: improve robustness of the audit queue handling

[Paul Moore]

If the audit daemon were ever to get stuck in a stopped state the
kernel's kauditd_thread() could get blocked attempting to send audit
records to the userspace audit daemon.  With the kernel thread
blocked it is possible that the audit queue could grow unbounded as
certain audit record generating events must be exempt from the queue
limits else the system enter a deadlock state.

This patch resolves this problem by lowering the kernel thread's
socket sending timeout from MAX_SCHEDULE_TIMEOUT to HZ/10 and tweaks
the kauditd_send_queue() function to better manage the various audit
queues when connection problems occur between the kernel and the
audit daemon.  With this patch, the backlog may temporarily grow
beyond the defined limits when the audit daemon is stopped and the
system is under heavy audit pressure, but kauditd_thread() will
continue to make progress and drain the queues as it would for other
connection problems.  For example, with the audit daemon put into a
stopped state and the system configured to audit every syscall it
was still possible to shutdown the system without a kernel panic,
deadlock, etc.; granted, the system was slow to shutdown but that is
to be expected given the extreme pressure of recording every syscall.

The timeout value of HZ/10 was chosen primarily through
experimentation and this developer's "gut feeling".  There is likely
no one perfect value, but as this scenario is limited in scope (root
privileges would be needed to send SIGSTOP to the audit daemon), it
is likely not worth exposing this as a tunable at present.  This can
always be done at a later date if it proves necessary.

Cc: stable@vger.kernel.org
Fixes: 5b52330bbfe63 ("audit: fix auditd/kernel connection state tracking")
Reported-by: Gaosheng Cui <cuigaosheng1@huawei.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 kernel/audit.c |   21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 121d37e700a6..4cebadb5f30d 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -718,7 +718,7 @@ static int kauditd_send_queue(struct sock *sk, u32 portid,
 {
 	int rc = 0;
 	struct sk_buff *skb;
-	static unsigned int failed = 0;
+	unsigned int failed = 0;
 
 	/* NOTE: kauditd_thread takes care of all our locking, we just use
 	 *       the netlink info passed to us (e.g. sk and portid) */
@@ -735,32 +735,30 @@ static int kauditd_send_queue(struct sock *sk, u32 portid,
 			continue;
 		}
 
+retry:
 		/* grab an extra skb reference in case of error */
 		skb_get(skb);
 		rc = netlink_unicast(sk, skb, portid, 0);
 		if (rc < 0) {
-			/* fatal failure for our queue flush attempt? */
+			/* send failed - try a few times unless fatal error */
 			if (++failed >= retry_limit ||
 			    rc == -ECONNREFUSED || rc == -EPERM) {
-				/* yes - error processing for the queue */
 				sk = NULL;
 				if (err_hook)
 					(*err_hook)(skb);
-				if (!skb_hook)
-					goto out;
-				/* keep processing with the skb_hook */
+				if (rc == -EAGAIN)
+					rc = 0;
+				/* continue to drain the queue */
 				continue;
 			} else
-				/* no - requeue to preserve ordering */
-				skb_queue_head(queue, skb);
+				goto retry;
 		} else {
-			/* it worked - drop the extra reference and continue */
+			/* skb sent - drop the extra reference and continue */
 			consume_skb(skb);
 			failed = 0;
 		}
 	}
 
-out:
 	return (rc >= 0 ? 0 : rc);
 }
 
@@ -1609,7 +1607,8 @@ static int __net_init audit_net_init(struct net *net)
 		audit_panic("cannot initialize netlink socket in namespace");
 		return -ENOMEM;
 	}
-	aunet->sk->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
+	/* limit the timeout in case auditd is blocked/stopped */
+	aunet->sk->sk_sndtimeo = HZ / 10;
 
 	return 0;
 }

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: improve robustness of the audit queue handling

[cuigaosheng]

在 2021/12/14 2:31, Paul Moore 写道:
> If the audit daemon were ever to get stuck in a stopped state the
> kernel's kauditd_thread() could get blocked attempting to send audit
> records to the userspace audit daemon.  With the kernel thread
> blocked it is possible that the audit queue could grow unbounded as
> certain audit record generating events must be exempt from the queue
> limits else the system enter a deadlock state.
>
> This patch resolves this problem by lowering the kernel thread's
> socket sending timeout from MAX_SCHEDULE_TIMEOUT to HZ/10 and tweaks
> the kauditd_send_queue() function to better manage the various audit
> queues when connection problems occur between the kernel and the
> audit daemon.  With this patch, the backlog may temporarily grow
> beyond the defined limits when the audit daemon is stopped and the
> system is under heavy audit pressure, but kauditd_thread() will
> continue to make progress and drain the queues as it would for other
> connection problems.  For example, with the audit daemon put into a
> stopped state and the system configured to audit every syscall it
> was still possible to shutdown the system without a kernel panic,
> deadlock, etc.; granted, the system was slow to shutdown but that is
> to be expected given the extreme pressure of recording every syscall.
>
> The timeout value of HZ/10 was chosen primarily through
> experimentation and this developer's "gut feeling".  There is likely
> no one perfect value, but as this scenario is limited in scope (root
> privileges would be needed to send SIGSTOP to the audit daemon), it
> is likely not worth exposing this as a tunable at present.  This can
> always be done at a later date if it proves necessary.
>
> Cc: stable@vger.kernel.org
> Fixes: 5b52330bbfe63 ("audit: fix auditd/kernel connection state tracking")
> Reported-by: Gaosheng Cui <cuigaosheng1@huawei.com>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>   kernel/audit.c |   21 ++++++++++-----------
>   1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 121d37e700a6..4cebadb5f30d 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -718,7 +718,7 @@ static int kauditd_send_queue(struct sock *sk, u32 portid,
>   {
>   	int rc = 0;
>   	struct sk_buff *skb;
> -	static unsigned int failed = 0;
> +	unsigned int failed = 0;
>   
>   	/* NOTE: kauditd_thread takes care of all our locking, we just use
>   	 *       the netlink info passed to us (e.g. sk and portid) */
> @@ -735,32 +735,30 @@ static int kauditd_send_queue(struct sock *sk, u32 portid,
>   			continue;
>   		}
>   
> +retry:
>   		/* grab an extra skb reference in case of error */
>   		skb_get(skb);
>   		rc = netlink_unicast(sk, skb, portid, 0);
>   		if (rc < 0) {
> -			/* fatal failure for our queue flush attempt? */
> +			/* send failed - try a few times unless fatal error */
>   			if (++failed >= retry_limit ||
>   			    rc == -ECONNREFUSED || rc == -EPERM) {
> -				/* yes - error processing for the queue */
>   				sk = NULL;
>   				if (err_hook)
>   					(*err_hook)(skb);
> -				if (!skb_hook)
> -					goto out;
> -				/* keep processing with the skb_hook */
> +				if (rc == -EAGAIN)
> +					rc = 0;
> +				/* continue to drain the queue */
>   				continue;
>   			} else
> -				/* no - requeue to preserve ordering */
> -				skb_queue_head(queue, skb);
> +				goto retry;
>   		} else {
> -			/* it worked - drop the extra reference and continue */
> +			/* skb sent - drop the extra reference and continue */
>   			consume_skb(skb);
>   			failed = 0;
>   		}
>   	}
>   
> -out:
>   	return (rc >= 0 ? 0 : rc);
>   }
>   
> @@ -1609,7 +1607,8 @@ static int __net_init audit_net_init(struct net *net)
>   		audit_panic("cannot initialize netlink socket in namespace");
>   		return -ENOMEM;
>   	}
> -	aunet->sk->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
> +	/* limit the timeout in case auditd is blocked/stopped */
> +	aunet->sk->sk_sndtimeo = HZ / 10;
>   
>   	return 0;
>   }
>
> .

Tested-by: Gaosheng Cui<cuigaosheng1@huawei.com>


--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: improve robustness of the audit queue handling

[Richard Guy Briggs]

On 2021-12-13 13:31, Paul Moore wrote:
> If the audit daemon were ever to get stuck in a stopped state the
> kernel's kauditd_thread() could get blocked attempting to send audit
> records to the userspace audit daemon.  With the kernel thread
> blocked it is possible that the audit queue could grow unbounded as
> certain audit record generating events must be exempt from the queue
> limits else the system enter a deadlock state.
> 
> This patch resolves this problem by lowering the kernel thread's
> socket sending timeout from MAX_SCHEDULE_TIMEOUT to HZ/10 and tweaks
> the kauditd_send_queue() function to better manage the various audit
> queues when connection problems occur between the kernel and the
> audit daemon.  With this patch, the backlog may temporarily grow
> beyond the defined limits when the audit daemon is stopped and the
> system is under heavy audit pressure, but kauditd_thread() will
> continue to make progress and drain the queues as it would for other
> connection problems.  For example, with the audit daemon put into a
> stopped state and the system configured to audit every syscall it
> was still possible to shutdown the system without a kernel panic,
> deadlock, etc.; granted, the system was slow to shutdown but that is
> to be expected given the extreme pressure of recording every syscall.

I assume that in the configuration state of f=2, it would still panic if
it was not able to deliver messages.

This seems like a reasonable approach, FWIW: Reviewed-by

> The timeout value of HZ/10 was chosen primarily through
> experimentation and this developer's "gut feeling".  There is likely
> no one perfect value, but as this scenario is limited in scope (root
> privileges would be needed to send SIGSTOP to the audit daemon), it
> is likely not worth exposing this as a tunable at present.  This can
> always be done at a later date if it proves necessary.
> 
> Cc: stable@vger.kernel.org
> Fixes: 5b52330bbfe63 ("audit: fix auditd/kernel connection state tracking")
> Reported-by: Gaosheng Cui <cuigaosheng1@huawei.com>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  kernel/audit.c |   21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 121d37e700a6..4cebadb5f30d 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -718,7 +718,7 @@ static int kauditd_send_queue(struct sock *sk, u32 portid,
>  {
>  	int rc = 0;
>  	struct sk_buff *skb;
> -	static unsigned int failed = 0;
> +	unsigned int failed = 0;
>  
>  	/* NOTE: kauditd_thread takes care of all our locking, we just use
>  	 *       the netlink info passed to us (e.g. sk and portid) */
> @@ -735,32 +735,30 @@ static int kauditd_send_queue(struct sock *sk, u32 portid,
>  			continue;
>  		}
>  
> +retry:
>  		/* grab an extra skb reference in case of error */
>  		skb_get(skb);
>  		rc = netlink_unicast(sk, skb, portid, 0);
>  		if (rc < 0) {
> -			/* fatal failure for our queue flush attempt? */
> +			/* send failed - try a few times unless fatal error */
>  			if (++failed >= retry_limit ||
>  			    rc == -ECONNREFUSED || rc == -EPERM) {
> -				/* yes - error processing for the queue */
>  				sk = NULL;
>  				if (err_hook)
>  					(*err_hook)(skb);
> -				if (!skb_hook)
> -					goto out;
> -				/* keep processing with the skb_hook */
> +				if (rc == -EAGAIN)
> +					rc = 0;
> +				/* continue to drain the queue */
>  				continue;
>  			} else
> -				/* no - requeue to preserve ordering */
> -				skb_queue_head(queue, skb);
> +				goto retry;
>  		} else {
> -			/* it worked - drop the extra reference and continue */
> +			/* skb sent - drop the extra reference and continue */
>  			consume_skb(skb);
>  			failed = 0;
>  		}
>  	}
>  
> -out:
>  	return (rc >= 0 ? 0 : rc);
>  }
>  
> @@ -1609,7 +1607,8 @@ static int __net_init audit_net_init(struct net *net)
>  		audit_panic("cannot initialize netlink socket in namespace");
>  		return -ENOMEM;
>  	}
> -	aunet->sk->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
> +	/* limit the timeout in case auditd is blocked/stopped */
> +	aunet->sk->sk_sndtimeo = HZ / 10;
>  
>  	return 0;
>  }
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://listman.redhat.com/mailman/listinfo/linux-audit
> 

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: improve robustness of the audit queue handling

[Paul Moore]

On Wed, Dec 15, 2021 at 12:53 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2021-12-13 13:31, Paul Moore wrote:
> > If the audit daemon were ever to get stuck in a stopped state the
> > kernel's kauditd_thread() could get blocked attempting to send audit
> > records to the userspace audit daemon.  With the kernel thread
> > blocked it is possible that the audit queue could grow unbounded as
> > certain audit record generating events must be exempt from the queue
> > limits else the system enter a deadlock state.
> >
> > This patch resolves this problem by lowering the kernel thread's
> > socket sending timeout from MAX_SCHEDULE_TIMEOUT to HZ/10 and tweaks
> > the kauditd_send_queue() function to better manage the various audit
> > queues when connection problems occur between the kernel and the
> > audit daemon.  With this patch, the backlog may temporarily grow
> > beyond the defined limits when the audit daemon is stopped and the
> > system is under heavy audit pressure, but kauditd_thread() will
> > continue to make progress and drain the queues as it would for other
> > connection problems.  For example, with the audit daemon put into a
> > stopped state and the system configured to audit every syscall it
> > was still possible to shutdown the system without a kernel panic,
> > deadlock, etc.; granted, the system was slow to shutdown but that is
> > to be expected given the extreme pressure of recording every syscall.
>
> I assume that in the configuration state of f=2, it would still panic if
> it was not able to deliver messages.

Yes, this patch doesn't really change any of the lost record behavior,
that is all preserved, it basically just makes sure that
kauditd_thread() isn't blocked when the audit daemon isn't able to
receive audit records.  Further, short lived audit daemon stoppages
shouldn't result in lost records either given a properly configured
system with a sufficient backlog as the retry mechanisms/queues are
still intact.  However, if you send a SIGSTOP to the audit daemon and
proceed to flood the audit subsystem with records, you're going to see
some lost records :)

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: improve robustness of the audit queue handling

[Paul Moore]

On Mon, Dec 13, 2021 at 1:31 PM Paul Moore <paul@paul-moore.com> wrote:
>
> If the audit daemon were ever to get stuck in a stopped state the
> kernel's kauditd_thread() could get blocked attempting to send audit
> records to the userspace audit daemon.  With the kernel thread
> blocked it is possible that the audit queue could grow unbounded as
> certain audit record generating events must be exempt from the queue
> limits else the system enter a deadlock state.
>
> This patch resolves this problem by lowering the kernel thread's
> socket sending timeout from MAX_SCHEDULE_TIMEOUT to HZ/10 and tweaks
> the kauditd_send_queue() function to better manage the various audit
> queues when connection problems occur between the kernel and the
> audit daemon.  With this patch, the backlog may temporarily grow
> beyond the defined limits when the audit daemon is stopped and the
> system is under heavy audit pressure, but kauditd_thread() will
> continue to make progress and drain the queues as it would for other
> connection problems.  For example, with the audit daemon put into a
> stopped state and the system configured to audit every syscall it
> was still possible to shutdown the system without a kernel panic,
> deadlock, etc.; granted, the system was slow to shutdown but that is
> to be expected given the extreme pressure of recording every syscall.
>
> The timeout value of HZ/10 was chosen primarily through
> experimentation and this developer's "gut feeling".  There is likely
> no one perfect value, but as this scenario is limited in scope (root
> privileges would be needed to send SIGSTOP to the audit daemon), it
> is likely not worth exposing this as a tunable at present.  This can
> always be done at a later date if it proves necessary.
>
> Cc: stable@vger.kernel.org
> Fixes: 5b52330bbfe63 ("audit: fix auditd/kernel connection state tracking")
> Reported-by: Gaosheng Cui <cuigaosheng1@huawei.com>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  kernel/audit.c |   21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)

Merged into audit/stable-5.16, thanks for the help everyone.  Assuming
all of the automated testing goes well, I'll plan on sending this up
to Linus tomorrow.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit