[B.A.T.M.A.N.] [PATCH 1/2] batman-adv: Fix reference counting of vlan object for tt_local_entry

[B.A.T.M.A.N.] [PATCH 1/2] batman-adv: Fix reference counting of vlan object for tt_local_entry

[Sven Eckelmann]

The batadv_tt_local_entry was specific to a batadv_softif_vlan and held an
implicit reference to it. But this reference was never stored in form of a
pointer in the tt_local_entry itself. Instead batadv_tt_local_remove,
batadv_tt_local_table_free and batadv_tt_local_purge_pending_clients depend
on a consistent state of bat_priv->softif_vlan_list and that
batadv_softif_vlan_get always returns the batadv_softif_vlan object which
it has a reference for. But batadv_softif_vlan_get cannot guarantee that
because it is working only with rcu_read_lock on this list. It can
therefore happen that an vid is in this list twice or that
batadv_softif_vlan_get cannot find the batadv_softif_vlan for an vid due to
some other list operations taking place at the same time.

Instead add a batadv_softif_vlan pointer directly in batadv_tt_local_entry
which will be used for the reference counter decremented on release of
batadv_tt_local_entry.

Fixes: 9729d2085c0f ("batman-adv: fix TT VLAN inconsistency on VLAN re-add")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
See https://www.open-mesh.org/issues/243

v1:
 - Remove the old code blocks
 - make the _put unconditional
---
 net/batman-adv/translation-table.c | 42 ++++----------------------------------
 net/batman-adv/types.h             |  2 ++
 2 files changed, 6 insertions(+), 38 deletions(-)

diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 2ed55f4..d325224 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -215,6 +215,8 @@ static void batadv_tt_local_entry_release(struct kref *ref)
 	tt_local_entry = container_of(ref, struct batadv_tt_local_entry,
 				      common.refcount);
 
+	batadv_softif_vlan_put(tt_local_entry->vlan);
+
 	kfree_rcu(tt_local_entry, common.rcu);
 }
 
@@ -673,6 +675,7 @@ bool batadv_tt_local_add(struct net_device *soft_iface, const u8 *addr,
 	kref_get(&tt_local->common.refcount);
 	tt_local->last_seen = jiffies;
 	tt_local->common.added_at = tt_local->last_seen;
+	tt_local->vlan = vlan;
 
 	/* the batman interface mac and multicast addresses should never be
 	 * purged
@@ -991,7 +994,6 @@ int batadv_tt_local_seq_print_text(struct seq_file *seq, void *offset)
 	struct batadv_tt_common_entry *tt_common_entry;
 	struct batadv_tt_local_entry *tt_local;
 	struct batadv_hard_iface *primary_if;
-	struct batadv_softif_vlan *vlan;
 	struct hlist_head *head;
 	unsigned short vid;
 	u32 i;
@@ -1027,14 +1029,6 @@ int batadv_tt_local_seq_print_text(struct seq_file *seq, void *offset)
 			last_seen_msecs = last_seen_msecs % 1000;
 
 			no_purge = tt_common_entry->flags & np_flag;
-
-			vlan = batadv_softif_vlan_get(bat_priv, vid);
-			if (!vlan) {
-				seq_printf(seq, "Cannot retrieve VLAN %d\n",
-					   BATADV_PRINT_VID(vid));
-				continue;
-			}
-
 			seq_printf(seq,
 				   " * %pM %4i [%c%c%c%c%c%c] %3u.%03u   (%#.8x)\n",
 				   tt_common_entry->addr,
@@ -1052,9 +1046,7 @@ int batadv_tt_local_seq_print_text(struct seq_file *seq, void *offset)
 				     BATADV_TT_CLIENT_ISOLA) ? 'I' : '.'),
 				   no_purge ? 0 : last_seen_secs,
 				   no_purge ? 0 : last_seen_msecs,
-				   vlan->tt.crc);
-
-			batadv_softif_vlan_put(vlan);
+				   tt_local->vlan->tt.crc);
 		}
 		rcu_read_unlock();
 	}
@@ -1099,7 +1091,6 @@ u16 batadv_tt_local_remove(struct batadv_priv *bat_priv, const u8 *addr,
 {
 	struct batadv_tt_local_entry *tt_local_entry;
 	u16 flags, curr_flags = BATADV_NO_FLAGS;
-	struct batadv_softif_vlan *vlan;
 	void *tt_entry_exists;
 
 	tt_local_entry = batadv_tt_local_hash_find(bat_priv, addr, vid);
@@ -1139,14 +1130,6 @@ u16 batadv_tt_local_remove(struct batadv_priv *bat_priv, const u8 *addr,
 	/* extra call to free the local tt entry */
 	batadv_tt_local_entry_put(tt_local_entry);
 
-	/* decrease the reference held for this vlan */
-	vlan = batadv_softif_vlan_get(bat_priv, vid);
-	if (!vlan)
-		goto out;
-
-	batadv_softif_vlan_put(vlan);
-	batadv_softif_vlan_put(vlan);
-
 out:
 	if (tt_local_entry)
 		batadv_tt_local_entry_put(tt_local_entry);
@@ -1219,7 +1202,6 @@ static void batadv_tt_local_table_free(struct batadv_priv *bat_priv)
 	spinlock_t *list_lock; /* protects write access to the hash lists */
 	struct batadv_tt_common_entry *tt_common_entry;
 	struct batadv_tt_local_entry *tt_local;
-	struct batadv_softif_vlan *vlan;
 	struct hlist_node *node_tmp;
 	struct hlist_head *head;
 	u32 i;
@@ -1241,14 +1223,6 @@ static void batadv_tt_local_table_free(struct batadv_priv *bat_priv)
 						struct batadv_tt_local_entry,
 						common);
 
-			/* decrease the reference held for this vlan */
-			vlan = batadv_softif_vlan_get(bat_priv,
-						      tt_common_entry->vid);
-			if (vlan) {
-				batadv_softif_vlan_put(vlan);
-				batadv_softif_vlan_put(vlan);
-			}
-
 			batadv_tt_local_entry_put(tt_local);
 		}
 		spin_unlock_bh(list_lock);
@@ -3310,7 +3284,6 @@ static void batadv_tt_local_purge_pending_clients(struct batadv_priv *bat_priv)
 	struct batadv_hashtable *hash = bat_priv->tt.local_hash;
 	struct batadv_tt_common_entry *tt_common;
 	struct batadv_tt_local_entry *tt_local;
-	struct batadv_softif_vlan *vlan;
 	struct hlist_node *node_tmp;
 	struct hlist_head *head;
 	spinlock_t *list_lock; /* protects write access to the hash lists */
@@ -3340,13 +3313,6 @@ static void batadv_tt_local_purge_pending_clients(struct batadv_priv *bat_priv)
 						struct batadv_tt_local_entry,
 						common);
 
-			/* decrease the reference held for this vlan */
-			vlan = batadv_softif_vlan_get(bat_priv, tt_common->vid);
-			if (vlan) {
-				batadv_softif_vlan_put(vlan);
-				batadv_softif_vlan_put(vlan);
-			}
-
 			batadv_tt_local_entry_put(tt_local);
 		}
 		spin_unlock_bh(list_lock);
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
index 9abfb3e..1480538 100644
--- a/net/batman-adv/types.h
+++ b/net/batman-adv/types.h
@@ -1073,10 +1073,12 @@ struct batadv_tt_common_entry {
  * struct batadv_tt_local_entry - translation table local entry data
  * @common: general translation table data
  * @last_seen: timestamp used for purging stale tt local entries
+ * @vlan: soft-interface vlan of the entry
  */
 struct batadv_tt_local_entry {
 	struct batadv_tt_common_entry common;
 	unsigned long last_seen;
+	struct batadv_softif_vlan *vlan;
 };
 
 /**
-- 
2.7.0

[B.A.T.M.A.N.] [PATCH 2/2] batman-adv: Fix reference counting of hardif_neigh_node object for neigh_node

[Sven Eckelmann]

The batadv_neigh_node was specific to a batadv_hardif_neigh_node and held
an implicit reference to it. But this reference was never stored in form of
a pointer in the batadv_neigh_node itself. Instead
batadv_neigh_node_release depends on a consistent state of
hard_iface->neigh_list and that batadv_hardif_neigh_get always returns the
batadv_hardif_neigh_node object which it has a reference for. But
batadv_hardif_neigh_get cannot guarantee that because it is working only
with rcu_read_lock on this list. It can therefore happen that a neigh_addr
is in this list twice or that batadv_hardif_neigh_get cannot find the
batadv_hardif_neigh_node for an neigh_addr due to some other list
operations taking place at the same time.

Instead add a batadv_hardif_neigh_node pointer directly in
batadv_neigh_node which will be used for the reference counter decremented
on release of batadv_neigh_node.

Fixes: fed2826b490c ("batman-adv: add list of unique single hop neighbors per hard-interface")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
See https://www.open-mesh.org/issues/242

v1:
 - Remove the old code blocks
 - make the _put unconditional
---
 net/batman-adv/originator.c | 16 +++++-----------
 net/batman-adv/types.h      |  2 ++
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/net/batman-adv/originator.c b/net/batman-adv/originator.c
index 2d288ea..217f8ed 100644
--- a/net/batman-adv/originator.c
+++ b/net/batman-adv/originator.c
@@ -250,7 +250,6 @@ static void batadv_neigh_node_release(struct kref *ref)
 {
 	struct hlist_node *node_tmp;
 	struct batadv_neigh_node *neigh_node;
-	struct batadv_hardif_neigh_node *hardif_neigh;
 	struct batadv_neigh_ifinfo *neigh_ifinfo;
 	struct batadv_algo_ops *bao;
 
@@ -262,13 +261,7 @@ static void batadv_neigh_node_release(struct kref *ref)
 		batadv_neigh_ifinfo_put(neigh_ifinfo);
 	}
 
-	hardif_neigh = batadv_hardif_neigh_get(neigh_node->if_incoming,
-					       neigh_node->addr);
-	if (hardif_neigh) {
-		/* batadv_hardif_neigh_get() increases refcount too */
-		batadv_hardif_neigh_put(hardif_neigh);
-		batadv_hardif_neigh_put(hardif_neigh);
-	}
+	batadv_hardif_neigh_put(neigh_node->hardif_neigh);
 
 	if (bao->bat_neigh_free)
 		bao->bat_neigh_free(neigh_node);
@@ -648,6 +641,10 @@ batadv_neigh_node_new(struct batadv_orig_node *orig_node,
 	neigh_node->if_incoming = hard_iface;
 	neigh_node->orig_node = orig_node;
 
+	/* increment unique neighbor refcount */
+	kref_get(&hardif_neigh->refcount);
+	neigh_node->hardif_neigh = hardif_neigh;
+
 	/* extra reference for return */
 	kref_init(&neigh_node->refcount);
 	kref_get(&neigh_node->refcount);
@@ -656,9 +653,6 @@ batadv_neigh_node_new(struct batadv_orig_node *orig_node,
 	hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
 	spin_unlock_bh(&orig_node->neigh_list_lock);
 
-	/* increment unique neighbor refcount */
-	kref_get(&hardif_neigh->refcount);
-
 	batadv_dbg(BATADV_DBG_BATMAN, orig_node->bat_priv,
 		   "Creating new neighbor %pM for orig_node %pM on interface %s\n",
 		   neigh_addr, orig_node->orig, hard_iface->net_dev->name);
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
index 1480538..58a3fa5 100644
--- a/net/batman-adv/types.h
+++ b/net/batman-adv/types.h
@@ -433,6 +433,7 @@ struct batadv_hardif_neigh_node {
  * @ifinfo_lock: lock protecting private ifinfo members and list
  * @if_incoming: pointer to incoming hard-interface
  * @last_seen: when last packet via this neighbor was received
+ * @hardif_neigh: hardif_neigh of this neighbor
  * @refcount: number of contexts the object is used
  * @rcu: struct used for freeing in an RCU-safe manner
  */
@@ -444,6 +445,7 @@ struct batadv_neigh_node {
 	spinlock_t ifinfo_lock;	/* protects ifinfo_list and its members */
 	struct batadv_hard_iface *if_incoming;
 	unsigned long last_seen;
+	struct batadv_hardif_neigh_node *hardif_neigh;
 	struct kref refcount;
 	struct rcu_head rcu;
 };
-- 
2.7.0

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: Fix reference counting of vlan object for tt_local_entry

[Antonio Quartulli]

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

On Fri, Mar 11, 2016 at 04:44:05PM +0100, Sven Eckelmann wrote:
> The batadv_tt_local_entry was specific to a batadv_softif_vlan and held an
> implicit reference to it. But this reference was never stored in form of a
> pointer in the tt_local_entry itself. Instead batadv_tt_local_remove,
> batadv_tt_local_table_free and batadv_tt_local_purge_pending_clients depend
> on a consistent state of bat_priv->softif_vlan_list and that
> batadv_softif_vlan_get always returns the batadv_softif_vlan object which
> it has a reference for. But batadv_softif_vlan_get cannot guarantee that
> because it is working only with rcu_read_lock on this list. It can
> therefore happen that an vid is in this list twice or that
> batadv_softif_vlan_get cannot find the batadv_softif_vlan for an vid due to
> some other list operations taking place at the same time.
> 
> Instead add a batadv_softif_vlan pointer directly in batadv_tt_local_entry
> which will be used for the reference counter decremented on release of
> batadv_tt_local_entry.
> 
> Fixes: 9729d2085c0f ("batman-adv: fix TT VLAN inconsistency on VLAN re-add")
> Signed-off-by: Sven Eckelmann <sven@narfation.org>

Thanks a lot Sven ! This change, on top of fixing the ref issue, also simplified
the code quite a lot!

Acked-by: Antonio Quartulli <a@unstable.cc>

-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: Fix reference counting of vlan object for tt_local_entry

[Marek Lindner]

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

On Thursday, April 21, 2016 18:22:52 Antonio Quartulli wrote:
> On Fri, Mar 11, 2016 at 04:44:05PM +0100, Sven Eckelmann wrote:
> > The batadv_tt_local_entry was specific to a batadv_softif_vlan and held an
> > implicit reference to it. But this reference was never stored in form of a
> > pointer in the tt_local_entry itself. Instead batadv_tt_local_remove,
> > batadv_tt_local_table_free and batadv_tt_local_purge_pending_clients
> > depend
> > on a consistent state of bat_priv->softif_vlan_list and that
> > batadv_softif_vlan_get always returns the batadv_softif_vlan object which
> > it has a reference for. But batadv_softif_vlan_get cannot guarantee that
> > because it is working only with rcu_read_lock on this list. It can
> > therefore happen that an vid is in this list twice or that
> > batadv_softif_vlan_get cannot find the batadv_softif_vlan for an vid due
> > to
> > some other list operations taking place at the same time.
> >
> > 
> >
> > Instead add a batadv_softif_vlan pointer directly in batadv_tt_local_entry
> > which will be used for the reference counter decremented on release of
> > batadv_tt_local_entry.
> >
> > 
> >
> > Fixes: 9729d2085c0f ("batman-adv: fix TT VLAN inconsistency on VLAN
> > re-add") Signed-off-by: Sven Eckelmann <sven@narfation.org>
> 
> Thanks a lot Sven ! This change, on top of fixing the ref issue, also
> simplified the code quite a lot!
> 
> Acked-by: Antonio Quartulli <a@unstable.cc>

Applied in revision 20ca393.

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 2/2] batman-adv: Fix reference counting of hardif_neigh_node object for neigh_node

[Marek Lindner]

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

On Friday, March 11, 2016 16:44:06 Sven Eckelmann wrote:
> The batadv_neigh_node was specific to a batadv_hardif_neigh_node and held
> an implicit reference to it. But this reference was never stored in form of
> a pointer in the batadv_neigh_node itself. Instead
> batadv_neigh_node_release depends on a consistent state of
> hard_iface->neigh_list and that batadv_hardif_neigh_get always returns the
> batadv_hardif_neigh_node object which it has a reference for. But
> batadv_hardif_neigh_get cannot guarantee that because it is working only
> with rcu_read_lock on this list. It can therefore happen that a neigh_addr
> is in this list twice or that batadv_hardif_neigh_get cannot find the
> batadv_hardif_neigh_node for an neigh_addr due to some other list
> operations taking place at the same time.
> 
> Instead add a batadv_hardif_neigh_node pointer directly in
> batadv_neigh_node which will be used for the reference counter decremented
> on release of batadv_neigh_node.
> 
> Fixes: fed2826b490c ("batman-adv: add list of unique single hop neighbors
> per hard-interface") Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
> See https://www.open-mesh.org/issues/242
> 
> v1:
>  - Remove the old code blocks
>  - make the _put unconditional
> ---
>  net/batman-adv/originator.c | 16 +++++-----------
>  net/batman-adv/types.h      |  2 ++
>  2 files changed, 7 insertions(+), 11 deletions(-)

Applied in revision a198a23.

Thanks,
Marek

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