[PATCH] [RFC] time: Make sure jiffies_to_msecs() preserves non-zero time periods

[PATCH] [RFC] time: Make sure jiffies_to_msecs() preserves non-zero time periods

[Geert Uytterhoeven]

For the common cases where 1000 is a multiple of HZ, or HZ is a multiple
of 1000, jiffies_to_msecs() never returns zero when passed a non-zero
time period.

However, if HZ > 1000 and not an integer multiple of 1000 (e.g. 2001),
jiffies_to_msecs() may return zero for small non-zero time periods.
This may break code that relies on receiving back a non-zero value, e.g.
drivers/char/tpm/tpm2-cmd.c:tpm2_do_selftest().

jiffies_to_usecs() does not need such a fix, as <linux/jiffies.h> does
not support values of HZ larger than 12287, thus rejecting any
problematic huge values of HZ.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
I noticed this issue due to the following compiler warning with
gcc-4.1.2:

    drivers/char/tpm/tpm2-cmd.c: In function ‘tpm2_do_selftest’:
    drivers/char/tpm/tpm2-cmd.c:851: warning: ‘rc’ may be used uninitialized in this function

With the fix above, this becomes a false positive.
Nevertheless, it may be a good idea to preinitialize rc anyway, but I
have no idea what's the correct value (else I would have sent a patch
to do so ;-).

Fixes: 87434f58be31a96d ("tpm: Use dynamic delay to wait for TPM 2.0 self test result")
---
 kernel/time/time.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/time/time.c b/kernel/time/time.c
index bd4e6c7dd6899d83..69b901a8739aad5e 100644
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -314,9 +314,10 @@ unsigned int jiffies_to_msecs(const unsigned long j)
 	return (j + (HZ / MSEC_PER_SEC) - 1)/(HZ / MSEC_PER_SEC);
 #else
 # if BITS_PER_LONG == 32
-	return (HZ_TO_MSEC_MUL32 * j) >> HZ_TO_MSEC_SHR32;
+	return (HZ_TO_MSEC_MUL32 * j + (1ULL << HZ_TO_MSEC_SHR32) - 1) >>
+	       HZ_TO_MSEC_SHR32;
 # else
-	return (j * HZ_TO_MSEC_NUM) / HZ_TO_MSEC_DEN;
+	return (j * HZ_TO_MSEC_NUM + HZ_TO_MSEC_DEN - 1) / HZ_TO_MSEC_DEN;
 # endif
 #endif
 }
-- 
2.7.4

Re: [PATCH] [RFC] time: Make sure jiffies_to_msecs() preserves non-zero time periods

[Arnd Bergmann]

On Tue, Nov 14, 2017 at 6:36 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> For the common cases where 1000 is a multiple of HZ, or HZ is a multiple
> of 1000, jiffies_to_msecs() never returns zero when passed a non-zero
> time period.
>
> However, if HZ > 1000 and not an integer multiple of 1000 (e.g. 2001),
> jiffies_to_msecs() may return zero for small non-zero time periods.
> This may break code that relies on receiving back a non-zero value, e.g.
> drivers/char/tpm/tpm2-cmd.c:tpm2_do_selftest().

For reference, there are exactly three platforms that allow HZ to be larger
than 1000, and they specifically use HZ=1024: alpha, itanium and
mips/decstation.

> With the fix above, this becomes a false positive.
> Nevertheless, it may be a good idea to preinitialize rc anyway, but I
> have no idea what's the correct value (else I would have sent a patch
> to do so ;-).

I think changing the while() loop into do{}while() would be appropriate here.

      Arnd

Re: [PATCH] [RFC] time: Make sure jiffies_to_msecs() preserves non-zero time periods

[Geert Uytterhoeven]

Hi Arnd,

On Tue, Nov 14, 2017 at 9:18 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Nov 14, 2017 at 6:36 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> For the common cases where 1000 is a multiple of HZ, or HZ is a multiple
>> of 1000, jiffies_to_msecs() never returns zero when passed a non-zero
>> time period.
>>
>> However, if HZ > 1000 and not an integer multiple of 1000 (e.g. 2001),
>> jiffies_to_msecs() may return zero for small non-zero time periods.
>> This may break code that relies on receiving back a non-zero value, e.g.
>> drivers/char/tpm/tpm2-cmd.c:tpm2_do_selftest().
>
> For reference, there are exactly three platforms that allow HZ to be larger
> than 1000, and they specifically use HZ=1024: alpha, itanium and
> mips/decstation.

jiffies_to_msecs(1) = zero if HZ=1024.

>> With the fix above, this becomes a false positive.
>> Nevertheless, it may be a good idea to preinitialize rc anyway, but I
>> have no idea what's the correct value (else I would have sent a patch
>> to do so ;-).
>
> I think changing the while() loop into do{}while() would be appropriate here.

Sounds reasonable.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-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

Re: [PATCH] [RFC] time: Make sure jiffies_to_msecs() preserves non-zero time periods

[Jarkko Sakkinen]

On Tue, Nov 14, 2017 at 06:36:26PM +0100, Geert Uytterhoeven wrote:
> For the common cases where 1000 is a multiple of HZ, or HZ is a multiple
> of 1000, jiffies_to_msecs() never returns zero when passed a non-zero
> time period.
> 
> However, if HZ > 1000 and not an integer multiple of 1000 (e.g. 2001),
> jiffies_to_msecs() may return zero for small non-zero time periods.
> This may break code that relies on receiving back a non-zero value, e.g.
> drivers/char/tpm/tpm2-cmd.c:tpm2_do_selftest().
> 
> jiffies_to_usecs() does not need such a fix, as <linux/jiffies.h> does
> not support values of HZ larger than 12287, thus rejecting any
> problematic huge values of HZ.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> I noticed this issue due to the following compiler warning with
> gcc-4.1.2:
> 
>     drivers/char/tpm/tpm2-cmd.c: In function ‘tpm2_do_selftest’:
>     drivers/char/tpm/tpm2-cmd.c:851: warning: ‘rc’ may be used uninitialized in this function
> 
> With the fix above, this becomes a false positive.
> Nevertheless, it may be a good idea to preinitialize rc anyway, but I
> have no idea what's the correct value (else I would have sent a patch
> to do so ;-).
> 
> Fixes: 87434f58be31a96d ("tpm: Use dynamic delay to wait for TPM 2.0 self test result")

I would consider the above fix just to masks the regression in the TPM
driver. The code in the TPM subsystem is incorrect by making such an
assumption and should be fixed there.

Arnd's suggestion is what I would do. Are you willing to contribute the
fix? If the answer is yes, please add suggested-by tag for Arnd. Thank
you for spotting this.

/Jarkko