[tpmdd-devel,v4,03/10] tpm: Provide strong locking for device removal
diff mbox

Message ID 1456766996-9300-4-git-send-email-stefanb@linux.vnet.ibm.com
State New
Headers show

Commit Message

Stefan Berger Feb. 29, 2016, 5:29 p.m. UTC
From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Add a read/write semaphore around the ops function pointers so
ops can be set to null when the driver un-registers.

Previously the tpm core expected module locking to be enough to
ensure that tpm_unregister could not be called during certain times,
however that hasn't been sufficient for a long time.

Introduce a read/write semaphore around 'ops' so the core can set
it to null when unregistering. This provides a strong fence around
the driver callbacks, guaranteeing to the driver that no callbacks
are running or will run again.

For now the ops_lock is placed very high in the call stack, it could
be pushed down and made more granular in future if necessary.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm-chip.c      | 72 ++++++++++++++++++++++++++++++++++++----
 drivers/char/tpm/tpm-dev.c       | 11 +++++-
 drivers/char/tpm/tpm-interface.c | 19 ++++++-----
 drivers/char/tpm/tpm-sysfs.c     |  5 +++
 drivers/char/tpm/tpm.h           | 14 +++++---
 5 files changed, 100 insertions(+), 21 deletions(-)

Comments

Jason Gunthorpe Feb. 29, 2016, 7:25 p.m. UTC | #1
On Mon, Feb 29, 2016 at 12:29:49PM -0500, Stefan Berger wrote:
>   * the device. As the last step this function adds the chip to the list of TPM
>   * chips available for in-kernel use.
>   *
> + * Once this function returns the driver call backs in 'op's will not be
> + * running and will no longer start.
> + *
>   * This function should be only called after the chip initialization is
>   * complete.
>   */

Not sure what happened here, but this hunk belongs with
tpm_chip_unregister

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Stefan Berger Feb. 29, 2016, 7:48 p.m. UTC | #2
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/29/2016 
02:25:32 PM:


> 
> On Mon, Feb 29, 2016 at 12:29:49PM -0500, Stefan Berger wrote:
> >   * the device. As the last step this function adds the chip to 
> the list of TPM
> >   * chips available for in-kernel use.
> >   *
> > + * Once this function returns the driver call backs in 'op's will not 
be
> > + * running and will no longer start.
> > + *
> >   * This function should be only called after the chip initialization 
is
> >   * complete.
> >   */
> 
> Not sure what happened here, but this hunk belongs with
> tpm_chip_unregister

That's where it is. The context 'tpm1_chip_unregister' the hunk shows is 
from the function 'above'.

   Stefan
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Jason Gunthorpe Feb. 29, 2016, 7:54 p.m. UTC | #3
On Mon, Feb 29, 2016 at 02:48:54PM -0500, Stefan Berger wrote:
>    Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/29/2016
>    02:25:32 PM:
>    >
>    > On Mon, Feb 29, 2016 at 12:29:49PM -0500, Stefan Berger wrote:
>    > >   * the device. As the last step this function adds the chip to
>    > the list of TPM
>    > >   * chips available for in-kernel use.
>    > >   *
>    > > + * Once this function returns the driver call backs in 'op's will
>    not be
>    > > + * running and will no longer start.
>    > > + *
>    > >   * This function should be only called after the chip
>    initialization is
>    > >   * complete.
>    > >   */
>    >
>    > Not sure what happened here, but this hunk belongs with
>    > tpm_chip_unregister
>    That's where it is. The context 'tpm1_chip_unregister' the hunk shows
>    is from the function 'above'.

Eh?

https://github.com/stefanberger/linux/blob/63f0cd1bae930167d31bbe5d5779d40a277e91c5/drivers/char/tpm/tpm-chip.c#L311

/*
 * tpm_chip_register() - create a character device for the TPM chip
 * @chip: TPM chip to use.
 *
 * Creates a character device for the TPM chip and adds sysfs attributes for
 * the device. As the last step this function adds the chip to the list of TPM
 * chips available for in-kernel use.
 *
 * Once this function returns the driver call backs in 'op's will not be
 * running and will no longer start.
 *
 * This function should be only called after the chip initialization is
 * complete.
 */
int tpm_chip_register(struct tpm_chip *chip)
{

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Stefan Berger Feb. 29, 2016, 8:04 p.m. UTC | #4
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/29/2016 
02:54:01 PM:

> 
> On Mon, Feb 29, 2016 at 02:48:54PM -0500, Stefan Berger wrote:
> >    Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 
02/29/2016
> >    02:25:32 PM:
> >    >
> >    > On Mon, Feb 29, 2016 at 12:29:49PM -0500, Stefan Berger wrote:
> >    > >   * the device. As the last step this function adds the chip to
> >    > the list of TPM
> >    > >   * chips available for in-kernel use.
> >    > >   *
> >    > > + * Once this function returns the driver call backs in 'op's 
will
> >    not be
> >    > > + * running and will no longer start.
> >    > > + *
> >    > >   * This function should be only called after the chip
> >    initialization is
> >    > >   * complete.
> >    > >   */
> >    >
> >    > Not sure what happened here, but this hunk belongs with
> >    > tpm_chip_unregister
> >    That's where it is. The context 'tpm1_chip_unregister' the hunk 
shows
> >    is from the function 'above'.
> 
> Eh?
> 
> https://github.com/stefanberger/linux/blob/
> 
63f0cd1bae930167d31bbe5d5779d40a277e91c5/drivers/char/tpm/tpm-chip.c#L311


Sorry. Fixed here : 
https://github.com/stefanberger/linux/commit/5808cd0b01b00e1a4e3b23d6970bac4efee69038

   Stefan
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Jarkko Sakkinen March 4, 2016, 6:10 p.m. UTC | #5
On Mon, Feb 29, 2016 at 03:04:10PM -0500, Stefan Berger wrote:
>    Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/29/2016
>    02:54:01 PM:
> 
>    >
>    > On Mon, Feb 29, 2016 at 02:48:54PM -0500, Stefan Berger wrote:
>    > >    Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on
>    02/29/2016
>    > >    02:25:32 PM:
>    > >    >
>    > >    > On Mon, Feb 29, 2016 at 12:29:49PM -0500, Stefan Berger wrote:
>    > >    > >   * the device. As the last step this function adds the chip to
>    > >    > the list of TPM
>    > >    > >   * chips available for in-kernel use.
>    > >    > >   *
>    > >    > > + * Once this function returns the driver call backs in 'op's
>    will
>    > >    not be
>    > >    > > + * running and will no longer start.
>    > >    > > + *
>    > >    > >   * This function should be only called after the chip
>    > >    initialization is
>    > >    > >   * complete.
>    > >    > >   */
>    > >    >
>    > >    > Not sure what happened here, but this hunk belongs with
>    > >    > tpm_chip_unregister
>    > >    That's where it is. The context 'tpm1_chip_unregister' the hunk
>    shows
>    > >    is from the function 'above'.
>    >
>    > Eh?
>    >
>    > https://github.com/stefanberger/linux/blob/
>    >
>    63f0cd1bae930167d31bbe5d5779d40a277e91c5/drivers/char/tpm/tpm-chip.c#L311
> 
>    Sorry. Fixed here :
>    https://github.com/stefanberger/linux/commit/5808cd0b01b00e1a4e3b23d6970bac4efee69038

Already applied v4. Could you send a fix for this to the list?
Thanks.

>       Stefan

/Jarkko

------------------------------------------------------------------------------
Stefan Berger March 4, 2016, 7:06 p.m. UTC | #6
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 03/04/2016 
01:10:38 PM:

> 
> On Mon, Feb 29, 2016 at 03:04:10PM -0500, Stefan Berger wrote:
> >    Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 
02/29/2016
> >    02:54:01 PM:
> > 
> >    >
> >    > On Mon, Feb 29, 2016 at 02:48:54PM -0500, Stefan Berger wrote:
> >    > >    Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on
> >    02/29/2016
> >    > >    02:25:32 PM:
> >    > >    >
> >    > >    > On Mon, Feb 29, 2016 at 12:29:49PM -0500, Stefan Berger 
wrote:
> >    > >    > >   * the device. As the last step this function adds 
> the chip to
> >    > >    > the list of TPM
> >    > >    > >   * chips available for in-kernel use.
> >    > >    > >   *
> >    > >    > > + * Once this function returns the driver call backs in 
'op's
> >    will
> >    > >    not be
> >    > >    > > + * running and will no longer start.
> >    > >    > > + *
> >    > >    > >   * This function should be only called after the chip
> >    > >    initialization is
> >    > >    > >   * complete.
> >    > >    > >   */
> >    > >    >
> >    > >    > Not sure what happened here, but this hunk belongs with
> >    > >    > tpm_chip_unregister
> >    > >    That's where it is. The context 'tpm1_chip_unregister' the 
hunk
> >    shows
> >    > >    is from the function 'above'.
> >    >
> >    > Eh?
> >    >
> >    > https://github.com/stefanberger/linux/blob/
> >    >
> > 
63f0cd1bae930167d31bbe5d5779d40a277e91c5/drivers/char/tpm/tpm-chip.c#L311
> > 
> >    Sorry. Fixed here :
> >    https://github.com/stefanberger/linux/commit/
> 5808cd0b01b00e1a4e3b23d6970bac4efee69038
> 
> Already applied v4. Could you send a fix for this to the list?
> Thanks.

I can post v5 that also has the Reviewed-by's applied.

https://github.com/stefanberger/linux/commits/vtpm-driver.v5

  Stefan


> 
> >       Stefan
> 
> /Jarkko
>
------------------------------------------------------------------------------
Jarkko Sakkinen March 4, 2016, 9:27 p.m. UTC | #7
On Fri, Mar 04, 2016 at 02:06:23PM -0500, Stefan Berger wrote:
>    Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 03/04/2016
>    01:10:38 PM:
>    >
>    > On Mon, Feb 29, 2016 at 03:04:10PM -0500, Stefan Berger wrote:
>    > >    Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on
>    02/29/2016
>    > >    02:54:01 PM:
>    > >
>    > >    >
>    > >    > On Mon, Feb 29, 2016 at 02:48:54PM -0500, Stefan Berger wrote:
>    > >    > >    Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on
>    > >    02/29/2016
>    > >    > >    02:25:32 PM:
>    > >    > >    >
>    > >    > >    > On Mon, Feb 29, 2016 at 12:29:49PM -0500, Stefan Berger
>    wrote:
>    > >    > >    > >   * the device. As the last step this function adds
>    > the chip to
>    > >    > >    > the list of TPM
>    > >    > >    > >   * chips available for in-kernel use.
>    > >    > >    > >   *
>    > >    > >    > > + * Once this function returns the driver call backs in
>    'op's
>    > >    will
>    > >    > >    not be
>    > >    > >    > > + * running and will no longer start.
>    > >    > >    > > + *
>    > >    > >    > >   * This function should be only called after the chip
>    > >    > >    initialization is
>    > >    > >    > >   * complete.
>    > >    > >    > >   */
>    > >    > >    >
>    > >    > >    > Not sure what happened here, but this hunk belongs with
>    > >    > >    > tpm_chip_unregister
>    > >    > >    That's where it is. The context 'tpm1_chip_unregister' the
>    hunk
>    > >    shows
>    > >    > >    is from the function 'above'.
>    > >    >
>    > >    > Eh?
>    > >    >
>    > >    > https://github.com/stefanberger/linux/blob/
>    > >    >
>    > >  
>     63f0cd1bae930167d31bbe5d5779d40a277e91c5/drivers/char/tpm/tpm-chip.c#L311
>    > >
>    > >    Sorry. Fixed here :
>    > >    https://github.com/stefanberger/linux/commit/
>    > 5808cd0b01b00e1a4e3b23d6970bac4efee69038
>    >
>    > Already applied v4. Could you send a fix for this to the list?
>    > Thanks.
> 
>    I can post v5 that also has the Reviewed-by's applied.
> 
>    https://github.com/stefanberger/linux/commits/vtpm-driver.v5

That works too. I then just replace patches in my master with new ones.
BTW, before your post. Could you check my comment about the driver name.
I think it would be better if the name was tpm_vtpm for consistency
reasons.

>      Stefan

/Jarkko

------------------------------------------------------------------------------
Stefan Berger March 4, 2016, 10:01 p.m. UTC | #8
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 03/04/2016 
04:27:16 PM:

> On Fri, Mar 04, 2016 at 02:06:23PM -0500, Stefan Berger wrote:
> >    Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 
03/04/2016
> >    01:10:38 PM:
> >    >
> >    > On Mon, Feb 29, 2016 at 03:04:10PM -0500, Stefan Berger wrote:
> >    > >    Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on
> >    02/29/2016
> >    > >    02:54:01 PM:
> >    > >
> >    > >    >
> >    > >    > On Mon, Feb 29, 2016 at 02:48:54PM -0500, Stefan Berger 
wrote:
> >    > >    > >    Jason Gunthorpe <jgunthorpe@obsidianresearch.com> 
wrote on
> >    > >    02/29/2016
> >    > >    > >    02:25:32 PM:
> >    > >    > >    >
> >    > >    > >    > On Mon, Feb 29, 2016 at 12:29:49PM -0500, Stefan 
Berger
> >    wrote:
> >    > >    > >    > >   * the device. As the last step this function 
adds
> >    > the chip to
> >    > >    > >    > the list of TPM
> >    > >    > >    > >   * chips available for in-kernel use.
> >    > >    > >    > >   *
> >    > >    > >    > > + * Once this function returns the driver call 
backs in
> >    'op's
> >    > >    will
> >    > >    > >    not be
> >    > >    > >    > > + * running and will no longer start.
> >    > >    > >    > > + *
> >    > >    > >    > >   * This function should be only called after the 
chip
> >    > >    > >    initialization is
> >    > >    > >    > >   * complete.
> >    > >    > >    > >   */
> >    > >    > >    >
> >    > >    > >    > Not sure what happened here, but this hunk belongs 
with
> >    > >    > >    > tpm_chip_unregister
> >    > >    > >    That's where it is. The context 
'tpm1_chip_unregister' the
> >    hunk
> >    > >    shows
> >    > >    > >    is from the function 'above'.
> >    > >    >
> >    > >    > Eh?
> >    > >    >
> >    > >    > https://github.com/stefanberger/linux/blob/
> >    > >    >
> >    > > 
> >     63f0cd1bae930167d31bbe5d5779d40a277e91c5/drivers/char/tpm/tpm-
> chip.c#L311
> >    > >
> >    > >    Sorry. Fixed here :
> >    > >    https://github.com/stefanberger/linux/commit/
> >    > 5808cd0b01b00e1a4e3b23d6970bac4efee69038
> >    >
> >    > Already applied v4. Could you send a fix for this to the list?
> >    > Thanks.
> > 
> >    I can post v5 that also has the Reviewed-by's applied.
> > 
> >    https://github.com/stefanberger/linux/commits/vtpm-driver.v5
> 
> That works too. I then just replace patches in my master with new ones.
> BTW, before your post. Could you check my comment about the driver name.
> I think it would be better if the name was tpm_vtpm for consistency
> reasons.
> 

I renamed the driver file to tpm_vtpm.c . I tested it a bit and it's 
available here now:

https://github.com/stefanberger/linux/commits/vtpm-driver.v5


   Stefan
------------------------------------------------------------------------------

Patch
diff mbox

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index c21d81c..1ae30f2 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -36,10 +36,60 @@  static DEFINE_SPINLOCK(driver_lock);
 struct class *tpm_class;
 dev_t tpm_devt;
 
-/*
- * tpm_chip_find_get - return tpm_chip for a given chip number
- * @chip_num the device number for the chip
+/**
+ * tpm_try_get_ops() - Get a ref to the tpm_chip
+ * @chip: Chip to ref
+ *
+ * The caller must already have some kind of locking to ensure that chip is
+ * valid. This function will lock the chip so that the ops member can be
+ * accessed safely. The locking prevents tpm_chip_unregister from
+ * completing, so it should not be held for long periods.
+ *
+ * Returns -ERRNO if the chip could not be got.
  */
+int tpm_try_get_ops(struct tpm_chip *chip)
+{
+	int rc = -EIO;
+
+	get_device(&chip->dev);
+
+	down_read(&chip->ops_sem);
+	if (!chip->ops)
+		goto out_lock;
+
+	if (!try_module_get(chip->dev.parent->driver->owner))
+		goto out_lock;
+
+	return 0;
+out_lock:
+	up_read(&chip->ops_sem);
+	put_device(&chip->dev);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(tpm_try_get_ops);
+
+/**
+ * tpm_put_ops() - Release a ref to the tpm_chip
+ * @chip: Chip to put
+ *
+ * This is the opposite pair to tpm_try_get_ops(). After this returns chip may
+ * be kfree'd.
+ */
+void tpm_put_ops(struct tpm_chip *chip)
+{
+	module_put(chip->dev.parent->driver->owner);
+	up_read(&chip->ops_sem);
+	put_device(&chip->dev);
+}
+EXPORT_SYMBOL_GPL(tpm_put_ops);
+
+/**
+ * tpm_chip_find_get() - return tpm_chip for a given chip number
+ * @chip_num: id to find
+ *
+ * The return'd chip has been tpm_try_get_ops'd and must be released via
+ * tpm_put_ops
+  */
 struct tpm_chip *tpm_chip_find_get(int chip_num)
 {
 	struct tpm_chip *pos, *chip = NULL;
@@ -49,10 +99,10 @@  struct tpm_chip *tpm_chip_find_get(int chip_num)
 		if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
 			continue;
 
-		if (try_module_get(pos->dev.parent->driver->owner)) {
+		/* rcu prevents chip from being free'd */
+		if (!tpm_try_get_ops(pos))
 			chip = pos;
-			break;
-		}
+		break;
 	}
 	rcu_read_unlock();
 	return chip;
@@ -95,6 +145,7 @@  struct tpm_chip *tpmm_chip_alloc(struct device *dev,
 		return ERR_PTR(-ENOMEM);
 
 	mutex_init(&chip->tpm_mutex);
+	init_rwsem(&chip->ops_sem);
 	INIT_LIST_HEAD(&chip->list);
 
 	chip->ops = ops;
@@ -180,6 +231,12 @@  static int tpm_add_char_device(struct tpm_chip *chip)
 static void tpm_del_char_device(struct tpm_chip *chip)
 {
 	cdev_del(&chip->cdev);
+
+	/* Make the driver uncallable. */
+	down_write(&chip->ops_sem);
+	chip->ops = NULL;
+	up_write(&chip->ops_sem);
+
 	device_del(&chip->dev);
 }
 
@@ -218,6 +275,9 @@  static void tpm1_chip_unregister(struct tpm_chip *chip)
  * the device. As the last step this function adds the chip to the list of TPM
  * chips available for in-kernel use.
  *
+ * Once this function returns the driver call backs in 'op's will not be
+ * running and will no longer start.
+ *
  * This function should be only called after the chip initialization is
  * complete.
  */
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 4009765..f5d4521 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -136,9 +136,18 @@  static ssize_t tpm_write(struct file *file, const char __user *buf,
 		return -EFAULT;
 	}
 
-	/* atomic tpm command send and result receive */
+	/* atomic tpm command send and result receive. We only hold the ops
+	 * lock during this period so that the tpm can be unregistered even if
+	 * the char dev is held open.
+	 */
+	if (tpm_try_get_ops(priv->chip)) {
+		mutex_unlock(&priv->buffer_mutex);
+		return -EPIPE;
+	}
 	out_size = tpm_transmit(priv->chip, priv->data_buffer,
 				sizeof(priv->data_buffer));
+
+	tpm_put_ops(priv->chip);
 	if (out_size < 0) {
 		mutex_unlock(&priv->buffer_mutex);
 		return out_size;
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 483f86f..5caf154 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -700,7 +700,7 @@  int tpm_is_tpm2(u32 chip_num)
 
 	rc = (chip->flags & TPM_CHIP_FLAG_TPM2) != 0;
 
-	tpm_chip_put(chip);
+	tpm_put_ops(chip);
 
 	return rc;
 }
@@ -729,7 +729,7 @@  int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf)
 		rc = tpm2_pcr_read(chip, pcr_idx, res_buf);
 	else
 		rc = tpm_pcr_read_dev(chip, pcr_idx, res_buf);
-	tpm_chip_put(chip);
+	tpm_put_ops(chip);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_pcr_read);
@@ -764,7 +764,7 @@  int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
 		rc = tpm2_pcr_extend(chip, pcr_idx, hash);
-		tpm_chip_put(chip);
+		tpm_put_ops(chip);
 		return rc;
 	}
 
@@ -774,7 +774,7 @@  int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
 	rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE,
 			      "attempting extend a PCR value");
 
-	tpm_chip_put(chip);
+	tpm_put_ops(chip);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_pcr_extend);
@@ -855,7 +855,7 @@  int tpm_send(u32 chip_num, void *cmd, size_t buflen)
 
 	rc = tpm_transmit_cmd(chip, cmd, buflen, "attempting tpm_cmd");
 
-	tpm_chip_put(chip);
+	tpm_put_ops(chip);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_send);
@@ -1037,7 +1037,7 @@  int tpm_get_random(u32 chip_num, u8 *out, size_t max)
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
 		err = tpm2_get_random(chip, out, max);
-		tpm_chip_put(chip);
+		tpm_put_ops(chip);
 		return err;
 	}
 
@@ -1059,7 +1059,7 @@  int tpm_get_random(u32 chip_num, u8 *out, size_t max)
 		num_bytes -= recd;
 	} while (retries-- && total < max);
 
-	tpm_chip_put(chip);
+	tpm_put_ops(chip);
 	return total ? total : -EIO;
 }
 EXPORT_SYMBOL_GPL(tpm_get_random);
@@ -1085,7 +1085,7 @@  int tpm_seal_trusted(u32 chip_num, struct trusted_key_payload *payload,
 
 	rc = tpm2_seal_trusted(chip, payload, options);
 
-	tpm_chip_put(chip);
+	tpm_put_ops(chip);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_seal_trusted);
@@ -1111,7 +1111,8 @@  int tpm_unseal_trusted(u32 chip_num, struct trusted_key_payload *payload,
 
 	rc = tpm2_unseal_trusted(chip, payload, options);
 
-	tpm_chip_put(chip);
+	tpm_put_ops(chip);
+
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_unseal_trusted);
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index d93736a..34e7fc7 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -295,5 +295,10 @@  int tpm_sysfs_add_device(struct tpm_chip *chip)
 
 void tpm_sysfs_del_device(struct tpm_chip *chip)
 {
+	/* The sysfs routines rely on an implicit tpm_try_get_ops, this
+	 * function is called before ops is null'd and the sysfs core
+	 * synchronizes this removal so that no callbacks are running or can
+	 * run again
+	 */
 	sysfs_remove_group(&chip->dev.parent->kobj, &tpm_dev_group);
 }
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 5d33ba5..c6376b1 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -170,7 +170,13 @@  struct tpm_chip {
 	struct device dev;
 	struct cdev cdev;
 
+	/* A driver callback under ops cannot be run unless ops_sem is held
+	 * (sometimes implicitly, eg for the sysfs code). ops becomes null
+	 * when the driver is unregistered, see tpm_try_get_ops.
+	 */
+	struct rw_semaphore ops_sem;
 	const struct tpm_class_ops *ops;
+
 	unsigned int flags;
 
 	int dev_num;		/* /dev/tpm# */
@@ -195,11 +201,6 @@  struct tpm_chip {
 
 #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
 
-static inline void tpm_chip_put(struct tpm_chip *chip)
-{
-	module_put(chip->dev.parent->driver->owner);
-}
-
 static inline int tpm_read_index(int base, int index)
 {
 	outb(index, base);
@@ -507,6 +508,9 @@  extern int wait_for_tpm_stat(struct tpm_chip *, u8, unsigned long,
 			     wait_queue_head_t *, bool);
 
 struct tpm_chip *tpm_chip_find_get(int chip_num);
+__must_check int tpm_try_get_ops(struct tpm_chip *chip);
+void tpm_put_ops(struct tpm_chip *chip);
+
 extern struct tpm_chip *tpmm_chip_alloc(struct device *dev,
 				       const struct tpm_class_ops *ops);
 extern int tpm_chip_register(struct tpm_chip *chip);