netdev - Re: [PATCH] pkt_sched: Destroy gen estimators under rtnl

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: <20080818201236.GA3366@ami.dom.local>
Date:	Mon, 18 Aug 2008 22:12:36 +0200
From:	Jarek Poplawski <jarkao2@...il.com>
To:	David Miller <davem@...emloft.net>
Cc:	herbert@...dor.apana.org.au, netdev@...r.kernel.org,
	denys@...p.net.lb
Subject: Re: [PATCH] pkt_sched: Destroy gen estimators under rtnl_lock().

David Miller wrote, On 08/18/2008 12:32 AM:
...
> What is the real problem besides the correct notify_and_destroy()
> issue you discovered?
> 
> The locking we have now is very simple:

Probably simple for you, but I'm not even sure of this. If it were
true there would be no this and a few other threads concerned with
this.

> 
> 1) Only under RTNL can qdisc roots change.
> 
> 2) Therefore, sch_tree_lock() and tcf_tree_lock() are fully valid
>    and lock the entire qdisc tree state, if and only if used under
>    RTNL lock.
> 
> 3) Before modifying a qdisc, we dev_deactivate(), which synchronizes
>    with asynchronous TX/RX packet handling contexts.
> 
> 4) The qdisc root and all children are protected by the root qdiscs
>    lock, which is taken when asynchonous contexts need to blocked
>    while modifying some root or inner qdisc's state.
> 
> Yes, of course, if you apply a hammer and add a bit lock at the
> top of all of this it will fix whatever bugs remain, but as you
> know I don't think that's the solution.

I don't think it's true. What matters here is the qdisc lock. And
within the qdisc's tree only one such lock can matter because current
code doesn't use qdisc locks on lower levels. So we need to take this
one top lock. But as we have seen in notify_and_destroy() or
qdisc_watchdog() it's easy to do this wrong because you've always
analyze the tree plus activated/deactivated state. My proposal is to
simply have still this one lock but easy accessible at some well
known place (actually, with an exception for builtin qdiscs).

> The only substance I've seen is that you've found a violation of #4 in
> notify_and_destroy(), so great let's test the fix for that.

Actually, after partly fixing this we have currently messed this up
again. We can't destroy a qdisc without RCU or some other delayed
method while we're holding a lock inside this - and this is a case
of root qdiscs... It's quite simple too, but why we've missed this
so easily?

As mentioned there was also this tricky qdisc_watchdog (or other
direct __netif_scheduling), but this is not all. Try to look at
qdisc_create() and gen_new_estimator() call. It passes
qdisc_root_lock(sch) somewhere. This is similar to qdisc_watchdog()
problem, but are you really sure we always save the right spinlock
here? I don't think so.

So, of course, if you think such problems are exceptional, and will
not return after current fixes, then we can continue, no problem
for me, but I'm not sure how about Denys or other testers and users.

Jarek P.
--
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