Re: [PATCH] audit: Fix data race in audit_log_start()

[Ricardo Robaina <rrobaina@redhat.com>]

Hi Chi,

Good catch, the patch looks good to me.

However, I found three additional instances of the same pattern that
should be included in this patch for completeness:

  1. Line 953 in kauditd_thread():
     wait_event_freezable(kauditd_wait,
                          (skb_queue_len(&audit_queue) ? 1 : 0));

  2. Line 1286 in audit_receive_msg() (AUDIT_GET handler):
     s.backlog = skb_queue_len(&audit_queue);

  3. Line 1630 in audit_receive():
     if (audit_backlog_limit &&
         (skb_queue_len(&audit_queue) > audit_backlog_limit)) {

All three read audit_queue.qlen without synchronization while the queue
is concurrently modified.

diff --git a/kernel/audit.c b/kernel/audit.c
index dcc657d35776..c074c3717946 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -950,7 +950,7 @@ static int kauditd_thread(void *dummy)
                 *       do the multicast send and rotate records from the
                 *       main queue to the retry/hold queues */
                wait_event_freezable(kauditd_wait,
-                                    (skb_queue_len(&audit_queue) ? 1 : 0));
+                                    (skb_queue_len_lockless(&audit_queue) ? 1 : 0));
        }
 
        return 0;
@@ -1283,7 +1283,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
                s.rate_limit               = audit_rate_limit;
                s.backlog_limit            = audit_backlog_limit;
                s.lost                     = atomic_read(&audit_lost);
-               s.backlog                  = skb_queue_len(&audit_queue);
+               s.backlog                  = skb_queue_len_lockless(&audit_queue);
                s.feature_bitmap           = AUDIT_FEATURE_BITMAP_ALL;
                s.backlog_wait_time        = audit_backlog_wait_time;
                s.backlog_wait_time_actual = atomic_read(&audit_backlog_wait_time_actual);
@@ -1627,7 +1627,7 @@ static void audit_receive(struct sk_buff *skb)
 
        /* can't block with the ctrl lock, so penalize the sender now */
        if (audit_backlog_limit &&
-           (skb_queue_len(&audit_queue) > audit_backlog_limit)) {
+           (skb_queue_len_lockless(&audit_queue) > audit_backlog_limit)) {
                DECLARE_WAITQUEUE(wait, current);
 
                /* wake kauditd to try and flush the queue */

Reviewed-by: Ricardo Robaina <rrobaina@redhat.com>

I hope it helps!
-Ricardo