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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 13 Feb 2021 23:41:11 +0800
From:   Coly Li <colyli@...e.de>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     "linux-bcache@...r.kernel.org" <linux-bcache@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dongdong tao <dongdong.tao@...onical.com>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Kent Overstreet <kent.overstreet@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Christina Jacob <cjacob@...vell.com>,
        Hariprasad Kelam <hkelam@...vell.com>,
        Sunil Goutham <sgoutham@...vell.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>
Subject: Re: [PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit

On 2/13/21 12:42 AM, David Laight wrote:
> From: Coly Li <colyli@...e.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@...e.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