[PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit

[PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Cast multiple variables to (int64_t) in order to give the compiler
complete information about the proper arithmetic to use. Notice that
these variables are being used in contexts that expect expressions of
type int64_t  (64 bit, signed). And currently, such expressions are
being evaluated using 32-bit arithmetic.

Fixes: d0cf9503e908 ("octeontx2-pf: ethtool fec mode support")
Addresses-Coverity-ID: 1501724 ("Unintentional integer overflow")
Addresses-Coverity-ID: 1501725 ("Unintentional integer overflow")
Addresses-Coverity-ID: 1501726 ("Unintentional integer overflow")
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/md/bcache/writeback.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 82d4e0880a99..4fb635c0baa0 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -110,13 +110,13 @@ static void __update_writeback_rate(struct cached_dev *dc)
 		int64_t fps;
 
 		if (c->gc_stats.in_use <= BCH_WRITEBACK_FRAGMENT_THRESHOLD_MID) {
-			fp_term = dc->writeback_rate_fp_term_low *
+			fp_term = (int64_t)dc->writeback_rate_fp_term_low *
 			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_LOW);
 		} else if (c->gc_stats.in_use <= BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH) {
-			fp_term = dc->writeback_rate_fp_term_mid *
+			fp_term = (int64_t)dc->writeback_rate_fp_term_mid *
 			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_MID);
 		} else {
-			fp_term = dc->writeback_rate_fp_term_high *
+			fp_term = (int64_t)dc->writeback_rate_fp_term_high *
 			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH);
 		}
 		fps = div_s64(dirty, dirty_buckets) * fp_term;
-- 
2.27.0

Re: [PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit

[Coly Li]

On 2/12/21 8:50 PM, Gustavo A. R. Silva wrote:
> Cast multiple variables to (int64_t) in order to give the compiler
> complete information about the proper arithmetic to use. Notice that
> these variables are being used in contexts that expect expressions of
> type int64_t  (64 bit, signed). And currently, such expressions are
> being evaluated using 32-bit arithmetic.
> 
> Fixes: d0cf9503e908 ("octeontx2-pf: ethtool fec mode support")
> Addresses-Coverity-ID: 1501724 ("Unintentional integer overflow")
> Addresses-Coverity-ID: 1501725 ("Unintentional integer overflow")
> Addresses-Coverity-ID: 1501726 ("Unintentional integer overflow")
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  drivers/md/bcache/writeback.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
> index 82d4e0880a99..4fb635c0baa0 100644
> --- a/drivers/md/bcache/writeback.c
> +++ b/drivers/md/bcache/writeback.c
> @@ -110,13 +110,13 @@ static void __update_writeback_rate(struct cached_dev *dc)
>  		int64_t fps;
>  
>  		if (c->gc_stats.in_use <= BCH_WRITEBACK_FRAGMENT_THRESHOLD_MID) {
> -			fp_term = dc->writeback_rate_fp_term_low *
> +			fp_term = (int64_t)dc->writeback_rate_fp_term_low *
>  			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_LOW);
>  		} else if (c->gc_stats.in_use <= BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH) {
> -			fp_term = dc->writeback_rate_fp_term_mid *
> +			fp_term = (int64_t)dc->writeback_rate_fp_term_mid *
>  			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_MID);
>  		} else {
> -			fp_term = dc->writeback_rate_fp_term_high *
> +			fp_term = (int64_t)dc->writeback_rate_fp_term_high *
>  			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH);
>  		}
>  		fps = div_s64(dirty, dirty_buckets) * fp_term;
> 

Hmm, should such thing be handled by compiler ?  Otherwise this kind of
potential overflow issue will be endless time to time.

I am not a compiler expert, should we have to do such explicit type cast
all the time ?

Coly Li

RE: [PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit

[David Laight]

> >  		if (c->gc_stats.in_use <= BCH_WRITEBACK_FRAGMENT_THRESHOLD_MID) {
> > -			fp_term = dc->writeback_rate_fp_term_low *
> > +			fp_term = (int64_t)dc->writeback_rate_fp_term_low *
> >  			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_LOW);
> >  		} else if (c->gc_stats.in_use <= BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH) {
> > -			fp_term = dc->writeback_rate_fp_term_mid *
> > +			fp_term = (int64_t)dc->writeback_rate_fp_term_mid *
> >  			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_MID);
> >  		} else {
> > -			fp_term = dc->writeback_rate_fp_term_high *
> > +			fp_term = (int64_t)dc->writeback_rate_fp_term_high *
> >  			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH);
> >  		}
> >  		fps = div_s64(dirty, dirty_buckets) * fp_term;
> >
> 
> Hmm, should such thing be handled by compiler ?  Otherwise this kind of
> potential overflow issue will be endless time to time.
> 
> I am not a compiler expert, should we have to do such explicit type cast
> all the time ?

We do to get a 64bit product from two 32bit values.
An alternative for the above would be:
		fp_term = c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH;
		fp_term *= dc->writeback_rate_fp_term_high;

I hope BCH_WRITEBACK_FRAGMENT_THRESHOLD_LOW is zero :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit

[Coly Li]

On 2/12/21 11:31 PM, David Laight wrote:
>>>  		if (c->gc_stats.in_use <= BCH_WRITEBACK_FRAGMENT_THRESHOLD_MID) {
>>> -			fp_term = dc->writeback_rate_fp_term_low *
>>> +			fp_term = (int64_t)dc->writeback_rate_fp_term_low *
>>>  			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_LOW);
>>>  		} else if (c->gc_stats.in_use <= BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH) {
>>> -			fp_term = dc->writeback_rate_fp_term_mid *
>>> +			fp_term = (int64_t)dc->writeback_rate_fp_term_mid *
>>>  			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_MID);
>>>  		} else {
>>> -			fp_term = dc->writeback_rate_fp_term_high *
>>> +			fp_term = (int64_t)dc->writeback_rate_fp_term_high *
>>>  			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH);
>>>  		}
>>>  		fps = div_s64(dirty, dirty_buckets) * fp_term;
>>>
>>
>> Hmm, should such thing be handled by compiler ?  Otherwise this kind of
>> potential overflow issue will be endless time to time.
>>
>> I am not a compiler expert, should we have to do such explicit type cast
>> all the time ?
> 

Hi David,

I add Dongdong Tao Cced, who is author of this patch.

Could you please offer me more information about the following lines?
Let me ask more for my questions.

> We do to get a 64bit product from two 32bit values.
> An alternative for the above would be:
> 		fp_term = c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH;
> 		fp_term *= dc->writeback_rate_fp_term_high;

The original line is,
fp_term = dc->writeback_rate_fp_term_high * (c->gc_stats.in_use -
BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH)

The first value dc->writeback_rate_fp_term_high is unsigned int (32bit),
and the second value (c->gc_stats.in_use -
BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH) is unsigned int (32bit) too. And
fp_term is 64bit, if the product is larger than 32bits, the compiler
should know fp_term is 64bit and upgrade the product to 64bit.

The above is just my guess, because I feel compiling should have the
clue for the product upgrade to avoid overflow. But I almost know
nothing about compiler internal ....

> 
> I hope BCH_WRITEBACK_FRAGMENT_THRESHOLD_LOW is zero :-)

Why BCH_WRITEBACK_FRAGMENT_THRESHOLD_LOW being zero can be helpful to
avoid the overflow ? Could you please to provide more detailed information.

I am not challenging you, I just want to extend my knowledge by learning
from you. Thanks in advance.

Coly Li

RE: [PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit

[David Laight]

From: Coly Li <colyli@suse.de>
> Sent: 12 February 2021 16:02
> 
> On 2/12/21 11:31 PM, David Laight wrote:
> >>>  		if (c->gc_stats.in_use <= BCH_WRITEBACK_FRAGMENT_THRESHOLD_MID) {
> >>> -			fp_term = dc->writeback_rate_fp_term_low *
> >>> +			fp_term = (int64_t)dc->writeback_rate_fp_term_low *
> >>>  			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_LOW);
> >>>  		} else if (c->gc_stats.in_use <= BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH) {
> >>> -			fp_term = dc->writeback_rate_fp_term_mid *
> >>> +			fp_term = (int64_t)dc->writeback_rate_fp_term_mid *
> >>>  			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_MID);
> >>>  		} else {
> >>> -			fp_term = dc->writeback_rate_fp_term_high *
> >>> +			fp_term = (int64_t)dc->writeback_rate_fp_term_high *
> >>>  			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH);
> >>>  		}
> >>>  		fps = div_s64(dirty, dirty_buckets) * fp_term;
> >>>
> >>
> >> Hmm, should such thing be handled by compiler ?  Otherwise this kind of
> >> potential overflow issue will be endless time to time.
> >>
> >> I am not a compiler expert, should we have to do such explicit type cast
> >> all the time ?
> >
> 
> Hi David,
> 
> I add Dongdong Tao Cced, who is author of this patch.
> 
> Could you please offer me more information about the following lines?
> Let me ask more for my questions.
> 
> > We do to get a 64bit product from two 32bit values.
> > An alternative for the above would be:
> > 		fp_term = c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH;
> > 		fp_term *= dc->writeback_rate_fp_term_high;
> 
> The original line is,
> fp_term = dc->writeback_rate_fp_term_high * (c->gc_stats.in_use -
> BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH)
> 
> The first value dc->writeback_rate_fp_term_high is unsigned int (32bit),
> and the second value (c->gc_stats.in_use -
> BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH) is unsigned int (32bit) too. And
> fp_term is 64bit, if the product is larger than 32bits, the compiler
> should know fp_term is 64bit and upgrade the product to 64bit.
> 
> The above is just my guess, because I feel compiling should have the
> clue for the product upgrade to avoid overflow. But I almost know
> nothing about compiler internal ....

No, the expression is evaluated as 32bit and then extended for the assignment.

Or, more correctly, integer variables smaller than int (usually short and char)
are extended to int prior to any arithmetic.
If one argument to an operator is larger than int it is extended.
If there is a sign v unsigned mismatch the signed value is cast to unsigned
(same bit pattern on 2's compliment systems).

There are some oddities:
- 'unsigned char/short' gets converted to 'signed int' unless
  char/short and int are the same size (which is allowed).
  (Although if char and int are the same size there are issues
  with the value for EOF.)
- (1 << 31) is a signed integer, it will get sign extended if used
  to mask a 64bit value.

K&R C converted 'unsigned char' to 'unsigned int' - but the ANSI
standards body decided otherwise.

The compiler is allowed to use an 'as if' rule to use the 8bit and
16bit arithmetic/registers on x86.
But on almost everything else arithmetic on char/short local variables
requires the compiler repeatedly mask the value back to 8/16 bits.

The C language has some other oddities - that are allowed but never done.
(Except for 'thought-machines' in comp.lang.c)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit

[Coly Li]

On 2/13/21 12:42 AM, David Laight wrote:
> From: Coly Li <colyli@suse.de>
>> Sent: 12 February 2021 16:02
>>
>> On 2/12/21 11:31 PM, David Laight wrote:
>>>>>  		if (c->gc_stats.in_use <= BCH_WRITEBACK_FRAGMENT_THRESHOLD_MID) {
>>>>> -			fp_term = dc->writeback_rate_fp_term_low *
>>>>> +			fp_term = (int64_t)dc->writeback_rate_fp_term_low *
>>>>>  			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_LOW);
>>>>>  		} else if (c->gc_stats.in_use <= BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH) {
>>>>> -			fp_term = dc->writeback_rate_fp_term_mid *
>>>>> +			fp_term = (int64_t)dc->writeback_rate_fp_term_mid *
>>>>>  			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_MID);
>>>>>  		} else {
>>>>> -			fp_term = dc->writeback_rate_fp_term_high *
>>>>> +			fp_term = (int64_t)dc->writeback_rate_fp_term_high *
>>>>>  			(c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH);
>>>>>  		}
>>>>>  		fps = div_s64(dirty, dirty_buckets) * fp_term;
>>>>>
>>>>
>>>> Hmm, should such thing be handled by compiler ?  Otherwise this kind of
>>>> potential overflow issue will be endless time to time.
>>>>
>>>> I am not a compiler expert, should we have to do such explicit type cast
>>>> all the time ?
>>>
>>
>> Hi David,
>>
>> I add Dongdong Tao Cced, who is author of this patch.
>>
>> Could you please offer me more information about the following lines?
>> Let me ask more for my questions.
>>
>>> We do to get a 64bit product from two 32bit values.
>>> An alternative for the above would be:
>>> 		fp_term = c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH;
>>> 		fp_term *= dc->writeback_rate_fp_term_high;
>>
>> The original line is,
>> fp_term = dc->writeback_rate_fp_term_high * (c->gc_stats.in_use -
>> BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH)
>>
>> The first value dc->writeback_rate_fp_term_high is unsigned int (32bit),
>> and the second value (c->gc_stats.in_use -
>> BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH) is unsigned int (32bit) too. And
>> fp_term is 64bit, if the product is larger than 32bits, the compiler
>> should know fp_term is 64bit and upgrade the product to 64bit.
>>
>> The above is just my guess, because I feel compiling should have the
>> clue for the product upgrade to avoid overflow. But I almost know
>> nothing about compiler internal ....
> 
> No, the expression is evaluated as 32bit and then extended for the assignment.

I do some test, and you are right. I am so surprised that the product
does not extended and the overflowed result is 0. Thank you for letting
me know this, and I correct my mistaken concept not too late :-)

If you want me to take this patch, it is OK to me. I will have it in
v5.12. If you want to handle this patch to upstream in your track, you
may have my
	Acked-by: Coly Li <colyli@suse.de>

Thanks for your patient explaining.

> 
> Or, more correctly, integer variables smaller than int (usually short and char)
> are extended to int prior to any arithmetic.
> If one argument to an operator is larger than int it is extended.
> If there is a sign v unsigned mismatch the signed value is cast to unsigned
> (same bit pattern on 2's compliment systems).
> 
> There are some oddities:
> - 'unsigned char/short' gets converted to 'signed int' unless
>   char/short and int are the same size (which is allowed).
>   (Although if char and int are the same size there are issues
>   with the value for EOF.)
> - (1 << 31) is a signed integer, it will get sign extended if used
>   to mask a 64bit value.
> 
> K&R C converted 'unsigned char' to 'unsigned int' - but the ANSI
> standards body decided otherwise.
> 
> The compiler is allowed to use an 'as if' rule to use the 8bit and
> 16bit arithmetic/registers on x86.
> But on almost everything else arithmetic on char/short local variables
> requires the compiler repeatedly mask the value back to 8/16 bits.
> 
> The C language has some other oddities - that are allowed but never done.
> (Except for 'thought-machines' in comp.lang.c)

I know the above things, but I DO NOT know product of two 32bit values
multiplying does not extend to 64bit. Good to know the compiling is not
that smart.

Thank you!

Coly Li