| Message ID | 20150605093315.GD24871@mwanda |
|---|---|
| State | RFC |
| Delegated to: | Pablo Neira |
| Headers | show |
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> -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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> -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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> -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html