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@xxxxxxxxxx> wrote:
> On 30. 01. 25, 21:14, David Laight wrote:
> > On Thu, 30 Jan 2025 18:43:17 +0000
> > Easwar Hariharan <eahariha@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx

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