[Patch] ipvs: close race conditions on ip_vs_conn_tab list modification

[Patch] ipvs: close race conditions on ip_vs_conn_tab list modification

[Neil Horman]

[-- Attachment #1: Type: text/plain, Size: 1732 bytes --]

Hello there-
	Patch to close a race condition in ip_vs_conn_flush.  In an smp system,
it is possible for an connection timer to expire, calling ip_vs_conn_expire
while the connection table is being flushed, before ct_write_lock_bh is
acquired.  Since the list iterator loop in ip_vs_con_flush releases and
re-acquires the spinlock (even though it doesn't re-enable softirqs), it is
possible for the expiration function to modify the connection list, while it is
being traversed in ip_vs_conn_flush.  The result is that the next pointer gets
set to NULL, and subsequently dereferenced, resulting in an oops.  This patch
removes the lock release and re-aquisition from the loop, closing the race
window.  Tested by myself, and those who origionally experienced the crash and
reported it to me, with successful results.

Signed-off-by: Neil Horman <nhorman@redhat.com>

 ip_vs_conn.c |    2 --
 1 files changed, 2 deletions(-)

 
--- linux-2.6.git/net/ipv4/ipvs/ip_vs_conn.c.orig	2005-06-23 13:11:00.910372471 -0400
+++ linux-2.6.git/net/ipv4/ipvs/ip_vs_conn.c	2005-06-23 13:15:54.459852393 -0400
@@ -840,7 +838,6 @@
 
 		list_for_each_entry(cp, &ip_vs_conn_tab[idx], c_list) {
 			atomic_inc(&cp->refcnt);
-			ct_write_unlock(idx);
 
 			if ((ct = cp->control))
 				atomic_inc(&ct->refcnt);
@@ -850,7 +847,6 @@
 				IP_VS_DBG(4, "del conn template\n");
 				ip_vs_conn_expire_now(ct);
 			}
-			ct_write_lock(idx);
 		}
 		ct_write_unlock_bh(idx);
 	}
-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@redhat.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Patch] ipvs: close race conditions on ip_vs_conn_tab list modification

[Julian Anastasov]

	Hello,

	adding netdev to CC

On Thu, 23 Jun 2005, Neil Horman wrote:

> Hello there-
> 	Patch to close a race condition in ip_vs_conn_flush.  In an smp system,
> it is possible for an connection timer to expire, calling ip_vs_conn_expire
> while the connection table is being flushed, before ct_write_lock_bh is
> acquired.  Since the list iterator loop in ip_vs_con_flush releases and
> re-acquires the spinlock (even though it doesn't re-enable softirqs), it is
> possible for the expiration function to modify the connection list, while it is
> being traversed in ip_vs_conn_flush.  The result is that the next pointer gets
> set to NULL, and subsequently dereferenced, resulting in an oops.  This patch
> removes the lock release and re-aquisition from the loop, closing the race
> window.  Tested by myself, and those who origionally experienced the crash and
> reported it to me, with successful results.
>
> Signed-off-by: Neil Horman <nhorman@redhat.com>
>
>  ip_vs_conn.c |    2 --
>  1 files changed, 2 deletions(-)
>
>
> --- linux-2.6.git/net/ipv4/ipvs/ip_vs_conn.c.orig	2005-06-23 13:11:00.910372471 -0400
> +++ linux-2.6.git/net/ipv4/ipvs/ip_vs_conn.c	2005-06-23 13:15:54.459852393 -0400
> @@ -840,7 +838,6 @@
>
>  		list_for_each_entry(cp, &ip_vs_conn_tab[idx], c_list) {
>  			atomic_inc(&cp->refcnt);
> -			ct_write_unlock(idx);
>
>  			if ((ct = cp->control))
>  				atomic_inc(&ct->refcnt);
> @@ -850,7 +847,6 @@
>  				IP_VS_DBG(4, "del conn template\n");
>  				ip_vs_conn_expire_now(ct);
>  			}
> -			ct_write_lock(idx);
>  		}
>  		ct_write_unlock_bh(idx);
>  	}

	Looks ok but can you test an extended version:

- remove these atomic_inc for cp and ct and the corresponding
__ip_vs_conn_put from ip_vs_conn_expire_now

- do the same for ip_vs_random_dropentry, it looks wrong in the same
way because it is not running anymore together with the connection 
expiration in same sltimer_handler

	Also, 2.4 needs the same changes, I hope you can continue?

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [Patch] ipvs: close race conditions on ip_vs_conn_tab list modification

[Neil Horman]

On Fri, Jun 24, 2005 at 11:46:21AM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> 	adding netdev to CC
> 
> On Thu, 23 Jun 2005, Neil Horman wrote:
><snip> 
> 	Looks ok but can you test an extended version:
> 
> - remove these atomic_inc for cp and ct and the corresponding
> __ip_vs_conn_put from ip_vs_conn_expire_now
> 
> - do the same for ip_vs_random_dropentry, it looks wrong in the same
> way because it is not running anymore together with the connection 
> expiration in same sltimer_handler
> 
> 	Also, 2.4 needs the same changes, I hope you can continue?
> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>

No problem.  New patch attached with the above corrections/enhancements made.
I've tested them here, and had good results.

Signed-off-by: Neil Horman <nhorman@redhat.com>

 ip_vs_conn.c |   15 ---------------
 1 files changed, 15 deletions(-)


--- linux-2.6.git/net/ipv4/ipvs/ip_vs_conn.c.orig	2005-06-23 13:11:00.000000000 -0400
+++ linux-2.6.git/net/ipv4/ipvs/ip_vs_conn.c	2005-06-24 08:57:30.000000000 -0400
@@ -548,7 +548,6 @@
 {
 	if (del_timer(&cp->timer))
 		mod_timer(&cp->timer, jiffies);
-	__ip_vs_conn_put(cp);
 }
 
 
@@ -801,21 +800,12 @@
 					continue;
 			}
 
-			/*
-			 * Drop the entry, and drop its ct if not referenced
-			 */
-			atomic_inc(&cp->refcnt);
-			ct_write_unlock(hash);
-
-			if ((ct = cp->control))
-				atomic_inc(&ct->refcnt);
 			IP_VS_DBG(4, "del connection\n");
 			ip_vs_conn_expire_now(cp);
 			if (ct) {
 				IP_VS_DBG(4, "del conn template\n");
 				ip_vs_conn_expire_now(ct);
 			}
-			ct_write_lock(hash);
 		}
 		ct_write_unlock(hash);
 	}
@@ -839,18 +829,13 @@
 		ct_write_lock_bh(idx);
 
 		list_for_each_entry(cp, &ip_vs_conn_tab[idx], c_list) {
-			atomic_inc(&cp->refcnt);
-			ct_write_unlock(idx);
 
-			if ((ct = cp->control))
-				atomic_inc(&ct->refcnt);
 			IP_VS_DBG(4, "del connection\n");
 			ip_vs_conn_expire_now(cp);
 			if (ct) {
 				IP_VS_DBG(4, "del conn template\n");
 				ip_vs_conn_expire_now(ct);
 			}
-			ct_write_lock(idx);
 		}
 		ct_write_unlock_bh(idx);
 	}
-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@redhat.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

Re: [Patch] ipvs: close race conditions on ip_vs_conn_tab list modification

[Julian Anastasov]

	Hello,

On Fri, 24 Jun 2005, Neil Horman wrote:

>  			if (ct) {
>  				IP_VS_DBG(4, "del conn template\n");
>  				ip_vs_conn_expire_now(ct);
>  			}

	Don't forget to use cp->control instead of ct, ct is not needed
anymore.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [Patch] ipvs: close race conditions on ip_vs_conn_tab list modification

[Neil Horman]

On Fri, Jun 24, 2005 at 06:09:40PM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Fri, 24 Jun 2005, Neil Horman wrote:
> 
> >  			if (ct) {
> >  				IP_VS_DBG(4, "del conn template\n");
> >  				ip_vs_conn_expire_now(ct);
> >  			}
> 
> 	Don't forget to use cp->control instead of ct, ct is not needed
> anymore.
> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>
> 


Good catch.  Sorry, should have seen that earlier.  New patch attached with
corrections.  When you're comfortable with this, I'll post the 2.4 version of
the patch.

Regards 
Neil

Signed-off-by: Neil Horman <nhorman@redhat.com>

 ip_vs_conn.c |   24 ++++--------------------
 1 files changed, 4 insertions(+), 20 deletions(-)


--- linux-2.6.git/net/ipv4/ipvs/ip_vs_conn.c.orig	2005-06-23 13:11:00.000000000 -0400
+++ linux-2.6.git/net/ipv4/ipvs/ip_vs_conn.c	2005-06-24 13:33:03.000000000 -0400
@@ -548,7 +548,6 @@
 {
 	if (del_timer(&cp->timer))
 		mod_timer(&cp->timer, jiffies);
-	__ip_vs_conn_put(cp);
 }
 
 
@@ -801,21 +800,12 @@
 					continue;
 			}
 
-			/*
-			 * Drop the entry, and drop its ct if not referenced
-			 */
-			atomic_inc(&cp->refcnt);
-			ct_write_unlock(hash);
-
-			if ((ct = cp->control))
-				atomic_inc(&ct->refcnt);
 			IP_VS_DBG(4, "del connection\n");
 			ip_vs_conn_expire_now(cp);
-			if (ct) {
+			if (cp->control) {
 				IP_VS_DBG(4, "del conn template\n");
-				ip_vs_conn_expire_now(ct);
+				ip_vs_conn_expire_now(cp->control);
 			}
-			ct_write_lock(hash);
 		}
 		ct_write_unlock(hash);
 	}
@@ -829,7 +819,6 @@
 {
 	int idx;
 	struct ip_vs_conn *cp;
-	struct ip_vs_conn *ct;
 
   flush_again:
 	for (idx=0; idx<IP_VS_CONN_TAB_SIZE; idx++) {
@@ -839,18 +828,13 @@
 		ct_write_lock_bh(idx);
 
 		list_for_each_entry(cp, &ip_vs_conn_tab[idx], c_list) {
-			atomic_inc(&cp->refcnt);
-			ct_write_unlock(idx);
 
-			if ((ct = cp->control))
-				atomic_inc(&ct->refcnt);
 			IP_VS_DBG(4, "del connection\n");
 			ip_vs_conn_expire_now(cp);
-			if (ct) {
+			if (cp->control) {
 				IP_VS_DBG(4, "del conn template\n");
-				ip_vs_conn_expire_now(ct);
+				ip_vs_conn_expire_now(cp->control);
 			}
-			ct_write_lock(idx);
 		}
 		ct_write_unlock_bh(idx);
 	}
-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@redhat.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

Re: [Patch] ipvs: close race conditions on ip_vs_conn_tab list modification

[Wensong Zhang]

Hello Neil,

It's a good fix. Please continue on kernel 2.4 version of your patch.

Thanks,

Wensong


On Fri, 24 Jun 2005, Neil Horman wrote:

> On Fri, Jun 24, 2005 at 06:09:40PM +0300, Julian Anastasov wrote:
>>
>> 	Hello,
>>
>> On Fri, 24 Jun 2005, Neil Horman wrote:
>>
>>>  			if (ct) {
>>>  				IP_VS_DBG(4, "del conn template\n");
>>>  				ip_vs_conn_expire_now(ct);
>>>  			}
>>
>> 	Don't forget to use cp->control instead of ct, ct is not needed
>> anymore.
>>
>> Regards
>>
>> --
>> Julian Anastasov <ja@ssi.bg>
>>
>
>
> Good catch.  Sorry, should have seen that earlier.  New patch attached with
> corrections.  When you're comfortable with this, I'll post the 2.4 version of
> the patch.
>
> Regards
> Neil
>
> Signed-off-by: Neil Horman <nhorman@redhat.com>
>
> ip_vs_conn.c |   24 ++++--------------------
> 1 files changed, 4 insertions(+), 20 deletions(-)
>
>
> --- linux-2.6.git/net/ipv4/ipvs/ip_vs_conn.c.orig	2005-06-23 13:11:00.000000000 -0400
> +++ linux-2.6.git/net/ipv4/ipvs/ip_vs_conn.c	2005-06-24 13:33:03.000000000 -0400
> @@ -548,7 +548,6 @@
> {
> 	if (del_timer(&cp->timer))
> 		mod_timer(&cp->timer, jiffies);
> -	__ip_vs_conn_put(cp);
> }
>
>
> @@ -801,21 +800,12 @@
> 					continue;
> 			}
>
> -			/*
> -			 * Drop the entry, and drop its ct if not referenced
> -			 */
> -			atomic_inc(&cp->refcnt);
> -			ct_write_unlock(hash);
> -
> -			if ((ct = cp->control))
> -				atomic_inc(&ct->refcnt);
> 			IP_VS_DBG(4, "del connection\n");
> 			ip_vs_conn_expire_now(cp);
> -			if (ct) {
> +			if (cp->control) {
> 				IP_VS_DBG(4, "del conn template\n");
> -				ip_vs_conn_expire_now(ct);
> +				ip_vs_conn_expire_now(cp->control);
> 			}
> -			ct_write_lock(hash);
> 		}
> 		ct_write_unlock(hash);
> 	}
> @@ -829,7 +819,6 @@
> {
> 	int idx;
> 	struct ip_vs_conn *cp;
> -	struct ip_vs_conn *ct;
>
>   flush_again:
> 	for (idx=0; idx<IP_VS_CONN_TAB_SIZE; idx++) {
> @@ -839,18 +828,13 @@
> 		ct_write_lock_bh(idx);
>
> 		list_for_each_entry(cp, &ip_vs_conn_tab[idx], c_list) {
> -			atomic_inc(&cp->refcnt);
> -			ct_write_unlock(idx);
>
> -			if ((ct = cp->control))
> -				atomic_inc(&ct->refcnt);
> 			IP_VS_DBG(4, "del connection\n");
> 			ip_vs_conn_expire_now(cp);
> -			if (ct) {
> +			if (cp->control) {
> 				IP_VS_DBG(4, "del conn template\n");
> -				ip_vs_conn_expire_now(ct);
> +				ip_vs_conn_expire_now(cp->control);
> 			}
> -			ct_write_lock(idx);
> 		}
> 		ct_write_unlock_bh(idx);
> 	}
> -- 
> /***************************************************
> *Neil Horman
> *Software Engineer
> *Red Hat, Inc.
> *nhorman@redhat.com
> *gpg keyid: 1024D / 0x92A74FA1
> *http://pgp.mit.edu
> ***************************************************/
>