netdev - Re: [RFC,PATCH] ipvs: Fix race condition in lblb and lblcr schedulers

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <alpine.LNX.1.10.0808150801560.12259@titan.stealer.net>
Date:	Fri, 15 Aug 2008 08:03:32 +0200 (CEST)
From:	Sven Wegener <sven.wegener@...aler.net>
To:	Simon Horman <horms@...ge.net.au>
cc:	lvs-devel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [RFC,PATCH] ipvs: Fix race condition in lblb and lblcr
 schedulers

On Wed, 13 Aug 2008, Sven Wegener wrote:

> On Wed, 13 Aug 2008, Simon Horman wrote:
> 
> > On Wed, Aug 13, 2008 at 08:59:32AM +0200, Sven Wegener wrote:
> > > On Wed, 13 Aug 2008, Simon Horman wrote:
> > > 
> > > > On Tue, Aug 12, 2008 at 03:07:48PM +0200, Sven Wegener wrote:
> > > > > 
> > > > > +
> > > > > +		/*
> > > > > +		 * If the destination is not available, i.e. it's in the trash,
> > > > > +		 * ignore it, as it may be removed from under our feet
> > > > > +		 */
> > > > > +
> > > > > +		if (!(dest->flags & IP_VS_DEST_F_AVAILABLE))
> > > > > +			dest = NULL;
> > > > >  	}
> > > > > -	en->lastuse = jiffies;
> > > > 
> > > > At this point we know that dest exists and is not in the trash.
> > > > We know that dest can't disapear given that its not already
> > > > in the trash and our caller holds a read lock on __ip_vs_svc_lock.
> > > > So dest will be safe until after ip_vs_lblc_schedule() returns.
> > > > 
> > > > Dest seems ok :-)
> > > > 
> > > > Ok, that seems complex but non-racy to me :-)
> > > > 
> > > > Perhaps a longer comment would be in order.
> > > 
> > > Yeah, that's the point where we prevent the race with the trash purging. 
> > > We could change the purging of destinations from the trash to be under 
> > > __ip_vs_svc_lock write locked, then we know that all destinations, even 
> > > the ones in the trash are valid. Might make more sense than duplicating 
> > > this logic in other schedulers.
> > 
> > That sounds like a good way to simplify things.
> 
> Looking at the code it doesn't seem that simple. We lock __ip_vs_svc_lock 
> for writing and then wait for all current users of the service to go away. 
> But it's still possible that we clean up destinations from other services. 
> And when we restrict it to only clean up the destinations that match our 
> service, we have a chance of leaving destinations in the trash for good.

Still thinking about it, I'm against doing it globally. It's too much 
hassle and in the end we still need to check for IP_VS_DEST_F_AVAILABLE in 
the schedulers, because destinations in the trash shouldn't be used for 
further connections.

Sven
--
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