Ying, This is my current view on what is the best and simplest solution to this. If you still disagree or think you see problems, I am looking forward to a patch from you.
Regards ///jon On 02/19/2016 08:16 PM, Jon Maloy wrote: > When the TIPC module is unloaded, we have identified a race condition > that allows a node reference counter to go to zero and the node instance > being freed before the node timer is finished with accessing it. This > leads to occasional crashes, especially in multi-namespace environments. > > The scenario goes as follows: > > CPU0:(node_stop) CPU1:(node_timeout) // ref == 2 > > 1: if(!mod_timer()) > 2: if (del_timer()) > 3: tipc_node_put() // ref -> 1 > 4: tipc_node_put() // ref -> 0 > 5: kfree_rcu(node); > 6: tipc_node_get(node) > 7: // BOOM! > > We now clean up this functionality as follows: > > 1) We remove the node pointer from the node lookup table before we > attempt deactivating the timer. This way, we reduce the risk that > tipc_node_find() may obtain a valid pointer to an instance marked > for deletion; a harmless but undesirable situation. > > 2) We use del_timer_sync() instead of del_timer() to safely deactivate > the node timer without any risk that it might be reactivated by the > timeout handler. There is no risk of deadlock here, since the two > functions never touch the same spinlocks. > > 3: We remove a pointless tipc_node_get() + tipc_node_put() from the > timeout handler. > > v3: - changed to del_timer_sync() > - don't test for return value of mod_timer() in tipc_node_timeout(), > and don't touch kref. > > Reported-by: Zhijiang Hu <[email protected]> > Signed-off-by: Jon Maloy <[email protected]> > --- > net/tipc/node.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/net/tipc/node.c b/net/tipc/node.c > index f310e931..1fdaed0 100644 > --- a/net/tipc/node.c > +++ b/net/tipc/node.c > @@ -225,9 +225,10 @@ static unsigned int tipc_hashfn(u32 addr) > > static void tipc_node_kref_release(struct kref *kref) > { > - struct tipc_node *node = container_of(kref, struct tipc_node, kref); > + struct tipc_node *n = container_of(kref, struct tipc_node, kref); > > - tipc_node_delete(node); > + kfree(n->bc_entry.link); > + kfree_rcu(n, rcu); > } > > static void tipc_node_put(struct tipc_node *node) > @@ -395,8 +396,10 @@ static void tipc_node_delete(struct tipc_node *node) > { > list_del_rcu(&node->list); > hlist_del_rcu(&node->hash); > - kfree(node->bc_entry.link); > - kfree_rcu(node, rcu); > + tipc_node_put(node); > + > + del_timer_sync(&node->timer); > + tipc_node_put(node); > } > > void tipc_node_stop(struct net *net) > @@ -405,11 +408,8 @@ void tipc_node_stop(struct net *net) > struct tipc_node *node, *t_node; > > spin_lock_bh(&tn->node_list_lock); > - list_for_each_entry_safe(node, t_node, &tn->node_list, list) { > - if (del_timer(&node->timer)) > - tipc_node_put(node); > - tipc_node_put(node); > - } > + list_for_each_entry_safe(node, t_node, &tn->node_list, list) > + tipc_node_delete(node); > spin_unlock_bh(&tn->node_list_lock); > } > > @@ -530,9 +530,7 @@ static void tipc_node_timeout(unsigned long data) > if (rc & TIPC_LINK_DOWN_EVT) > tipc_node_link_down(n, bearer_id, false); > } > - if (!mod_timer(&n->timer, jiffies + n->keepalive_intv)) > - tipc_node_get(n); > - tipc_node_put(n); > + mod_timer(&n->timer, jiffies + n->keepalive_intv); > } > > /** ------------------------------------------------------------------------------ Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now! http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140 _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
