[patch] ipvs: prevent some underflows

[patch] ipvs: prevent some underflows

[Dan Carpenter]

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 <dan.carpenter@oracle.com>
---
Please review this one carefully, because I'm not very sure of myself
here.

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index b08ba95..b4e148b 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -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);
 	}

Re: [patch] ipvs: prevent some underflows

[Julian Anastasov]

	Hello,

On Fri, 5 Jun 2015, Dan Carpenter wrote:

> @@ -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.

>  		       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.

>  		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);

	May be ipvs->recv_mesg_maxlen = max(dev->mtu, 1500);
This is single buffer allocated when backup starts...

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

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [patch] ipvs: prevent some underflows

[Marcelo Ricardo Leitner]

Hi,

On Mon, Jun 08, 2015 at 10:16:23PM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Fri, 5 Jun 2015, Dan Carpenter wrote:
> 
> > @@ -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

> >  		       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.
> 
> >  		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);
> 
> 	May be ipvs->recv_mesg_maxlen = max(dev->mtu, 1500);
> This is single buffer allocated when backup starts...
> 
> >  		IP_VS_DBG(7, "setting the maximum length of sync receiving "
> >  			  "message %d.\n", ipvs->recv_mesg_maxlen);
> >  	}
> > 
> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>
> --
> To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch] ipvs: prevent some underflows

[Julian Anastasov]

	Hello,

On Tue, 9 Jun 2015, Marcelo Ricardo Leitner wrote:

> On Mon, Jun 08, 2015 at 10:16:23PM +0300, Julian Anastasov wrote:
> > 
> > 	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 <ja@ssi.bg>

Re: [patch] ipvs: prevent some underflows

[Marcelo Ricardo Leitner]

Hi,

On Thu, Jun 11, 2015 at 08:18:06AM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Tue, 9 Jun 2015, Marcelo Ricardo Leitner wrote:
> 
> > On Mon, Jun 08, 2015 at 10:16:23PM +0300, Julian Anastasov wrote:
> > > 
> > > 	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.

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

Re: [patch] ipvs: prevent some underflows

[Dan Carpenter]

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

Re: [patch] ipvs: prevent some underflows

[Julian Anastasov]

	Hello,

On Thu, 11 Jun 2015, Marcelo Ricardo Leitner wrote:

> > > On Mon, Jun 08, 2015 at 10:16:23PM +0300, Julian Anastasov wrote:
> > 
> > 	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?

	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 <ja@ssi.bg>