netdev - Re: [Patch net-next v2 3/4] net_sched: remove tc class reference counting

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:   Fri, 25 Aug 2017 11:18:50 +0200
From:   Florian Westphal <fw@...len.de>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org,
        jhs@...atatu.com, fw@...len.de
Subject: Re: [Patch net-next v2 3/4] net_sched: remove tc class reference
 counting

Jiri Pirko <jiri@...nulli.us> wrote:
> Fri, Aug 25, 2017 at 01:51:29AM CEST, xiyou.wangcong@...il.com wrote:
> >For TC classes, their ->get() and ->put() are always paired, and the
> >reference counting is completely useless, because:
> >
> >1) For class modification and dumping paths, we already hold RTNL lock,
> >   so all of these ->get(),->change(),->put() are atomic.
> 
> There is ongoing initiative by Florian to avoid taking RTNL for some
> rtnetlink calls. I think that for dumping it could be done in tc as well.
> Don't we need the refcnt then?

Dumping is a problem at this time because several places depend on RTNL
to ensure we get a consistent state, even "simple" functions like
rtnl_fill_ifinfo, see e.g.  2907c35ff64708065e5a7fd54e8ded8263eb3074
(net: hold rtnl again in dump callbacks).

So for these places we already need some other way (e.g. seqlock)
to ensure we don't put garbage in netlink skb.

At this time I think that it is better if Congs patches go in
(Unless there are other problems of course) as they simplify
things quite a bit, and I am not sure that we need refcount.

It might be enough to use rcu and detect when the class we just read
from might have been in inconsistent state (so we can retry).

Does that make sense to you?