| Message ID | 20160411174037.GA371@obsidianresearch.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Apr 11, 2016 at 11:40:37AM -0600, Jason Gunthorpe wrote: > On Mon, Apr 11, 2016 at 07:05:20PM +0300, Jarkko Sakkinen wrote: > > rmmod crashes the driver because tpm_chip_unregister() already sets ops > > to NULL. This commit fixes the issue by moving tpm2_shutdown() to > > tpm_chip_unregister(). This commit is also cleanup because it removes > > duplicate code from tpm_crb and tpm_tis to the core. > > > > 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 | 3 +++ > > drivers/char/tpm/tpm_crb.c | 3 --- > > drivers/char/tpm/tpm_tis.c | 3 --- > > 3 files changed, 3 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > > index f62c851..2642cca 100644 > > +++ b/drivers/char/tpm/tpm-chip.c > > @@ -361,6 +361,9 @@ void tpm_chip_unregister(struct tpm_chip *chip) > > if (!(chip->flags & TPM_CHIP_FLAG_REGISTERED)) > > return; > > > > + if (chip->flags & TPM_CHIP_FLAG_TPM2) > > + tpm2_shutdown(chip, TPM2_SU_CLEAR); > > + > > if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) > > sysfs_remove_link(&chip->dev.parent->kobj, "ppi"); > > This needs to be after ops is fenced, something like this. I would appreciate a supporting argument. I guess the argument here is that this will prevent user space from issuing TPM commands after the shutdown command has been sent? /Jarkko > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index 2642cca05cac..2ea2f1561e59 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -269,6 +269,8 @@ static void tpm_del_char_device(struct tpm_chip *chip) > > /* Make the driver uncallable. */ > down_write(&chip->ops_sem); > + if (chip->flags & TPM_CHIP_FLAG_TPM2) > + tpm2_shutdown(chip, TPM2_SU_CLEAR); > chip->ops = NULL; > up_write(&chip->ops_sem); > } > @@ -361,9 +363,6 @@ void tpm_chip_unregister(struct tpm_chip *chip) > if (!(chip->flags & TPM_CHIP_FLAG_REGISTERED)) > return; > > - if (chip->flags & TPM_CHIP_FLAG_TPM2) > - tpm2_shutdown(chip, TPM2_SU_CLEAR); > - > if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) > sysfs_remove_link(&chip->dev.parent->kobj, "ppi"); > ------------------------------------------------------------------------------ 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! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
On Tue, Apr 12, 2016 at 07:26:27AM +0300, Jarkko Sakkinen wrote: > > This needs to be after ops is fenced, something like this. > > I would appreciate a supporting argument. > > I guess the argument here is that this will prevent user space from > issuing TPM commands after the shutdown command has been sent? It prevents everything including the kernel from issuing a command after shutdown. The shutdown is sent as the last command and no other commands can follow it. It doesn't make any sense to allow commands to follow shutdown. Jason ------------------------------------------------------------------------------ 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! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
On Tue, Apr 12, 2016 at 11:26:55AM -0600, Jason Gunthorpe wrote: > On Tue, Apr 12, 2016 at 07:26:27AM +0300, Jarkko Sakkinen wrote: > > > This needs to be after ops is fenced, something like this. > > > > I would appreciate a supporting argument. > > > > I guess the argument here is that this will prevent user space from > > issuing TPM commands after the shutdown command has been sent? > > It prevents everything including the kernel from issuing a command ^^^^^^^^^^^^^^^^^^^^ That lock is not used to exclude kernel access. Read lock is only taken for the user space device in tpm-dev.c. > after shutdown. The shutdown is sent as the last command and no other > commands can follow it. > > It doesn't make any sense to allow commands to follow shutdown. I agree with this. > 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! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
On Wed, Apr 13, 2016 at 09:16:51AM +0300, Jarkko Sakkinen wrote: > On Tue, Apr 12, 2016 at 11:26:55AM -0600, Jason Gunthorpe wrote: > > On Tue, Apr 12, 2016 at 07:26:27AM +0300, Jarkko Sakkinen wrote: > > > > This needs to be after ops is fenced, something like this. > > > > > > I would appreciate a supporting argument. > > > > > > I guess the argument here is that this will prevent user space from > > > issuing TPM commands after the shutdown command has been sent? > > > > It prevents everything including the kernel from issuing a command > ^^^^^^^^^^^^^^^^^^^^ > > That lock is not used to exclude kernel access. Read lock is only taken > for the user space device in tpm-dev.c. My bad. For the in-kernel API it is taken through tpm_find_get(). I'll update the fix. /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! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
On Wed, Apr 13, 2016 at 09:58:05AM +0300, Jarkko Sakkinen wrote: > > > It prevents everything including the kernel from issuing a command > > ^^^^^^^^^^^^^^^^^^^^ > > > > That lock is not used to exclude kernel access. Read lock is only taken > > for the user space device in tpm-dev.c. > > My bad. For the in-kernel API it is taken through tpm_find_get(). Right. At this point any thing that can run in-kernel concurrently with the write exclusion is a kernel bug. Those places will need to grab the read side of the lock. Jason ------------------------------------------------------------------------------ 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! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index 2642cca05cac..2ea2f1561e59 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -269,6 +269,8 @@ static void tpm_del_char_device(struct tpm_chip *chip) /* Make the driver uncallable. */ down_write(&chip->ops_sem); + if (chip->flags & TPM_CHIP_FLAG_TPM2) + tpm2_shutdown(chip, TPM2_SU_CLEAR); chip->ops = NULL; up_write(&chip->ops_sem); } @@ -361,9 +363,6 @@ void tpm_chip_unregister(struct tpm_chip *chip) if (!(chip->flags & TPM_CHIP_FLAG_REGISTERED)) return; - if (chip->flags & TPM_CHIP_FLAG_TPM2) - tpm2_shutdown(chip, TPM2_SU_CLEAR); - if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) sysfs_remove_link(&chip->dev.parent->kobj, "ppi");