On Thu, 2007-07-12 at 09:37 +0200, Jarek Poplawski wrote: > On Wed, Jul 11, 2007 at 04:41:37PM +0300, Ranko Zivojnovic wrote: > > Signed-off-by: Ranko Zivojnovic <[EMAIL PROTECTED]> > > Maybe it's only my issue, but it seems there are no tabs: all spaces...
Nope - you are right - just noticed my mailer converts tabs into spaces - sorry about this... > > ...plus some old doubts: > > - setup_timer(&elist[idx].timer, est_timer, est->interval); > + setup_timer(&elist[idx].timer, est_timer, idx); > I left this because setup_timer expects unsigned long. > ... > > /** > > * gen_kill_estimator - remove a rate estimator > > * @bstats: basic statistics > > @@ -195,31 +201,28 @@ int gen_new_estimator(struct gnet_stats_basic *bstats, > > * > > * Removes the rate estimator specified by &bstats and &rate_est > > * and deletes the timer. > > + * > > + * NOTE: Called under rtnl_mutex > > */ > > void gen_kill_estimator(struct gnet_stats_basic *bstats, > > struct gnet_stats_rate_est *rate_est) > > { > ... > > + list_for_each_entry_safe(e, n, &elist[idx].list, list) { > > + if (e->rate_est != rate_est || e->bstats != bstats) > > + continue; > > > > - kfree(est); > > - killed++; > > + list_del_rcu(&e->list); > > + call_rcu(&e->e_rcu, __gen_kill_estimator); > > } > > - if (killed && elist[idx].list == NULL) > > - del_timer(&elist[idx].timer); > > } > > } > > I've done a bit of mess last time, so maybe it was forgotten, but I > still think this kind of race is possible: > > - gen_kill_estimator is called during qdisc_destroy under > dev->queue_lock, > - est_timer is running and waiting on this lock just on the > list entry of the destroyed class, > - gen_kill_estimator kills the entry and returns, > - in xxx_destroy_class kfree(cl) is done etc., > - est_timer gets the lock and does nbytes = e->bstats->bytes or > e->rate_est-bps = ... with freed memory. Unfortunately I cannot say for sure that _currently_ anything really bad can happen here... someone with better knowledge will have to confirm this - Patrick? Here's why - the only updates that are being done in the est_timer are to the structure pointed by 'e'. Yes - it is possible the values it will refer to could be random (freed memory), _however_ the 'e' structure is still valid as it is _not_ kfree()'d directly from gen_kill_estimator() - i.e. it will not be kfree()'d until rcu_read_unlock() is called - which is outside the loop. I don't mind fixing all the classful qdiscs to call_rcu() to release their class structures for consistency purposes ... in fact ... that is exactly what I will do in order to avoid any potential future mishaps. One may actually decide in the future to add a callback to a user defined function to update some qdisc/class specific rates and given this inconsistency in handling qdiscs vs classes - it could have a nasty backfire. Will have a new patch soon. Regards, Ranko - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html