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(-)
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
On Wed, 31 Jan 2018 18:24:07 -0600
"Gustavo A. R. Silva" <[email protected]> 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
Hi Alan,
Quoting Alan Cox <[email protected]>:
> On Wed, 31 Jan 2018 18:24:07 -0600
> "Gustavo A. R. Silva" <[email protected]> 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
On Wed, Jan 31, 2018 at 07:07:49PM -0600, Gustavo A. R. Silva wrote:
>
> Hi Alan,
>
> Quoting Alan Cox <[email protected]>:
>
> >On Wed, 31 Jan 2018 18:24:07 -0600
> >"Gustavo A. R. Silva" <[email protected]> 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
> > 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
From: "Gustavo A. R. Silva" <[email protected]>
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 <[email protected]>
Sorry, I'm not applying this, the domain of the input values
means that the calculation cannot overflow.
Hi David,
Quoting David Laight <[email protected]>:
>> > 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
Hi Andrew,
Quoting Andrew Lunn <[email protected]>:
> On Wed, Jan 31, 2018 at 07:07:49PM -0600, Gustavo A. R. Silva wrote:
>>
>> Hi Alan,
>>
>> Quoting Alan Cox <[email protected]>:
>>
>> >On Wed, 31 Jan 2018 18:24:07 -0600
>> >"Gustavo A. R. Silva" <[email protected]> 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
Hi David,
Quoting David Miller <[email protected]>:
> From: "Gustavo A. R. Silva" <[email protected]>
> 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 <[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.
--
Gustavo
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
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