[Chilli] Ippool exhaustion bug?

[David Bird]

Hmm.. 

I think I see how this can happen. I likely added that ability to
allocate a static IP out of tun_ind() so that truly static IPs would
work (though, looking at it now, I don't think the code helps that
situation since as you noted, it's not a fully configured dhcp lease at
that point, just an entry in the ippool). I'm thinking it'll be wise to
remove the call to ippool_newip() at this point in the code. Instead,
perhaps it just goes ahead and accepts the packet if within the statip
range -- then, presumably, the computer with the statip will reply to
the packets, which will create the dhcpconn/appconn/etc properly (from
the packet originating from the dhcpif). 

David


On Fri, 2009-11-13 at 08:31 +0200, Gunther Mayer wrote:
> David Bird wrote:
> > Hi Gunther,
> >
> > You mention one IP being allocated by tun_ind(), which means it's the
> > result of packets coming into tun0/tap0, which means it's not the
> > traffic from the end user side of chilli -- rather it's the WAN side.
> >   
> Hmm, could be my openvpn tunnel or other pre-existing connections from 
> before which still try to pump traffic from the WAN side to my client 
> but I doubt this - as in the below example cb_tun_ind() acts always a 
> second before the dhcp request is acted upon (I can send you more logs 
> to show this if you like).
> > I'm unclear how this is filling up the pool, however. Is it multiple
> > entries for the same IP or always allocating new IPs out of tun_ind() ?
> >   
> 
> It's not *always* allocating new IPs out of cb_tun_ind() but seems to do 
> so most of the time if my client has had connections to chilli before 
> (even if chilli itself has already released this IP), most likely 
> because my dhcp client tries to recycle still valid leases though I'm 
> not sure. The pool is getting filled up by cb_tun_ind() allocating one 
> IP which never gets "garbage collected" followed shortly by a real dhcp 
> dynamic ip allocation which does get deallocated properly upon expiry, 
> that's apparent in my logs.
> > Are you using --usetap (if so, try without).
> Nope, standard tun device, no tap.
> > Are you narrowing your DHCP
> > pool only for testing or is your real world IP allocation really a
> > network smaller than /24 ?
> >   
> 
> As I said in my first email I'm narrowing my pool only for testing 
> purposes, in the real world it goes from 129 to 254 (126 ip's) but to 
> get such a large pool to be exhausted is impossible to reproduce in a 
> testing environment.
> 
> I have actually made progress on this issue since - seeing that I did 
> narrow it down to cb_tun_ind() being the culprit I saw that its call to 
> ippool_newip() wasn't there in 1.0.11 (r147 I think) which is the 
> previous stable version we used. So I reverted this piece of code to the 
> old behaviour, taking ippool_newip() totally out of cb_tun_ind() and now 
> everything works for me 100%.
> 
> However, there must be a reason you (or another developer?) added the 
> ippool_newip() statement in there in the first place. I'm not sure for 
> which use cases it's required but it seems to me that while it allocates 
> a new ip for the client, it doesn't properly update the other house 
> keeping data structures which means that
> 
>     a) when it comes to actually processing the dhcp request the code 
> doesn't pick up that the allocation has already happened
>     b) when the lease expires the ip is returned back to the pool (inuse 
> = 0)
> 
> Gunther