netdev - Re: pkt_sched: gen_estimator: more fuel for Jarek and Changli

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]

Date:	Wed, 9 Jun 2010 08:14:15 +0000
From:	Jarek Poplawski <jarkao2@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Changli Gao <xiaosuo@...il.com>,
	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	Stephen Hemminger <shemminger@...tta.com>,
	Patrick McHardy <kaber@...sh.net>
Subject: Re: pkt_sched: gen_estimator: more fuel for Jarek and Changli

On Wed, Jun 09, 2010 at 09:36:04AM +0200, Eric Dumazet wrote:
> Le mercredi 09 juin 2010 ?? 06:51 +0000, Jarek Poplawski a écrit :
> > On Wed, Jun 09, 2010 at 08:13:17AM +0200, Eric Dumazet wrote:
> > > 
> > > With un-modified kernel, I ran following scripts on my machine
> > 
> > Why not modified with your other patch quite obviously needed by
> > rateest?:
> > 
> 
> First patch is obvious, but I cooked same script to trigger both bugs,
> because you needed some evidences.
> 
> > > [PATCH net-2.6] pkt_sched: gen_estimator: add a new lock
> > > 
> > > gen_kill_estimator() / gen_new_estimator() is not always called with
> > > RTNL held.
> > 
> > Btw, I agree with Changli that adding RTNL to rateest (if possible),
> > and doing the RTNL replacement later, seems more proper.
> > 
> 
> I wont be the guy adding RTNL to netfilter, thats for sure. That would
> be a step backward. 
> Sometimes, the 'obvious' fix is also a dumb one.
> Do you really think I didnt had this idea too ?
> 
> xt_RATEEST is an unfortunate domain intersection (netfilter / sched).
> 
> We can solve this using a fine grained lock, instead of interesting
> lockdep issues, yet to be discovered.
> 
> You can submit your patch, but I wont Ack it, I'll Nack it for all these
> reasons.

I'm not going to submit my patch. The reason for using the RTNL is
the current gen_estimator API. If rateest were done properly it would
have to use it, and I'm astonished Patrick missed it, since it took
place just around this API fix (with Patrick's part) with the patch I
mentioned:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=0929c2dd83317813425b937fbc0041013b8685ff

So, the best would be to get Patrick's opinion. Otherwise, of course
your patch is the next alternative.

> 
> Why dont we remove all locks we have 'because we can use RTNL and be
> with it' ?
> 
> qdisc_mod_lock could be removed quite instantly, qdisc_base could be
> protected by RTNL... And so on...
> 

Yes, but it should be done independently from fixing bugs.

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