| Message ID | 20250225-converge-secs-to-jiffies-part-two-v3-6-a43967e36c88@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Converge on using secs_to_jiffies() part two | expand |
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. Hoping I got casting right: #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)); return 0; } gives : res = -600 res = 429496130 with msec, the previous code would catch the overflow, now it overflows silently. untested, but maybe: if (result.uint_32 > INT_MAX / HZ) goto out_of_range; ? CJ > > Acked-by: Ilya Dryomov <idryomov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > Signed-off-by: Easwar Hariharan <eahariha-1pm0nblsJy7Jp67UH1NAhkEOCMrvLtNR@public.gmane.org> > --- > drivers/block/rbd.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index faafd7ff43d6ef53110ab3663cc7ac322214cc8c..41207133e21e9203192adf3b92390818e8fa5a58 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -108,7 +108,7 @@ static int atomic_dec_return_safe(atomic_t *v) > #define RBD_OBJ_PREFIX_LEN_MAX 64 > > #define RBD_NOTIFY_TIMEOUT 5 /* seconds */ > -#define RBD_RETRY_DELAY msecs_to_jiffies(1000) > +#define RBD_RETRY_DELAY secs_to_jiffies(1) > > /* Feature bits */ > > @@ -4162,7 +4162,7 @@ static void rbd_acquire_lock(struct work_struct *work) > dout("%s rbd_dev %p requeuing lock_dwork\n", __func__, > rbd_dev); > mod_delayed_work(rbd_dev->task_wq, &rbd_dev->lock_dwork, > - msecs_to_jiffies(2 * RBD_NOTIFY_TIMEOUT * MSEC_PER_SEC)); > + secs_to_jiffies(2 * RBD_NOTIFY_TIMEOUT)); > } > } > > @@ -6283,9 +6283,7 @@ static int rbd_parse_param(struct fs_parameter *param, > break; > case Opt_lock_timeout: > /* 0 is "wait forever" (i.e. infinite timeout) */ > - if (result.uint_32 > INT_MAX / 1000) > - goto out_of_range; > - opt->lock_timeout = msecs_to_jiffies(result.uint_32 * 1000); > + opt->lock_timeout = secs_to_jiffies(result.uint_32); > break; > case Opt_pool_ns: > kfree(pctx->spec->pool_ns); >
On 2/25/2025 1:09 PM, Christophe JAILLET 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. > > > Hoping I got casting right: > > #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)); > > return 0; > } > > > gives : > > res = -600 > res = 429496130 > > with msec, the previous code would catch the overflow, now it overflows silently. > > untested, but maybe: > if (result.uint_32 > INT_MAX / HZ) > goto out_of_range; > > ? > > CJ > Thanks for the review! I was able to replicate your results, I'll try this range check and get back. Thanks, Easwar (he/him)
On Tue, 25 Feb 2025 at 22:10, Christophe JAILLET <christophe.jaillet@wanadoo.fr> 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? Now, you'll get that mentioned correct result with: #define secs_to_jiffies(_secs) ((unsigned long)(_secs) * HZ) Still, why unsigned? What if you wanted to convert -5 seconds to jiffies? > 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. > untested, but maybe: > if (result.uint_32 > INT_MAX / HZ) > goto out_of_range; > > ? > > CJ > > > > > > Acked-by: Ilya Dryomov <idryomov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > > Signed-off-by: Easwar Hariharan <eahariha-1pm0nblsJy7Jp67UH1NAhkEOCMrvLtNR@public.gmane.org> > > --- > > drivers/block/rbd.c | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > > index faafd7ff43d6ef53110ab3663cc7ac322214cc8c..41207133e21e9203192adf3b92390818e8fa5a58 100644 > > --- a/drivers/block/rbd.c > > +++ b/drivers/block/rbd.c > > @@ -108,7 +108,7 @@ static int atomic_dec_return_safe(atomic_t *v) > > #define RBD_OBJ_PREFIX_LEN_MAX 64 > > > > #define RBD_NOTIFY_TIMEOUT 5 /* seconds */ > > -#define RBD_RETRY_DELAY msecs_to_jiffies(1000) > > +#define RBD_RETRY_DELAY secs_to_jiffies(1) > > > > /* Feature bits */ > > > > @@ -4162,7 +4162,7 @@ static void rbd_acquire_lock(struct work_struct *work) > > dout("%s rbd_dev %p requeuing lock_dwork\n", __func__, > > rbd_dev); > > mod_delayed_work(rbd_dev->task_wq, &rbd_dev->lock_dwork, > > - msecs_to_jiffies(2 * RBD_NOTIFY_TIMEOUT * MSEC_PER_SEC)); > > + secs_to_jiffies(2 * RBD_NOTIFY_TIMEOUT)); > > } > > } > > > > @@ -6283,9 +6283,7 @@ static int rbd_parse_param(struct fs_parameter *param, > > break; > > case Opt_lock_timeout: > > /* 0 is "wait forever" (i.e. infinite timeout) */ > > - if (result.uint_32 > INT_MAX / 1000) > > - goto out_of_range; > > - opt->lock_timeout = msecs_to_jiffies(result.uint_32 * 1000); > > + opt->lock_timeout = secs_to_jiffies(result.uint_32); > > break; > > case Opt_pool_ns: > > kfree(pctx->spec->pool_ns); > > > >
Le 26/02/2025 à 08:28, Daniel Vacek a écrit : > On Tue, 25 Feb 2025 at 22:10, Christophe JAILLET > <christophe.jaillet-39ZsbGIQGT5GWvitb5QawA@public.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. > > 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 >> ...
On Wed, 26 Feb 2025 at 09:10, Christophe JAILLET <christophe.jaillet@wanadoo.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@public.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 > >> > > ...
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index faafd7ff43d6ef53110ab3663cc7ac322214cc8c..41207133e21e9203192adf3b92390818e8fa5a58 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -108,7 +108,7 @@ static int atomic_dec_return_safe(atomic_t *v) #define RBD_OBJ_PREFIX_LEN_MAX 64 #define RBD_NOTIFY_TIMEOUT 5 /* seconds */ -#define RBD_RETRY_DELAY msecs_to_jiffies(1000) +#define RBD_RETRY_DELAY secs_to_jiffies(1) /* Feature bits */ @@ -4162,7 +4162,7 @@ static void rbd_acquire_lock(struct work_struct *work) dout("%s rbd_dev %p requeuing lock_dwork\n", __func__, rbd_dev); mod_delayed_work(rbd_dev->task_wq, &rbd_dev->lock_dwork, - msecs_to_jiffies(2 * RBD_NOTIFY_TIMEOUT * MSEC_PER_SEC)); + secs_to_jiffies(2 * RBD_NOTIFY_TIMEOUT)); } } @@ -6283,9 +6283,7 @@ static int rbd_parse_param(struct fs_parameter *param, break; case Opt_lock_timeout: /* 0 is "wait forever" (i.e. infinite timeout) */ - if (result.uint_32 > INT_MAX / 1000) - goto out_of_range; - opt->lock_timeout = msecs_to_jiffies(result.uint_32 * 1000); + opt->lock_timeout = secs_to_jiffies(result.uint_32); break; case Opt_pool_ns: kfree(pctx->spec->pool_ns);
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. Acked-by: Ilya Dryomov <idryomov@gmail.com> Signed-off-by: Easwar Hariharan <eahariha@linux.microsoft.com> --- drivers/block/rbd.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)