linux-kernel - Re: [PATCH] jiffies: Cast to unsigned long for 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: <CAMuHMdUNjKJ0CFw+i1qgVsHO2LU6uOqkAq5iGL0EZyCtrfzM=A@mail.gmail.com>
Date: Fri, 31 Jan 2025 09:10:25 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Jiri Slaby <jirislaby@...nel.org>
Cc: David Laight <david.laight.linux@...il.com>, 
	Easwar Hariharan <eahariha@...ux.microsoft.com>, Thomas Gleixner <tglx@...utronix.de>, 
	Anna-Maria Behnsen <anna-maria@...utronix.de>, 
	Luiz Augusto von Dentz <luiz.von.dentz@...el.com>, Miguel Ojeda <ojeda@...nel.org>, 
	open list <linux-kernel@...r.kernel.org>, stable@...r.kernel.org, 
	Andrew Morton <akpm@...ux-foundation.org>, kernel test robot <lkp@...el.com>, 
	linux-xfs <linux-xfs@...r.kernel.org>
Subject: Re: [PATCH] jiffies: Cast to unsigned long for secs_to_jiffies() conversion

Hi Jiri,

CC linux-xfs

On Fri, 31 Jan 2025 at 08:05, Jiri Slaby <jirislaby@...nel.org> wrote:
> On 30. 01. 25, 21:14, David Laight wrote:
> > On Thu, 30 Jan 2025 18:43:17 +0000
> > Easwar Hariharan <eahariha@...ux.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@...ux-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