[PATCH] (4/6) ipvs -- use reader locks and refcounts correctly.

[PATCH] (4/6) ipvs -- use reader locks and refcounts correctly.

[Stephen Hemminger]

The code to get the service table was not taking a reader lock
because it needed to copy_to_user.

Do it right:
	- hold refcnt when copying out, and use a read lock
	- make lock local to ip_svc
	- get rid of IP_VS_WAIT_WHILE, use proper read locks and refcounts.
	- move write_lock_bh in flush to outside of loop
	- tag user pointers (for sparse).

diff -Nru a/include/net/ip_vs.h b/include/net/ip_vs.h
--- a/include/net/ip_vs.h	Tue Sep 16 14:08:55 2003
+++ b/include/net/ip_vs.h	Tue Sep 16 14:08:55 2003
@@ -319,8 +319,6 @@
 #define LeaveFunction(level)   do {} while (0)
 #endif
 
-#define	IP_VS_WAIT_WHILE(expr)	while (expr) { cpu_relax(); }
-
 
 /*
  *      The port number of FTP service (in network order).
diff -Nru a/net/ipv4/ipvs/ip_vs_ctl.c b/net/ipv4/ipvs/ip_vs_ctl.c
--- a/net/ipv4/ipvs/ip_vs_ctl.c	Tue Sep 16 14:08:55 2003
+++ b/net/ipv4/ipvs/ip_vs_ctl.c	Tue Sep 16 14:08:55 2003
@@ -48,7 +48,7 @@
 static DECLARE_MUTEX(__ip_vs_mutex);
 
 /* lock for service table */
-rwlock_t __ip_vs_svc_lock = RW_LOCK_UNLOCKED;
+static rwlock_t __ip_vs_svc_lock = RW_LOCK_UNLOCKED;
 
 /* lock for table with the real services */
 static rwlock_t __ip_vs_rs_lock = RW_LOCK_UNLOCKED;
@@ -808,11 +808,6 @@
 
 		write_lock_bh(&__ip_vs_svc_lock);
 
-		/*
-		 * Wait until all other svc users go away.
-		 */
-		IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
-
 		list_add(&dest->n_list, &svc->destinations);
 		svc->num_dests++;
 
@@ -838,11 +833,6 @@
 
 	write_lock_bh(&__ip_vs_svc_lock);
 
-	/*
-	 * Wait until all other svc users go away.
-	 */
-	IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
-
 	list_add(&dest->n_list, &svc->destinations);
 	svc->num_dests++;
 
@@ -980,12 +970,6 @@
 	}
 
 	write_lock_bh(&__ip_vs_svc_lock);
-
-	/*
-	 *	Wait until all other svc users go away.
-	 */
-	IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
-
 	/*
 	 *	Unlink dest from the service
 	 */
@@ -1118,11 +1102,6 @@
 	write_lock_bh(&__ip_vs_svc_lock);
 
 	/*
-	 * Wait until all other svc users go away.
-	 */
-	IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
-
-	/*
 	 * Set the flags and timeout value
 	 */
 	svc->flags = u->flags | IP_VS_SVC_F_HASHED;
@@ -1235,11 +1214,6 @@
 
 	ip_vs_svc_unhash(svc);
 
-	/*
-	 * Wait until all the svc users go away.
-	 */
-	IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
-
 	__ip_vs_del_service(svc);
 
 	write_unlock_bh(&__ip_vs_svc_lock);
@@ -1259,16 +1233,11 @@
 	/*
 	 * Flush the service table hashed by <protocol,addr,port>
 	 */
+	write_lock_bh(&__ip_vs_svc_lock);
 	for(idx = 0; idx < IP_VS_SVC_TAB_SIZE; idx++) {
 		list_for_each_entry_safe(svc, nxt, &ip_vs_svc_table[idx], s_list) {
-			write_lock_bh(&__ip_vs_svc_lock);
 			ip_vs_svc_unhash(svc);
-			/*
-			 * Wait until all the svc users go away.
-			 */
-			IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 0);
 			__ip_vs_del_service(svc);
-			write_unlock_bh(&__ip_vs_svc_lock);
 		}
 	}
 
@@ -1278,16 +1247,11 @@
 	for(idx = 0; idx < IP_VS_SVC_TAB_SIZE; idx++) {
 		list_for_each_entry_safe(svc, nxt, 
 					 &ip_vs_svc_fwm_table[idx], f_list) {
-			write_lock_bh(&__ip_vs_svc_lock);
 			ip_vs_svc_unhash(svc);
-			/*
-			 * Wait until all the svc users go away.
-			 */
-			IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 0);
 			__ip_vs_del_service(svc);
-			write_unlock_bh(&__ip_vs_svc_lock);
 		}
 	}
+	write_unlock_bh(&__ip_vs_svc_lock);
 
 	return 0;
 }
@@ -1926,7 +1890,7 @@
 
 static inline int
 __ip_vs_get_service_entries(const struct ip_vs_get_services *get,
-			    struct ip_vs_get_services *uptr)
+			    struct ip_vs_get_services __user *uptr)
 {
 	int idx, count=0;
 	struct ip_vs_service *svc;
@@ -1966,7 +1930,7 @@
 
 static inline int
 __ip_vs_get_dest_entries(const struct ip_vs_get_dests *get,
-			 struct ip_vs_get_dests *uptr)
+			 struct ip_vs_get_dests __user *uptr)
 {
 	struct ip_vs_service *svc;
 	int ret = 0;
@@ -1981,6 +1945,7 @@
 		struct ip_vs_dest *dest;
 		struct ip_vs_dest_entry entry;
 
+		read_lock_bh(&__ip_vs_svc_lock);
 		list_for_each_entry(dest, &svc->destinations, n_list) {
 			if (count >= get->num_dests)
 				break;
@@ -1994,15 +1959,25 @@
 			entry.activeconns = atomic_read(&dest->activeconns);
 			entry.inactconns = atomic_read(&dest->inactconns);
 			entry.persistconns = atomic_read(&dest->persistconns);
+
 			ip_vs_copy_stats(&entry.stats, &dest->stats);
+
+			atomic_inc(&dest->refcnt);
+			read_unlock_bh(&__ip_vs_svc_lock);
 			if (copy_to_user(&uptr->entrytable[count],
 					 &entry, sizeof(entry))) {
 				ret = -EFAULT;
 				break;
 			}
 			count++;
+
+			read_lock_bh(&__ip_vs_svc_lock);
+			if (atomic_dec_and_test(&dest->refcnt)) 
+				kfree(dest);
+
 		}
 		ip_vs_service_put(svc);
+		read_unlock_bh(&__ip_vs_svc_lock);
 	} else
 		ret = -ESRCH;
 	return ret;
@@ -2043,7 +2018,7 @@
 };
 
 static int
-do_ip_vs_get_ctl(struct sock *sk, int cmd, void *user, int *len)
+do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len)
 {
 	unsigned char arg[128];
 	int ret = 0;

Re: [PATCH] (4/6) ipvs -- use reader locks and refcounts correctly.

[Wensong Zhang]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 6219 bytes --]



Hi,

The locking change may be not very correct.

On Tue, 16 Sep 2003, Stephen Hemminger wrote:

> The code to get the service table was not taking a reader lock
> because it needed to copy_to_user.
> 
> Do it right:
> 	- hold refcnt when copying out, and use a read lock

There is no need to hold lock, because thread has already gained 
__ip_vs_mutex, there is no writer of service table, although net softirq 
may read service table, there is no conflict in reading the service table 
and copying out.

> 	- make lock local to ip_svc

Good.

> 	- get rid of IP_VS_WAIT_WHILE, use proper read locks and refcounts.

We have to use IP_VS_WAIT_WHILE to wait that all service users put back 
the service, because the user doesn't hold read locks all the time while 
using the service, it just increase the usecnt to indicate that the 
service is used and release the read lock immediately. Before the user put 
back the service, we cannot remove it.

> 	- move write_lock_bh in flush to outside of loop

Not necessary, because it has already hold __ip_vs_mutex, there is only 
one writer.

> 	- tag user pointers (for sparse).
> 

OK.

So, maybe we just apply the simple patch with making the lock local and 
tagging user pointers.

Thanks,

Wensong

> diff -Nru a/include/net/ip_vs.h b/include/net/ip_vs.h
> --- a/include/net/ip_vs.h	Tue Sep 16 14:08:55 2003
> +++ b/include/net/ip_vs.h	Tue Sep 16 14:08:55 2003
> @@ -319,8 +319,6 @@
>  #define LeaveFunction(level)   do {} while (0)
>  #endif
>  
> -#define	IP_VS_WAIT_WHILE(expr)	while (expr) { cpu_relax(); }
> -
>  
>  /*
>   *      The port number of FTP service (in network order).
> diff -Nru a/net/ipv4/ipvs/ip_vs_ctl.c b/net/ipv4/ipvs/ip_vs_ctl.c
> --- a/net/ipv4/ipvs/ip_vs_ctl.c	Tue Sep 16 14:08:55 2003
> +++ b/net/ipv4/ipvs/ip_vs_ctl.c	Tue Sep 16 14:08:55 2003
> @@ -48,7 +48,7 @@
>  static DECLARE_MUTEX(__ip_vs_mutex);
>  
>  /* lock for service table */
> -rwlock_t __ip_vs_svc_lock = RW_LOCK_UNLOCKED;
> +static rwlock_t __ip_vs_svc_lock = RW_LOCK_UNLOCKED;
>  
>  /* lock for table with the real services */
>  static rwlock_t __ip_vs_rs_lock = RW_LOCK_UNLOCKED;
> @@ -808,11 +808,6 @@
>  
>  		write_lock_bh(&__ip_vs_svc_lock);
>  
> -		/*
> -		 * Wait until all other svc users go away.
> -		 */
> -		IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
> -
>  		list_add(&dest->n_list, &svc->destinations);
>  		svc->num_dests++;
>  
> @@ -838,11 +833,6 @@
>  
>  	write_lock_bh(&__ip_vs_svc_lock);
>  
> -	/*
> -	 * Wait until all other svc users go away.
> -	 */
> -	IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
> -
>  	list_add(&dest->n_list, &svc->destinations);
>  	svc->num_dests++;
>  
> @@ -980,12 +970,6 @@
>  	}
>  
>  	write_lock_bh(&__ip_vs_svc_lock);
> -
> -	/*
> -	 *	Wait until all other svc users go away.
> -	 */
> -	IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
> -
>  	/*
>  	 *	Unlink dest from the service
>  	 */
> @@ -1118,11 +1102,6 @@
>  	write_lock_bh(&__ip_vs_svc_lock);
>  
>  	/*
> -	 * Wait until all other svc users go away.
> -	 */
> -	IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
> -
> -	/*
>  	 * Set the flags and timeout value
>  	 */
>  	svc->flags = u->flags | IP_VS_SVC_F_HASHED;
> @@ -1235,11 +1214,6 @@
>  
>  	ip_vs_svc_unhash(svc);
>  
> -	/*
> -	 * Wait until all the svc users go away.
> -	 */
> -	IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
> -
>  	__ip_vs_del_service(svc);
>  
>  	write_unlock_bh(&__ip_vs_svc_lock);
> @@ -1259,16 +1233,11 @@
>  	/*
>  	 * Flush the service table hashed by <protocol,addr,port>
>  	 */
> +	write_lock_bh(&__ip_vs_svc_lock);
>  	for(idx = 0; idx < IP_VS_SVC_TAB_SIZE; idx++) {
>  		list_for_each_entry_safe(svc, nxt, &ip_vs_svc_table[idx], s_list) {
> -			write_lock_bh(&__ip_vs_svc_lock);
>  			ip_vs_svc_unhash(svc);
> -			/*
> -			 * Wait until all the svc users go away.
> -			 */
> -			IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 0);
>  			__ip_vs_del_service(svc);
> -			write_unlock_bh(&__ip_vs_svc_lock);
>  		}
>  	}
>  
> @@ -1278,16 +1247,11 @@
>  	for(idx = 0; idx < IP_VS_SVC_TAB_SIZE; idx++) {
>  		list_for_each_entry_safe(svc, nxt, 
>  					 &ip_vs_svc_fwm_table[idx], f_list) {
> -			write_lock_bh(&__ip_vs_svc_lock);
>  			ip_vs_svc_unhash(svc);
> -			/*
> -			 * Wait until all the svc users go away.
> -			 */
> -			IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 0);
>  			__ip_vs_del_service(svc);
> -			write_unlock_bh(&__ip_vs_svc_lock);
>  		}
>  	}
> +	write_unlock_bh(&__ip_vs_svc_lock);
>  
>  	return 0;
>  }
> @@ -1926,7 +1890,7 @@
>  
>  static inline int
>  __ip_vs_get_service_entries(const struct ip_vs_get_services *get,
> -			    struct ip_vs_get_services *uptr)
> +			    struct ip_vs_get_services __user *uptr)
>  {
>  	int idx, count=0;
>  	struct ip_vs_service *svc;
> @@ -1966,7 +1930,7 @@
>  
>  static inline int
>  __ip_vs_get_dest_entries(const struct ip_vs_get_dests *get,
> -			 struct ip_vs_get_dests *uptr)
> +			 struct ip_vs_get_dests __user *uptr)
>  {
>  	struct ip_vs_service *svc;
>  	int ret = 0;
> @@ -1981,6 +1945,7 @@
>  		struct ip_vs_dest *dest;
>  		struct ip_vs_dest_entry entry;
>  
> +		read_lock_bh(&__ip_vs_svc_lock);
>  		list_for_each_entry(dest, &svc->destinations, n_list) {
>  			if (count >= get->num_dests)
>  				break;
> @@ -1994,15 +1959,25 @@
>  			entry.activeconns = atomic_read(&dest->activeconns);
>  			entry.inactconns = atomic_read(&dest->inactconns);
>  			entry.persistconns = atomic_read(&dest->persistconns);
> +
>  			ip_vs_copy_stats(&entry.stats, &dest->stats);
> +
> +			atomic_inc(&dest->refcnt);
> +			read_unlock_bh(&__ip_vs_svc_lock);
>  			if (copy_to_user(&uptr->entrytable[count],
>  					 &entry, sizeof(entry))) {
>  				ret = -EFAULT;
>  				break;
>  			}
>  			count++;
> +
> +			read_lock_bh(&__ip_vs_svc_lock);
> +			if (atomic_dec_and_test(&dest->refcnt)) 
> +				kfree(dest);
> +
>  		}
>  		ip_vs_service_put(svc);
> +		read_unlock_bh(&__ip_vs_svc_lock);
>  	} else
>  		ret = -ESRCH;
>  	return ret;
> @@ -2043,7 +2018,7 @@
>  };
>  
>  static int
> -do_ip_vs_get_ctl(struct sock *sk, int cmd, void *user, int *len)
> +do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len)
>  {
>  	unsigned char arg[128];
>  	int ret = 0;
> 


[-- Attachment #2: Type: TEXT/PLAIN, Size: 1282 bytes --]

diff -Nru a/net/ipv4/ipvs/ip_vs_ctl.c b/net/ipv4/ipvs/ip_vs_ctl.c
--- a/net/ipv4/ipvs/ip_vs_ctl.c Thu Sep 18 00:43:00 2003
+++ b/net/ipv4/ipvs/ip_vs_ctl.c Thu Sep 18 00:43:00 2003
@@ -46,7 +46,7 @@
 static DECLARE_MUTEX(__ip_vs_mutex);
 
 /* lock for service table */
-rwlock_t __ip_vs_svc_lock = RW_LOCK_UNLOCKED;
+static rwlock_t __ip_vs_svc_lock = RW_LOCK_UNLOCKED;
 
 /* lock for table with the real services */
 static rwlock_t __ip_vs_rs_lock = RW_LOCK_UNLOCKED;
@@ -1912,7 +1912,7 @@
 
 static inline int
 __ip_vs_get_service_entries(const struct ip_vs_get_services *get,
-                   struct ip_vs_get_services *uptr)
+                   struct ip_vs_get_services __user *uptr)
 {
      int idx, count=0;
      struct ip_vs_service *svc;
@@ -1955,7 +1955,7 @@
 
 static inline int
 __ip_vs_get_dest_entries(const struct ip_vs_get_dests *get,
-                struct ip_vs_get_dests *uptr)
+                struct ip_vs_get_dests __user *uptr)
 {
      struct ip_vs_service *svc;
      int ret = 0;
@@ -2034,7 +2034,7 @@
 };
 
 static int
-do_ip_vs_get_ctl(struct sock *sk, int cmd, void *user, int *len)
+do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len)
 {
      unsigned char arg[128];
      int ret = 0;

Re: [PATCH] (4/6) ipvs -- use reader locks and refcounts correctly.

[David S. Miller]

On Thu, 18 Sep 2003 00:44:16 +0800 (CST)
Wensong Zhang <wensong@linux-vs.org> wrote:

> So, maybe we just apply the simple patch with making the lock local and 
> tagging user pointers.

Can someone put such a patch together for me?

Thanks.

Re: [PATCH] (4/6) ipvs -- use reader locks and refcounts correctly.

[Wensong Zhang]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 347 bytes --]



On Sat, 20 Sep 2003, David S. Miller wrote:

> On Thu, 18 Sep 2003 00:44:16 +0800 (CST)
> Wensong Zhang <wensong@linux-vs.org> wrote:
> 
> > So, maybe we just apply the simple patch with making the lock local and 
> > tagging user pointers.
> 
> Can someone put such a patch together for me?
> 

Please see the attached patch.

Thanks,

Wensong

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1831 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1315  -> 1.1316 
#	net/ipv4/ipvs/ip_vs_ctl.c	1.4     -> 1.5    
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/09/18	shemminger@osdl.org	1.1316
# [IPV4] IPVS: some type declaration tidy up
# 
# - make the __ip_vs_svc_lock local
# - tag user pointers __user
# --------------------------------------------
#
diff -Nru a/net/ipv4/ipvs/ip_vs_ctl.c b/net/ipv4/ipvs/ip_vs_ctl.c
--- a/net/ipv4/ipvs/ip_vs_ctl.c	Sat Sep 20 22:55:39 2003
+++ b/net/ipv4/ipvs/ip_vs_ctl.c	Sat Sep 20 22:55:39 2003
@@ -46,7 +46,7 @@
 static DECLARE_MUTEX(__ip_vs_mutex);
 
 /* lock for service table */
-rwlock_t __ip_vs_svc_lock = RW_LOCK_UNLOCKED;
+static rwlock_t __ip_vs_svc_lock = RW_LOCK_UNLOCKED;
 
 /* lock for table with the real services */
 static rwlock_t __ip_vs_rs_lock = RW_LOCK_UNLOCKED;
@@ -1912,7 +1912,7 @@
 
 static inline int
 __ip_vs_get_service_entries(const struct ip_vs_get_services *get,
-			    struct ip_vs_get_services *uptr)
+			    struct ip_vs_get_services __user *uptr)
 {
 	int idx, count=0;
 	struct ip_vs_service *svc;
@@ -1955,7 +1955,7 @@
 
 static inline int
 __ip_vs_get_dest_entries(const struct ip_vs_get_dests *get,
-			 struct ip_vs_get_dests *uptr)
+			 struct ip_vs_get_dests __user *uptr)
 {
 	struct ip_vs_service *svc;
 	int ret = 0;
@@ -2034,7 +2034,7 @@
 };
 
 static int
-do_ip_vs_get_ctl(struct sock *sk, int cmd, void *user, int *len)
+do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len)
 {
 	unsigned char arg[128];
 	int ret = 0;

Re: [PATCH] (4/6) ipvs -- use reader locks and refcounts correctly.

[David S. Miller]

On Sat, 20 Sep 2003 23:39:10 +0800 (CST)
Wensong Zhang <wensong@linux-vs.org> wrote:

> On Sat, 20 Sep 2003, David S. Miller wrote:
> 
> > On Thu, 18 Sep 2003 00:44:16 +0800 (CST)
> > Wensong Zhang <wensong@linux-vs.org> wrote:
> > 
> > > So, maybe we just apply the simple patch with making the lock local and 
> > > tagging user pointers.
> > 
> > Can someone put such a patch together for me?
> > 
> 
> Please see the attached patch.

Applied, thanks Wensong.