ipvs: Use atomic operations atomicly

PATCH Applied Simon Horman 2 participants 6 messages Aug 28, 2009 – Sep 19, 2009

Subsystems (5)

Networking Network drivers netfilter IPv4/IPv6 +1 more subsystems

lore

Share & tools

lore

Simon Horman

author maintainer

2009-08-28 02:37 UTC #0 lore raw

PATCH 1/1 ipvs: Use atomic operations atomicly

A pointed out by Shin Hong, IPVS doesn't always use atomic operations in an atomic manner. While this seems unlikely to be manifest in strange behaviour, it seems appropriate to clean this up.

Cc 홍신 shin hong <[email protected]>

Signed-off-by Simon Horman <[email protected]>

net/netfilter/ipvs/ip_vs_core.c +3 -3

Index: net-next-2.6/net/netfilter/ipvs/ip_vs_core.c
===================================================================
@@ -1259,7 +1259,7 @@ ip_vs_in(unsigned int hooknum, struct sk
 	struct ip_vs_iphdr iph;
 	struct ip_vs_protocol *pp;
 	struct ip_vs_conn *cp;
-	int ret, restart, af;
+	int ret, restart, af, pkts;
 
 	af = (skb->protocol == htons(ETH_P_IP)) ? AF_INET : AF_INET6;

@@ -1346,12 +1346,12 @@ ip_vs_in(unsigned int hooknum, struct sk
 	 * Sync connection if it is about to close to
 	 * encorage the standby servers to update the connections timeout
 	 */
-	atomic_inc(&cp->in_pkts);
+	pkts = atomic_add_return(1, &cp->in_pkts);
 	if (af == AF_INET &&
 	    (ip_vs_sync_state & IP_VS_STATE_MASTER) &&
 	    (((cp->protocol != IPPROTO_TCP ||
 	       cp->state == IP_VS_TCP_S_ESTABLISHED) &&
-	      (atomic_read(&cp->in_pkts) % sysctl_ip_vs_sync_threshold[1]
+	      (pkts % sysctl_ip_vs_sync_threshold[1]
 	       == sysctl_ip_vs_sync_threshold[0])) ||
 	     ((cp->protocol == IPPROTO_TCP) && (cp->old_state != cp->state) &&
 	      ((cp->state == IP_VS_TCP_S_FIN_WAIT) ||

net/netfilter/ipvs/ip_vs_wrr.c +4 -3

Index: net-next-2.6/net/netfilter/ipvs/ip_vs_wrr.c
===================================================================
@@ -77,11 +77,12 @@ static int ip_vs_wrr_gcd_weight(struct i
 static int ip_vs_wrr_max_weight(struct ip_vs_service *svc)
 {
 	struct ip_vs_dest *dest;
-	int weight = 0;
+	int new_weight, weight = 0;
 
 	list_for_each_entry(dest, &svc->destinations, n_list) {
-		if (atomic_read(&dest->weight) > weight)
-			weight = atomic_read(&dest->weight);
+		new_weight = atomic_read(&dest->weight);
+		if (new_weight > weight)
+			weight = new_weight;
 	}
 
 	return weight;

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Patrick McHardy

Applied

2009-08-31 12:22 UTC #1 lore raw

Quoted 6 lines · Simon Horman

A pointed out by Shin Hong, IPVS doesn't always use atomic operations in an atomic manner. While this seems unlikely to be manifest in strange behaviour, it seems appropriate to clean this up.
Cc 홍신 shin hong <[email protected]>
Signed-off-by Simon Horman <[email protected]>

Applied, thanks.

net/netfilter/ipvs/ip_vs_core.c ip_vs_in(unsigned int hooknum, struct sk · line 1354

Quoted 6 lines

net/netfilter/ipvs/ip_vs_core.c +1 -1

 	if (af == AF_INET &&
 	    (ip_vs_sync_state & IP_VS_STATE_MASTER) &&
 	    (((cp->protocol != IPPROTO_TCP ||
 	       cp->state == IP_VS_TCP_S_ESTABLISHED) &&
-	      (atomic_read(&cp->in_pkts) % sysctl_ip_vs_sync_threshold[1]
+	      (pkts % sysctl_ip_vs_sync_threshold[1]

It seems that proc_do_sync_threshold() should check whether this value is zero. The current checks also look racy since incorrect values are first updated, then overwritten again.

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Simon Horman

author maintainer

2009-08-31 14:49 UTC #2 lore raw

Quoted 10 lines · Patrick McHardy

Quoted 6 lines · Simon Horman
A pointed out by Shin Hong, IPVS doesn't always use atomic operations in an atomic manner. While this seems unlikely to be manifest in strange behaviour, it seems appropriate to clean this up.
Cc 홍신 shin hong <[email protected]>
Signed-off-by Simon Horman <[email protected]>
Applied, thanks.

Quoted 10 lines

net/netfilter/ipvs/ip_vs_core.c ip_vs_in(unsigned int hooknum, struct sk · line 1354
Quoted 6 lines
net/netfilter/ipvs/ip_vs_core.c +1 -1
 	if (af == AF_INET &&
 	    (ip_vs_sync_state & IP_VS_STATE_MASTER) &&
 	    (((cp->protocol != IPPROTO_TCP ||
 	       cp->state == IP_VS_TCP_S_ESTABLISHED) &&
-	      (atomic_read(&cp->in_pkts) % sysctl_ip_vs_sync_threshold[1]
+	      (pkts % sysctl_ip_vs_sync_threshold[1]
It seems that proc_do_sync_threshold() should check whether this value is zero. The current checks also look racy since incorrect values are first updated, then overwritten again.

Thanks, I'll look into that.

Simon Horman

author maintainer

2009-09-01 01:59 UTC #3 lore raw

In reply to Patrick McHardy

Quoted 10 lines · Patrick McHardy

Quoted 6 lines · Simon Horman
A pointed out by Shin Hong, IPVS doesn't always use atomic operations in an atomic manner. While this seems unlikely to be manifest in strange behaviour, it seems appropriate to clean this up.
Cc 홍신 shin hong <[email protected]>
Signed-off-by Simon Horman <[email protected]>
Applied, thanks.

Quoted 10 lines

Review
Quoted 6 lines
(quoted diff) +1 -1
 	if (af == AF_INET &&
 	    (ip_vs_sync_state & IP_VS_STATE_MASTER) &&
 	    (((cp->protocol != IPPROTO_TCP ||
 	       cp->state == IP_VS_TCP_S_ESTABLISHED) &&
-	      (atomic_read(&cp->in_pkts) % sysctl_ip_vs_sync_threshold[1]
+	      (pkts % sysctl_ip_vs_sync_threshold[1]
It seems that proc_do_sync_threshold() should check whether this value is zero. The current checks also look racy since incorrect values are first updated, then overwritten again.

Hi,

I'm wondering if an approach along the lines of the following is valid. The idea is that the value in the ctl_table is essentially a scratch value that is used by the parser and then copied into ip_vs_sync_threshold if it is valid. I'm concerned that there are atomicity issues surrounding writing ip_vs_sync_threshold while there might be readers.

include/net/ip_vs.h +1 -1

index 98978e7..28d0c4f 100644
@@ -774,8 +774,8 @@ extern int ip_vs_leave(struct ip_vs_service *svc, struct sk_buff *skb,
 extern int sysctl_ip_vs_cache_bypass;
 extern int sysctl_ip_vs_expire_nodest_conn;
 extern int sysctl_ip_vs_expire_quiescent_template;
-extern int sysctl_ip_vs_sync_threshold[2];
 extern int sysctl_ip_vs_nat_icmp_send;
+extern int ip_vs_sync_threshold[2];
 extern struct ip_vs_stats ip_vs_stats;
 extern const struct ctl_path net_vs_ctl_path[];

net/netfilter/ipvs/ip_vs_core.c +1 -2

index b95699f..f3572b6 100644
@@ -1362,8 +1362,7 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb,
 	    (ip_vs_sync_state & IP_VS_STATE_MASTER) &&
 	    (((cp->protocol != IPPROTO_TCP ||
 	       cp->state == IP_VS_TCP_S_ESTABLISHED) &&
-	      (pkts % sysctl_ip_vs_sync_threshold[1]
-	       == sysctl_ip_vs_sync_threshold[0])) ||
+	      (pkts % ip_vs_sync_threshold[1] == ip_vs_sync_threshold[0])) ||
 	     ((cp->protocol == IPPROTO_TCP) && (cp->old_state != cp->state) &&
 	      ((cp->state == IP_VS_TCP_S_FIN_WAIT) ||
 	       (cp->state == IP_VS_TCP_S_CLOSE_WAIT) ||

net/netfilter/ipvs/ip_vs_ctl.c +12 ~~-10~~

index fba2892..8a9ff21 100644
@@ -76,6 +76,11 @@ static atomic_t ip_vs_dropentry = ATOMIC_INIT(0);
 /* number of virtual services */
 static int ip_vs_num_services = 0;
 
+/* threshold handling */
+static int ip_vs_sync_threshold_min = 0;
+static int ip_vs_sync_threshold_max = INT_MAX;
+int ip_vs_sync_threshold[2] = { 3, 50 };
+
 /* sysctl variables */
 static int sysctl_ip_vs_drop_entry = 0;
 static int sysctl_ip_vs_drop_packet = 0;

@@ -85,7 +90,7 @@ static int sysctl_ip_vs_am_droprate = 10;
 int sysctl_ip_vs_cache_bypass = 0;
 int sysctl_ip_vs_expire_nodest_conn = 0;
 int sysctl_ip_vs_expire_quiescent_template = 0;
-int sysctl_ip_vs_sync_threshold[2] = { 3, 50 };
+static int sysctl_ip_vs_sync_threshold[2];
 int sysctl_ip_vs_nat_icmp_send = 0;

@@ -1521,17 +1526,12 @@ proc_do_sync_threshold(ctl_table *table, int write, struct file *filp,
 		       void __user *buffer, size_t *lenp, loff_t *ppos)
 {
 	int *valp = table->data;
-	int val[2];
 	int rc;
 
-	/* backup the value first */
-	memcpy(val, valp, sizeof(val));
-
-	rc = proc_dointvec(table, write, filp, buffer, lenp, ppos);
-	if (write && (valp[0] < 0 || valp[1] < 0 || valp[0] >= valp[1])) {
-		/* Restore the correct value */
-		memcpy(valp, val, sizeof(val));
-	}
+	rc = proc_dointvec_minmax(table, write, filp, buffer, lenp, ppos);
+	if (write && (valp[0] < valp[1]))
+		memcpy(ip_vs_sync_threshold, valp,
+		       sizeof(ip_vs_sync_threshold));
 	return rc;
 }

@@ -1698,6 +1698,8 @@ static struct ctl_table vs_vars[] = {
 		.maxlen		= sizeof(sysctl_ip_vs_sync_threshold),
 		.mode		= 0644,
 		.proc_handler	= proc_do_sync_threshold,
+		.extra1		= &ip_vs_sync_threshold_min,
+		.extra2		= &ip_vs_sync_threshold_max,
 	},
 	{
 		.procname	= "nat_icmp_send",

net/netfilter/ipvs/ip_vs_sync.c +1 -1

index e177f0d..d3322fb 100644
@@ -438,7 +438,7 @@ static void ip_vs_process_message(const char *buffer, const size_t buflen)
 
 		if (opt)
 			memcpy(&cp->in_seq, opt, sizeof(*opt));
-		atomic_set(&cp->in_pkts, sysctl_ip_vs_sync_threshold[0]);
+		atomic_set(&cp->in_pkts, ip_vs_sync_threshold[0]);
 		cp->state = state;
 		cp->old_state = cp->state;
 		/*

Patrick McHardy

2009-09-02 12:56 UTC #4 lore raw

Quoted 9 lines · Simon Horman

Quoted 3 lines · Patrick McHardy
It seems that proc_do_sync_threshold() should check whether this value is zero. The current checks also look racy since incorrect values are first updated, then overwritten again.
I'm wondering if an approach along the lines of the following is valid. The idea is that the value in the ctl_table is essentially a scratch value that is used by the parser and then copied into ip_vs_sync_threshold if it is valid.

Even simpler would be to use a temporary buffer on the stack for copying the values from userspace and then copy them to the final buffer after validation.

Quoted 2 lines

I'm concerned that there are atomicity issues
surrounding writing ip_vs_sync_threshold while there might be readers.

That might be a problem if they are required to be "synchronized".

Quoted 17 lines

net/netfilter/ipvs/ip_vs_core.c +1 -2

@@ -1362,8 +1362,7 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb,
 	    (ip_vs_sync_state & IP_VS_STATE_MASTER) &&
 	    (((cp->protocol != IPPROTO_TCP ||
 	       cp->state == IP_VS_TCP_S_ESTABLISHED) &&
-	      (pkts % sysctl_ip_vs_sync_threshold[1]
-	       == sysctl_ip_vs_sync_threshold[0])) ||
+	      (pkts % ip_vs_sync_threshold[1] == ip_vs_sync_threshold[0])) ||
 	     ((cp->protocol == IPPROTO_TCP) && (cp->old_state != cp->state) &&
 	      ((cp->state == IP_VS_TCP_S_FIN_WAIT) ||
 	       (cp->state == IP_VS_TCP_S_CLOSE_WAIT) ||

net/netfilter/ipvs/ip_vs_ctl.c +0 -0

index fba2892..8a9ff21 100644

net/netfilter/ipvs/ip_vs_ctl.c static atomic_t ip_vs_dropentry = ATOMIC_INIT(0); · line 83

Quoted 8 lines

net/netfilter/ipvs/ip_vs_ctl.c +5 -0

@@ -76,6 +76,11 @@ static atomic_t ip_vs_dropentry = ATOMIC_INIT(0);
 /* number of virtual services */
 static int ip_vs_num_services = 0;
 
+/* threshold handling */
+static int ip_vs_sync_threshold_min = 0;
+static int ip_vs_sync_threshold_max = INT_MAX;
+int ip_vs_sync_threshold[2] = { 3, 50 };
+

min should be 1 I guess or you still need to manually check that ip_vs_sync_threshold[1] != 0 to avoid a division be zero.

Simon Horman

author maintainer

2009-09-19 09:51 UTC #5 lore raw

[ re-post with CC list ]

Quoted 6 lines · Patrick McHardy

Quoted 5 lines · Simon Horman
Quoted 3 lines · Patrick McHardy
It seems that proc_do_sync_threshold() should check whether this value is zero. The current checks also look racy since incorrect values are first updated, then overwritten again.

Quoted 8 lines

Quoted 4 lines
I'm wondering if an approach along the lines of the following is valid. The idea is that the value in the ctl_table is essentially a scratch value that is used by the parser and then copied into ip_vs_sync_threshold if it is valid.
Even simpler would be to use a temporary buffer on the stack for copying the values from userspace and then copy them to the final buffer after validation.

Do you mean something like what I have done using read_table in the new patch below?

Quoted 4 lines

Quoted 2 lines
I'm concerned that there are atomicity issues
surrounding writing ip_vs_sync_threshold while there might be readers.
That might be a problem if they are required to be "synchronized".

I don't think that the sychronisation needs extend beyond their use in this snippet.

Quoted 21 lines

net/netfilter/ipvs/ip_vs_core.c +1 -2

@@ -1362,8 +1362,7 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb,
 	    (ip_vs_sync_state & IP_VS_STATE_MASTER) &&
 	    (((cp->protocol != IPPROTO_TCP ||
 	       cp->state == IP_VS_TCP_S_ESTABLISHED) &&
-	      (pkts % sysctl_ip_vs_sync_threshold[1]
-	       == sysctl_ip_vs_sync_threshold[0])) ||
+	      (pkts % ip_vs_sync_threshold[1] == ip_vs_sync_threshold[0])) ||
 	     ((cp->protocol == IPPROTO_TCP) && (cp->old_state != cp->state) &&
 	      ((cp->state == IP_VS_TCP_S_FIN_WAIT) ||
 	       (cp->state == IP_VS_TCP_S_CLOSE_WAIT) ||

net/netfilter/ipvs/ip_vs_ctl.c +0 -0

index fba2892..8a9ff21 100644
@@ -76,6 +76,11 @@ static atomic_t ip_vs_dropentry = ATOMIC_INIT(0);
 /* number of virtual services */
 static int ip_vs_num_services = 0;

Quoted 8 lines

net/netfilter/ipvs/ip_vs_ctl.c static atomic_t ip_vs_dropentry = ATOMIC_INIT(0); · line 83
Quoted 5 lines
net/netfilter/ipvs/ip_vs_ctl.c +5 -0
+/* threshold handling */
+static int ip_vs_sync_threshold_min = 0;
+static int ip_vs_sync_threshold_max = INT_MAX;
+int ip_vs_sync_threshold[2] = { 3, 50 };
+
min should be 1 I guess or you still need to manually check that ip_vs_sync_threshold[1] != 0 to avoid a division be zero.

I think that the check that val[0] < val[1] ensures this.

Something that I noticed while putting this new patch together, should the code be checking the value of rc and only assigning values if its 0. If so, I probably needs to be changed in a few places.

net/netfilter/ipvs/ip_vs_ctl.c +10 -6

index fba2892..c33ef7d 100644
@@ -76,6 +76,9 @@ static atomic_t ip_vs_dropentry = ATOMIC_INIT(0);
 /* number of virtual services */
 static int ip_vs_num_services = 0;
 
+static int ip_vs_sync_threshold_min = 0;
+static int ip_vs_sync_threshold_max = INT_MAX;
+
 /* sysctl variables */
 static int sysctl_ip_vs_drop_entry = 0;
 static int sysctl_ip_vs_drop_packet = 0;

@@ -1520,18 +1523,17 @@ static int
 proc_do_sync_threshold(ctl_table *table, int write, struct file *filp,
 		       void __user *buffer, size_t *lenp, loff_t *ppos)
 {
+	struct ctl_table read_table;
 	int *valp = table->data;
 	int val[2];
 	int rc;
 
-	/* backup the value first */
-	memcpy(val, valp, sizeof(val));
+	memcpy(&read_table, table, sizeof(read_table));
+	read_table.data = &val;
 
-	rc = proc_dointvec(table, write, filp, buffer, lenp, ppos);
-	if (write && (valp[0] < 0 || valp[1] < 0 || valp[0] >= valp[1])) {
-		/* Restore the correct value */
+	rc = proc_dointvec_minmax(&read_table, write, filp, buffer, lenp, ppos);
+	if (write && (val[0] >= val[1]))
 		memcpy(valp, val, sizeof(val));
-	}
 	return rc;
 }

@@ -1698,6 +1700,8 @@ static struct ctl_table vs_vars[] = {
 		.maxlen		= sizeof(sysctl_ip_vs_sync_threshold),
 		.mode		= 0644,
 		.proc_handler	= proc_do_sync_threshold,
+		.extra1		= &ip_vs_sync_threshold_min,
+		.extra2		= &ip_vs_sync_threshold_max,
 	},
 	{
 		.procname	= "nat_icmp_send",