Re: [PATCH] jiffies: Cast to unsigned long for secs_to_jiffies() conversion

Re: [PATCH] jiffies: Cast to unsigned long for secs_to_jiffies() conversion

[Geert Uytterhoeven]

Hi Jiri,

CC linux-xfs

On Fri, 31 Jan 2025 at 08:05, Jiri Slaby <jirislaby@kernel.org> wrote:
> On 30. 01. 25, 21:14, David Laight wrote:
> > On Thu, 30 Jan 2025 18:43:17 +0000
> > Easwar Hariharan <eahariha@linux.microsoft.com> wrote:
> >
> >> While converting users of msecs_to_jiffies(), lkp reported that some
> >> range checks would always be true because of the mismatch between the
> >> implied int value of secs_to_jiffies() vs the unsigned long
> >> return value of the msecs_to_jiffies() calls it was replacing. Fix this
> >> by casting secs_to_jiffies() values as unsigned long.
> >
> > Surely 'unsigned long' can't be the right type ?
> > It changes between 32bit and 64bit systems.
> > Either it is allowed to wrap - so should be 32bit on both,
> > or wrapping is unexpected and it needs to be 64bit on both.
>
> But jiffies are really ulong.

That's a good reason to make the change.
E.g. msecs_to_jiffies() does return unsigned long.

Note that this change may cause fall-out, e.g.

    int val = 5.

    pr_debug("timeout = %u jiffies\n", secs_to_jiffies(val));
                        ^^
                        must be changed to %lu

More importantly, I doubt this change is guaranteed to fix the
reported issue.  The code[*] in retry_timeout_seconds_store() does:

    int val;
    ...
    if (val < -1 || val > 86400)
            return -EINVAL;
    ...
    if (val != -1)
            ASSERT(secs_to_jiffies(val) < LONG_MAX);

As HZ is a known (rather small) constant, and val is range-checked
before, the compiler can still devise that the condition is always true.
So I think that assertion should just be removed.

[*] Before commit b524e0335da22473 ("xfs: convert timeouts to
    secs_to_jiffies()"), which was applied to the MM tree only 3
    days ago, the code used msecs_to_jiffies() * MSEC_PER_SEC,
    which is more complex than a simple multiplication, and harder for
    the compiler to analyze statically, thus not triggering the warning
    that easily...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] jiffies: Cast to unsigned long for secs_to_jiffies() conversion

[Easwar Hariharan]

On 1/31/2025 12:10 AM, Geert Uytterhoeven wrote:
> Hi Jiri,
> 
> CC linux-xfs
> 
> On Fri, 31 Jan 2025 at 08:05, Jiri Slaby <jirislaby@kernel.org> wrote:
>> On 30. 01. 25, 21:14, David Laight wrote:
>>> On Thu, 30 Jan 2025 18:43:17 +0000
>>> Easwar Hariharan <eahariha@linux.microsoft.com> wrote:
>>>
>>>> While converting users of msecs_to_jiffies(), lkp reported that some
>>>> range checks would always be true because of the mismatch between the
>>>> implied int value of secs_to_jiffies() vs the unsigned long
>>>> return value of the msecs_to_jiffies() calls it was replacing. Fix this
>>>> by casting secs_to_jiffies() values as unsigned long.
>>>
>>> Surely 'unsigned long' can't be the right type ?
>>> It changes between 32bit and 64bit systems.
>>> Either it is allowed to wrap - so should be 32bit on both,
>>> or wrapping is unexpected and it needs to be 64bit on both.
>>
>> But jiffies are really ulong.
> 
> That's a good reason to make the change.
> E.g. msecs_to_jiffies() does return unsigned long.
> 
> Note that this change may cause fall-out, e.g.
> 
>     int val = 5.
> 
>     pr_debug("timeout = %u jiffies\n", secs_to_jiffies(val));
>                         ^^
>                         must be changed to %lu
> 
> More importantly, I doubt this change is guaranteed to fix the
> reported issue.  The code[*] in retry_timeout_seconds_store() does:
> 
>     int val;
>     ...
>     if (val < -1 || val > 86400)
>             return -EINVAL;
>     ...
>     if (val != -1)
>             ASSERT(secs_to_jiffies(val) < LONG_MAX);
> 
> As HZ is a known (rather small) constant, and val is range-checked
> before, the compiler can still devise that the condition is always true.
> So I think that assertion should just be removed.
> 
> [*] Before commit b524e0335da22473 ("xfs: convert timeouts to
>     secs_to_jiffies()"), which was applied to the MM tree only 3
>     days ago, the code used msecs_to_jiffies() * MSEC_PER_SEC,
>     which is more complex than a simple multiplication, and harder for
>     the compiler to analyze statically, thus not triggering the warning
>     that easily...
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Thanks, Jiri and Geert. Geert, am I correct in understanding you that
you're suggesting v2 of the series[1] to convert msecs_to_jiffies()
calls to secs_to_jiffies() remove the ASSERT as redundant, while also
keeping this patch because ulong is the right type for jiffies?

[1]
https://lore.kernel.org/all/20250128-converge-secs-to-jiffies-part-two-v1-0-9a6ecf0b2308@linux.microsoft.com/

Thanks,
Easwar

Re: [PATCH] jiffies: Cast to unsigned long for secs_to_jiffies() conversion

[Easwar Hariharan]

On 1/31/2025 9:55 AM, Easwar Hariharan wrote:
> On 1/31/2025 12:10 AM, Geert Uytterhoeven wrote:
>> Hi Jiri,
>>
>> CC linux-xfs
>>
>> On Fri, 31 Jan 2025 at 08:05, Jiri Slaby <jirislaby@kernel.org> wrote:
>>> On 30. 01. 25, 21:14, David Laight wrote:
>>>> On Thu, 30 Jan 2025 18:43:17 +0000
>>>> Easwar Hariharan <eahariha@linux.microsoft.com> wrote:
>>>>
>>>>> While converting users of msecs_to_jiffies(), lkp reported that some
>>>>> range checks would always be true because of the mismatch between the
>>>>> implied int value of secs_to_jiffies() vs the unsigned long
>>>>> return value of the msecs_to_jiffies() calls it was replacing. Fix this
>>>>> by casting secs_to_jiffies() values as unsigned long.
>>>>
>>>> Surely 'unsigned long' can't be the right type ?
>>>> It changes between 32bit and 64bit systems.
>>>> Either it is allowed to wrap - so should be 32bit on both,
>>>> or wrapping is unexpected and it needs to be 64bit on both.
>>>
>>> But jiffies are really ulong.
>>
>> That's a good reason to make the change.
>> E.g. msecs_to_jiffies() does return unsigned long.
>>
>> Note that this change may cause fall-out, e.g.
>>
>>     int val = 5.
>>
>>     pr_debug("timeout = %u jiffies\n", secs_to_jiffies(val));
>>                         ^^
>>                         must be changed to %lu

That was wrong even before the conversion to secs_to_jiffies() (or this
patch) because as Jiri says, jiffies are ulong.

<snip>

- Easwar (he/him)

Re: [PATCH] jiffies: Cast to unsigned long for secs_to_jiffies() conversion

[Easwar Hariharan]

On 1/31/2025 9:55 AM, Easwar Hariharan wrote:
> On 1/31/2025 12:10 AM, Geert Uytterhoeven wrote:

<snip>

>>
>> More importantly, I doubt this change is guaranteed to fix the
>> reported issue.  The code[*] in retry_timeout_seconds_store() does:
>>
>>     int val;
>>     ...
>>     if (val < -1 || val > 86400)
>>             return -EINVAL;
>>     ...
>>     if (val != -1)
>>             ASSERT(secs_to_jiffies(val) < LONG_MAX);
>>
>> As HZ is a known (rather small) constant, and val is range-checked
>> before, the compiler can still devise that the condition is always true.
>> So I think that assertion should just be removed.
>>

Following the lkp instructions to repro the issue, with this patch, the
compiler does not continue reporting that the condition is always true.
This patch is sufficient IMHO, without needing to remove the assert.

- Easwar (he/him)