netdev - Re: [PATCHv2 net-next] ipmr: remove cache_resolve_queue

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, 5 Sep 2019 11:37:10 +0800
From:   Hangbin Liu <liuhangbin@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     netdev@...r.kernel.org, Phil Karn <karn@...q.net>,
        Sukumar Gopalakrishnan <sukumarg1973@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>
Subject: Re: [PATCHv2 net-next] ipmr: remove cache_resolve_queue_len

On Wed, Sep 04, 2019 at 09:50:15AM +0200, Eric Dumazet wrote:
> > +static int queue_count(struct mr_table *mrt)
> > +{
> > +	struct list_head *pos;
> > +	int count = 0;
> > +
> > +	spin_lock_bh(&mfc_unres_lock);
> > +	list_for_each(pos, &mrt->mfc_unres_queue)
> > +		count++;
> > +	spin_unlock_bh(&mfc_unres_lock);
> > +
> > +	return count;
> > +}
> 
> I guess that even if we remove a limit on the number of items, we probably should
> keep the atomic counter (no code churn, patch much easier to review...)
> 
> Your patch could be a one liner really [1]
> 
> Eventually replacing this linear list with an RB-tree, so that we can be on the safe side.
> 
> [1]
> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index c07bc82cbbe96d53d05c1665b2f03faa055f1084..313470f6bb148326b4afbc00d265b6a1e40d93bd 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c
> @@ -1134,8 +1134,8 @@ static int ipmr_cache_unresolved(struct mr_table *mrt, vifi_t vifi,
>  
>         if (!found) {
>                 /* Create a new entry if allowable */
> -               if (atomic_read(&mrt->cache_resolve_queue_len) >= 10 ||
> -                   (c = ipmr_cache_alloc_unres()) == NULL) {
> +               c = ipmr_cache_alloc_unres();
> +               if (!c) {
>                         spin_unlock_bh(&mfc_unres_lock);
>  
>                         kfree_skb(skb);

hmm, that looks more clear and easy to review..

Hi David, Alexey,

What do you think? If you also agree, I could post a new version patch.

Thanks
Hangbin