netdev - Re: [NET]: gen_estimator deadlock fix

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 12 Jul 2007 09:37:46 +0200
From:	Jarek Poplawski <jarkao2@...pl>
To:	Ranko Zivojnovic <ranko@...dernet.net>
Cc:	netdev@...r.kernel.org, Andrew Morton <akpm@...ux-foundation.org>,
	Patrick McHardy <kaber@...sh.net>
Subject: Re: [NET]: gen_estimator deadlock fix

On Wed, Jul 11, 2007 at 04:41:37PM +0300, Ranko Zivojnovic wrote:
> Fixes ABBA deadlock noted by Patrick McHardy <kaber@...sh.net>:
> > There is at least one ABBA deadlock, est_timer() does:
> > read_lock(&est_lock)
> > spin_lock(e->stats_lock) (which is dev->queue_lock)
> > 
> > and qdisc_destroy calls htb_destroy under dev->queue_lock, which
> > calls htb_destroy_class, then gen_kill_estimator and this
> > write_locks est_lock.
> 
> est_lock completely removed and the RCU list implemented instead.
> Both gen_kill_estimator and gen_new_estimator are already called under
> rtnl_mutex.
> 
> Signed-off-by: Ranko Zivojnovic <ranko@...dernet.net>

Maybe it's only my issue, but it seems there are no tabs: all spaces...

...plus some old doubts:

> ---
>  net/core/gen_estimator.c |   73 ++++++++++++++++++++++++----------------------
>  1 files changed, 38 insertions(+), 35 deletions(-)
> 
> diff --git a/net/core/gen_estimator.c b/net/core/gen_estimator.c
> index cc84d8d..444b2bd 100644
> --- a/net/core/gen_estimator.c
> +++ b/net/core/gen_estimator.c
...

> @@ -174,20 +175,25 @@ int gen_new_estimator(struct gnet_stats_basic *bstats,
>         est->last_packets = bstats->packets;
>         est->avpps = rate_est->pps<<10;
> 
> -       est->next = elist[est->interval].list;
> -       if (est->next == NULL) {
> -               init_timer(&elist[est->interval].timer);
> -               elist[est->interval].timer.data = est->interval;
> -               elist[est->interval].timer.expires = jiffies + ((HZ<<est->interval)/4);
> -               elist[est->interval].timer.function = est_timer;
> -               add_timer(&elist[est->interval].timer);
> +       if (!elist[idx].timer.function) {
> +               INIT_LIST_HEAD(&elist[idx].list);
> +               setup_timer(&elist[idx].timer, est_timer, est->interval);


 -               setup_timer(&elist[idx].timer, est_timer, est->interval);
 +               setup_timer(&elist[idx].timer, est_timer, idx);

...
>  /**
>   * 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.

Regards,
Jarek P.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html