msleep() bug causes Nuvoton I2C TPM device driver delays

Need a description and methods to recreate.

Please provide details for this issue

------- Comment From <email address hidden> 2017-02-24 15:35 EDT-------
From <email address hidden> Fri Feb 24 11:30:55 2017
From: Mimi Zohar <email address hidden>
To: Jarkko Sakkinen <email address hidden>
Date: Fri, 24 Feb 2017 12:29:02 -0500
Message-Id: <email address hidden>
Cc: Morav <email address hidden>, <email address hidden>,
<email address hidden>, linux-ima-devel
<email address hidden>, linux-security-module
<email address hidden>, tpmdd-devel
<email address hidden>, linux-fsdevel
<email address hidden>, Gleixner <email address hidden>
Subject: Re: [tpmdd-devel] [RFC PATCH] tpm: msleep() delays - replace with
usleep_range() in i2c nuvoton driver

On Fri, 2017-02-24 at 19:01 +0200, Jarkko Sakkinen wrote:
> On Thu, Feb 23, 2017 at 06:46:18PM -0500, Mimi Zohar wrote:
> > Commit 500462a9de65 "timers: Switch to a non-cascading wheel" replaced
> > the 'classic' timer wheel, which aimed for near 'exact' expiry of the
> > timers. Their analysis was that the vast majority of timeout timers
> > are used as safeguards, not as real timers, and are cancelled or
> > rearmed before expiration. The only exception noted to this were
> > networking timers with a small expiry time.
> >
> > Not included in the analysis was the TPM polling timer, which resulted
> > in a longer normal delay and, every so often, a very long delay. The
> > non-cascading wheel delay is based on CONFIG_HZ. For a description of
> > the different rings and their delays, refer to the comments in
> > kernel/time/timer.c.
> >
> > Below are the delays given for rings 0 - 2, which explains the longer
> > "normal" delays and the very, long delays as seen on systems with
> > CONFIG_HZ 250.
> >
> > * HZ 1000 steps
> > * Level Offset Granularity Range
> > * 0 0 1 ms 0 ms - 63 ms
> > * 1 64 8 ms 64 ms - 511 ms
> > * 2 128 64 ms 512 ms - 4095 ms (512ms - ~4s)
> >
> > * HZ 250
> > * Level Offset Granularity Range
> > * 0 0 4 ms 0 ms - 255 ms
> > * 1 64 32 ms 256 ms - 2047 ms (256ms - ~2s)
> > * 2 128 256 ms 2048 ms - 16383 ms (~2s - ~16s)
> >
> > Below is a comparison of extending the TPM with 1000 measurements,
> > using msleep() vs. usleep_delay() when configured for 1000 hz vs. 250
> > hz, before and after commit 500462a9de65.
> >
> > linux-4.7 | msleep() usleep_range()
> > 1000 hz: 0m44.628s | 1m34.497s 29.243s
> > 250 hz: 1m28.510s | 4m49.269s 32.386s
> >
> > linux-4.7 | min-max (msleep) min-max (usleep_range)
> > 1000 hz: 0:017 - 2:760s | 0:015 - 3:967s 0:014 - 0:418s
> > 250 hz: 0:028 - 1:954s | 0:040 - 4:096s 0:016 - 0:816s
> >
> > This patch replaces the msleep() with usleep_range() calls in the
> > i2c nuvoton driver with a consistent max range value.
> >
> > Signed-of-by: Mimi Zohar <email address hidden>
> > Reviewed-by: Nayna Jain <email address hidden>
>
> So why doesn't it go to level 0 with msleep()? I quickly skimme...

"This patch replaces the msleep() with usleep_range() calls in the i2c nuvoton driver with a consistent max range value."

Is there a patch (?to checksecurity?) attached that bugproxy yet has to mirror?
Could you as a failsafe point a link to the upstream change?

Those changes are on the tpm mailing list for the Kernel portion of tpm.
drivers/char/tpm/tpm_i2c_nuvoton.c is a kernel file.

Did you mean the kernel patch behind https://sourceforge.net/p/tpmdd/mailman/message/35686697/ ?
As far as I can see that is not even through the tpm community and due to that not upstream either.

Sorry, but the few bits this bug force us to guess, so for now I'll add the linux-kernel for awareness but mark both incomplete.

------- Comment From <email address hidden> 2017-03-15 10:32 EDT-------
The original patch is in Jarkko's master branch, but will be replaced with a newer version, posted by Nayna, that addresses a few minor review comments.

git://git.infradead.org/users/jjs/linux-tpmdd.git:
109e5b554dc3 tpm: msleep() delays - replace with usleep_range() in i2c nuvoton driver (original)

In addition, Nayna posted an additional patch to further improve performance.
[PATCH 2/2] tpm: add sleep only for retry in i2c_nuvoton_write_status()

Both of these patches should be staged shortly by Jarkko:
https://www.spinics.net/lists/stable/msg162931.html
https://www.spinics.net/lists/stable/msg162461.html

Mimi

------- Comment (attachment only) From <email address hidden> 2017-03-15 17:13 EDT-------

------- Comment (attachment only) From <email address hidden> 2017-03-15 17:14 EDT-------

------- Comment From <email address hidden> 2017-03-20 01:51 EDT-------
Both the patches are applied by Jarkko and are available in Jarkko's master branch.

https://www.spinics.net/lists/stable/msg163302.html
https://www.spinics.net/lists/stable/msg163664.html

Thanks & Regards,
- Nayna

IBM, is there a problem with checksecurity that needs to be addressed? Patches? Or was checksecurity set in error for this bug report?

------- Comment From <email address hidden> 2017-03-21 14:13 EDT-------
(In reply to comment #19)
> IBM, is there a problem with checksecurity that needs to be addressed?
> Patches? Or was checksecurity set in error for this bug report?

George opened this bugzilla on my behalf, but is out. I have no idea what "checksecurity" is. This is not a "security" bug per-se, but a driver timing problem.

Marking the checksecurity component as invalid. If there is an issue here that needs to be addressed, add an appropriate description and reopen this bug.

This bug was fixed in the package linux - 4.10.0-14.16

---------------
linux (4.10.0-14.16) zesty; urgency=low

  [ Tim Gardner ]

  * Release Tracking Bug
    - LP: #1673805

  * msleep() bug causes Nuvoton I2C TPM device driver delays (LP: #1667567)
    - tpm: msleep() delays - replace with usleep_range() in i2c nuvoton driver
    - SAUCE: tpm: add sleep only for retry in i2c_nuvoton_write_status()

  * C++ demangling support missing from perf (LP: #1396654)
    - [Config] added binutils-dev to Build-deps

  * dm-queue-length module is not included in installer/initramfs (LP: #1673350)
    - [Config] d-i: Also add dm-queue-length to multipath modules

  * move aufs.ko from -extra to linux-image package (LP: #1673498)
    - [config] aufs.ko moved to linux-image package

  * Using an NVMe drive causes huge power drain (LP: #1664602)
    - nvme: Add a quirk mechanism that uses identify_ctrl
    - nvme: Enable autonomous power state transitions

  * Broadcom bluetooth modules sometimes fail to initialize (LP: #1483101)
    - Bluetooth: btbcm: Add a delay for module reset

  * Need support of Broadcom bluetooth device [413c:8143] (LP: #1166113)
    - Bluetooth: btusb: Add support for 413c:8143

  * Zesty update to v4.10.3 stable release (LP: #1673118)
    - serial: 8250_pci: Add MKS Tenta SCOM-0800 and SCOM-0801 cards
    - KVM: s390: Disable dirty log retrieval for UCONTROL guests
    - KVM: VMX: use correct vmcs_read/write for guest segment selector/base
    - Bluetooth: Add another AR3012 04ca:3018 device
    - phy: qcom-ufs: Don't kfree devres resource
    - phy: qcom-ufs: Fix misplaced jump label
    - s390/qdio: clear DSCI prior to scanning multiple input queues
    - s390/dcssblk: fix device size calculation in dcssblk_direct_access()
    - s390/kdump: Use "LINUX" ELF note name instead of "CORE"
    - s390/chsc: Add exception handler for CHSC instruction
    - s390: TASK_SIZE for kernel threads
    - s390/topology: correct allocation of topology information
    - s390: make setup_randomness work
    - s390: use correct input data address for setup_randomness
    - net: mvpp2: fix DMA address calculation in mvpp2_txq_inc_put()
    - cxl: Prevent read/write to AFU config space while AFU not configured
    - cxl: fix nested locking hang during EEH hotplug
    - brcmfmac: fix incorrect event channel deduction
    - mnt: Tuck mounts under others instead of creating shadow/side mounts.
    - IB/ipoib: Fix deadlock between rmmod and set_mode
    - IB/IPoIB: Add destination address when re-queue packet
    - IB/mlx5: Fix out-of-bound access
    - IB/SRP: Avoid using IB_MR_TYPE_SG_GAPS
    - IB/srp: Avoid that duplicate responses trigger a kernel bug
    - IB/srp: Fix race conditions related to task management
    - Btrfs: fix data loss after truncate when using the no-holes feature
    - orangefs: Use RCU for destroy_inode
    - memory/atmel-ebi: Fix ns <-> cycles conversions
    - tracing: Fix return value check in trace_benchmark_reg()
    - ktest: Fix child exit code processing
    - ceph: remove req from unsafe list when unregistering it
    - target: Fix NULL dereference during LUN lookup + active I/O shutdown
    - drivers/pci/hotplug: Han...

------- Comment From <email address hidden> 2019-01-08 17:54 EDT-------
My impression from comment 22 is that this is ready to verify.