| Message ID | 1510680986-29391-1-git-send-email-geert@linux-m68k.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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 }
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(-)