Re: [Bugme-new] [Bug 11469] New: TUN with 1024 neighbours: ip6_dst_lookup_tail NULL crash

[David Miller <davem@davemloft.net>]

From: Rémi Denis-Courmont <rdenis@simphalempin.com>
Date: Sun, 7 Sep 2008 21:19:49 +0300

> Le dimanche 7 septembre 2008 21:11:09 Evgeniy Polyakov, vous avez écrit :
> > Since dst entry is allowed not to have neighbour entry, flush it just
> > like with incomplete one. This drops performance of your application
> > with more than 1024 neighbours to 1024 messages, to fix it you should
> > tune ipv6 routing parameters (gc intervals, gc threshold, maximum number
> > of entries and so on). There may be another problem with perfomance
> > though, at least I was able to bump it 10 times with different settings,
> > but still two times smaller than with 4k neighbours.
> 
> That looks like a trivial local DoS against the IPv6 stack though?
> 
> Especially in the case that the interface has IFF_NOARP, that seems like a 
> weird limitation. Oh well...

It's less of a DoS than the NULL pointer deref we get now, isn't it? :-)

But I don't like this patch for several reasons:

1) Slapping on a NULL check in response to a OOPS at that exact
   location is usually a very big red flag, and deserves high scrutiny
   instead of blind acceptance.

2) Looking at the indentation of this DAD code block (it's all one tab
   too much) it's obviously a very shitty cut and paste job.  If the
   coding style was too difficult to get right, what does this say
   about that change that brought the code here, semantically? :-/

   This means we should figure out how this code got to this place,
   and what kind of invariants existed at the old location that might
   make this dst->neighbour dereference valid, and what implications
   there are for the fact that it can now be NULL.

Really, we really need to understand much more deeply this situation.