From: Marek Lindner mareklindner@neomailbox.ch
commit d9f179877e50ae2681fe7b0b83e0d9f63b6165ad upstream.
The access of the neigh_node->orig_node is not allowed anymore when the batadv_neigh_node is freed. But this function is still trying to do it go get an routing algorithm specific function pointer via the neigh_node->orig_node. This function is then used to free the routing algorithm specific neighbor node data.
This sometimes causes a crash when the orig_node was already freed when the this function tries to read from its old memory region.
The code can just be removed since no routing algorithm implements the function pointer.
Reported-by: Lars Bußmann ffsoest@kill-you.net Signed-off-by: Marek Lindner mareklindner@neomailbox.ch [sven@narfation.org: rewritten commit message to make clear that it is an bugfix to an user reported crash] Signed-off-by: Sven Eckelmann sven@narfation.org Signed-off-by: Simon Wunderlich sw@simonwunderlich.de --- net/batman-adv/originator.c | 6 ------ net/batman-adv/types.h | 3 --- 2 files changed, 9 deletions(-)
diff --git a/net/batman-adv/originator.c b/net/batman-adv/originator.c index 17851d3..6282f02 100644 --- a/net/batman-adv/originator.c +++ b/net/batman-adv/originator.c @@ -197,18 +197,12 @@ static void batadv_neigh_node_release(struct batadv_neigh_node *neigh_node) { struct hlist_node *node_tmp; struct batadv_neigh_ifinfo *neigh_ifinfo; - struct batadv_algo_ops *bao; - - bao = neigh_node->orig_node->bat_priv->bat_algo_ops;
hlist_for_each_entry_safe(neigh_ifinfo, node_tmp, &neigh_node->ifinfo_list, list) { batadv_neigh_ifinfo_free_ref(neigh_ifinfo); }
- if (bao->bat_neigh_free) - bao->bat_neigh_free(neigh_node); - batadv_hardif_free_ref(neigh_node->if_incoming);
kfree_rcu(neigh_node, rcu); diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h index d260efd..cbd347c 100644 --- a/net/batman-adv/types.h +++ b/net/batman-adv/types.h @@ -1136,8 +1136,6 @@ struct batadv_forw_packet { * @bat_neigh_is_equiv_or_better: check if neigh1 is equally good or better * than neigh2 for their respective outgoing interface from the metric * prospective - * @bat_neigh_free: free the resources allocated by the routing algorithm for a - * neigh_node object * @bat_orig_print: print the originator table (optional) * @bat_orig_free: free the resources allocated by the routing algorithm for an * orig_node object @@ -1165,7 +1163,6 @@ struct batadv_algo_ops { struct batadv_hard_iface *if_outgoing1, struct batadv_neigh_node *neigh2, struct batadv_hard_iface *if_outgoing2); - void (*bat_neigh_free)(struct batadv_neigh_node *neigh); /* orig_node handling API */ void (*bat_orig_print)(struct batadv_priv *priv, struct seq_file *seq, struct batadv_hard_iface *hard_iface);
From: Marek Lindner mareklindner@neomailbox.ch
commit d9f179877e50ae2681fe7b0b83e0d9f63b6165ad upstream.
The access of the neigh_node->orig_node is not allowed anymore when the batadv_neigh_node is freed. But this function is still trying to do it go get an routing algorithm specific function pointer via the neigh_node->orig_node. This function is then used to free the routing algorithm specific neighbor node data.
This sometimes causes a crash when the orig_node was already freed when the this function tries to read from its old memory region.
The code can just be removed since no routing algorithm implements the function pointer.
Reported-by: Lars Bußmann ffsoest@kill-you.net Signed-off-by: Marek Lindner mareklindner@neomailbox.ch [sven@narfation.org: rewritten commit message to make clear that it is an bugfix to an user reported crash] Signed-off-by: Sven Eckelmann sven@narfation.org Signed-off-by: Simon Wunderlich sw@simonwunderlich.de --- net/batman-adv/originator.c | 5 ----- net/batman-adv/types.h | 3 --- 2 files changed, 8 deletions(-)
diff --git a/net/batman-adv/originator.c b/net/batman-adv/originator.c index c355a82..0039e25 100644 --- a/net/batman-adv/originator.c +++ b/net/batman-adv/originator.c @@ -251,10 +251,8 @@ static void batadv_neigh_node_release(struct kref *ref) struct hlist_node *node_tmp; struct batadv_neigh_node *neigh_node; struct batadv_neigh_ifinfo *neigh_ifinfo; - struct batadv_algo_ops *bao;
neigh_node = container_of(ref, struct batadv_neigh_node, refcount); - bao = neigh_node->orig_node->bat_priv->bat_algo_ops;
hlist_for_each_entry_safe(neigh_ifinfo, node_tmp, &neigh_node->ifinfo_list, list) { @@ -263,9 +261,6 @@ static void batadv_neigh_node_release(struct kref *ref)
batadv_hardif_neigh_put(neigh_node->hardif_neigh);
- if (bao->bat_neigh_free) - bao->bat_neigh_free(neigh_node); - batadv_hardif_put(neigh_node->if_incoming);
kfree_rcu(neigh_node, rcu); diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h index 1e47fbe..b0e7d36 100644 --- a/net/batman-adv/types.h +++ b/net/batman-adv/types.h @@ -1270,8 +1270,6 @@ struct batadv_forw_packet { * better than neigh2 for their respective outgoing interface from the metric * prospective * @bat_neigh_print: print the single hop neighbor list (optional) - * @bat_neigh_free: free the resources allocated by the routing algorithm for a - * neigh_node object * @bat_orig_print: print the originator table (optional) * @bat_orig_free: free the resources allocated by the routing algorithm for an * orig_node object @@ -1302,7 +1300,6 @@ struct batadv_algo_ops { struct batadv_neigh_node *neigh2, struct batadv_hard_iface *if_outgoing2); void (*bat_neigh_print)(struct batadv_priv *priv, struct seq_file *seq); - void (*bat_neigh_free)(struct batadv_neigh_node *neigh); /* orig_node handling API */ void (*bat_orig_print)(struct batadv_priv *priv, struct seq_file *seq, struct batadv_hard_iface *hard_iface);
From: Marek Lindner mareklindner@neomailbox.ch
commit d9f179877e50ae2681fe7b0b83e0d9f63b6165ad upstream.
The access of the neigh_node->orig_node is not allowed anymore when the batadv_neigh_node is freed. But this function is still trying to do it go get an routing algorithm specific function pointer via the neigh_node->orig_node. This function is then used to free the routing algorithm specific neighbor node data.
This sometimes causes a crash when the orig_node was already freed when the this function tries to read from its old memory region.
The code can just be removed since no routing algorithm implements the function pointer.
Reported-by: Lars Bußmann ffsoest@kill-you.net Signed-off-by: Marek Lindner mareklindner@neomailbox.ch [sven@narfation.org: rewritten commit message to make clear that it is an bugfix to an user reported crash] Signed-off-by: Sven Eckelmann sven@narfation.org Signed-off-by: Simon Wunderlich sw@simonwunderlich.de --- net/batman-adv/originator.c | 5 ----- net/batman-adv/types.h | 3 --- 2 files changed, 8 deletions(-)
diff --git a/net/batman-adv/originator.c b/net/batman-adv/originator.c index ab8c4f9..1da7bfc 100644 --- a/net/batman-adv/originator.c +++ b/net/batman-adv/originator.c @@ -251,10 +251,8 @@ static void batadv_neigh_node_release(struct kref *ref) struct hlist_node *node_tmp; struct batadv_neigh_node *neigh_node; struct batadv_neigh_ifinfo *neigh_ifinfo; - struct batadv_algo_ops *bao;
neigh_node = container_of(ref, struct batadv_neigh_node, refcount); - bao = neigh_node->orig_node->bat_priv->bat_algo_ops;
hlist_for_each_entry_safe(neigh_ifinfo, node_tmp, &neigh_node->ifinfo_list, list) { @@ -263,9 +261,6 @@ static void batadv_neigh_node_release(struct kref *ref)
batadv_hardif_neigh_put(neigh_node->hardif_neigh);
- if (bao->bat_neigh_free) - bao->bat_neigh_free(neigh_node); - batadv_hardif_put(neigh_node->if_incoming);
kfree_rcu(neigh_node, rcu); diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h index 74d865a..40aed16 100644 --- a/net/batman-adv/types.h +++ b/net/batman-adv/types.h @@ -1284,8 +1284,6 @@ struct batadv_forw_packet { * better than neigh2 for their respective outgoing interface from the metric * prospective * @bat_neigh_print: print the single hop neighbor list (optional) - * @bat_neigh_free: free the resources allocated by the routing algorithm for a - * neigh_node object * @bat_orig_print: print the originator table (optional) * @bat_orig_free: free the resources allocated by the routing algorithm for an * orig_node object @@ -1316,7 +1314,6 @@ struct batadv_algo_ops { struct batadv_neigh_node *neigh2, struct batadv_hard_iface *if_outgoing2); void (*bat_neigh_print)(struct batadv_priv *priv, struct seq_file *seq); - void (*bat_neigh_free)(struct batadv_neigh_node *neigh); /* orig_node handling API */ void (*bat_orig_print)(struct batadv_priv *priv, struct seq_file *seq, struct batadv_hard_iface *hard_iface);
On Tue, Sep 06, 2016 at 09:25:29AM +0200, Simon Wunderlich wrote:
From: Marek Lindner mareklindner@neomailbox.ch
commit d9f179877e50ae2681fe7b0b83e0d9f63b6165ad upstream.
That is not the commit id of this patch :(
What is the real one?
Please be more careful...
thanks,
greg k-h
On Montag, 12. September 2016 16:52:09 CEST Greg KH wrote:
On Tue, Sep 06, 2016 at 09:25:29AM +0200, Simon Wunderlich wrote:
From: Marek Lindner mareklindner@neomailbox.ch
commit d9f179877e50ae2681fe7b0b83e0d9f63b6165ad upstream.
That is not the commit id of this patch :(
What is the real one?
Please be more careful...
Just checked it and it looks like the patch already mentioned in the "commit xxxxx upstream" line:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d...
I've only rephrased the commit message (see above my signed-off-by line) to explain why it is a user reported [1] problem. The initial patch didn't explain this at all and made it look like a simple cleanup change. We only became aware of the importance of this change after the original "cleanup" version was accepted by Linus (part of 4.8-rc1 when I remember correctly).
Kind regards, Sven
b.a.t.m.a.n@lists.open-mesh.org