| Message ID | 1460033770-20586-1-git-send-email-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
------------------------------------------------------------------------------
On Thu, Apr 07, 2016 at 07:36:54AM -0700, Jason Gunthorpe wrote: > I will have to look closer after the conference, but this does not look > right. > > I vaguely recall commenting on this before. Move the shutdown into the > core code to fix it. This fix that I sent is not the right way to do it. One example scenario: 1. TIS driver gets detached, which causes tpm_tis_remove() to be called. 2. Some in-kernel subsystem uses TPM, which should not be done since the hardware is already unitialized. 3. The devres subsystem sets ops to NULL. Even though the fix is wrong I feel that it might put the rwsem into question. I'm just thinking that maybe there could be a release callback in tpm_class_ops that could be called by tpm_del_char_device(). There can't be clients for the chip at that point so no synchronization mechanism is needed. > Jason /Jarkko ------------------------------------------------------------------------------ Find and fix application performance issues faster with Applications Manager Applications Manager provides deep performance insights into multiple tiers of your business applications. It resolves application problems quickly and reduces your MTTR. Get your free trial! http://pubads.g.doubleclick.net/ gampad/clk?id=1444514301&iu=/ca-pub-7940484522588532
On Mon, Apr 11, 2016 at 11:41:24AM +0300, Jarkko Sakkinen wrote: > On Thu, Apr 07, 2016 at 07:36:54AM -0700, Jason Gunthorpe wrote: > > I will have to look closer after the conference, but this does not look > > right. > > > > I vaguely recall commenting on this before. Move the shutdown into the > > core code to fix it. > > This fix that I sent is not the right way to do it. > > One example scenario: > > 1. TIS driver gets detached, which causes tpm_tis_remove() to be called. > 2. Some in-kernel subsystem uses TPM, which should not be done since the > hardware is already unitialized. > 3. The devres subsystem sets ops to NULL. > > Even though the fix is wrong I feel that it might put the rwsem into > question. > > I'm just thinking that maybe there could be a release callback in > tpm_class_ops that could be called by tpm_del_char_device(). There can't > be clients for the chip at that point so no synchronization mechanism > is needed. As a fix for this regression moving shutdown to tmp_chip_unregister() does make more sense since the patch is already merged to next. Lets not get stuck into locking discussion... /Jarkko ------------------------------------------------------------------------------ Find and fix application performance issues faster with Applications Manager Applications Manager provides deep performance insights into multiple tiers of your business applications. It resolves application problems quickly and reduces your MTTR. Get your free trial! http://pubads.g.doubleclick.net/ gampad/clk?id=1444514301&iu=/ca-pub-7940484522588532
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index f62c851..5241bc4 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -127,6 +127,11 @@ static void tpm_dev_release(struct device *dev) idr_remove(&dev_nums_idr, chip->dev_num); mutex_unlock(&idr_lock); + /* Make the driver uncallable. */ + down_write(&chip->ops_sem); + chip->ops = NULL; + up_write(&chip->ops_sem); + kfree(chip); } @@ -266,11 +271,6 @@ static void tpm_del_char_device(struct tpm_chip *chip) mutex_lock(&idr_lock); idr_replace(&dev_nums_idr, NULL, chip->dev_num); mutex_unlock(&idr_lock); - - /* Make the driver uncallable. */ - down_write(&chip->ops_sem); - chip->ops = NULL; - up_write(&chip->ops_sem); } static int tpm1_chip_register(struct tpm_chip *chip)
rmmod crashes the driver because tpm_chip_unregister() already sets ops to NULL. Release ops in tpm_dev_release() so that tpm2_shutdown() can be cleanly executed and also because it is symmetrical where they are allocated (in tpmm_chip_alloc()). Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Fixes: 4d3eac5e156a ("tpm: Provide strong locking for device removal") --- drivers/char/tpm/tpm-chip.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)