tcp_lp: use 64-bit arithmetic instead of 32-bit

PATCH Superseded Gustavo A. R. Silva 7 participants 11 messages Feb 1, 2018 – Feb 2, 2018

Subsystems (4)

Networking Network drivers TCP TCP low priority module

lore

Share & tools

lore

Gustavo A. R. Silva

author

2018-02-01 00:24 UTC #0 lore raw

PATCH 1/1 tcp_lp: use 64-bit arithmetic instead of 32-bit

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 <[email protected]>

---
 net/ipv4/tcp_lp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

net/ipv4/tcp_lp.c +4 -4

index ae10ed6..4999111 100644
@@ -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;
 	}

Signature · 1 line

2.7.4

Alan Cox

2018-02-01 00:33 UTC #1 lore raw

Quoted 7 lines · On Wed, 31 Jan 2018 18:24:07 -0600 "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.

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

Gustavo A. R. Silva

2018-02-01 01:07 UTC #2 lore raw

Hi Alan,

Quoting Alan Cox <[email protected]>:

Quoted 8 lines

Quoted 5 lines · On Wed, 31 Jan 2018 18:24:07 -0600 "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).

Quoted 6 lines

Quoted 2 lines
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

Andrew Lunn

2018-02-01 01:51 UTC #3 lore raw

Quoted 16 lines · Gustavo A. R. Silva

Hi Alan,
Quoting Alan Cox <[email protected]>:
Quoted 11 lines
Quoted 7 lines · On Wed, 31 Jan 2018 18:24:07 -0600 "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.

Quoted 8 lines

Quoted 3 lines
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

Gustavo A. R. Silva

2018-02-02 02:32 UTC #4 lore raw

Hi Andrew,

Quoting Andrew Lunn <[email protected]>:

Quoted 26 lines · Gustavo A. R. Silva

Quoted 16 lines
Hi Alan,
Quoting Alan Cox <[email protected]>:
Quoted 11 lines
Quoted 7 lines · On Wed, 31 Jan 2018 18:24:07 -0600 "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.
Quoted 8 lines
Quoted 3 lines
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.

Quoted 6 lines

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

David Laight

2018-02-01 10:13 UTC #5 lore raw

In reply to Gustavo A. R. Silva

Quoted 9 lines

Quoted 3 lines
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

Gustavo A. R. Silva

2018-02-02 02:33 UTC #6 lore raw

Hi David,

Quoting David Laight <[email protected]>:

Quoted 10 lines

Quoted 9 lines
Quoted 3 lines
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.

Quoted 5 lines

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

David Miller

maintainer

2018-02-01 14:45 UTC #7 lore raw

In reply to Gustavo A. R. Silva

From: "Gustavo A. R. Silva" <[email protected]>
Date: Wed, 31 Jan 2018 18:24:07 -0600

Quoted 5 lines

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

Quoted 7 lines

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 <[email protected]>

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

Gustavo A. R. Silva

2018-02-02 02:38 UTC #8 lore raw

Hi David,

Quoting David Miller <[email protected]>:

Quoted 11 lines

From: "Gustavo A. R. Silva" <[email protected]>
Date: Wed, 31 Jan 2018 18:24:07 -0600
Quoted 8 lines
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.

Quoted 7 lines

Quoted 4 lines
Addresses-Coverity-ID: 200687
Addresses-Coverity-ID: 200688
Addresses-Coverity-ID: 200689
Signed-off-by Gustavo A. R. Silva <[email protected]>
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.

kbuild test robot

build bot

2018-02-02 09:32 UTC #9 lore raw

In reply to Gustavo A. R. Silva

Thank you for the patch! Yet something to improve:

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

kbuild test robot

build bot

2018-02-02 10:44 UTC #10 lore raw

In reply to Gustavo A. R. Silva

Thank you for the patch! Yet something to improve:

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