linux-kernel - Re: [patch 20/60] ipvs: Fix possible deadlock in estimator code

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] [day] [month] [year] [list]

Message-ID: <20080819003135.GA23626@verge.net.au>
Date:	Tue, 19 Aug 2008 10:31:35 +1000
From:	Simon Horman <horms@...ge.net.au>
To:	Greg KH <gregkh@...e.de>
Cc:	linux-kernel@...r.kernel.org, stable@...nel.org, jejb@...nel.org,
	Justin Forbes <jmforbes@...uxtx.org>,
	Zwane Mwaikambo <zwane@....linux.org.uk>,
	Theodore Ts'o <tytso@....edu>,
	Randy Dunlap <rdunlap@...otime.net>,
	Dave Jones <davej@...hat.com>,
	Chuck Wolber <chuckw@...ntumlinux.com>,
	Chris Wedgwood <reviews@...cw.f00f.org>,
	Michael Krufky <mkrufky@...uxtv.org>,
	Chuck Ebbert <cebbert@...hat.com>,
	Domenico Andreoli <cavokz@...il.com>, Willy Tarreau <w@....eu>,
	Rodrigo Rubira Branco <rbranco@...checkpoint.com>,
	Jake Edge <jake@....net>, Eugene Teo <eteo@...hat.com>,
	torvalds@...ux-foundation.org, akpm@...ux-foundation.org,
	alan@...rguk.ukuu.org.uk, Sven Wegener <sven.wegener@...aler.net>
Subject: Re: [patch 20/60] ipvs: Fix possible deadlock in estimator code

On Mon, Aug 18, 2008 at 11:43:10AM -0700, Greg KH wrote:
> 2.6.26-stable review patch.  If anyone has any objections, please let us know.
> 
> ------------------
> From: Sven Wegener <sven.wegener@...aler.net>
> 
> commit 8ab19ea36c5c5340ff598e4d15fc084eb65671dc upstream
> 
> There is a slight chance for a deadlock in the estimator code. We can't call
> del_timer_sync() while holding our lock, as the timer might be active and
> spinning for the lock on another cpu. Work around this issue by using
> try_to_del_timer_sync() and releasing the lock. We could actually delete the
> timer outside of our lock, as the add and kill functions are only every called
> from userspace via [gs]etsockopt() and are serialized by a mutex, but better
> make this explicit.
> 
> Signed-off-by: Sven Wegener <sven.wegener@...aler.net>
> Acked-by: Simon Horman <horms@...ge.net.au>
> Signed-off-by: Greg Kroah-Hartman <gregkh@...e.de>
> 
> ---
>  net/ipv4/ipvs/ip_vs_est.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> --- a/net/ipv4/ipvs/ip_vs_est.c
> +++ b/net/ipv4/ipvs/ip_vs_est.c
> @@ -172,8 +172,11 @@ void ip_vs_kill_estimator(struct ip_vs_s
>  		kfree(est);
>  		killed++;
>  	}
> -	if (killed && est_list == NULL)
> -		del_timer_sync(&est_timer);
> +	while (killed && !est_list && try_to_del_timer_sync(&est_timer) < 0) {
> +		write_unlock_bh(&est_lock);
> +		cpu_relax();
> +		write_lock_bh(&est_lock);
> +	}
>  	write_unlock_bh(&est_lock);
>  }
>  

Hi,

I am comfortable with this change for both 2.6.25-stable and 2.6.26-stable.

An arguably cleaner though more invasive fix, which will likely go
into 2.6.28 is to use init and cleanup fuctions for the estimators
as the empty case can actually only occur during the unload of IPVS.

See "ipvs: Create init functions for estimator code", which was posted by
Sven to netdev & lvs-devel, and is currently living in horms/lvs-2.6.git on
git.kernel.org.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/