| 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
| ||
|
Message-ID: <CAPjX3Fcr+BoMRgZGbqqgpF+w-sHU+SqGT8QJ3QCp8uvJbnaFsQ@mail.gmail.com>
Date: Wed, 26 Feb 2025 08:28:48 +0100
From: Daniel Vacek <neelx@...e.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: Easwar Hariharan <eahariha@...ux.microsoft.com>, 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, 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 Tue, 25 Feb 2025 at 22:10, Christophe JAILLET
<christophe.jaillet@...adoo.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@...lic.gmane.org>
> > Signed-off-by: Easwar Hariharan <eahariha-1pm0nblsJy7Jp67UH1NAhkEOCMrvLtNR@...lic.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);
> >
>
>
Powered by blists - more mailing lists