[B.A.T.M.A.N.] [PATCH maint] batman-adv: fix potential TT client + orig-node memory leak
13 Dec
2014
13 Dec
'14
10:32 p.m.
This patch fixes a potential memory leak which can occur once an
originator times out. On timeout the according global translation table
entry might not get purged correctly. Furthermore, the non purged TT
entry will cause its orig-node to leak, too. Which additionally can lead
to the new multicast optimization feature not kicking in because of a
therefore bogus counter.
In the wild with larger mesh networks we saw this leak quite regularly,
resulting in routers to reboot or killed processes. This was because
of a combination of two bugs: The bug fixed by commit
"batman-adv: fix delayed foreign originator recognition" (8a2ad5204674)
amplified this memory leak heavily. Since that commit I'd expect
it to happen rarely, probably only in paused and resumed VMs and
devices previously in stand-by.
The issue this patch fixes is caused by batadv_orig_node_free_rcu()
never being called because of not yet released references to the
orig-node. References which were supposed to be released through
batadv_orig_node_free_rcu()->batadv_tt_global_del_orig().
Fixing the issue by moving batadv_tt_global_del_orig() out of the rcu
callback.
Signed-off-by: Linus Lüssing <linus.luessing(a)c0d3.blue>
---
originator.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/originator.c b/originator.c
index 648bdba..bea8198 100644
--- a/originator.c
+++ b/originator.c
@@ -570,9 +570,6 @@ static void batadv_orig_node_free_rcu(struct rcu_head *rcu)
batadv_frag_purge_orig(orig_node, NULL);
- batadv_tt_global_del_orig(orig_node->bat_priv, orig_node, -1,
- "originator timed out");
-
if (orig_node->bat_priv->bat_algo_ops->bat_orig_free)
orig_node->bat_priv->bat_algo_ops->bat_orig_free(orig_node);
@@ -978,6 +975,9 @@ static void _batadv_purge_orig(struct batadv_priv *bat_priv)
if (batadv_purge_orig_node(bat_priv, orig_node)) {
batadv_gw_node_delete(bat_priv, orig_node);
hlist_del_rcu(&orig_node->hash_entry);
+ batadv_tt_global_del_orig(orig_node->bat_priv,
+ orig_node, -1,
+ "originator timed out");
batadv_orig_node_free_ref(orig_node);
continue;
}
--
1.7.10.4
29 Dec
29 Dec
3:52 a.m.
New subject: [B.A.T.M.A.N.] [PATCH maint] batman-adv: fix potential TT client + orig-node memory leak
On Saturday 13 December 2014 23:32:15 Linus Lüssing wrote:
This patch fixes a potential memory leak which can
occur once an
originator times out. On timeout the according global translation table
entry might not get purged correctly. Furthermore, the non purged TT
entry will cause its orig-node to leak, too. Which additionally can lead
to the new multicast optimization feature not kicking in because of a
therefore bogus counter.
So far, I am with you ..
In the wild with larger mesh networks we saw this leak
quite regularly,
resulting in routers to reboot or killed processes. This was because
of a combination of two bugs: The bug fixed by commit
"batman-adv: fix delayed foreign originator recognition" (8a2ad5204674)
amplified this memory leak heavily. Since that commit I'd expect
it to happen rarely, probably only in paused and resumed VMs and
devices previously in stand-by.
This section shouldn't be part of the official commit message. It is hardly
relevant to the reviewer how often a memleak occurs and whether or not you
need a VM to trigger it. The provided commit id isn't valid in the Linux tree.
The issue this patch fixes is caused by
batadv_orig_node_free_rcu()
never being called because of not yet released references to the
orig-node. References which were supposed to be released through
batadv_orig_node_free_rcu()->batadv_tt_global_del_orig().
Could you please provide addition insight as to which references are still
held ? I did look around but nothing obvious jumped at me.
Generally, it wouldn't be bad if the commit message went into deeper detail
describing the nature of the bug instead of the middle section above to make
it easy to understand what is being fixed.
Cheers,
Marek
2:32 p.m.
On Mon, Dec 29, 2014 at 11:52:53AM +0800, Marek Lindner wrote:
The batadv_tt_global_entry->orig_list holds the reference to the
orig-node. Usually this reference is released after
BATADV_PURGE_TIMEOUT through: _batadv_purge_orig()->
batadv_purge_orig_node()->batadv_update_route()->_batadv_update_route()->
batadv_tt_global_del_orig() which purges this global tt entry and
releases the reference to the orig-node.
However, if between two batadv_purge_orig_node() calls the orig-node
timeout grew to 2*BATADV_PURGE_TIMEOUT then this call path isn't
reached (*). Instead the according orig-node is removed from the
originator hash in _batadv_purge_orig(), the batadv_update_route()
part is skipped and won't be reached anymore.
It seems that in that case batadv_orig_node_free_rcu()->batadv_tt_global_del_orig()
is supposed to purge the global tt entry and to release the orig-node
reference but it's not called because batadv_orig_node_free_rcu() is
only called once all references are freed: A chicken 'n' egg
situation.
The issue this
patch fixes is caused by batadv_orig_node_free_rcu()
never being called because of not yet released references to the
orig-node. References which were supposed to be released through
batadv_orig_node_free_rcu()->batadv_tt_global_del_orig().
Could you please provide addition insight as to which references are still
held ? I did look around but nothing obvious jumped at me.
Generally, it wouldn't be bad if the commit message went into deeper detail
describing the nature of the bug instead of the middle section above to make
it easy to understand what is being fixed.
Hm, hm, my intention was to somehow/somewhere document how these
two, small and rare bugs together created this severe bug.
The issue fixed by 8a2ad5204674 was the main trigger for (*) in
previous releases.
But we can also skip that middle section if you want. Then I'll
just add a note to the ticket on redmine.
Cheers,
Marek
Cheers, Linus
4 Jan
4 Jan
4:05 p.m.
On 29/12/14 04:52, Marek Lindner wrote:
On Saturday 13 December 2014 23:32:15 Linus Lüssing
wrote:
Hi Linus and thanks for fixing this TT bug!
I agree with Marek about extending the commit message with a better
explanation of the actual bug, but at the same time I think it is good
to keep the commit message and ID of the other involved patches.
Moreover, please keep the commit ID of our batman-adv.git tree - I'll
then take care of converting them when sending the patches upstream.
Cheers,
--
Antonio Quartulli
This patch fixes a potential memory leak which
can occur once an
originator times out. On timeout the according global translation table
entry might not get purged correctly. Furthermore, the non purged TT
entry will cause its orig-node to leak, too. Which additionally can lead
to the new multicast optimization feature not kicking in because of a
therefore bogus counter.
So far, I am with you ..
In the wild with larger mesh networks we saw this
leak quite regularly,
resulting in routers to reboot or killed processes. This was because
of a combination of two bugs: The bug fixed by commit
"batman-adv: fix delayed foreign originator recognition" (8a2ad5204674)
amplified this memory leak heavily. Since that commit I'd expect
it to happen rarely, probably only in paused and resumed VMs and
devices previously in stand-by.
This section shouldn't be part of the official commit message. It is hardly
relevant to the reviewer how often a memleak occurs and whether or not you
need a VM to trigger it. The provided commit id isn't valid in the Linux tree.
The issue this patch fixes is caused by
batadv_orig_node_free_rcu()
never being called because of not yet released references to the
orig-node. References which were supposed to be released through
batadv_orig_node_free_rcu()->batadv_tt_global_del_orig().
Could you please provide addition insight as to which references are still
held ? I did look around but nothing obvious jumped at me.
Generally, it wouldn't be bad if the commit message went into deeper detail
describing the nature of the bug instead of the middle section above to make
it easy to understand what is being fixed.
4:11 p.m.
On 04/01/15 17:05, Antonio Quartulli wrote:
Hi Linus and thanks for fixing this TT bug!
I agree with Marek about extending the commit message with a better
explanation of the actual bug, but at the same time I think it is good
to keep the commit message and ID of the other involved patches.
Moreover, please keep the commit ID of our batman-adv.git tree - I'll
then take care of converting them when sending the patches upstream.
I forgot..for the patch:
Acked-by: Antonio Quartulli <antonio(a)meshcoding.com>
--
Antonio Quartulli
5 Jan
5 Jan
5:22 p.m.
New subject: [B.A.T.M.A.N.] [PATCH maint] batman-adv: fix potential TT client + orig-node memory leak
On Sunday 04 January 2015 17:11:43 Antonio Quartulli wrote:
On 04/01/15 17:05, Antonio Quartulli wrote:
Applied in revision 21fa264 with commit message beautifications.
Thanks,
Marek
Hi Linus and thanks for fixing this TT bug!
I agree with Marek about extending the commit message with a better
explanation of the actual bug, but at the same time I think it is good
to keep the commit message and ID of the other involved patches.
Moreover, please keep the commit ID of our batman-adv.git tree - I'll
then take care of converting them when sending the patches upstream.
I forgot..for the patch:
Acked-by: Antonio Quartulli <antonio(a)meshcoding.com>
3068
days inactive
3091
days old
b.a.t.m.a.n@lists.open-mesh.org
5 comments
3 participants
participants (3)
-
Antonio Quartulli -
Linus Lüssing -
Marek Lindner