[PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Cast to s64 some variables and a macro in order to give the
compiler complete information about the proper arithmetic to
use. Notice that these elements are used in contexts that
expect expressions of type s64 (64 bits, signed).

Currently such expression are being evaluated using 32-bit
arithmetic.

Addresses-Coverity-ID: 200687
Addresses-Coverity-ID: 200688
Addresses-Coverity-ID: 200689
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 net/ipv4/tcp_lp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/tcp_lp.c b/net/ipv4/tcp_lp.c
index ae10ed6..4999111 100644
--- a/net/ipv4/tcp_lp.c
+++ b/net/ipv4/tcp_lp.c
@@ -134,7 +134,7 @@ static u32 tcp_lp_remote_hz_estimator(struct sock *sk)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct lp *lp = inet_csk_ca(sk);
-	s64 rhz = lp->remote_hz << 6;	/* remote HZ << 6 */
+	s64 rhz = (s64)lp->remote_hz << 6;	/* remote HZ << 6 */
 	s64 m = 0;
 
 	/* not yet record reference time
@@ -147,7 +147,7 @@ static u32 tcp_lp_remote_hz_estimator(struct sock *sk)
 	    tp->rx_opt.rcv_tsecr == lp->local_ref_time)
 		goto out;
 
-	m = TCP_TS_HZ *
+	m = (s64)TCP_TS_HZ *
 	    (tp->rx_opt.rcv_tsval - lp->remote_ref_time) /
 	    (tp->rx_opt.rcv_tsecr - lp->local_ref_time);
 	if (m < 0)
@@ -193,8 +193,8 @@ static u32 tcp_lp_owd_calculator(struct sock *sk)
 
 	if (lp->flag & LP_VALID_RHZ) {
 		owd =
-		    tp->rx_opt.rcv_tsval * (LP_RESOL / lp->remote_hz) -
-		    tp->rx_opt.rcv_tsecr * (LP_RESOL / TCP_TS_HZ);
+		    (s64)tp->rx_opt.rcv_tsval * (LP_RESOL / lp->remote_hz) -
+		    (s64)tp->rx_opt.rcv_tsecr * (LP_RESOL / TCP_TS_HZ);
 		if (owd < 0)
 			owd = -owd;
 	}
-- 
2.7.4

Re: [PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[Alan Cox]

On Wed, 31 Jan 2018 18:24:07 -0600
"Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:

> Cast to s64 some variables and a macro in order to give the
> compiler complete information about the proper arithmetic to
> use. Notice that these elements are used in contexts that
> expect expressions of type s64 (64 bits, signed).
> 
> Currently such expression are being evaluated using 32-bit
> arithmetic.

The question you need to ask is 'can it overflow 32bit maths', otherwise
you are potentially making the system do extra work for no reason.

Alan

Re: [PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Hi Alan,

Quoting Alan Cox <gnomes@lxorguk.ukuu.org.uk>:

> On Wed, 31 Jan 2018 18:24:07 -0600
> "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
>
>> Cast to s64 some variables and a macro in order to give the
>> compiler complete information about the proper arithmetic to
>> use. Notice that these elements are used in contexts that
>> expect expressions of type s64 (64 bits, signed).
>>
>> Currently such expression are being evaluated using 32-bit
>> arithmetic.
>
> The question you need to ask is 'can it overflow 32bit maths', otherwise
> you are potentially making the system do extra work for no reason.
>

Yeah, I get your point and it seems that in this particular case there  
is no risk of a 32bit overflow, but in general and IMHO as the code  
evolves, the use of incorrect arithmetic may have security  
implications in the future, so I advocate for code correctness in this  
case.

Thanks
--
Gustavo

Re: [PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[Andrew Lunn]

On Wed, Jan 31, 2018 at 07:07:49PM -0600, Gustavo A. R. Silva wrote:
> 
> Hi Alan,
> 
> Quoting Alan Cox <gnomes@lxorguk.ukuu.org.uk>:
> 
> >On Wed, 31 Jan 2018 18:24:07 -0600
> >"Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
> >
> >>Cast to s64 some variables and a macro in order to give the
> >>compiler complete information about the proper arithmetic to
> >>use. Notice that these elements are used in contexts that
> >>expect expressions of type s64 (64 bits, signed).
> >>
> >>Currently such expression are being evaluated using 32-bit
> >>arithmetic.
> >
> >The question you need to ask is 'can it overflow 32bit maths', otherwise
> >you are potentially making the system do extra work for no reason.
> >
> 
> Yeah, I get your point and it seems that in this particular case there is no
> risk of a 32bit overflow, but in general and IMHO as the code evolves, the
> use of incorrect arithmetic may have security implications in the future, so
> I advocate for code correctness in this case.

Hi Gustavo

Is this on the hotpath? How much overhead does it add to 32 bit
architectures which don't have 64 bit arithmetic in hardware? There
are a lot of embedded systems which are 32 bit.

    Andrew

RE: [PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[David Laight]

> > The question you need to ask is 'can it overflow 32bit maths', otherwise
> > you are potentially making the system do extra work for no reason.
> >
> 
> Yeah, I get your point and it seems that in this particular case there
> is no risk of a 32bit overflow, but in general and IMHO as the code
> evolves, the use of incorrect arithmetic may have security
> implications in the future, so I advocate for code correctness in this
> case.

Even if the variable are 64bit you still need to worry (maybe less)
about arithmetic overflow.
The only real way to avoid overflow is to understand the domain
of the values being used.

	David

Re: [PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[David Miller]

From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Date: Wed, 31 Jan 2018 18:24:07 -0600

> Cast to s64 some variables and a macro in order to give the
> compiler complete information about the proper arithmetic to
> use. Notice that these elements are used in contexts that
> expect expressions of type s64 (64 bits, signed).
> 
> Currently such expression are being evaluated using 32-bit
> arithmetic.
> 
> Addresses-Coverity-ID: 200687
> Addresses-Coverity-ID: 200688
> Addresses-Coverity-ID: 200689
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>

Sorry, I'm not applying this, the domain of the input values
means that the calculation cannot overflow.

Re: [PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Hi Andrew,

Quoting Andrew Lunn <andrew@lunn.ch>:

> On Wed, Jan 31, 2018 at 07:07:49PM -0600, Gustavo A. R. Silva wrote:
>>
>> Hi Alan,
>>
>> Quoting Alan Cox <gnomes@lxorguk.ukuu.org.uk>:
>>
>> >On Wed, 31 Jan 2018 18:24:07 -0600
>> >"Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
>> >
>> >>Cast to s64 some variables and a macro in order to give the
>> >>compiler complete information about the proper arithmetic to
>> >>use. Notice that these elements are used in contexts that
>> >>expect expressions of type s64 (64 bits, signed).
>> >>
>> >>Currently such expression are being evaluated using 32-bit
>> >>arithmetic.
>> >
>> >The question you need to ask is 'can it overflow 32bit maths', otherwise
>> >you are potentially making the system do extra work for no reason.
>> >
>>
>> Yeah, I get your point and it seems that in this particular case there is no
>> risk of a 32bit overflow, but in general and IMHO as the code evolves, the
>> use of incorrect arithmetic may have security implications in the future, so
>> I advocate for code correctness in this case.
>
> Hi Gustavo
>
> Is this on the hotpath? How much overhead does it add to 32 bit
> architectures which don't have 64 bit arithmetic in hardware? There
> are a lot of embedded systems which are 32 bit.
>

I'm sorry, I don't have access to 32-bit hardware at the moment.

Thanks
--
Gustavo

Re: [PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Hi David,

Quoting David Laight <David.Laight@aculab.com>:

>> > The question you need to ask is 'can it overflow 32bit maths', otherwise
>> > you are potentially making the system do extra work for no reason.
>> >
>>
>> Yeah, I get your point and it seems that in this particular case there
>> is no risk of a 32bit overflow, but in general and IMHO as the code
>> evolves, the use of incorrect arithmetic may have security
>> implications in the future, so I advocate for code correctness in this
>> case.
>
> Even if the variable are 64bit you still need to worry (maybe less)
> about arithmetic overflow.
> The only real way to avoid overflow is to understand the domain
> of the values being used.
>

Yep, that's correct.

Thanks for the feedback.
--
Gustavo

Re: [PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Hi David,

Quoting David Miller <davem@davemloft.net>:

> From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
> Date: Wed, 31 Jan 2018 18:24:07 -0600
>
>> Cast to s64 some variables and a macro in order to give the
>> compiler complete information about the proper arithmetic to
>> use. Notice that these elements are used in contexts that
>> expect expressions of type s64 (64 bits, signed).
>>
>> Currently such expression are being evaluated using 32-bit
>> arithmetic.
>>
>> Addresses-Coverity-ID: 200687
>> Addresses-Coverity-ID: 200688
>> Addresses-Coverity-ID: 200689
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>
> Sorry, I'm not applying this, the domain of the input values
> means that the calculation cannot overflow.

Yep, that's fine.

Thanks for the feedback.
--
Gustavo

Re: [PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]

Hi Gustavo,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]
[also build test ERROR on v4.15 next-20180202]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Gustavo-A-R-Silva/tcp_lp-use-64-bit-arithmetic-instead-of-32-bit/20180202-135349
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   WARNING: modpost: missing MODULE_LICENSE() in drivers/auxdisplay/img-ascii-lcd.o
   see include/linux/module.h for more information
   WARNING: modpost: missing MODULE_LICENSE() in drivers/iio/accel/kxsd9-i2c.o
   see include/linux/module.h for more information
   WARNING: modpost: missing MODULE_LICENSE() in drivers/iio/adc/qcom-vadc-common.o
   see include/linux/module.h for more information
   WARNING: modpost: missing MODULE_LICENSE() in drivers/media/platform/mtk-vcodec/mtk-vcodec-common.o
   see include/linux/module.h for more information
   WARNING: modpost: missing MODULE_LICENSE() in drivers/media/platform/soc_camera/soc_scale_crop.o
   see include/linux/module.h for more information
   WARNING: modpost: missing MODULE_LICENSE() in drivers/media/platform/tegra-cec/tegra_cec.o
   see include/linux/module.h for more information
   WARNING: modpost: missing MODULE_LICENSE() in drivers/pinctrl/pxa/pinctrl-pxa2xx.o
   see include/linux/module.h for more information
>> ERROR: "__divdi3" [net/ipv4/tcp_lp.ko] undefined!
>> ERROR: "__udivdi3" [net/ipv4/tcp_lp.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 62692 bytes --]

Re: [PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2556 bytes --]

Hi Gustavo,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]
[also build test ERROR on v4.15 next-20180202]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Gustavo-A-R-Silva/tcp_lp-use-64-bit-arithmetic-instead-of-32-bit/20180202-135349
config: i386-randconfig-i0-201804 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   net/ipv4/tcp_lp.o: In function `tcp_lp_remote_hz_estimator':
>> net/ipv4/tcp_lp.c:150: undefined reference to `__divdi3'

vim +150 net/ipv4/tcp_lp.c

   125	
   126	/**
   127	 * tcp_lp_remote_hz_estimator
   128	 *
   129	 * Estimate remote HZ.
   130	 * We keep on updating the estimated value, where original TCP-LP
   131	 * implementation only guest it for once and use forever.
   132	 */
   133	static u32 tcp_lp_remote_hz_estimator(struct sock *sk)
   134	{
   135		struct tcp_sock *tp = tcp_sk(sk);
   136		struct lp *lp = inet_csk_ca(sk);
   137		s64 rhz = (s64)lp->remote_hz << 6;	/* remote HZ << 6 */
   138		s64 m = 0;
   139	
   140		/* not yet record reference time
   141		 * go away!! record it before come back!! */
   142		if (lp->remote_ref_time == 0 || lp->local_ref_time == 0)
   143			goto out;
   144	
   145		/* we can't calc remote HZ with no different!! */
   146		if (tp->rx_opt.rcv_tsval == lp->remote_ref_time ||
   147		    tp->rx_opt.rcv_tsecr == lp->local_ref_time)
   148			goto out;
   149	
 > 150		m = (s64)TCP_TS_HZ *
   151		    (tp->rx_opt.rcv_tsval - lp->remote_ref_time) /
   152		    (tp->rx_opt.rcv_tsecr - lp->local_ref_time);
   153		if (m < 0)
   154			m = -m;
   155	
   156		if (rhz > 0) {
   157			m -= rhz >> 6;	/* m is now error in remote HZ est */
   158			rhz += m;	/* 63/64 old + 1/64 new */
   159		} else
   160			rhz = m << 6;
   161	
   162	 out:
   163		/* record time for successful remote HZ calc */
   164		if ((rhz >> 6) > 0)
   165			lp->flag |= LP_VALID_RHZ;
   166		else
   167			lp->flag &= ~LP_VALID_RHZ;
   168	
   169		/* record reference time stamp */
   170		lp->remote_ref_time = tp->rx_opt.rcv_tsval;
   171		lp->local_ref_time = tp->rx_opt.rcv_tsecr;
   172	
   173		return rhz >> 6;
   174	}
   175	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27357 bytes --]