[B.A.T.M.A.N.] [PATCH-maintv3 0/4] fix multiif regressions

[B.A.T.M.A.N.] [PATCH-maintv3 0/4] fix multiif regressions

[Simon Wunderlich]

There are some regressions introduced my network wide multi interface
optimization patchset which are adressed by this patchset. All of them
fix some kind of leaks caused reference counter imbalances, which may
lead to problems when an interface is removed which was previously used
by batman-adv.

The second series was only sent partially to the mailing list, and in
this third revision another bugfix was added and a sparse warning was
fixed.

Thanks Antonio for your help tracking that down!

Cheers,
        Simon

Simon Wunderlich (4):
  batman-adv: fix neigh_ifinfo imbalance
  batman-adv: fix neigh reference imbalance
  batman-adv: always run purge_orig_neighbors
  batman-adv: fix removing neigh_ifinfo

 bat_iv_ogm.c |    2 ++
 originator.c |   59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 57 insertions(+), 4 deletions(-)

-- 
1.7.10.4

[B.A.T.M.A.N.] [PATCH-maintv3 1/4] batman-adv: fix neigh_ifinfo imbalance

[Simon Wunderlich]

From: Simon Wunderlich <simon@open-mesh.com>

The neigh_ifinfo object must be freed if it has been used in
batadv_iv_ogm_process_per_outif().

This is a regression introduced by
9bb33b8d88e318c4879d37d06ad28e3e018b9036 ("batman-adv: split tq
information in neigh_node struct")

Reported-by: Antonio Quartulli <antonio@meshcoding.com>
Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
---
 bat_iv_ogm.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/bat_iv_ogm.c b/bat_iv_ogm.c
index 8323bce..d074d06 100644
--- a/bat_iv_ogm.c
+++ b/bat_iv_ogm.c
@@ -1545,6 +1545,8 @@ out_neigh:
 	if ((orig_neigh_node) && (!is_single_hop_neigh))
 		batadv_orig_node_free_ref(orig_neigh_node);
 out:
+	if (router_ifinfo)
+		batadv_neigh_ifinfo_free_ref(router_ifinfo);
 	if (router)
 		batadv_neigh_node_free_ref(router);
 	if (router_router)
-- 
1.7.10.4

Re: [B.A.T.M.A.N.] [PATCH-maintv3 1/4] batman-adv: fix neigh_ifinfo imbalance

[Marek Lindner]

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

On Wednesday 26 March 2014 15:46:21 Simon Wunderlich wrote:
> From: Simon Wunderlich <simon@open-mesh.com>
> 
> The neigh_ifinfo object must be freed if it has been used in
> batadv_iv_ogm_process_per_outif().
> 
> This is a regression introduced by
> 9bb33b8d88e318c4879d37d06ad28e3e018b9036 ("batman-adv: split tq
> information in neigh_node struct")
> 
> Reported-by: Antonio Quartulli <antonio@meshcoding.com>
> Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
> ---
>  bat_iv_ogm.c |    2 ++
>  1 file changed, 2 insertions(+)

Applied in revision a424cd5.

Thanks,
Marek

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

[B.A.T.M.A.N.] [PATCH-maintv3 2/4] batman-adv: fix neigh reference imbalance

[Simon Wunderlich]

From: Simon Wunderlich <simon@open-mesh.com>

When an interface is removed from batman-adv, the orig_ifinfo of a
orig_node may be removed without releasing the router first.
This will prevent the reference for the neighbor pointed at by the
orig_ifinfo->router to be released, and this leak may result in
reference leaks for the interface used by this neighbor. Fix that.

This is a regression introduced by
de6bcc76ea84fecb136f8c8f5ba1862e4a13f06b ("batman-adv: split out router
from orig_node").

Reported-by: Antonio Quartulli <antonio@meshcoding.com>
Signed-off-by: Simon Wunderlich <simon@open-mesh.com>

Changes to PATCHv2:
 * take care of the rcu sparse warning
---
 originator.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/originator.c b/originator.c
index 8539416..25df60d 100644
--- a/originator.c
+++ b/originator.c
@@ -500,12 +500,17 @@ batadv_neigh_node_get(const struct batadv_orig_node *orig_node,
 static void batadv_orig_ifinfo_free_rcu(struct rcu_head *rcu)
 {
 	struct batadv_orig_ifinfo *orig_ifinfo;
+	struct batadv_neigh_node *router;
 
 	orig_ifinfo = container_of(rcu, struct batadv_orig_ifinfo, rcu);
 
 	if (orig_ifinfo->if_outgoing != BATADV_IF_DEFAULT)
 		batadv_hardif_free_ref_now(orig_ifinfo->if_outgoing);
 
+	/* this is the last reference to this object */
+	router = rcu_dereference_protected(orig_ifinfo->router, true);
+	if (router)
+		batadv_neigh_node_free_ref_now(router);
 	kfree(orig_ifinfo);
 }
 
-- 
1.7.10.4

Re: [B.A.T.M.A.N.] [PATCH-maintv3 2/4] batman-adv: fix neigh reference imbalance

[Marek Lindner]

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

On Wednesday 26 March 2014 15:46:22 Simon Wunderlich wrote:
> From: Simon Wunderlich <simon@open-mesh.com>
> 
> When an interface is removed from batman-adv, the orig_ifinfo of a
> orig_node may be removed without releasing the router first.
> This will prevent the reference for the neighbor pointed at by the
> orig_ifinfo->router to be released, and this leak may result in
> reference leaks for the interface used by this neighbor. Fix that.
> 
> This is a regression introduced by
> de6bcc76ea84fecb136f8c8f5ba1862e4a13f06b ("batman-adv: split out router
> from orig_node").
> 
> Reported-by: Antonio Quartulli <antonio@meshcoding.com>
> Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
> 
> Changes to PATCHv2:
>  * take care of the rcu sparse warning
> ---
>  originator.c |    5 +++++
>  1 file changed, 5 insertions(+)

Applied in revision cdd09f6.

Thanks,
Marek

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

[B.A.T.M.A.N.] [PATCH-maintv3 3/4] batman-adv: always run purge_orig_neighbors

[Simon Wunderlich]

From: Simon Wunderlich <simon@open-mesh.com>

The current code will not execute batadv_purge_orig_neighbors() when an
orig_ifinfo has already been purged. However we need to run it in any
case. Fix that.

This is a regression introduced by
de6bcc76ea84fecb136f8c8f5ba1862e4a13f06b ("batman-adv: split out router
from orig_node")

Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
---
Changes to PATCHv1:

 * check two change variables separately
---
 originator.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/originator.c b/originator.c
index 25df60d..47b0886 100644
--- a/originator.c
+++ b/originator.c
@@ -857,7 +857,7 @@ static bool batadv_purge_orig_node(struct batadv_priv *bat_priv,
 {
 	struct batadv_neigh_node *best_neigh_node;
 	struct batadv_hard_iface *hard_iface;
-	bool changed;
+	bool changed_ifinfo, changed_neigh;
 
 	if (batadv_has_timed_out(orig_node->last_seen,
 				 2 * BATADV_PURGE_TIMEOUT)) {
@@ -867,10 +867,10 @@ static bool batadv_purge_orig_node(struct batadv_priv *bat_priv,
 			   jiffies_to_msecs(orig_node->last_seen));
 		return true;
 	}
-	changed = batadv_purge_orig_ifinfo(bat_priv, orig_node);
-	changed = changed || batadv_purge_orig_neighbors(bat_priv, orig_node);
+	changed_ifinfo = batadv_purge_orig_ifinfo(bat_priv, orig_node);
+	changed_neigh = batadv_purge_orig_neighbors(bat_priv, orig_node);
 
-	if (!changed)
+	if (!changed_ifinfo && !changed_neigh)
 		return false;
 
 	/* first for NULL ... */
-- 
1.7.10.4

Re: [B.A.T.M.A.N.] [PATCH-maintv3 3/4] batman-adv: always run purge_orig_neighbors

[Marek Lindner]

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

On Wednesday 26 March 2014 15:46:23 Simon Wunderlich wrote:
> From: Simon Wunderlich <simon@open-mesh.com>
> 
> The current code will not execute batadv_purge_orig_neighbors() when an
> orig_ifinfo has already been purged. However we need to run it in any
> case. Fix that.
> 
> This is a regression introduced by
> de6bcc76ea84fecb136f8c8f5ba1862e4a13f06b ("batman-adv: split out router
> from orig_node")
> 
> Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
> ---
> Changes to PATCHv1:
> 
>  * check two change variables separately
> ---
>  originator.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Applied in revision 7212515.

Thanks,
Marek

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

[B.A.T.M.A.N.] [PATCH-maintv3 4/4] batman-adv: fix removing neigh_ifinfo

[Simon Wunderlich]

From: Simon Wunderlich <simon@open-mesh.com>

When an interface is removed separately, all neighbors need to be
checked if they have a neigh_ifinfo structure for that particular
interface. If that is the case, remove that ifinfo so any references to
a hard interface can be freed.

This is a regression introduced by
9bb33b8d88e318c4879d37d06ad28e3e018b9036 ("batman-adv: split tq
information in neigh_node struct")

Reported-by: Antonio Quartulli <antonio@meshcoding.com>
Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
---
 originator.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/originator.c b/originator.c
index 47b0886..aa2468b 100644
--- a/originator.c
+++ b/originator.c
@@ -702,6 +702,47 @@ free_orig_node:
 }
 
 /**
+ * batadv_purge_neigh_ifinfo - purge obsolete ifinfo entries from neighbor
+ * @bat_priv: the bat priv with all the soft interface information
+ * @neigh_node: orig node which is to be checked
+ */
+static void
+batadv_purge_neigh_ifinfo(struct batadv_priv *bat_priv,
+			  struct batadv_neigh_node *neigh)
+{
+	struct batadv_neigh_ifinfo *neigh_ifinfo;
+	struct batadv_hard_iface *if_outgoing;
+	struct hlist_node *node_tmp;
+
+	spin_lock_bh(&neigh->ifinfo_lock);
+
+	/* for all ifinfo objects for this neighinator */
+	hlist_for_each_entry_safe(neigh_ifinfo, node_tmp,
+				  &neigh->ifinfo_list, list) {
+		if_outgoing = neigh_ifinfo->if_outgoing;
+
+		/* always keep the default interface */
+		if (if_outgoing == BATADV_IF_DEFAULT)
+			continue;
+
+		/* don't purge if the interface is not (going) down */
+		if ((if_outgoing->if_status != BATADV_IF_INACTIVE) &&
+		    (if_outgoing->if_status != BATADV_IF_NOT_IN_USE) &&
+		    (if_outgoing->if_status != BATADV_IF_TO_BE_REMOVED))
+			continue;
+
+		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
+			   "neighbor/ifinfo purge: neighbor %pM, iface: %s\n",
+			   neigh->addr, if_outgoing->net_dev->name);
+
+		hlist_del_rcu(&neigh_ifinfo->list);
+		batadv_neigh_ifinfo_free_ref(neigh_ifinfo);
+	}
+
+	spin_unlock_bh(&neigh->ifinfo_lock);
+}
+
+/**
  * batadv_purge_orig_ifinfo - purge obsolete ifinfo entries from originator
  * @bat_priv: the bat priv with all the soft interface information
  * @orig_node: orig node which is to be checked
@@ -800,6 +841,11 @@ batadv_purge_orig_neighbors(struct batadv_priv *bat_priv,
 
 			hlist_del_rcu(&neigh_node->list);
 			batadv_neigh_node_free_ref(neigh_node);
+		} else {
+			/* only neccesary if not the whole neighbor is to be deleted,
+			 * but some interface has been removed.
+			 */
+			batadv_purge_neigh_ifinfo(bat_priv, neigh_node);
 		}
 	}
 
-- 
1.7.10.4

Re: [B.A.T.M.A.N.] [PATCH-maintv3 4/4] batman-adv: fix removing neigh_ifinfo

[Marek Lindner]

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

On Wednesday 26 March 2014 15:46:24 Simon Wunderlich wrote:
> From: Simon Wunderlich <simon@open-mesh.com>
> 
> When an interface is removed separately, all neighbors need to be
> checked if they have a neigh_ifinfo structure for that particular
> interface. If that is the case, remove that ifinfo so any references to
> a hard interface can be freed.
> 
> This is a regression introduced by
> 9bb33b8d88e318c4879d37d06ad28e3e018b9036 ("batman-adv: split tq
> information in neigh_node struct")
> 
> Reported-by: Antonio Quartulli <antonio@meshcoding.com>
> Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
> ---
>  originator.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)

Applied in revision 9b9cdbe.

Thanks,
Marek

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