ipvs: prevent some underflows

PATCH In review Dan Carpenter 3 participants 7 messages Jun 5, 2015 – Jun 16, 2015

Subsystems (4)

Networking Kernel janitors netfilter IPVS

lore

Share & tools

lore

Dan Carpenter

author

2015-06-05 09:33 UTC #0 lore raw

PATCH 1/1 ipvs: prevent some underflows

Quite a few drivers allow very low settings for dev->mtu. My static checker complains this could cause some underflow problems when we do the subtractions in set_sync_mesg_maxlen().

I don't know that it's harmful necessarily, but it seems like an easy thing to prevent the underflows.

Signed-off-by Dan Carpenter <[email protected]>

---
Please review this one carefully, because I'm not very sure of myself here.

net/netfilter/ipvs/ip_vs_sync.c +8 -4

index b08ba95..b4e148b 100644
@@ -1352,7 +1352,7 @@ static int set_sync_mesg_maxlen(struct net *net, int sync_state)
 {
 	struct netns_ipvs *ipvs = net_ipvs(net);
 	struct net_device *dev;
-	int num;
+	unsigned int num;
 
 	if (sync_state = IP_VS_STATE_MASTER) {
 		dev = __dev_get_by_name(net, ipvs->master_mcast_ifn);

@@ -1363,7 +1363,8 @@ static int set_sync_mesg_maxlen(struct net *net, int sync_state)
 		       sizeof(struct udphdr) -
 		       SYNC_MESG_HEADER_LEN - 20) / SIMPLE_CONN_SIZE;
 		ipvs->send_mesg_maxlen = SYNC_MESG_HEADER_LEN +
-			SIMPLE_CONN_SIZE * min(num, MAX_CONNS_PER_SYNCBUFF);
+			SIMPLE_CONN_SIZE * min_t(uint, num,
+						 MAX_CONNS_PER_SYNCBUFF);
 		IP_VS_DBG(7, "setting the maximum length of sync sending "
 			  "message %d.\n", ipvs->send_mesg_maxlen);
 	} else if (sync_state = IP_VS_STATE_BACKUP) {

@@ -1371,8 +1372,11 @@ static int set_sync_mesg_maxlen(struct net *net, int sync_state)
 		if (!dev)
 			return -ENODEV;
 
-		ipvs->recv_mesg_maxlen = dev->mtu -
-			sizeof(struct iphdr) - sizeof(struct udphdr);
+		if (dev->mtu < sizeof(struct iphdr) + sizeof(struct udphdr))
+			ipvs->recv_mesg_maxlen = 0;
+		else
+			ipvs->recv_mesg_maxlen = dev->mtu -
+				sizeof(struct iphdr) - sizeof(struct udphdr);
 		IP_VS_DBG(7, "setting the maximum length of sync receiving "
 			  "message %d.\n", ipvs->recv_mesg_maxlen);
 	}

Julian Anastasov

maintainer

2015-06-08 19:16 UTC #1 lore raw

Hello,

Review static int set_sync_mesg_maxlen(struct net *net, int sync_state)

Quoted 1 line · Dan Carpenter

(quoted diff) +0 -0

@@ -1363,7 +1363,8 @@ static int set_sync_mesg_maxlen(struct net *net, int sync_state)

	May be we should use min(dev->mtu, 1500) instead of

dev->mtu to avoid sending too large packets for the common case.

net/netfilter/ipvs/ip_vs_sync.c static int set_sync_mesg_maxlen(struct net *net, int sync_state) · line 1367

Quoted 6 lines

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

 		       sizeof(struct udphdr) -
 		       SYNC_MESG_HEADER_LEN - 20) / SIMPLE_CONN_SIZE;
 		ipvs->send_mesg_maxlen = SYNC_MESG_HEADER_LEN +
-			SIMPLE_CONN_SIZE * min(num, MAX_CONNS_PER_SYNCBUFF);
+			SIMPLE_CONN_SIZE * min_t(uint, num,
+						 MAX_CONNS_PER_SYNCBUFF);

	ipvs->send_mesg_maxlen = max(min(dev->mtu, 1500) -
		sizeof(struct iphdr) - sizeof(struct udphdr),
		/* Some new const is needed here: */
		2 * FULL_CONN_SIZE);

	And may be we should add more correct checks in

ip_vs_sync_conn() in case dev->mtu was changed after thread start. Currently, it is assumed that fresh new buffer from ip_vs_sync_buff_create() will have space for at least one message... We can even set inet->pmtudisc to IP_PMTUDISC_DONT so that packets can be fragmented for too small MTU.

Quoted 4 lines

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

 		IP_VS_DBG(7, "setting the maximum length of sync sending "
 			  "message %d.\n", ipvs->send_mesg_maxlen);
 	} else if (sync_state = IP_VS_STATE_BACKUP) {

net/netfilter/ipvs/ip_vs_sync.c static int set_sync_mesg_maxlen(struct net *net, int sync_state) · line 1379

Quoted 10 lines

net/netfilter/ipvs/ip_vs_sync.c +5 -2

@@ -1371,8 +1372,11 @@ static int set_sync_mesg_maxlen(struct net *net, int sync_state)
 		if (!dev)
 			return -ENODEV;
 
-		ipvs->recv_mesg_maxlen = dev->mtu -
-			sizeof(struct iphdr) - sizeof(struct udphdr);
+		if (dev->mtu < sizeof(struct iphdr) + sizeof(struct udphdr))
+			ipvs->recv_mesg_maxlen = 0;
+		else
+			ipvs->recv_mesg_maxlen = dev->mtu -
+				sizeof(struct iphdr) - sizeof(struct udphdr);

	May be ipvs->recv_mesg_maxlen = max(dev->mtu, 1500);

This is single buffer allocated when backup starts...

net/netfilter/ipvs/ip_vs_sync.c static int set_sync_mesg_maxlen(struct net *net, int sync_state) · line 1382

Quoted 4 lines

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

 		IP_VS_DBG(7, "setting the maximum length of sync receiving "
 			  "message %d.\n", ipvs->recv_mesg_maxlen);
 	}

Regards

--
Julian Anastasov <[email protected]>

Marcelo Ricardo Leitner

2015-06-09 12:26 UTC #2 lore raw

Hi,

Quoted 10 lines · Julian Anastasov

Hello,
Review static int set_sync_mesg_maxlen(struct net *net, int sync_state)
Quoted 1 line · Dan Carpenter
(quoted diff) +0 -0
@@ -1363,7 +1363,8 @@ static int set_sync_mesg_maxlen(struct net *net, int sync_state)
	May be we should use min(dev->mtu, 1500) instead of
dev->mtu to avoid sending too large packets for the common case.

That will put an upper limit on it, no? If the load balancers share a big MTU, why not use it?

Thanks,
Marcelo

Quoted 51 lines

net/netfilter/ipvs/ip_vs_sync.c static int set_sync_mesg_maxlen(struct net *net, int sync_state) · line 1367
Quoted 6 lines
net/netfilter/ipvs/ip_vs_sync.c +2 -1
 		       sizeof(struct udphdr) -
 		       SYNC_MESG_HEADER_LEN - 20) / SIMPLE_CONN_SIZE;
 		ipvs->send_mesg_maxlen = SYNC_MESG_HEADER_LEN +
-			SIMPLE_CONN_SIZE * min(num, MAX_CONNS_PER_SYNCBUFF);
+			SIMPLE_CONN_SIZE * min_t(uint, num,
+						 MAX_CONNS_PER_SYNCBUFF);
	ipvs->send_mesg_maxlen = max(min(dev->mtu, 1500) -
		sizeof(struct iphdr) - sizeof(struct udphdr),
		/* Some new const is needed here: */
		2 * FULL_CONN_SIZE);

	And may be we should add more correct checks in
ip_vs_sync_conn() in case dev->mtu was changed after thread start. Currently, it is assumed that fresh new buffer from ip_vs_sync_buff_create() will have space for at least one message... We can even set inet->pmtudisc to IP_PMTUDISC_DONT so that packets can be fragmented for too small MTU.
Quoted 4 lines
net/netfilter/ipvs/ip_vs_sync.c +0 -0
 		IP_VS_DBG(7, "setting the maximum length of sync sending "
 			  "message %d.\n", ipvs->send_mesg_maxlen);
 	} else if (sync_state = IP_VS_STATE_BACKUP) {
net/netfilter/ipvs/ip_vs_sync.c static int set_sync_mesg_maxlen(struct net *net, int sync_state) · line 1379
Quoted 10 lines
net/netfilter/ipvs/ip_vs_sync.c +5 -2
@@ -1371,8 +1372,11 @@ static int set_sync_mesg_maxlen(struct net *net, int sync_state)
 		if (!dev)
 			return -ENODEV;
 
-		ipvs->recv_mesg_maxlen = dev->mtu -
-			sizeof(struct iphdr) - sizeof(struct udphdr);
+		if (dev->mtu < sizeof(struct iphdr) + sizeof(struct udphdr))
+			ipvs->recv_mesg_maxlen = 0;
+		else
+			ipvs->recv_mesg_maxlen = dev->mtu -
+				sizeof(struct iphdr) - sizeof(struct udphdr);
	May be ipvs->recv_mesg_maxlen = max(dev->mtu, 1500);
This is single buffer allocated when backup starts...
net/netfilter/ipvs/ip_vs_sync.c static int set_sync_mesg_maxlen(struct net *net, int sync_state) · line 1382
Quoted 4 lines
net/netfilter/ipvs/ip_vs_sync.c +0 -0
 		IP_VS_DBG(7, "setting the maximum length of sync receiving "
 			  "message %d.\n", ipvs->recv_mesg_maxlen);
 	}
Regards
--
Julian Anastasov <[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe lvs-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Julian Anastasov

maintainer

2015-06-11 05:18 UTC #3 lore raw

Hello,

Quoted 8 lines · Marcelo Ricardo Leitner

Quoted 4 lines · Julian Anastasov
	May be we should use min(dev->mtu, 1500) instead of
dev->mtu to avoid sending too large packets for the common case.
That will put an upper limit on it, no? If the load balancers share a big MTU, why not use it?

Yes, without any configuration (eg. sysctl value),
such defaults should reduce the chance for incompatibilities. I.e. we should not send more than 1500 and should be able to receive at least 1500.

Regards

--
Julian Anastasov <[email protected]>

Marcelo Ricardo Leitner

2015-06-11 12:57 UTC #4 lore raw

Hi,

Quoted 11 lines · Julian Anastasov

Hello,
Quoted 6 lines · Marcelo Ricardo Leitner
Quoted 4 lines · Julian Anastasov
	May be we should use min(dev->mtu, 1500) instead of
dev->mtu to avoid sending too large packets for the common case.

Quoted 7 lines

Quoted 2 lines
That will put an upper limit on it, no? If the load balancers share a big MTU, why not use it?
Yes, without any configuration (eg. sysctl value),
such defaults should reduce the chance for incompatibilities. I.e. we should not send more than 1500 and should be able to receive at least 1500.

I'm not sure we should anticipate bad networking setups like that. Probably ipvs wouldn't be the only one to suffer from such bad config.

Anyway, then we should include that sysctl together with that min(), because otherwise it will be impossible to send packets larger than 1500 and this may be a regression to some users. Or maybe you don't think that large packets are worth for the sync at all?

Thanks,
Marcelo

Dan Carpenter

author

2015-06-11 13:17 UTC #5 lore raw

Is there any way I can bail out of this patch and someone else do it? I'm a complete newbie here doing static checker fixes and this stuff is too complicated for me to have an opinion about.

regards,
dan carpenter

Julian Anastasov

maintainer

2015-06-16 19:28 UTC #6 lore raw

In reply to Marcelo Ricardo Leitner

Hello,

Quoted 7 lines · Marcelo Ricardo Leitner

Quoted 6 lines
Quoted 1 line · Julian Anastasov
Yes, without any configuration (eg. sysctl value),
such defaults should reduce the chance for incompatibilities. I.e. we should not send more than 1500 and should be able to receive at least 1500.

Quoted 7 lines

I'm not sure we should anticipate bad networking setups like that. Probably ipvs wouldn't be the only one to suffer from such bad config.
Anyway, then we should include that sysctl together with that min(), because otherwise it will be impossible to send packets larger than 1500 and this may be a regression to some users. Or maybe you don't think that large packets are worth for the sync at all?

Sorry for the delay. I'll try that in the
following days. There should be benefits from large packets, so better to allow it.

Regards

--
Julian Anastasov <[email protected]>