netdev - Re: [PATCH net-next-2.6] pkt_sched: gen_estimator: kill est

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <AANLkTimgmYjA7Xp19gycRHDXmwP1w1twn92cfT08tCv2@mail.gmail.com>
Date:	Mon, 7 Jun 2010 22:53:37 +0800
From:	Changli Gao <xiaosuo@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	Stephen Hemminger <shemminger@...tta.com>,
	Jarek Poplawski <jarkao2@...il.com>
Subject: Re: [PATCH net-next-2.6] pkt_sched: gen_estimator: kill est_lock 
	rwlock

On Mon, Jun 7, 2010 at 10:32 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> We can use RCU in est_timer() and kill est_lock rwlock.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@...il.com>
> ---
>  net/core/gen_estimator.c |   45 ++++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 27 deletions(-)
>
> diff --git a/net/core/gen_estimator.c b/net/core/gen_estimator.c
> index cf8e703..406d880 100644
> --- a/net/core/gen_estimator.c
> +++ b/net/core/gen_estimator.c
> @@ -102,9 +102,6 @@ struct gen_estimator_head
>
>  static struct gen_estimator_head elist[EST_MAX_INTERVAL+1];
>
> -/* Protects against NULL dereference */
> -static DEFINE_RWLOCK(est_lock);
> -
>  /* Protects against soft lockup during large deletion */
>  static struct rb_root est_root = RB_ROOT;
>
> @@ -115,29 +112,25 @@ static void est_timer(unsigned long arg)
>
>        rcu_read_lock();
>        list_for_each_entry_rcu(e, &elist[idx].list, list) {
> -               u64 nbytes;
> -               u64 brate;
> -               u32 npackets;
> -               u32 rate;
> +               struct gnet_stats_basic_packed *bstats;
>
>                spin_lock(e->stats_lock);
> -               read_lock(&est_lock);
> -               if (e->bstats == NULL)
> -                       goto skip;
> -
> -               nbytes = e->bstats->bytes;
> -               npackets = e->bstats->packets;
> -               brate = (nbytes - e->last_bytes)<<(7 - idx);
> -               e->last_bytes = nbytes;
> -               e->avbps += (brate >> e->ewma_log) - (e->avbps >> e->ewma_log);
> -               e->rate_est->bps = (e->avbps+0xF)>>5;
> -
> -               rate = (npackets - e->last_packets)<<(12 - idx);
> -               e->last_packets = npackets;
> -               e->avpps += (rate >> e->ewma_log) - (e->avpps >> e->ewma_log);
> -               e->rate_est->pps = (e->avpps+0x1FF)>>10;
> -skip:
> -               read_unlock(&est_lock);
> +               bstats = rcu_dereference(e->bstats);
> +               if (bstats) {
> +                       u64 nbytes = ACCESS_ONCE(bstats->bytes);
> +                       u32 npackets = ACCESS_ONCE(bstats->packets);
> +                       u64 brate = (nbytes - e->last_bytes)<<(7 - idx);
> +                       u32 rate;
> +
> +                       e->last_bytes = nbytes;
> +                       e->avbps += (brate >> e->ewma_log) - (e->avbps >> e->ewma_log);
> +                       e->rate_est->bps = (e->avbps+0xF)>>5;
> +
> +                       rate = (npackets - e->last_packets)<<(12 - idx);
> +                       e->last_packets = npackets;
> +                       e->avpps += (rate >> e->ewma_log) - (e->avpps >> e->ewma_log);
> +                       e->rate_est->pps = (e->avpps+0x1FF)>>10;
> +               }
>                spin_unlock(e->stats_lock);
>        }
>
> @@ -271,9 +264,7 @@ void gen_kill_estimator(struct gnet_stats_basic_packed *bstats,
>        while ((e = gen_find_node(bstats, rate_est))) {
>                rb_erase(&e->node, &est_root);
>
> -               write_lock_bh(&est_lock);
> -               e->bstats = NULL;
> -               write_unlock_bh(&est_lock);
> +               rcu_assign_pointer(e->bstats, NULL);
>
>                list_del_rcu(&e->list);
>                call_rcu(&e->e_rcu, __gen_kill_estimator);
>

Hmm. I don't think it is correct.

Look at this code:

void xt_rateest_put(struct xt_rateest *est)
{
        mutex_lock(&xt_rateest_mutex);
        if (--est->refcnt == 0) {
                hlist_del(&est->list);
                gen_kill_estimator(&est->bstats, &est->rstats);
                kfree(est);
        }
        mutex_unlock(&xt_rateest_mutex);
}

est will be released after gen_kill_estimator() returns. After est is
freed, there may still be some other CPUs running in the branch:
     if (bstats) {
         ....
     }

-- 
Regards,
Changli Gao(xiaosuo@...il.com)
--
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