Re: [PATCH] audit: make sure we don't let the retry queue grow without bounds

[Paul Moore <paul@paul-moore.com>]

On Mon, Apr 10, 2017 at 4:54 PM, Paul Moore <pmoore@redhat.com> wrote:
> From: Paul Moore <paul@paul-moore.com>
>
> The retry queue is intended to provide a temporary buffer in the case
> of transient errors when communicating with auditd, it is not meant
> as a long life queue, that functionality is provided by the hold
> queue.
>
> This patch fixes a problem identified by Seth where the retry queue
> could grow uncontrollably if an auditd instance did not connect to
> the kernel to drain the queues.  This commit fixes this by doing the
> following:
>
> * Make sure we always call auditd_reset() if we decide the connection
> with audit is really dead.  There were some cases in
> kauditd_hold_skb() where we did not reset the connection, this patch
> relocates the reset calls to kauditd_thread() so all the error
> conditions are caught and the connection reset.  As a side effect,
> this means we could move auditd_reset() and get rid of the forward
> definition at the top of kernel/audit.c.
>
> * We never checked the status of the auditd connection when
> processing the main audit queue which meant that the retry queue
> could grow unchecked.  This patch adds a call to auditd_reset()
> after the main queue has been processed if auditd is not connected,
> the auditd_reset() call will make sure the retry and hold queues are
> correctly managed/flushed so that the retry queue remains reasonable.
>
> Reported-by: Seth Forshee <seth.forshee@canonical.com>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  kernel/audit.c |   67 +++++++++++++++++++++++++++-----------------------------
>  1 file changed, 32 insertions(+), 35 deletions(-)

FWIW, I just ran this for about 30m randomly starting and stopping
auditd under heavy audit load and didn't notice any change in memory
usage on my test system.  If I don't hear anything negative by
tomorrow I'll send this up to Linus.

> diff --git a/kernel/audit.c b/kernel/audit.c
> index 2f4964cfde0b..a871bf80fde1 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -160,7 +160,6 @@ static LIST_HEAD(audit_freelist);
>
>  /* queue msgs to send via kauditd_task */
>  static struct sk_buff_head audit_queue;
> -static void kauditd_hold_skb(struct sk_buff *skb);
>  /* queue msgs due to temporary unicast send problems */
>  static struct sk_buff_head audit_retry_queue;
>  /* queue msgs waiting for new auditd connection */
> @@ -454,30 +453,6 @@ static void auditd_set(int pid, u32 portid, struct net *net)
>  }
>
>  /**
> - * auditd_reset - Disconnect the auditd connection
> - *
> - * Description:
> - * Break the auditd/kauditd connection and move all the queued records into the
> - * hold queue in case auditd reconnects.
> - */
> -static void auditd_reset(void)
> -{
> -       struct sk_buff *skb;
> -
> -       /* if it isn't already broken, break the connection */
> -       rcu_read_lock();
> -       if (auditd_conn.pid)
> -               auditd_set(0, 0, NULL);
> -       rcu_read_unlock();
> -
> -       /* flush all of the main and retry queues to the hold queue */
> -       while ((skb = skb_dequeue(&audit_retry_queue)))
> -               kauditd_hold_skb(skb);
> -       while ((skb = skb_dequeue(&audit_queue)))
> -               kauditd_hold_skb(skb);
> -}
> -
> -/**
>   * kauditd_print_skb - Print the audit record to the ring buffer
>   * @skb: audit record
>   *
> @@ -505,9 +480,6 @@ static void kauditd_rehold_skb(struct sk_buff *skb)
>  {
>         /* put the record back in the queue at the same place */
>         skb_queue_head(&audit_hold_queue, skb);
> -
> -       /* fail the auditd connection */
> -       auditd_reset();
>  }
>
>  /**
> @@ -544,9 +516,6 @@ static void kauditd_hold_skb(struct sk_buff *skb)
>         /* we have no other options - drop the message */
>         audit_log_lost("kauditd hold queue overflow");
>         kfree_skb(skb);
> -
> -       /* fail the auditd connection */
> -       auditd_reset();
>  }
>
>  /**
> @@ -567,6 +536,30 @@ static void kauditd_retry_skb(struct sk_buff *skb)
>  }
>
>  /**
> + * auditd_reset - Disconnect the auditd connection
> + *
> + * Description:
> + * Break the auditd/kauditd connection and move all the queued records into the
> + * hold queue in case auditd reconnects.
> + */
> +static void auditd_reset(void)
> +{
> +       struct sk_buff *skb;
> +
> +       /* if it isn't already broken, break the connection */
> +       rcu_read_lock();
> +       if (auditd_conn.pid)
> +               auditd_set(0, 0, NULL);
> +       rcu_read_unlock();
> +
> +       /* flush all of the main and retry queues to the hold queue */
> +       while ((skb = skb_dequeue(&audit_retry_queue)))
> +               kauditd_hold_skb(skb);
> +       while ((skb = skb_dequeue(&audit_queue)))
> +               kauditd_hold_skb(skb);
> +}
> +
> +/**
>   * auditd_send_unicast_skb - Send a record via unicast to auditd
>   * @skb: audit record
>   *
> @@ -758,6 +751,7 @@ static int kauditd_thread(void *dummy)
>                                         NULL, kauditd_rehold_skb);
>                 if (rc < 0) {
>                         sk = NULL;
> +                       auditd_reset();
>                         goto main_queue;
>                 }
>
> @@ -767,6 +761,7 @@ static int kauditd_thread(void *dummy)
>                                         NULL, kauditd_hold_skb);
>                 if (rc < 0) {
>                         sk = NULL;
> +                       auditd_reset();
>                         goto main_queue;
>                 }
>
> @@ -775,16 +770,18 @@ static int kauditd_thread(void *dummy)
>                  * unicast, dump failed record sends to the retry queue; if
>                  * sk == NULL due to previous failures we will just do the
>                  * multicast send and move the record to the retry queue */
> -               kauditd_send_queue(sk, portid, &audit_queue, 1,
> -                                  kauditd_send_multicast_skb,
> -                                  kauditd_retry_skb);
> +               rc = kauditd_send_queue(sk, portid, &audit_queue, 1,
> +                                       kauditd_send_multicast_skb,
> +                                       kauditd_retry_skb);
> +               if (sk == NULL || rc < 0)
> +                       auditd_reset();
> +               sk = NULL;
>
>                 /* drop our netns reference, no auditd sends past this line */
>                 if (net) {
>                         put_net(net);
>                         net = NULL;
>                 }
> -               sk = NULL;
>
>                 /* we have processed all the queues so wake everyone */
>                 wake_up(&audit_backlog_wait);
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

-- 
paul moore
www.paul-moore.com