Ying, del_timer() definitely returns 1 after the timer function has called mod_timer(), which is the case here. After mod_timer() has been called, the timer is active, no matter its previous state and return value. So, in the brief interval between mod_timer() and tipc_node_get() this scenario can happen. Please explain why you don’t see a problem with this. Let me also remind you that we actually see a crash happening here.
I am aware about how this is done in the socket code, and it is the correct solution there, but not in our case, or at least it is unnecessarily complex. They have done it that way because they anticipate that sk_reset_timer() may be called after sk_stop_timer(), and hence restart a stopped timer. This never happens in our case. The timer is started when the node is created, and continues running until it is deleted. Full stop. Between those two points in time kref is kept incremented, and the fact that the timer is “ïnactive” during most of the execution of the timer function changes nothing to that. The timer is once again, unconditionally, active when that function returns. Why make things more complex than necessary? Regards ///jon From: Xue, Ying [mailto:[email protected]] Sent: Tuesday, 23 February, 2016 05:20 To: jason Cc: Jon Maloy; Richard Alpe; Parthasarathy Bhuvaragan; [email protected]; Jon Maloy Subject: RE: [PATCH net-next v3 1/1] tipc: fix crash during node removal Even if node timeout function restarts the node timer with mod_timer(), I don’t see any problem here. If you concern the usage about dealing with node timer and its refcount, please look at another similar example which can demonstrate how to maintain the relationship between sk_timer and sk_refcnt. For instance, you can refer to both sk_reset_timer() and sk_stop_timer(). Regards, Ying From: jason [mailto:[email protected]] Sent: 2016年2月23日 17:55 To: Xue, Ying Cc: Jon Maloy; Richard Alpe; [email protected]<mailto:[email protected]>; [email protected]<mailto:[email protected]>; Jon Maloy Subject: RE: [PATCH net-next v3 1/1] tipc: fix crash during node removal On Feb 22, 2016 11:22 PM, "Xue, Ying" <[email protected]<mailto:[email protected]>> wrote: > > Hi Jon, > > I think the scenario described below is not true. This is because del_timer() > doesn't return 1 while node_timeout() is being called. Please take a look at > __run_timers() defined in kernel/time/timer.c. When a timer is expired, > __run_timers() will call the timeout function attached to the timer. But > before __run_timers() runs timeout function, it first detaches the expired > timer, Hi Ying, But the timeout function here do mod_timer() which attach itself again? and then performs its timeout function. That means del_timer() will definitely return 0 while node_timeout() is under execution. So, the following scenario cannot happen at all. > > Regards, > Ying > > -----Original Message----- > From: Jon Maloy [mailto:[email protected]<mailto:[email protected]>] > Sent: 2016年2月20日 9:17 > To: > [email protected]<mailto:[email protected]>; > > [email protected]<mailto:[email protected]>; > Xue, Ying; [email protected]<mailto:[email protected]>; > [email protected]<mailto:[email protected]>; > [email protected]<mailto:[email protected]> > Cc: [email protected]<mailto:[email protected]> > Subject: [PATCH net-next v3 1/1] tipc: fix crash during node removal > > 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]<mailto:[email protected]>> > Signed-off-by: Jon Maloy > <[email protected]<mailto:[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); > } > > /** > -- > 1.9.1 > ------------------------------------------------------------------------------ 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
