linux-kernel - Re: [PATCH v3 06/16] rbd: convert timeouts to secs_to

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <7b8346a1-8a7d-4fcf-a026-119d77f2ca85@wanadoo.fr>
Date: Wed, 26 Feb 2025 09:10:07 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: neelx@...e.com
Cc: Frank.Li@....com, James.Bottomley@...senpartnership.com,
 Julia.Lawall@...ia.fr, Shyam-sundar.S-k@....com, akpm@...ux-foundation.org,
 axboe@...nel.dk, broonie@...nel.org, cassel@...nel.org, cem@...nel.org,
 ceph-devel@...r.kernel.org, christophe.jaillet@...adoo.fr, clm@...com,
 cocci@...ia.fr, dick.kennedy@...adcom.com, djwong@...nel.org,
 dlemoal@...nel.org, dongsheng.yang@...ystack.cn,
 dri-devel@...ts.freedesktop.org, dsterba@...e.com,
 eahariha@...ux.microsoft.com, festevam@...il.com, hch@....de,
 hdegoede@...hat.com, hmh@....eng.br, ibm-acpi-devel@...ts.sourceforge.net,
 idryomov@...il.com, ilpo.jarvinen@...ux.intel.com, imx@...ts.linux.dev,
 james.smart@...adcom.com, jgg@...pe.ca, josef@...icpanda.com,
 kalesh-anakkur.purayil@...adcom.com, kbusch@...nel.org,
 kernel@...gutronix.de, leon@...nel.org,
 linux-arm-kernel@...ts.infradead.org, linux-block@...r.kernel.org,
 linux-btrfs@...r.kernel.org, linux-ide@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-nvme@...ts.infradead.org,
 linux-pm@...r.kernel.org, linux-rdma@...r.kernel.org,
 linux-scsi@...r.kernel.org, linux-sound@...r.kernel.org,
 linux-spi@...r.kernel.org, linux-xfs@...r.kernel.org,
 martin.petersen@...cle.com, nicolas.palix@...g.fr, ogabbay@...nel.org,
 perex@...ex.cz, platform-driver-x86@...r.kernel.org, s.hauer@...gutronix.de,
 sagi@...mberg.me, selvin.xavier@...adcom.com, shawnguo@...nel.org,
 sre@...nel.org, tiwai@...e.com, xiubli@...hat.com, yaron.avizrat@...el.com
Subject: Re: [PATCH v3 06/16] rbd: convert timeouts to secs_to_jiffies()

Le 26/02/2025 à 08:28, Daniel Vacek a écrit :
> On Tue, 25 Feb 2025 at 22:10, Christophe JAILLET
> <christophe.jaillet-39ZsbGIQGT5GWvitb5QawA@...lic.gmane.org> wrote:
>>
>> Le 25/02/2025 à 21:17, Easwar Hariharan a écrit :
>>> Commit b35108a51cf7 ("jiffies: Define secs_to_jiffies()") introduced
>>> secs_to_jiffies().  As the value here is a multiple of 1000, use
>>> secs_to_jiffies() instead of msecs_to_jiffies() to avoid the multiplication
>>>
>>> This is converted using scripts/coccinelle/misc/secs_to_jiffies.cocci with
>>> the following Coccinelle rules:
>>>
>>> @depends on patch@ expression E; @@
>>>
>>> -msecs_to_jiffies(E * 1000)
>>> +secs_to_jiffies(E)
>>>
>>> @depends on patch@ expression E; @@
>>>
>>> -msecs_to_jiffies(E * MSEC_PER_SEC)
>>> +secs_to_jiffies(E)
>>>
>>> While here, remove the no-longer necessary check for range since there's
>>> no multiplication involved.
>>
>> I'm not sure this is correct.
>> Now you multiply by HZ and things can still overflow.
> 
> This does not deal with any additional multiplications. If there is an
> overflow, it was already there before to begin with, IMO.
> 
>> Hoping I got casting right:
> 
> Maybe not exactly? See below...
> 
>> #define MSEC_PER_SEC    1000L
>> #define HZ 100
>>
>>
>> #define secs_to_jiffies(_secs) (unsigned long)((_secs) * HZ)
>>
>> static inline unsigned long _msecs_to_jiffies(const unsigned int m)
>> {
>>          return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ);
>> }
>>
>> int main() {
>>
>>          int n = INT_MAX - 5;
>>
>>          printf("res  = %ld\n", secs_to_jiffies(n));
>>          printf("res  = %ld\n", _msecs_to_jiffies(1000 * n));
> 
> I think the format should actually be %lu giving the below results:
> 
> res  = 18446744073709551016
> res  = 429496130
> 
> Which is still wrong nonetheless. But here, *both* results are wrong
> as the expected output should be 214748364200 which you'll get with
> the correct helper/macro.
> 
> But note another thing, the 1000 * (INT_MAX - 5) already overflows
> even before calling _msecs_to_jiffies(). See?

Agreed and intentional in my test C code.

That is the point.

The "if (result.uint_32 > INT_MAX / 1000)" in the original code was 
handling such values.

> 
> Now, you'll get that mentioned correct result with:
> 
> #define secs_to_jiffies(_secs) ((unsigned long)(_secs) * HZ)

Not looked in details, but I think I would second on you on this, in 
this specific example. Not sure if it would handle all possible uses of 
secs_to_jiffies().

But it is not how secs_to_jiffies() is defined up to now. See [1].

[1]: 
https://elixir.bootlin.com/linux/v6.14-rc4/source/include/linux/jiffies.h#L540

> 
> Still, why unsigned? What if you wanted to convert -5 seconds to jiffies?

See commit bb2784d9ab495 which added the cast.

> 
>>          return 0;
>> }
>>
>>
>> gives :
>>
>> res  = -600
>> res  = 429496130
>>
>> with msec, the previous code would catch the overflow, now it overflows
>> silently.
> 
> What compiler options are you using? I'm not getting any warnings.

I mean, with:
	if (result.uint_32 > INT_MAX / 1000)
		goto out_of_range;
the overflow would be handled *at runtime*.

Without such a check, an unexpected value could be stored in 
opt->lock_timeout.

I think that a test is needed and with secs_to_jiffies(), I tentatively 
proposed:
	if (result.uint_32 > INT_MAX / HZ)
		goto out_of_range;

CJ

> 
>> untested, but maybe:
>>          if (result.uint_32 > INT_MAX / HZ)
>>                  goto out_of_range;
>>
>> ?
>>
>> CJ
>>

...