batman-adv: tp_meter: prevent parallel modifications of last_recv

When last_recv is updated to store the last receive sequence number, it is
assuming that nothing is modifying in parallel while:

* check for outdated packets is done
* out of order check is performed (and packets are stored in out-of-order
  queue)
* the out-of-order queue was searched for closed gaps
* sequence number for next ack is calculated

Nothing of that was actually protected. It could therefore happen that the
last_recv was updated multiple times in parallel and the final sequence
number was calculated with deltas which had no connection to the sequence
number they were added to.

Lock this whole region with the same lock which was already used to protect
the unacked (out-of-order) list.

Cc: stable@kernel.org
Fixes: 33a3bb4a3345 ("batman-adv: throughput meter implementation")
Signed-off-by: Sven Eckelmann <sven@narfation.org>

diff --git a/net/batman-adv/tp_meter.c b/net/batman-adv/tp_meter.c
index fb87fa141e32ae0b994bbe97be79c9f6af430555..055aa1ee6ac5c72be5a7e332a766d666463d0a35 100644 (file)
--- a/net/batman-adv/tp_meter.c
+++ b/net/batman-adv/tp_meter.c
@@ -1402,6 +1402,7 @@ out:
  */
 static bool batadv_tp_handle_out_of_order(struct batadv_tp_receiver *tp_vars,
                                          const struct sk_buff *skb)
+       __must_hold(&tp_vars->common.unacked_lock)
 {
        const struct batadv_icmp_tp_packet *icmp;
        struct batadv_tp_unacked *un, *new;
@@ -1418,12 +1419,11 @@ static bool batadv_tp_handle_out_of_order(struct batadv_tp_receiver *tp_vars,
        payload_len = skb->len - sizeof(struct batadv_unicast_packet);
        new->len = payload_len;
 
-       spin_lock_bh(&tp_vars->common.unacked_lock);
        /* if the list is empty immediately attach this new object */
        if (list_empty(&tp_vars->common.unacked_list)) {
                list_add(&new->list, &tp_vars->common.unacked_list);
                tp_vars->common.unacked_count++;
-               goto out;
+               return true;
        }
 
        /* otherwise loop over the list and either drop the packet because this
@@ -1472,9 +1472,6 @@ static bool batadv_tp_handle_out_of_order(struct batadv_tp_receiver *tp_vars,
                tp_vars->common.unacked_count--;
        }
 
-out:
-       spin_unlock_bh(&tp_vars->common.unacked_lock);
-
        return true;
 }
 
@@ -1484,6 +1481,7 @@ out:
  * @tp_vars: the private data of the current TP meter session
  */
 static void batadv_tp_ack_unordered(struct batadv_tp_receiver *tp_vars)
+       __must_hold(&tp_vars->common.unacked_lock)
 {
        struct batadv_tp_unacked *un, *safe;
        u32 to_ack;
@@ -1491,7 +1489,6 @@ static void batadv_tp_ack_unordered(struct batadv_tp_receiver *tp_vars)
        /* go through the unacked packet list and possibly ACK them as
         * well
         */
-       spin_lock_bh(&tp_vars->common.unacked_lock);
        list_for_each_entry_safe(un, safe, &tp_vars->common.unacked_list, list) {
                /* the list is ordered, therefore it is possible to stop as soon
                 * there is a gap between the last acked seqno and the seqno of
@@ -1509,7 +1506,6 @@ static void batadv_tp_ack_unordered(struct batadv_tp_receiver *tp_vars)
                kfree(un);
                tp_vars->common.unacked_count--;
        }
-       spin_unlock_bh(&tp_vars->common.unacked_lock);
 }
 
 /**
@@ -1588,6 +1584,7 @@ static void batadv_tp_recv_msg(struct batadv_priv *bat_priv,
        const struct batadv_icmp_tp_packet *icmp;
        struct batadv_tp_receiver *tp_vars;
        size_t packet_size;
+       u32 to_ack;
        u32 seqno;
 
        icmp = (struct batadv_icmp_tp_packet *)skb->data;
@@ -1616,6 +1613,8 @@ static void batadv_tp_recv_msg(struct batadv_priv *bat_priv,
                WRITE_ONCE(tp_vars->last_recv_time, jiffies);
        }
 
+       spin_lock_bh(&tp_vars->common.unacked_lock);
+
        /* if the packet is a duplicate, it may be the case that an ACK has been
         * lost. Resend the ACK
         */
@@ -1627,8 +1626,10 @@ static void batadv_tp_recv_msg(struct batadv_priv *bat_priv,
                /* exit immediately (and do not send any ACK) if the packet has
                 * not been enqueued correctly
                 */
-               if (!batadv_tp_handle_out_of_order(tp_vars, skb))
+               if (!batadv_tp_handle_out_of_order(tp_vars, skb)) {
+                       spin_unlock_bh(&tp_vars->common.unacked_lock);
                        goto out;
+               }
 
                /* send a duplicate ACK */
                goto send_ack;
@@ -1642,11 +1643,14 @@ static void batadv_tp_recv_msg(struct batadv_priv *bat_priv,
        batadv_tp_ack_unordered(tp_vars);
 
 send_ack:
+       to_ack = tp_vars->last_recv;
+       spin_unlock_bh(&tp_vars->common.unacked_lock);
+
        /* send the ACK. If the received packet was out of order, the ACK that
         * is going to be sent is a duplicate (the sender will count them and
         * possibly enter Fast Retransmit as soon as it has reached 3)
         */
-       batadv_tp_send_ack(bat_priv, icmp->orig, tp_vars->last_recv,
+       batadv_tp_send_ack(bat_priv, icmp->orig, to_ack,
                           icmp->timestamp, icmp->session, icmp->uid);
 out:
        batadv_tp_receiver_put(tp_vars);

diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
index 9fa8e73ff6e59548d38494613e631d677c5fef88..c1b3f989566f534a027015d1a2d5d5be1f1104ea 100644 (file)
--- a/net/batman-adv/types.h
+++ b/net/batman-adv/types.h
@@ -1363,7 +1363,7 @@ struct batadv_tp_vars_common {
        /** @unacked_list: list of unacked packets (meta-info only) */
        struct list_head unacked_list;
 
-       /** @unacked_lock: protect unacked_list */
+       /** @unacked_lock: protect unacked_list + &batadv_tp_receiver.last_recv */
        spinlock_t unacked_lock;
 
        /** @unacked_count: number of unacked entries */