netdev - Re: [PATCH net-next] net: sched: change tcf_del_walker() to use concurrent-safe delete

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, 6 Sep 2018 12:58:42 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Vlad Buslov <vladbu@...lanox.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        David Miller <davem@...emloft.net>
Subject: Re: [PATCH net-next] net: sched: change tcf_del_walker() to use
 concurrent-safe delete

On Thu, Sep 6, 2018 at 4:14 AM Vlad Buslov <vladbu@...lanox.com> wrote:
> > Isn't a concurrent tcf_idr_check_alloc() able to livelock here with
> > your change?
> >
> > idr_for_each_entry_ul{
> >    spin_lock(&idrinfo->lock);
> >    idr_remove();
> >    spin_unlock(&idrinfo->lock);
> >       // tcf_idr_check_alloc() jumps in,
> >      // allocates next ID which can be found
> >       // by idr_get_next_ul()
> > } // the whole loop goes _literately_ infinite...
>
> idr_for_each_entry_ul traverses idr entries with ascending order of
> identifiers, so infinite livelock like this is not possible because it
> never goes back to newly added entries with id<current_id.

I said "literately infinite", it could go from 1 to UINT_MAX,
sufficient to prove my point of livelock.


> >
> > Also, idr_for_each_entry_ul() is supposed to be protected either
> > by RCU or idrinfo->lock, no? With your change or without any change,
> > it doesn't even have any lock after removing RTNL?
>
> After reading this comment I checked actual idr implementation and I
> think you are right. Even though idr_for_each_entry_ul() macro (and
> function idr_get_next_ul() that it uses to iterate over idr entries)
> doesn't specify any locking requirements in comment description (that is
> why this patch doesn't use any), its implementation seems to require
> external synchronization.

Yeah, it is also a reader, so either a reader lock like RCU or a writer lock
like idrinfo->lock.

>
> You suggest I should just hold idrinfo->lock for whole del_walker loop
> duration, or play nicely with potential concurrent users and
> take/release it per action?

My suggestion is pretty clear, you just missed it, let me copy-n-paste:

With what I suggest:

spin_lock(&idrinfo->lock);
idr_for_each_entry_ul{
   idr_remove();
}
spin_unlock(&idrinfo->lock);