linux-bcache.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit
@ 2021-02-12 12:50 Gustavo A. R. Silva
  2021-02-12 14:22 ` Coly Li
  0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2021-02-12 12:50 UTC (permalink / raw)
  To: Coly Li, Kent Overstreet, David S. Miller, Christina Jacob,
	Hariprasad Kelam, Sunil Goutham, Jesse Brandeburg
  Cc: linux-bcache, linux-kernel, 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit
  2021-02-12 12:50 [PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit Gustavo A. R. Silva
@ 2021-02-12 14:22 ` Coly Li
  2021-02-12 15:31   ` David Laight
  0 siblings, 1 reply; 6+ messages in thread
From: Coly Li @ 2021-02-12 14:22 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Kent Overstreet, David S. Miller,
	Christina Jacob, Hariprasad Kelam, Sunil Goutham,
	Jesse Brandeburg
  Cc: linux-bcache, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit
  2021-02-12 14:22 ` Coly Li
@ 2021-02-12 15:31   ` David Laight
  2021-02-12 16:01     ` Coly Li
  0 siblings, 1 reply; 6+ messages in thread
From: David Laight @ 2021-02-12 15:31 UTC (permalink / raw)
  To: 'Coly Li',
	Gustavo A. R. Silva, Kent Overstreet, David S. Miller,
	Christina Jacob, Hariprasad Kelam, Sunil Goutham,
	Jesse Brandeburg
  Cc: linux-bcache, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit
  2021-02-12 15:31   ` David Laight
@ 2021-02-12 16:01     ` Coly Li
  2021-02-12 16:42       ` David Laight
  0 siblings, 1 reply; 6+ messages in thread
From: Coly Li @ 2021-02-12 16:01 UTC (permalink / raw)
  To: David Laight, Gustavo A. R. Silva, Kent Overstreet,
	David S. Miller, Christina Jacob, Hariprasad Kelam,
	Sunil Goutham, Jesse Brandeburg
  Cc: linux-bcache, linux-kernel, dongdong tao

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit
  2021-02-12 16:01     ` Coly Li
@ 2021-02-12 16:42       ` David Laight
  2021-02-13 15:41         ` Coly Li
  0 siblings, 1 reply; 6+ messages in thread
From: David Laight @ 2021-02-12 16:42 UTC (permalink / raw)
  To: 'Coly Li',
	Gustavo A. R. Silva, Kent Overstreet, David S. Miller,
	Christina Jacob, Hariprasad Kelam, Sunil Goutham,
	Jesse Brandeburg
  Cc: linux-bcache, linux-kernel, dongdong tao

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)

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit
  2021-02-12 16:42       ` David Laight
@ 2021-02-13 15:41         ` Coly Li
  0 siblings, 0 replies; 6+ messages in thread
From: Coly Li @ 2021-02-13 15:41 UTC (permalink / raw)
  To: David Laight
  Cc: linux-bcache, linux-kernel, dongdong tao, Gustavo A. R. Silva,
	Kent Overstreet, David S. Miller, Christina Jacob,
	Hariprasad Kelam, Sunil Goutham, Jesse Brandeburg

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-02-13 15:42 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-12 12:50 [PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit Gustavo A. R. Silva
2021-02-12 14:22 ` Coly Li
2021-02-12 15:31   ` David Laight
2021-02-12 16:01     ` Coly Li
2021-02-12 16:42       ` David Laight
2021-02-13 15:41         ` Coly Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).