netdev - rcu_read_lock() in tcf_block

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: <CAM_iQpVKVAAQdAEp01uWBvOYUMKf29LOH+7fgGMLywvpwM=WXQ@mail.gmail.com>
Date:   Fri, 28 Sep 2018 10:19:58 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...nulli.us>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Vlad Buslov <vladbu@...lanox.com>
Subject: rcu_read_lock() in tcf_block_find()

(Changing $subject as the discussion is going to a completely different topic)

On Thu, Sep 27, 2018 at 3:19 PM Eric Dumazet <eric.dumazet@...il.com> wrote:
>
>
>
> On 09/27/2018 02:36 PM, Cong Wang wrote:
>
> > I don't understand what you mean by changing ip command, you must
> > mean tc command, but still, I have no idea about how restarting failed
> > syscall could be related to my patch and why we need to restart anything
> > here. If the refcnt goes to 0, it will never come back, retrying won't help
> > anything.
> >
>
> Yep, tc command it is.
>
> I was not especially commenting your patch (replacing an english message by another does
> not seem very big deal), but the fact that the code right there seems to be prepared
> for parallel changes.
>
> But using RCU lookups in control path will lead to occasional failures
> that most user space tools would not expect.
>

I already discussed this with Vlad in the beginning of his RTNL
removal patches, we both agreed some lock is still needed, it is not
completely lockless. Take a look at tc action code now, two spinlocks
are still needed even after we will remove the RTNL there.


> Lets assume two tasks are launching "tc qdisc replace dev eth0 root XXX" in whatever order/parallelism.
>
> Both should succeed, after/before major RTNL->other_locking_mechanism


Yes, it is never going to be completely lockless.


>
> Control paths are usually using a mutex or a spinlock so that they never hit a 0-refcount at all.


For dev->qdisc, sure, we should already hold a refcnt, it can't be gone.

For qdisc_lookup_rcu(), it could be that refcnt goes to 0 before we
remove it from hashtable, right? call_rcu() is only called after
refcnt==0, so rcu read lock can't help here.

Thanks.