| Message ID | 1455885728-10315-5-git-send-email-stefanb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Feb 19, 2016 at 07:42:01AM -0500, Stefan Berger wrote: > 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. Could you be more concrete with the scenario? You could describe a sequence of events where this could happen. > 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. Makes perfect sense. Fine-tuning performance is not a high priority goal at this point of time. > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > --- > drivers/char/tpm/tpm-chip.c | 71 ++++++++++++++++++++++++++++++++++++---- > drivers/char/tpm/tpm-dev.c | 11 ++++++- > drivers/char/tpm/tpm-interface.c | 18 +++++----- > drivers/char/tpm/tpm-sysfs.c | 5 +++ > drivers/char/tpm/tpm.h | 14 +++++--- > 5 files changed, 98 insertions(+), 21 deletions(-) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index 5df0149..0561400 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 What do you mean when you use the term "some kind of locking"? > + * 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); I'm just thinking is the try_module_get() and module_put() even necessary after this change? You know that device is not unregistered from chip->ops field, which is protected by that RW-semaphore. > + > +/** > + * 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,9 @@ 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)) { > + if (!tpm_try_get_ops(pos)) > chip = pos; > - break; > - } > + break; > } > rcu_read_unlock(); > return chip; > @@ -94,6 +143,7 @@ struct tpm_chip *tpmm_chip_alloc(struct device *dev, > if (chip == NULL) > return ERR_PTR(-ENOMEM); > > + init_rwsem(&chip->ops_sem); > mutex_init(&chip->tpm_mutex); > INIT_LIST_HEAD(&chip->list); > > @@ -180,6 +230,12 @@ static int tpm_dev_add_device(struct tpm_chip *chip) > static void tpm_dev_del_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 +274,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 65dc7fd..1dfe2ce 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -683,7 +683,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; > } > @@ -712,7 +712,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); > @@ -747,7 +747,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; > } > > @@ -757,7 +757,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); > @@ -838,7 +838,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); > @@ -1020,7 +1020,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; > } > > @@ -1042,7 +1042,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); > @@ -1068,7 +1068,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); > @@ -1094,7 +1094,7 @@ 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 31d9a8e..2a8373e 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -174,7 +174,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# */ > @@ -199,11 +205,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); > @@ -511,6 +512,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); > -- > 2.4.3 > /Jarkko ------------------------------------------------------------------------------ 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
On Mon, Feb 22, 2016 at 11:08:59PM +0200, Jarkko Sakkinen wrote: > On Fri, Feb 19, 2016 at 07:42:01AM -0500, Stefan Berger wrote: > > 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. > > Could you be more concrete with the scenario? You could describe > a sequence of events where this could happen. echo 1 > /sys/.../remove Is basically the same as a module unload without requiring module unload. module locking cannot be relied upon for correctness like this ever since hot remove was added to the kernel. > > + * @chip: Chip to ref > > + * > > + * The caller must already have some kind of locking to ensure that chip is > > What do you mean when you use the term "some kind of locking"? Eg the IDR lookup code uses a idr specific mutex. The char device uses a ref held by the cdev subsystem. > > +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); > > I'm just thinking is the try_module_get() and module_put() even > necessary after this change? You know that device is not unregistered > from chip->ops field, which is protected by that RW-semaphore. The module locking is not about correctness. Some subsystems will hold the module lock on their driver specifically to prevent rmmod as a hint to the user that the driver module is being used by an open /dev/tpmX for instance. Eg rtc does that. We need to decide if we want that for TPM or not. Orthogonal issue. I'm OK with getting rid of the module lock, TPM has a long lived user space daemon, rtc does not. 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
On Mon, Feb 22, 2016 at 03:20:17PM -0700, Jason Gunthorpe wrote: > On Mon, Feb 22, 2016 at 11:08:59PM +0200, Jarkko Sakkinen wrote: > > On Fri, Feb 19, 2016 at 07:42:01AM -0500, Stefan Berger wrote: > > > 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. > > > > Could you be more concrete with the scenario? You could describe > > a sequence of events where this could happen. > > echo 1 > /sys/.../remove > > Is basically the same as a module unload without requiring module > unload. module locking cannot be relied upon for correctness like this > ever since hot remove was added to the kernel. The 'remove' file applies only to PCI devices AFAIK. /Jarkko ------------------------------------------------------------------------------ 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
On Tue, Feb 23, 2016 at 09:40:14PM +0200, Jarkko Sakkinen wrote: > > echo 1 > /sys/.../remove > > > > Is basically the same as a module unload without requiring module > > unload. module locking cannot be relied upon for correctness like this > > ever since hot remove was added to the kernel. > > The 'remove' file applies only to PCI devices AFAIK. Not only, PCI has remove but many other bus types do too, eg i2c, spi, openfirmware, etc can all be hot-removed in various ways. As a general subsystem core tpm cannot assume it's drivers are only using non-hotplugable devices. 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
On Tue, Feb 23, 2016 at 12:52:46PM -0700, Jason Gunthorpe wrote: > On Tue, Feb 23, 2016 at 09:40:14PM +0200, Jarkko Sakkinen wrote: > > > > echo 1 > /sys/.../remove > > > > > > Is basically the same as a module unload without requiring module > > > unload. module locking cannot be relied upon for correctness like this > > > ever since hot remove was added to the kernel. > > > > The 'remove' file applies only to PCI devices AFAIK. > > Not only, PCI has remove but many other bus types do too, eg i2c, spi, > openfirmware, etc can all be hot-removed in various ways. > > As a general subsystem core tpm cannot assume it's drivers are only > using non-hotplugable devices. Right, various ways, got confused from a concrete example :) /Jarkko ------------------------------------------------------------------------------ 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
On Mon, Feb 22, 2016 at 03:20:17PM -0700, Jason Gunthorpe wrote: > > I'm just thinking is the try_module_get() and module_put() even > > necessary after this change? You know that device is not unregistered > > from chip->ops field, which is protected by that RW-semaphore. > > The module locking is not about correctness. Some subsystems will hold > the module lock on their driver specifically to prevent rmmod as a > hint to the user that the driver module is being used by an open > /dev/tpmX for instance. Eg rtc does that. > > We need to decide if we want that for TPM or not. Orthogonal issue. > > I'm OK with getting rid of the module lock, TPM has a long lived > user space daemon, rtc does not. You might want to use TPM without a daemon on small embedded devices but on the other hand in these use cases the environment is fairly well controlled. I'm not sure which side to take. > Jason /Jarkko ------------------------------------------------------------------------------ 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
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index 5df0149..0561400 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,9 @@ 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)) { + if (!tpm_try_get_ops(pos)) chip = pos; - break; - } + break; } rcu_read_unlock(); return chip; @@ -94,6 +143,7 @@ struct tpm_chip *tpmm_chip_alloc(struct device *dev, if (chip == NULL) return ERR_PTR(-ENOMEM); + init_rwsem(&chip->ops_sem); mutex_init(&chip->tpm_mutex); INIT_LIST_HEAD(&chip->list); @@ -180,6 +230,12 @@ static int tpm_dev_add_device(struct tpm_chip *chip) static void tpm_dev_del_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 +274,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 65dc7fd..1dfe2ce 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -683,7 +683,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; } @@ -712,7 +712,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); @@ -747,7 +747,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; } @@ -757,7 +757,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); @@ -838,7 +838,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); @@ -1020,7 +1020,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; } @@ -1042,7 +1042,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); @@ -1068,7 +1068,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); @@ -1094,7 +1094,7 @@ 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 31d9a8e..2a8373e 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -174,7 +174,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# */ @@ -199,11 +205,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); @@ -511,6 +512,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);