[B.A.T.M.A.N.] [PATCH v2 1/3] batman-adv: Fix orig_node_vlan leak on orig_node_release

[B.A.T.M.A.N.] [PATCH v2 1/3] batman-adv: Fix orig_node_vlan leak on orig_node_release

[Sven Eckelmann]

batadv_orig_node_new uses batadv_orig_node_vlan_new to allocate a new
batadv_orig_node_vlan and add it to batadv_orig_node::vlan_list. References
to this list have also to be cleaned when the batadv_orig_node is removed.

Fixes: 21a57f6e7a3b ("batman-adv: make the TT CRC logic VLAN specific")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
--
v2:
 - nothing changed
---
 net/batman-adv/originator.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/batman-adv/originator.c b/net/batman-adv/originator.c
index 8755e5c..40626b4 100644
--- a/net/batman-adv/originator.c
+++ b/net/batman-adv/originator.c
@@ -786,6 +786,7 @@ static void batadv_orig_node_release(struct kref *ref)
 	struct batadv_neigh_node *neigh_node;
 	struct batadv_orig_node *orig_node;
 	struct batadv_orig_ifinfo *orig_ifinfo;
+	struct batadv_orig_node_vlan *vlan;
 
 	orig_node = container_of(ref, struct batadv_orig_node, refcount);
 
@@ -805,6 +806,13 @@ static void batadv_orig_node_release(struct kref *ref)
 	}
 	spin_unlock_bh(&orig_node->neigh_list_lock);
 
+	spin_lock_bh(&orig_node->vlan_list_lock);
+	hlist_for_each_entry_safe(vlan, node_tmp, &orig_node->vlan_list, list) {
+		hlist_del_rcu(&vlan->list);
+		batadv_orig_node_vlan_put(vlan);
+	}
+	spin_unlock_bh(&orig_node->vlan_list_lock);
+
 	/* Free nc_nodes */
 	batadv_nc_purge_orig(orig_node->bat_priv, orig_node, NULL);
 
-- 
2.8.1

[B.A.T.M.A.N.] [PATCH v2 2/3] batman-adv: Fix non-atomic bla_claim::backbone_gw access

[Sven Eckelmann]

The pointer batadv_bla_claim::backbone_gw can be changed at any time.
Therefore, access to it must be protected to ensure that two function
accessing the same backbone_gw are actually accessing the same. This is
especially important when the crc_lock is used or when the backbone_gw of a
claim is exchanged.

Not doing so leads to invalid memory access and/or reference leaks.

Fixes: a9ce0dc43e2c ("batman-adv: add basic bridge loop avoidance code")
Fixes: b307e72d119f ("batman-adv: lock crc access in bridge loop avoidance")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
v2:
 - Move backbone release of claim to batadv_claim_release
 - added kerneldoc for backbone_lock
---
 net/batman-adv/bridge_loop_avoidance.c | 110 ++++++++++++++++++++++++++-------
 net/batman-adv/types.h                 |   2 +
 2 files changed, 89 insertions(+), 23 deletions(-)

diff --git a/net/batman-adv/bridge_loop_avoidance.c b/net/batman-adv/bridge_loop_avoidance.c
index c5e62fa..ebc4c2b 100644
--- a/net/batman-adv/bridge_loop_avoidance.c
+++ b/net/batman-adv/bridge_loop_avoidance.c
@@ -178,10 +178,21 @@ static void batadv_backbone_gw_put(struct batadv_bla_backbone_gw *backbone_gw)
 static void batadv_claim_release(struct kref *ref)
 {
 	struct batadv_bla_claim *claim;
+	struct batadv_bla_backbone_gw *old_backbone_gw;
 
 	claim = container_of(ref, struct batadv_bla_claim, refcount);
 
-	batadv_backbone_gw_put(claim->backbone_gw);
+	spin_lock_bh(&claim->backbone_lock);
+	old_backbone_gw = claim->backbone_gw;
+	claim->backbone_gw = NULL;
+	spin_unlock_bh(&claim->backbone_lock);
+
+	spin_lock_bh(&old_backbone_gw->crc_lock);
+	old_backbone_gw->crc ^= crc16(0, claim->addr, ETH_ALEN);
+	spin_unlock_bh(&old_backbone_gw->crc_lock);
+
+	batadv_backbone_gw_put(old_backbone_gw);
+
 	kfree_rcu(claim, rcu);
 }
 
@@ -673,8 +684,10 @@ static void batadv_bla_add_claim(struct batadv_priv *bat_priv,
 				 const u8 *mac, const unsigned short vid,
 				 struct batadv_bla_backbone_gw *backbone_gw)
 {
+	struct batadv_bla_backbone_gw *old_backbone_gw;
 	struct batadv_bla_claim *claim;
 	struct batadv_bla_claim search_claim;
+	bool remove_crc = false;
 	int hash_added;
 
 	ether_addr_copy(search_claim.addr, mac);
@@ -688,8 +701,10 @@ static void batadv_bla_add_claim(struct batadv_priv *bat_priv,
 			return;
 
 		ether_addr_copy(claim->addr, mac);
+		spin_lock_init(&claim->backbone_lock);
 		claim->vid = vid;
 		claim->lasttime = jiffies;
+		kref_get(&backbone_gw->refcount);
 		claim->backbone_gw = backbone_gw;
 		kref_init(&claim->refcount);
 
@@ -718,15 +733,26 @@ static void batadv_bla_add_claim(struct batadv_priv *bat_priv,
 			   "bla_add_claim(): changing ownership for %pM, vid %d\n",
 			   mac, BATADV_PRINT_VID(vid));
 
-		spin_lock_bh(&claim->backbone_gw->crc_lock);
-		claim->backbone_gw->crc ^= crc16(0, claim->addr, ETH_ALEN);
-		spin_unlock_bh(&claim->backbone_gw->crc_lock);
-		batadv_backbone_gw_put(claim->backbone_gw);
+		remove_crc = true;
 	}
-	/* set (new) backbone gw */
+
+	/* replace backbone_gw atomically and adjust reference counters */
+	spin_lock_bh(&claim->backbone_lock);
+	old_backbone_gw = claim->backbone_gw;
 	kref_get(&backbone_gw->refcount);
 	claim->backbone_gw = backbone_gw;
+	spin_unlock_bh(&claim->backbone_lock);
+
+	if (remove_crc) {
+		/* remove claim address from old backbone_gw */
+		spin_lock_bh(&old_backbone_gw->crc_lock);
+		old_backbone_gw->crc ^= crc16(0, claim->addr, ETH_ALEN);
+		spin_unlock_bh(&old_backbone_gw->crc_lock);
+	}
 
+	batadv_backbone_gw_put(old_backbone_gw);
+
+	/* add claim address to new backbone_gw */
 	spin_lock_bh(&backbone_gw->crc_lock);
 	backbone_gw->crc ^= crc16(0, claim->addr, ETH_ALEN);
 	spin_unlock_bh(&backbone_gw->crc_lock);
@@ -737,6 +763,25 @@ claim_free_ref:
 }
 
 /**
+ * batadv_bla_claim_backbone_gw - Get valid reference for backbone_gw of claim
+ * @claim: claim whose backbone_gw should be returned
+ *
+ * Return: valid reference to claim::backbone_gw
+ */
+static struct batadv_bla_backbone_gw *
+batadv_bla_claim_backbone_gw(struct batadv_bla_claim *claim)
+{
+	struct batadv_bla_backbone_gw *backbone_gw;
+
+	spin_lock_bh(&claim->backbone_lock);
+	backbone_gw = claim->backbone_gw;
+	kref_get(&backbone_gw->refcount);
+	spin_unlock_bh(&claim->backbone_lock);
+
+	return backbone_gw;
+}
+
+/**
  * batadv_bla_del_claim - delete a claim from the claim hash
  * @bat_priv: the bat priv with all the soft interface information
  * @mac: mac address of the claim to be removed
@@ -760,10 +805,6 @@ static void batadv_bla_del_claim(struct batadv_priv *bat_priv,
 			   batadv_choose_claim, claim);
 	batadv_claim_put(claim); /* reference from the hash is gone */
 
-	spin_lock_bh(&claim->backbone_gw->crc_lock);
-	claim->backbone_gw->crc ^= crc16(0, claim->addr, ETH_ALEN);
-	spin_unlock_bh(&claim->backbone_gw->crc_lock);
-
 	/* don't need the reference from hash_find() anymore */
 	batadv_claim_put(claim);
 }
@@ -1216,6 +1257,7 @@ static void batadv_bla_purge_claims(struct batadv_priv *bat_priv,
 				    struct batadv_hard_iface *primary_if,
 				    int now)
 {
+	struct batadv_bla_backbone_gw *backbone_gw;
 	struct batadv_bla_claim *claim;
 	struct hlist_head *head;
 	struct batadv_hashtable *hash;
@@ -1230,14 +1272,17 @@ static void batadv_bla_purge_claims(struct batadv_priv *bat_priv,
 
 		rcu_read_lock();
 		hlist_for_each_entry_rcu(claim, head, hash_entry) {
+			backbone_gw = batadv_bla_claim_backbone_gw(claim);
 			if (now)
 				goto purge_now;
-			if (!batadv_compare_eth(claim->backbone_gw->orig,
+
+			if (!batadv_compare_eth(backbone_gw->orig,
 						primary_if->net_dev->dev_addr))
-				continue;
+				goto skip;
+
 			if (!batadv_has_timed_out(claim->lasttime,
 						  BATADV_BLA_CLAIM_TIMEOUT))
-				continue;
+				goto skip;
 
 			batadv_dbg(BATADV_DBG_BLA, bat_priv,
 				   "bla_purge_claims(): %pM, vid %d, time out\n",
@@ -1245,8 +1290,10 @@ static void batadv_bla_purge_claims(struct batadv_priv *bat_priv,
 
 purge_now:
 			batadv_handle_unclaim(bat_priv, primary_if,
-					      claim->backbone_gw->orig,
+					      backbone_gw->orig,
 					      claim->addr, claim->vid);
+skip:
+			batadv_backbone_gw_put(backbone_gw);
 		}
 		rcu_read_unlock();
 	}
@@ -1757,9 +1804,11 @@ batadv_bla_loopdetect_check(struct batadv_priv *bat_priv, struct sk_buff *skb,
 bool batadv_bla_rx(struct batadv_priv *bat_priv, struct sk_buff *skb,
 		   unsigned short vid, bool is_bcast)
 {
+	struct batadv_bla_backbone_gw *backbone_gw;
 	struct ethhdr *ethhdr;
 	struct batadv_bla_claim search_claim, *claim = NULL;
 	struct batadv_hard_iface *primary_if;
+	bool own_claim;
 	bool ret;
 
 	ethhdr = eth_hdr(skb);
@@ -1794,8 +1843,12 @@ bool batadv_bla_rx(struct batadv_priv *bat_priv, struct sk_buff *skb,
 	}
 
 	/* if it is our own claim ... */
-	if (batadv_compare_eth(claim->backbone_gw->orig,
-			       primary_if->net_dev->dev_addr)) {
+	backbone_gw = batadv_bla_claim_backbone_gw(claim);
+	own_claim = batadv_compare_eth(backbone_gw->orig,
+				       primary_if->net_dev->dev_addr);
+	batadv_backbone_gw_put(backbone_gw);
+
+	if (own_claim) {
 		/* ... allow it in any case */
 		claim->lasttime = jiffies;
 		goto allow;
@@ -1859,7 +1912,9 @@ bool batadv_bla_tx(struct batadv_priv *bat_priv, struct sk_buff *skb,
 {
 	struct ethhdr *ethhdr;
 	struct batadv_bla_claim search_claim, *claim = NULL;
+	struct batadv_bla_backbone_gw *backbone_gw;
 	struct batadv_hard_iface *primary_if;
+	bool client_roamed;
 	bool ret = false;
 
 	primary_if = batadv_primary_if_get_selected(bat_priv);
@@ -1889,8 +1944,12 @@ bool batadv_bla_tx(struct batadv_priv *bat_priv, struct sk_buff *skb,
 		goto allow;
 
 	/* check if we are responsible. */
-	if (batadv_compare_eth(claim->backbone_gw->orig,
-			       primary_if->net_dev->dev_addr)) {
+	backbone_gw = batadv_bla_claim_backbone_gw(claim);
+	client_roamed = batadv_compare_eth(backbone_gw->orig,
+					   primary_if->net_dev->dev_addr);
+	batadv_backbone_gw_put(backbone_gw);
+
+	if (client_roamed) {
 		/* if yes, the client has roamed and we have
 		 * to unclaim it.
 		 */
@@ -1938,6 +1997,7 @@ int batadv_bla_claim_table_seq_print_text(struct seq_file *seq, void *offset)
 	struct net_device *net_dev = (struct net_device *)seq->private;
 	struct batadv_priv *bat_priv = netdev_priv(net_dev);
 	struct batadv_hashtable *hash = bat_priv->bla.claim_hash;
+	struct batadv_bla_backbone_gw *backbone_gw;
 	struct batadv_bla_claim *claim;
 	struct batadv_hard_iface *primary_if;
 	struct hlist_head *head;
@@ -1962,17 +2022,21 @@ int batadv_bla_claim_table_seq_print_text(struct seq_file *seq, void *offset)
 
 		rcu_read_lock();
 		hlist_for_each_entry_rcu(claim, head, hash_entry) {
-			is_own = batadv_compare_eth(claim->backbone_gw->orig,
+			backbone_gw = batadv_bla_claim_backbone_gw(claim);
+
+			is_own = batadv_compare_eth(backbone_gw->orig,
 						    primary_addr);
 
-			spin_lock_bh(&claim->backbone_gw->crc_lock);
-			backbone_crc = claim->backbone_gw->crc;
-			spin_unlock_bh(&claim->backbone_gw->crc_lock);
+			spin_lock_bh(&backbone_gw->crc_lock);
+			backbone_crc = backbone_gw->crc;
+			spin_unlock_bh(&backbone_gw->crc_lock);
 			seq_printf(seq, " * %pM on %5d by %pM [%c] (%#.4x)\n",
 				   claim->addr, BATADV_PRINT_VID(claim->vid),
-				   claim->backbone_gw->orig,
+				   backbone_gw->orig,
 				   (is_own ? 'x' : ' '),
 				   backbone_crc);
+
+			batadv_backbone_gw_put(backbone_gw);
 		}
 		rcu_read_unlock();
 	}
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
index fb445c5..d82f6b4 100644
--- a/net/batman-adv/types.h
+++ b/net/batman-adv/types.h
@@ -1167,6 +1167,7 @@ struct batadv_bla_backbone_gw {
  * @addr: mac address of claimed non-mesh client
  * @vid: vlan id this client was detected on
  * @backbone_gw: pointer to backbone gw claiming this client
+ * @backbone_lock: lock protecting backbone_gw pointer
  * @lasttime: last time we heard of claim (locals only)
  * @hash_entry: hlist node for batadv_priv_bla::claim_hash
  * @refcount: number of contexts the object is used
@@ -1176,6 +1177,7 @@ struct batadv_bla_claim {
 	u8 addr[ETH_ALEN];
 	unsigned short vid;
 	struct batadv_bla_backbone_gw *backbone_gw;
+	spinlock_t backbone_lock; /* protects backbone_gw */
 	unsigned long lasttime;
 	struct hlist_node hash_entry;
 	struct rcu_head rcu;
-- 
2.8.1

[B.A.T.M.A.N.] [PATCH v2 3/3] batman-adv: Fix reference leak in batadv_find_router

[Sven Eckelmann]

The replacement of last_bonding_candidate in batadv_orig_node has to be an
atomic operation. Otherwise it is possible that the reference counter of a
batadv_orig_ifinfo is reduced which was no longer the
last_bonding_candidate when the new candidate is added. This can either
lead to an invalid memory access or to reference leaks which make it
impossible to an interface which was added to batman-adv.

Fixes: 797edd9e87ac ("batman-adv: add bonding again")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
v2:
 - get refcnt for new selected router before assigning it to returned
   variable
 - move refcnt cleanup of all remembered candidates/routers to central place
---
 net/batman-adv/routing.c | 52 ++++++++++++++++++++++++++++++++++++------------
 net/batman-adv/types.h   |  4 +++-
 2 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
index bba9276..a4253d4 100644
--- a/net/batman-adv/routing.c
+++ b/net/batman-adv/routing.c
@@ -462,6 +462,29 @@ static int batadv_check_unicast_packet(struct batadv_priv *bat_priv,
 }
 
 /**
+ * batadv_last_bonding_replace - Replace last_bonding_candidate of orig_node
+ * @orig_node: originator node whose bonding candidates should be replaced
+ * @new_candidate: new bonding candidate or NULL
+ */
+static void
+batadv_last_bonding_replace(struct batadv_orig_node *orig_node,
+			    struct batadv_orig_ifinfo *new_candidate)
+{
+	struct batadv_orig_ifinfo *old_candidate;
+
+	spin_lock_bh(&orig_node->neigh_list_lock);
+	old_candidate = orig_node->last_bonding_candidate;
+
+	if (new_candidate)
+		kref_get(&new_candidate->refcount);
+	orig_node->last_bonding_candidate = new_candidate;
+	spin_unlock_bh(&orig_node->neigh_list_lock);
+
+	if (old_candidate)
+		batadv_orig_ifinfo_put(old_candidate);
+}
+
+/**
  * batadv_find_router - find a suitable router for this originator
  * @bat_priv: the bat priv with all the soft interface information
  * @orig_node: the destination node
@@ -568,10 +591,6 @@ next:
 	}
 	rcu_read_unlock();
 
-	/* last_bonding_candidate is reset below, remove the old reference. */
-	if (orig_node->last_bonding_candidate)
-		batadv_orig_ifinfo_put(orig_node->last_bonding_candidate);
-
 	/* After finding candidates, handle the three cases:
 	 * 1) there is a next candidate, use that
 	 * 2) there is no next candidate, use the first of the list
@@ -580,21 +599,28 @@ next:
 	if (next_candidate) {
 		batadv_neigh_node_put(router);
 
-		/* remove references to first candidate, we don't need it. */
-		if (first_candidate) {
-			batadv_neigh_node_put(first_candidate_router);
-			batadv_orig_ifinfo_put(first_candidate);
-		}
+		kref_get(&next_candidate_router->refcount);
 		router = next_candidate_router;
-		orig_node->last_bonding_candidate = next_candidate;
+		batadv_last_bonding_replace(orig_node, next_candidate);
 	} else if (first_candidate) {
 		batadv_neigh_node_put(router);
 
-		/* refcounting has already been done in the loop above. */
+		kref_get(&first_candidate_router->refcount);
 		router = first_candidate_router;
-		orig_node->last_bonding_candidate = first_candidate;
+		batadv_last_bonding_replace(orig_node, first_candidate);
 	} else {
-		orig_node->last_bonding_candidate = NULL;
+		batadv_last_bonding_replace(orig_node, NULL);
+	}
+
+	/* cleanup of candidates */
+	if (first_candidate) {
+		batadv_neigh_node_put(first_candidate_router);
+		batadv_orig_ifinfo_put(first_candidate);
+	}
+
+	if (next_candidate) {
+		batadv_neigh_node_put(next_candidate_router);
+		batadv_orig_ifinfo_put(next_candidate);
 	}
 
 	return router;
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
index d82f6b4..96af6da 100644
--- a/net/batman-adv/types.h
+++ b/net/batman-adv/types.h
@@ -329,7 +329,9 @@ struct batadv_orig_node {
 	DECLARE_BITMAP(bcast_bits, BATADV_TQ_LOCAL_WINDOW_SIZE);
 	u32 last_bcast_seqno;
 	struct hlist_head neigh_list;
-	/* neigh_list_lock protects: neigh_list and router */
+	/* neigh_list_lock protects: neigh_list, ifinfo_list,
+	 * last_bonding_candidate and router
+	 */
 	spinlock_t neigh_list_lock;
 	struct hlist_node hash_entry;
 	struct batadv_priv *bat_priv;
-- 
2.8.1

[B.A.T.M.A.N.] [PATCH maint v2 4/4] batman-adv: Free last_bonding_candidate on release of orig_node

[Sven Eckelmann]

The orig_ifinfo reference counter for last_bonding_candidate in
batadv_orig_node has to be reduced when an originator node is released.
Otherwise the orig_ifinfo is leaked and the reference counter the netdevice
is not reduced correctly.

Fixes: 797edd9e87ac ("batman-adv: add bonding again")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
v2:
 - added new patch
---
 net/batman-adv/originator.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/batman-adv/originator.c b/net/batman-adv/originator.c
index 40626b4..8208276 100644
--- a/net/batman-adv/originator.c
+++ b/net/batman-adv/originator.c
@@ -787,6 +787,7 @@ static void batadv_orig_node_release(struct kref *ref)
 	struct batadv_orig_node *orig_node;
 	struct batadv_orig_ifinfo *orig_ifinfo;
 	struct batadv_orig_node_vlan *vlan;
+	struct batadv_orig_ifinfo *last_candidate;
 
 	orig_node = container_of(ref, struct batadv_orig_node, refcount);
 
@@ -804,8 +805,14 @@ static void batadv_orig_node_release(struct kref *ref)
 		hlist_del_rcu(&orig_ifinfo->list);
 		batadv_orig_ifinfo_put(orig_ifinfo);
 	}
+
+	last_candidate = orig_node->last_bonding_candidate;
+	orig_node->last_bonding_candidate = NULL;
 	spin_unlock_bh(&orig_node->neigh_list_lock);
 
+	if (last_candidate)
+		batadv_orig_ifinfo_put(last_candidate);
+
 	spin_lock_bh(&orig_node->vlan_list_lock);
 	hlist_for_each_entry_safe(vlan, node_tmp, &orig_node->vlan_list, list) {
 		hlist_del_rcu(&vlan->list);
-- 
2.8.1

Re: [B.A.T.M.A.N.] [PATCH v2 3/3] batman-adv: Fix reference leak in batadv_find_router

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 4556 bytes --]

Acked-by: Simon Wunderlich <sw@simonwunderlich.de>

Thanks, this also looks much cleaner now!
     Simon

On Thursday 30 June 2016 20:11:34 Sven Eckelmann wrote:
> The replacement of last_bonding_candidate in batadv_orig_node has to be an
> atomic operation. Otherwise it is possible that the reference counter of a
> batadv_orig_ifinfo is reduced which was no longer the
> last_bonding_candidate when the new candidate is added. This can either
> lead to an invalid memory access or to reference leaks which make it
> impossible to an interface which was added to batman-adv.
> 
> Fixes: 797edd9e87ac ("batman-adv: add bonding again")
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
> v2:
>  - get refcnt for new selected router before assigning it to returned
>    variable
>  - move refcnt cleanup of all remembered candidates/routers to central place
> ---
>  net/batman-adv/routing.c | 52
> ++++++++++++++++++++++++++++++++++++------------ net/batman-adv/types.h   |
>  4 +++-
>  2 files changed, 42 insertions(+), 14 deletions(-)
> 
> diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
> index bba9276..a4253d4 100644
> --- a/net/batman-adv/routing.c
> +++ b/net/batman-adv/routing.c
> @@ -462,6 +462,29 @@ static int batadv_check_unicast_packet(struct
> batadv_priv *bat_priv, }
> 
>  /**
> + * batadv_last_bonding_replace - Replace last_bonding_candidate of
> orig_node + * @orig_node: originator node whose bonding candidates should
> be replaced + * @new_candidate: new bonding candidate or NULL
> + */
> +static void
> +batadv_last_bonding_replace(struct batadv_orig_node *orig_node,
> +			    struct batadv_orig_ifinfo *new_candidate)
> +{
> +	struct batadv_orig_ifinfo *old_candidate;
> +
> +	spin_lock_bh(&orig_node->neigh_list_lock);
> +	old_candidate = orig_node->last_bonding_candidate;
> +
> +	if (new_candidate)
> +		kref_get(&new_candidate->refcount);
> +	orig_node->last_bonding_candidate = new_candidate;
> +	spin_unlock_bh(&orig_node->neigh_list_lock);
> +
> +	if (old_candidate)
> +		batadv_orig_ifinfo_put(old_candidate);
> +}
> +
> +/**
>   * batadv_find_router - find a suitable router for this originator
>   * @bat_priv: the bat priv with all the soft interface information
>   * @orig_node: the destination node
> @@ -568,10 +591,6 @@ next:
>  	}
>  	rcu_read_unlock();
> 
> -	/* last_bonding_candidate is reset below, remove the old reference. */
> -	if (orig_node->last_bonding_candidate)
> -		batadv_orig_ifinfo_put(orig_node->last_bonding_candidate);
> -
>  	/* After finding candidates, handle the three cases:
>  	 * 1) there is a next candidate, use that
>  	 * 2) there is no next candidate, use the first of the list
> @@ -580,21 +599,28 @@ next:
>  	if (next_candidate) {
>  		batadv_neigh_node_put(router);
> 
> -		/* remove references to first candidate, we don't need it. */
> -		if (first_candidate) {
> -			batadv_neigh_node_put(first_candidate_router);
> -			batadv_orig_ifinfo_put(first_candidate);
> -		}
> +		kref_get(&next_candidate_router->refcount);
>  		router = next_candidate_router;
> -		orig_node->last_bonding_candidate = next_candidate;
> +		batadv_last_bonding_replace(orig_node, next_candidate);
>  	} else if (first_candidate) {
>  		batadv_neigh_node_put(router);
> 
> -		/* refcounting has already been done in the loop above. */
> +		kref_get(&first_candidate_router->refcount);
>  		router = first_candidate_router;
> -		orig_node->last_bonding_candidate = first_candidate;
> +		batadv_last_bonding_replace(orig_node, first_candidate);
>  	} else {
> -		orig_node->last_bonding_candidate = NULL;
> +		batadv_last_bonding_replace(orig_node, NULL);
> +	}
> +
> +	/* cleanup of candidates */
> +	if (first_candidate) {
> +		batadv_neigh_node_put(first_candidate_router);
> +		batadv_orig_ifinfo_put(first_candidate);
> +	}
> +
> +	if (next_candidate) {
> +		batadv_neigh_node_put(next_candidate_router);
> +		batadv_orig_ifinfo_put(next_candidate);
>  	}
> 
>  	return router;
> diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
> index d82f6b4..96af6da 100644
> --- a/net/batman-adv/types.h
> +++ b/net/batman-adv/types.h
> @@ -329,7 +329,9 @@ struct batadv_orig_node {
>  	DECLARE_BITMAP(bcast_bits, BATADV_TQ_LOCAL_WINDOW_SIZE);
>  	u32 last_bcast_seqno;
>  	struct hlist_head neigh_list;
> -	/* neigh_list_lock protects: neigh_list and router */
> +	/* neigh_list_lock protects: neigh_list, ifinfo_list,
> +	 * last_bonding_candidate and router
> +	 */
>  	spinlock_t neigh_list_lock;
>  	struct hlist_node hash_entry;
>  	struct batadv_priv *bat_priv;

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH v2 2/3] batman-adv: Fix non-atomic bla_claim::backbone_gw access

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 974 bytes --]

On Thursday 30 June 2016 20:11:33 Sven Eckelmann wrote:
> The pointer batadv_bla_claim::backbone_gw can be changed at any time.
> Therefore, access to it must be protected to ensure that two function
> accessing the same backbone_gw are actually accessing the same. This is
> especially important when the crc_lock is used or when the backbone_gw of a
> claim is exchanged.
> 
> Not doing so leads to invalid memory access and/or reference leaks.
> 
> Fixes: a9ce0dc43e2c ("batman-adv: add basic bridge loop avoidance code")
> Fixes: b307e72d119f ("batman-adv: lock crc access in bridge loop avoidance")
> Signed-off-by: Sven Eckelmann <sven@narfation.org>

Technically, you can already add my "Acked-by". However I'd like to suggest 
one style change: Could you please rename batadv_bla_claim_backbone_gw into 
batadv_bla_claim_get_backbone_gw? I think this would make it clearer what the 
function does and also shows get/put pairs clearly in the code.

Thanks!
     Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[B.A.T.M.A.N.] [PATCH v3 2/4] batman-adv: Fix non-atomic bla_claim::backbone_gw access

[Sven Eckelmann]

The pointer batadv_bla_claim::backbone_gw can be changed at any time.
Therefore, access to it must be protected to ensure that two function
accessing the same backbone_gw are actually accessing the same. This is
especially important when the crc_lock is used or when the backbone_gw of a
claim is exchanged.

Not doing so leads to invalid memory access and/or reference leaks.

Fixes: a9ce0dc43e2c ("batman-adv: add basic bridge loop avoidance code")
Fixes: b307e72d119f ("batman-adv: lock crc access in bridge loop avoidance")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
v3:
 - rename batadv_bla_claim_backbone_gw to batadv_bla_claim_get_backbone_gw
v2:
 - Move backbone release of claim to batadv_claim_release
 - added kerneldoc for backbone_lock
---
 net/batman-adv/bridge_loop_avoidance.c | 111 ++++++++++++++++++++++++++-------
 net/batman-adv/types.h                 |   2 +
 2 files changed, 90 insertions(+), 23 deletions(-)

diff --git a/net/batman-adv/bridge_loop_avoidance.c b/net/batman-adv/bridge_loop_avoidance.c
index 748a9ea..a796bde 100644
--- a/net/batman-adv/bridge_loop_avoidance.c
+++ b/net/batman-adv/bridge_loop_avoidance.c
@@ -177,10 +177,21 @@ static void batadv_backbone_gw_put(struct batadv_bla_backbone_gw *backbone_gw)
 static void batadv_claim_release(struct kref *ref)
 {
 	struct batadv_bla_claim *claim;
+	struct batadv_bla_backbone_gw *old_backbone_gw;
 
 	claim = container_of(ref, struct batadv_bla_claim, refcount);
 
-	batadv_backbone_gw_put(claim->backbone_gw);
+	spin_lock_bh(&claim->backbone_lock);
+	old_backbone_gw = claim->backbone_gw;
+	claim->backbone_gw = NULL;
+	spin_unlock_bh(&claim->backbone_lock);
+
+	spin_lock_bh(&old_backbone_gw->crc_lock);
+	old_backbone_gw->crc ^= crc16(0, claim->addr, ETH_ALEN);
+	spin_unlock_bh(&old_backbone_gw->crc_lock);
+
+	batadv_backbone_gw_put(old_backbone_gw);
+
 	kfree_rcu(claim, rcu);
 }
 
@@ -674,8 +685,10 @@ static void batadv_bla_add_claim(struct batadv_priv *bat_priv,
 				 const u8 *mac, const unsigned short vid,
 				 struct batadv_bla_backbone_gw *backbone_gw)
 {
+	struct batadv_bla_backbone_gw *old_backbone_gw;
 	struct batadv_bla_claim *claim;
 	struct batadv_bla_claim search_claim;
+	bool remove_crc = false;
 	int hash_added;
 
 	ether_addr_copy(search_claim.addr, mac);
@@ -689,8 +702,10 @@ static void batadv_bla_add_claim(struct batadv_priv *bat_priv,
 			return;
 
 		ether_addr_copy(claim->addr, mac);
+		spin_lock_init(&claim->backbone_lock);
 		claim->vid = vid;
 		claim->lasttime = jiffies;
+		kref_get(&backbone_gw->refcount);
 		claim->backbone_gw = backbone_gw;
 
 		kref_init(&claim->refcount);
@@ -718,15 +733,26 @@ static void batadv_bla_add_claim(struct batadv_priv *bat_priv,
 			   "bla_add_claim(): changing ownership for %pM, vid %d\n",
 			   mac, BATADV_PRINT_VID(vid));
 
-		spin_lock_bh(&claim->backbone_gw->crc_lock);
-		claim->backbone_gw->crc ^= crc16(0, claim->addr, ETH_ALEN);
-		spin_unlock_bh(&claim->backbone_gw->crc_lock);
-		batadv_backbone_gw_put(claim->backbone_gw);
+		remove_crc = true;
 	}
-	/* set (new) backbone gw */
+
+	/* replace backbone_gw atomically and adjust reference counters */
+	spin_lock_bh(&claim->backbone_lock);
+	old_backbone_gw = claim->backbone_gw;
 	kref_get(&backbone_gw->refcount);
 	claim->backbone_gw = backbone_gw;
+	spin_unlock_bh(&claim->backbone_lock);
+
+	if (remove_crc) {
+		/* remove claim address from old backbone_gw */
+		spin_lock_bh(&old_backbone_gw->crc_lock);
+		old_backbone_gw->crc ^= crc16(0, claim->addr, ETH_ALEN);
+		spin_unlock_bh(&old_backbone_gw->crc_lock);
+	}
 
+	batadv_backbone_gw_put(old_backbone_gw);
+
+	/* add claim address to new backbone_gw */
 	spin_lock_bh(&backbone_gw->crc_lock);
 	backbone_gw->crc ^= crc16(0, claim->addr, ETH_ALEN);
 	spin_unlock_bh(&backbone_gw->crc_lock);
@@ -737,6 +763,26 @@ claim_free_ref:
 }
 
 /**
+ * batadv_bla_claim_get_backbone_gw - Get valid reference for backbone_gw of
+ *  claim
+ * @claim: claim whose backbone_gw should be returned
+ *
+ * Return: valid reference to claim::backbone_gw
+ */
+static struct batadv_bla_backbone_gw *
+batadv_bla_claim_get_backbone_gw(struct batadv_bla_claim *claim)
+{
+	struct batadv_bla_backbone_gw *backbone_gw;
+
+	spin_lock_bh(&claim->backbone_lock);
+	backbone_gw = claim->backbone_gw;
+	kref_get(&backbone_gw->refcount);
+	spin_unlock_bh(&claim->backbone_lock);
+
+	return backbone_gw;
+}
+
+/**
  * batadv_bla_del_claim - delete a claim from the claim hash
  * @bat_priv: the bat priv with all the soft interface information
  * @mac: mac address of the claim to be removed
@@ -760,10 +806,6 @@ static void batadv_bla_del_claim(struct batadv_priv *bat_priv,
 			   batadv_choose_claim, claim);
 	batadv_claim_put(claim); /* reference from the hash is gone */
 
-	spin_lock_bh(&claim->backbone_gw->crc_lock);
-	claim->backbone_gw->crc ^= crc16(0, claim->addr, ETH_ALEN);
-	spin_unlock_bh(&claim->backbone_gw->crc_lock);
-
 	/* don't need the reference from hash_find() anymore */
 	batadv_claim_put(claim);
 }
@@ -1216,6 +1258,7 @@ static void batadv_bla_purge_claims(struct batadv_priv *bat_priv,
 				    struct batadv_hard_iface *primary_if,
 				    int now)
 {
+	struct batadv_bla_backbone_gw *backbone_gw;
 	struct batadv_bla_claim *claim;
 	struct hlist_head *head;
 	struct batadv_hashtable *hash;
@@ -1230,14 +1273,17 @@ static void batadv_bla_purge_claims(struct batadv_priv *bat_priv,
 
 		rcu_read_lock();
 		hlist_for_each_entry_rcu(claim, head, hash_entry) {
+			backbone_gw = batadv_bla_claim_get_backbone_gw(claim);
 			if (now)
 				goto purge_now;
-			if (!batadv_compare_eth(claim->backbone_gw->orig,
+
+			if (!batadv_compare_eth(backbone_gw->orig,
 						primary_if->net_dev->dev_addr))
-				continue;
+				goto skip;
+
 			if (!batadv_has_timed_out(claim->lasttime,
 						  BATADV_BLA_CLAIM_TIMEOUT))
-				continue;
+				goto skip;
 
 			batadv_dbg(BATADV_DBG_BLA, bat_priv,
 				   "bla_purge_claims(): %pM, vid %d, time out\n",
@@ -1245,8 +1291,10 @@ static void batadv_bla_purge_claims(struct batadv_priv *bat_priv,
 
 purge_now:
 			batadv_handle_unclaim(bat_priv, primary_if,
-					      claim->backbone_gw->orig,
+					      backbone_gw->orig,
 					      claim->addr, claim->vid);
+skip:
+			batadv_backbone_gw_put(backbone_gw);
 		}
 		rcu_read_unlock();
 	}
@@ -1757,9 +1805,11 @@ batadv_bla_loopdetect_check(struct batadv_priv *bat_priv, struct sk_buff *skb,
 bool batadv_bla_rx(struct batadv_priv *bat_priv, struct sk_buff *skb,
 		   unsigned short vid, bool is_bcast)
 {
+	struct batadv_bla_backbone_gw *backbone_gw;
 	struct ethhdr *ethhdr;
 	struct batadv_bla_claim search_claim, *claim = NULL;
 	struct batadv_hard_iface *primary_if;
+	bool own_claim;
 	bool ret;
 
 	ethhdr = eth_hdr(skb);
@@ -1794,8 +1844,12 @@ bool batadv_bla_rx(struct batadv_priv *bat_priv, struct sk_buff *skb,
 	}
 
 	/* if it is our own claim ... */
-	if (batadv_compare_eth(claim->backbone_gw->orig,
-			       primary_if->net_dev->dev_addr)) {
+	backbone_gw = batadv_bla_claim_get_backbone_gw(claim);
+	own_claim = batadv_compare_eth(backbone_gw->orig,
+				       primary_if->net_dev->dev_addr);
+	batadv_backbone_gw_put(backbone_gw);
+
+	if (own_claim) {
 		/* ... allow it in any case */
 		claim->lasttime = jiffies;
 		goto allow;
@@ -1859,7 +1913,9 @@ bool batadv_bla_tx(struct batadv_priv *bat_priv, struct sk_buff *skb,
 {
 	struct ethhdr *ethhdr;
 	struct batadv_bla_claim search_claim, *claim = NULL;
+	struct batadv_bla_backbone_gw *backbone_gw;
 	struct batadv_hard_iface *primary_if;
+	bool client_roamed;
 	bool ret = false;
 
 	primary_if = batadv_primary_if_get_selected(bat_priv);
@@ -1889,8 +1945,12 @@ bool batadv_bla_tx(struct batadv_priv *bat_priv, struct sk_buff *skb,
 		goto allow;
 
 	/* check if we are responsible. */
-	if (batadv_compare_eth(claim->backbone_gw->orig,
-			       primary_if->net_dev->dev_addr)) {
+	backbone_gw = batadv_bla_claim_get_backbone_gw(claim);
+	client_roamed = batadv_compare_eth(backbone_gw->orig,
+					   primary_if->net_dev->dev_addr);
+	batadv_backbone_gw_put(backbone_gw);
+
+	if (client_roamed) {
 		/* if yes, the client has roamed and we have
 		 * to unclaim it.
 		 */
@@ -1938,6 +1998,7 @@ int batadv_bla_claim_table_seq_print_text(struct seq_file *seq, void *offset)
 	struct net_device *net_dev = (struct net_device *)seq->private;
 	struct batadv_priv *bat_priv = netdev_priv(net_dev);
 	struct batadv_hashtable *hash = bat_priv->bla.claim_hash;
+	struct batadv_bla_backbone_gw *backbone_gw;
 	struct batadv_bla_claim *claim;
 	struct batadv_hard_iface *primary_if;
 	struct hlist_head *head;
@@ -1962,17 +2023,21 @@ int batadv_bla_claim_table_seq_print_text(struct seq_file *seq, void *offset)
 
 		rcu_read_lock();
 		hlist_for_each_entry_rcu(claim, head, hash_entry) {
-			is_own = batadv_compare_eth(claim->backbone_gw->orig,
+			backbone_gw = batadv_bla_claim_get_backbone_gw(claim);
+
+			is_own = batadv_compare_eth(backbone_gw->orig,
 						    primary_addr);
 
-			spin_lock_bh(&claim->backbone_gw->crc_lock);
-			backbone_crc = claim->backbone_gw->crc;
-			spin_unlock_bh(&claim->backbone_gw->crc_lock);
+			spin_lock_bh(&backbone_gw->crc_lock);
+			backbone_crc = backbone_gw->crc;
+			spin_unlock_bh(&backbone_gw->crc_lock);
 			seq_printf(seq, " * %pM on %5d by %pM [%c] (%#.4x)\n",
 				   claim->addr, BATADV_PRINT_VID(claim->vid),
-				   claim->backbone_gw->orig,
+				   backbone_gw->orig,
 				   (is_own ? 'x' : ' '),
 				   backbone_crc);
+
+			batadv_backbone_gw_put(backbone_gw);
 		}
 		rcu_read_unlock();
 	}
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
index ba846b0..0051222 100644
--- a/net/batman-adv/types.h
+++ b/net/batman-adv/types.h
@@ -1042,6 +1042,7 @@ struct batadv_bla_backbone_gw {
  * @addr: mac address of claimed non-mesh client
  * @vid: vlan id this client was detected on
  * @backbone_gw: pointer to backbone gw claiming this client
+ * @backbone_lock: lock protecting backbone_gw pointer
  * @lasttime: last time we heard of claim (locals only)
  * @hash_entry: hlist node for batadv_priv_bla::claim_hash
  * @refcount: number of contexts the object is used
@@ -1051,6 +1052,7 @@ struct batadv_bla_claim {
 	u8 addr[ETH_ALEN];
 	unsigned short vid;
 	struct batadv_bla_backbone_gw *backbone_gw;
+	spinlock_t backbone_lock; /* protects backbone_gw */
 	unsigned long lasttime;
 	struct hlist_node hash_entry;
 	struct rcu_head rcu;
-- 
2.8.1

Re: [B.A.T.M.A.N.] [PATCH v3 2/4] batman-adv: Fix non-atomic bla_claim::backbone_gw access

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 726 bytes --]

On Friday 01 July 2016 15:49:43 Sven Eckelmann wrote:
> The pointer batadv_bla_claim::backbone_gw can be changed at any time.
> Therefore, access to it must be protected to ensure that two function
> accessing the same backbone_gw are actually accessing the same. This is
> especially important when the crc_lock is used or when the backbone_gw of a
> claim is exchanged.
> 
> Not doing so leads to invalid memory access and/or reference leaks.
> 
> Fixes: a9ce0dc43e2c ("batman-adv: add basic bridge loop avoidance code")
> Fixes: b307e72d119f ("batman-adv: lock crc access in bridge loop avoidance")
> Signed-off-by: Sven Eckelmann <sven@narfation.org>

Acked-by: Simon Wunderlich <sw@simonwunderlich.de>

Thanks,
     Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH v2 1/3] batman-adv: Fix orig_node_vlan leak on orig_node_release

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 577 bytes --]

On Thursday, June 30, 2016 20:10:46 Sven Eckelmann wrote:
> batadv_orig_node_new uses batadv_orig_node_vlan_new to allocate a new
> batadv_orig_node_vlan and add it to batadv_orig_node::vlan_list. References
> to this list have also to be cleaned when the batadv_orig_node is removed.
> 
> Fixes: 21a57f6e7a3b ("batman-adv: make the TT CRC logic VLAN specific")
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> --
> v2:
>  - nothing changed
> ---
>  net/batman-adv/originator.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

Applied in revision 719afd2.

Thanks,
Marek

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [B.A.T.M.A.N.] [PATCH v3 2/4] batman-adv: Fix non-atomic bla_claim::backbone_gw access

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]

On Friday, July 01, 2016 15:54:41 Simon Wunderlich wrote:
> On Friday 01 July 2016 15:49:43 Sven Eckelmann wrote:
> > The pointer batadv_bla_claim::backbone_gw can be changed at any time.
> > Therefore, access to it must be protected to ensure that two function
> > accessing the same backbone_gw are actually accessing the same. This is
> > especially important when the crc_lock is used or when the backbone_gw of
> > a
> > claim is exchanged.
> >
> > Not doing so leads to invalid memory access and/or reference leaks.
> >
> > Fixes: a9ce0dc43e2c ("batman-adv: add basic bridge loop avoidance code")
> > Fixes: b307e72d119f ("batman-adv: lock crc access in bridge loop
> > avoidance") Signed-off-by: Sven Eckelmann <sven@narfation.org>
> 
> Acked-by: Simon Wunderlich <sw@simonwunderlich.de>

Applied in revision e401297.

Thanks,
Marek

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [B.A.T.M.A.N.] [PATCH v2 3/3] batman-adv: Fix reference leak in batadv_find_router

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 989 bytes --]

On Thursday, June 30, 2016 20:11:34 Sven Eckelmann wrote:
> The replacement of last_bonding_candidate in batadv_orig_node has to be an
> atomic operation. Otherwise it is possible that the reference counter of a
> batadv_orig_ifinfo is reduced which was no longer the
> last_bonding_candidate when the new candidate is added. This can either
> lead to an invalid memory access or to reference leaks which make it
> impossible to an interface which was added to batman-adv.
> 
> Fixes: 797edd9e87ac ("batman-adv: add bonding again")
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
> v2:
>  - get refcnt for new selected router before assigning it to returned
>    variable
>  - move refcnt cleanup of all remembered candidates/routers to central place
> ---
>  net/batman-adv/routing.c | 52
> ++++++++++++++++++++++++++++++++++++------------
> net/batman-adv/types.h   |  4 +++-
>  2 files changed, 42 insertions(+), 14 deletions(-)

Applied in revision 6ecc711.

Thanks,
Marek


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [B.A.T.M.A.N.] [PATCH maint v2 4/4] batman-adv: Free last_bonding_candidate on release of orig_node

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 578 bytes --]

On Thursday, June 30, 2016 21:41:13 Sven Eckelmann wrote:
> The orig_ifinfo reference counter for last_bonding_candidate in
> batadv_orig_node has to be reduced when an originator node is released.
> Otherwise the orig_ifinfo is leaked and the reference counter the netdevice
> is not reduced correctly.
> 
> Fixes: 797edd9e87ac ("batman-adv: add bonding again")
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
> v2:
>  - added new patch
> ---
>  net/batman-adv/originator.c | 7 +++++++
>  1 file changed, 7 insertions(+)

Applied in revision 20df5c5.

Thanks,
Marek

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]