netdev - Re: [PATCH] fix NULL pointer + success return in route lookup path

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: <20090622110819.GB13043@hmsreliant.think-freely.org>
Date:	Mon, 22 Jun 2009 07:08:19 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Jarek Poplawski <jarkao2@...il.com>
Cc:	Alexey Kuznetsov <kuznet@....inr.ac.ru>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	mbizon@...ebox.fr, dada1@...mosbay.com, pekkas@...core.fi,
	jmorris@...ei.org, yoshfuji@...ux-ipv6.org
Subject: Re: [PATCH] fix NULL pointer + success return in route lookup path

On Mon, Jun 22, 2009 at 11:00:23AM +0000, Jarek Poplawski wrote:
> On Mon, Jun 22, 2009 at 12:59:50PM +0400, Alexey Kuznetsov wrote:
> > On Mon, Jun 22, 2009 at 05:43:15AM +0000, Jarek Poplawski wrote:
> > > Maybe it can work, but it needs a thorough checking now and adds a new
> > > code path to track later while looking for bugs. So, I wonder if it's
> > > not better to link such dsts in rt_intern_hash anyway, probably as a
> > > separate list, scanned only for expired entries.
> > 
> > Such a list already exists, it is gc list in core/dst.c.
> > 
> > The fix to the problem could be replacing rt_drop() with rt_free()
> > (adding rt_free() after the patch, which deleted rt_drop()), something like:
> > 
> > 	if (!rt_caching(dev_net(rt->u.dst.dev))) {
> > 		/* ..... */
> > +		rt_free(rt);
> > 		goto report_and_exit;
> > 	}
> > 
> > rt_free() will put the route on that gc list and it will be releases
> > as soon as refcnt becomes 0.
> 
> One little doubt would be RCU: if it's currently used in rt_free, and
> some code depends on it, there would be a change: rt could be freed
> just after atomic dec, without waiting for RCU yet. The second one is

Not sure that I see the concern.  How are we going to call dst_rcu_free without
waiting for a quiesence of the use of this route?  When it enters rt_intern_hash
is use count is one, and if we call rt_free in this case, we are guaranteeing
that this path is the only user (no one else will be able to get a reference to
it as they can't look it up in the cache).  Once the route is dropped at the end
of this route lookup, its use is over and the garbage collector will reap it.

> about timing: freeing this always from a workqueue could probably
> make a problem if softirqs are often disabled.
> 
You mean often, or permanently?  If softirqs are often disabled, thats no big
deal, as long as they are enabled at least sometimes, the garbage colelctor will
run, and we'll be ok.  If softirqs are disabled permanently (or for sufficiently
long periods that we gather huge number of routes waiting to be reaped), I think
we have larger problems on our hands

Neil

> 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