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: <CAPjX3Fc1UuWvih_krriaF32aPCbGP0SPg2TSrBA8Xb7a=Ozc5Q@mail.gmail.com>
Date: Wed, 26 Feb 2025 09:29:36 +0100
From: Daniel Vacek <neelx@...e.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
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, 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()

On Wed, 26 Feb 2025 at 09:10, Christophe JAILLET
<christophe.jaillet@...adoo.fr> wrote:
>
> 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.

I see. But that was rather an unrelated side-effect. Still you're
right, it needs to be handled carefully not to remove additional
guarantees which were implied unintentionally. At least in places
where these were provided in the first place.

> >
> > 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().

Yeah, I was referring only in context of the example you presented,
not for the rest of the kernel. Sorry about the confusion.

> 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.

Hmmm, fishy. Maybe a function would be better than a macro?

> >
> >>          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*.

Got it. But that may still fail if you configure HZ to 5000 or
anything above 1000. Not that anyone should go this way but...

> 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;

Right, that should correctly handle any HZ value. Looks good to me.

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