Message ID | 1456766996-9300-4-git-send-email-stefanb@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
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
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
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
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
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 ------------------------------------------------------------------------------
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 > ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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);