The bla.num_requests is increased when no request_sent was in progress. And
it is decremented in various places (announcemnt was received, backbone is
purged, periodic work). But the check if the request_sent is actually set
to a specific state and the atomic_dec/_inc are not safe because they are
not atomic (TOCTOU) and multiple such code portions can run concurrently.
At the same time, it is necessary to modify request_sent and
bla.num_requests at the same time. Otherwise batadv_bla_send_request()
might set request_sent to 1 and is interrupted. batadv_handle_announce()
can then set request_sent back to 0 and decrement num_requests before
batadv_bla_send_request() incremented it.
The two operations must therefore be locked. And since request_sent is only
accessed inside this lock, it can be converted to a simpler datatype.
Fixes: a9ce0dc43e2c ("batman-adv: add basic bridge loop avoidance code")
Signed-off-by: Sven Eckelmann <[email protected]>
---
net/batman-adv/bridge_loop_avoidance.c | 45 +++++++++++++++++++++++-----------
net/batman-adv/mesh-interface.c | 1 +
net/batman-adv/types.h | 7 ++++--
3 files changed, 37 insertions(+), 16 deletions(-)
diff --git a/net/batman-adv/bridge_loop_avoidance.c
b/net/batman-adv/bridge_loop_avoidance.c
index df1dfdf4..29bfb052 100644
--- a/net/batman-adv/bridge_loop_avoidance.c
+++ b/net/batman-adv/bridge_loop_avoidance.c
@@ -514,7 +514,7 @@ batadv_bla_get_backbone_gw(struct batadv_priv *bat_priv,
const u8 *orig,
entry->crc = BATADV_BLA_CRC_INIT;
entry->bat_priv = bat_priv;
spin_lock_init(&entry->crc_lock);
- atomic_set(&entry->request_sent, 0);
+ entry->request_sent = 0;
atomic_set(&entry->wait_periods, 0);
ether_addr_copy(entry->orig, orig);
INIT_WORK(&entry->report_work, batadv_bla_loopdetect_report);
@@ -544,9 +544,13 @@ batadv_bla_get_backbone_gw(struct batadv_priv *bat_priv,
const u8 *orig,
batadv_bla_send_announce(bat_priv, entry);
/* this will be decreased in the worker thread */
- atomic_inc(&entry->request_sent);
- atomic_set(&entry->wait_periods, BATADV_BLA_WAIT_PERIODS);
- atomic_inc(&bat_priv->bla.num_requests);
+ spin_lock_bh(&bat_priv->bla.num_requests_lock);
+ if (!entry->request_sent) {
+ entry->request_sent = 1;
+ atomic_set(&entry->wait_periods,
BATADV_BLA_WAIT_PERIODS);
+ atomic_inc(&bat_priv->bla.num_requests);
+ }
+ spin_unlock_bh(&bat_priv->bla.num_requests_lock);
}
return entry;
@@ -649,10 +653,12 @@ static void batadv_bla_send_request(struct
batadv_bla_backbone_gw *backbone_gw)
backbone_gw->vid, BATADV_CLAIM_TYPE_REQUEST);
/* no local broadcasts should be sent or received, for now. */
- if (!atomic_read(&backbone_gw->request_sent)) {
+ spin_lock_bh(&backbone_gw->bat_priv->bla.num_requests_lock);
+ if (!backbone_gw->request_sent) {
+ backbone_gw->request_sent = 1;
atomic_inc(&backbone_gw->bat_priv->bla.num_requests);
- atomic_set(&backbone_gw->request_sent, 1);
}
+ spin_unlock_bh(&backbone_gw->bat_priv->bla.num_requests_lock);
}
/**
@@ -873,10 +879,12 @@ static bool batadv_handle_announce(struct batadv_priv
*bat_priv, u8 *an_addr,
/* if we have sent a request and the crc was OK,
* we can allow traffic again.
*/
- if (atomic_read(&backbone_gw->request_sent)) {
+ spin_lock_bh(&bat_priv->bla.num_requests_lock);
+ if (backbone_gw->request_sent) {
+ backbone_gw->request_sent = 0;
atomic_dec(&backbone_gw->bat_priv->bla.num_requests);
- atomic_set(&backbone_gw->request_sent, 0);
}
+ spin_unlock_bh(&bat_priv->bla.num_requests_lock);
}
batadv_backbone_gw_put(backbone_gw);
@@ -1254,9 +1262,14 @@ static void batadv_bla_purge_backbone_gw(struct
batadv_priv *bat_priv, int now)
purge_now:
purged = true;
- /* don't wait for the pending request anymore */
- if (atomic_read(&backbone_gw->request_sent))
+ /* don't wait for the pending request anymore,
+ * also don't set request_sent to 0 to avoid a
parallel
+ * RCU list iterator to increase num_requests
again
+ */
+ spin_lock_bh(&bat_priv->bla.num_requests_lock);
+ if (backbone_gw->request_sent)
atomic_dec(&bat_priv->bla.num_requests);
+
spin_unlock_bh(&bat_priv->bla.num_requests_lock);
batadv_bla_del_backbone_claims(backbone_gw);
@@ -1517,14 +1530,18 @@ static void batadv_bla_periodic_work(struct work_struct
*work)
* some grace time.
*/
- if (atomic_read(&backbone_gw->request_sent) == 0)
- continue;
+ spin_lock_bh(&bat_priv->bla.num_requests_lock);
+ if (!backbone_gw->request_sent)
+ goto unlock_next;
if (!atomic_dec_and_test(&backbone_gw->wait_periods))
- continue;
+ goto unlock_next;
+ backbone_gw->request_sent = 0;
atomic_dec(&backbone_gw->bat_priv->bla.num_requests);
- atomic_set(&backbone_gw->request_sent, 0);
+
+unlock_next:
+ spin_unlock_bh(&bat_priv->bla.num_requests_lock);
}
rcu_read_unlock();
}
diff --git a/net/batman-adv/mesh-interface.c b/net/batman-adv/mesh-interface.c
index 50c26037..cad65055 100644
--- a/net/batman-adv/mesh-interface.c
+++ b/net/batman-adv/mesh-interface.c
@@ -787,6 +787,7 @@ static int batadv_meshif_init_late(struct net_device *dev)
atomic_set(&bat_priv->tt.ogm_append_cnt, 0);
#ifdef CONFIG_BATMAN_ADV_BLA
atomic_set(&bat_priv->bla.num_requests, 0);
+ spin_lock_init(&bat_priv->bla.num_requests_lock);
#endif
atomic_set(&bat_priv->tp_num, 0);
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
index 739439e2..5b14ead8 100644
--- a/net/batman-adv/types.h
+++ b/net/batman-adv/types.h
@@ -1023,6 +1023,9 @@ struct batadv_priv_bla {
/** @num_requests: number of bla requests in flight */
atomic_t num_requests;
+ /** @num_requests_lock: locks update request_sent + num_request update
*/
+ spinlock_t num_requests_lock;
+
/**
* @claim_hash: hash table containing mesh nodes this host has claimed
*/
@@ -1701,9 +1704,9 @@ struct batadv_bla_backbone_gw {
/**
* @request_sent: if this bool is set to true we are out of sync with
* this backbone gateway - no bcast traffic is formwared until the
- * situation was resolved
+ * situation was resolved. Must only be access with num_requests_lock.
*/
- atomic_t request_sent;
+ u8 request_sent;
/** @crc: crc16 checksum over all claims */
u16 crc;
--
2.47.3
