Re: [PATCH v3 06/16] rbd: convert timeouts to secs_to_jiffies()

[Christophe JAILLET]

Le 26/02/2025 à 08:28, Daniel Vacek a écrit :
> On Tue, 25 Feb 2025 at 22:10, Christophe JAILLET
> <christophe.jaillet-39ZsbGIQGT5GWvitb5QawA@xxxxxxxxxxxxxxxx> 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

...