| Message ID | 20180620204236.1572523-2-stefanb@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote: > Implement tpm_chip_find() for other subsystems to find a TPM chip and > get a reference to that chip. Once done with using the chip, the reference > is released using tpm_chip_put(). > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > drivers/char/tpm/tpm-chip.c | 37 +++++++++++++++++++++++++++++++++++++ > include/linux/tpm.h | 9 +++++++++ > 2 files changed, 46 insertions(+) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index 0a62c19937b6..eaaf41ce73d8 100644 > +++ b/drivers/char/tpm/tpm-chip.c > @@ -81,6 +81,43 @@ void tpm_put_ops(struct tpm_chip *chip) > EXPORT_SYMBOL_GPL(tpm_put_ops); > > /** > + * tpm_chip_put() - Release a ref to the tpm_chip > + * @chip: Chip to put > + */ > +void tpm_chip_put(struct tpm_chip *chip) > +{ > + if (chip) > + put_device(&chip->dev); Rarely like to see those if's inside a put function.. Does it actually help anything? Jason
On 06/20/2018 04:50 PM, Jason Gunthorpe wrote: > On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote: >> Implement tpm_chip_find() for other subsystems to find a TPM chip and >> get a reference to that chip. Once done with using the chip, the reference >> is released using tpm_chip_put(). >> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> >> drivers/char/tpm/tpm-chip.c | 37 +++++++++++++++++++++++++++++++++++++ >> include/linux/tpm.h | 9 +++++++++ >> 2 files changed, 46 insertions(+) >> >> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c >> index 0a62c19937b6..eaaf41ce73d8 100644 >> +++ b/drivers/char/tpm/tpm-chip.c >> @@ -81,6 +81,43 @@ void tpm_put_ops(struct tpm_chip *chip) >> EXPORT_SYMBOL_GPL(tpm_put_ops); >> >> /** >> + * tpm_chip_put() - Release a ref to the tpm_chip >> + * @chip: Chip to put >> + */ >> +void tpm_chip_put(struct tpm_chip *chip) >> +{ >> + if (chip) >> + put_device(&chip->dev); > Rarely like to see those if's inside a put function.. Does it actually > help anything? Following put_device() 'pattern' here which also checks using 'if (dev)'. Stefan
On Wed, Jun 20, 2018 at 02:50:04PM -0600, Jason Gunthorpe wrote: > On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote: > > Implement tpm_chip_find() for other subsystems to find a TPM chip and > > get a reference to that chip. Once done with using the chip, the reference > > is released using tpm_chip_put(). > > > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > > drivers/char/tpm/tpm-chip.c | 37 +++++++++++++++++++++++++++++++++++++ > > include/linux/tpm.h | 9 +++++++++ > > 2 files changed, 46 insertions(+) > > > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > > index 0a62c19937b6..eaaf41ce73d8 100644 > > +++ b/drivers/char/tpm/tpm-chip.c > > @@ -81,6 +81,43 @@ void tpm_put_ops(struct tpm_chip *chip) > > EXPORT_SYMBOL_GPL(tpm_put_ops); > > > > /** > > + * tpm_chip_put() - Release a ref to the tpm_chip > > + * @chip: Chip to put > > + */ > > +void tpm_chip_put(struct tpm_chip *chip) > > +{ > > + if (chip) > > + put_device(&chip->dev); > > Rarely like to see those if's inside a put function.. Does it actually > help anything? > > Jason The check only helps to hidden regressions here... /Jarkko
On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote: > Implement tpm_chip_find() for other subsystems to find a TPM chip and > get a reference to that chip. Once done with using the chip, the reference > is released using tpm_chip_put(). > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> You should sort this out in a way that we don't end up with duplicate functions. /Jarkko
On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote: > On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote: >> Implement tpm_chip_find() for other subsystems to find a TPM chip and >> get a reference to that chip. Once done with using the chip, the reference >> is released using tpm_chip_put(). >> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > You should sort this out in a way that we don't end up with duplicate > functions. We cannot hold the ops_sem semaphore for a long time. So we need a function that gets us a reference and doesn't hold the ops semaphore and that's where this tpm_chip_find() comes from. Stefan > /Jarkko >
On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote: > On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote: >> Implement tpm_chip_find() for other subsystems to find a TPM chip and >> get a reference to that chip. Once done with using the chip, the reference >> is released using tpm_chip_put(). >> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > You should sort this out in a way that we don't end up with duplicate > functions. Do you want me to create a function *like* tpm_chip_find_get() that takes an additional parameter whether to get the ops semaphore and have that function called by the existing tpm_chip_find_get() and the new tpm_chip_find(). The latter would then not get the ops semphore. I didn't want to do this since one time the function returns with a lock held and the other time not. Stefan > > /Jarkko >
On Thu, Jun 21, 2018 at 01:45:03PM -0400, Stefan Berger wrote: > On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote: > >On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote: > >>Implement tpm_chip_find() for other subsystems to find a TPM chip and > >>get a reference to that chip. Once done with using the chip, the reference > >>is released using tpm_chip_put(). > >> > >>Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > >You should sort this out in a way that we don't end up with duplicate > >functions. > > Do you want me to create a function *like* tpm_chip_find_get() that takes an > additional parameter whether to get the ops semaphore and have that function > called by the existing tpm_chip_find_get() and the new tpm_chip_find(). The > latter would then not get the ops semphore. I didn't want to do this since > one time the function returns with a lock held and the other time not. Another option, and I haven't looked, is to revise the callers of tpm_chip_find_get to not require it to hold the ops semaphore for them. Either by giving them an API to do it, or revising the TPM entry points to do it. I didn't look, but how did the ops semaphore get grabbed in your revised patches? They do grab it, right? Jason
On 06/21/2018 01:56 PM, Jason Gunthorpe wrote: > On Thu, Jun 21, 2018 at 01:45:03PM -0400, Stefan Berger wrote: >> On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote: >>> On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote: >>>> Implement tpm_chip_find() for other subsystems to find a TPM chip and >>>> get a reference to that chip. Once done with using the chip, the reference >>>> is released using tpm_chip_put(). >>>> >>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> >>> You should sort this out in a way that we don't end up with duplicate >>> functions. >> Do you want me to create a function *like* tpm_chip_find_get() that takes an >> additional parameter whether to get the ops semaphore and have that function >> called by the existing tpm_chip_find_get() and the new tpm_chip_find(). The >> latter would then not get the ops semphore. I didn't want to do this since >> one time the function returns with a lock held and the other time not. > Another option, and I haven't looked, is to revise the callers of > tpm_chip_find_get to not require it to hold the ops semaphore for > them. We have tpm_chip_unregister calling tpm_del_char_device to set the ops to NULL once a chip is unregistered. All existing callers, if they pass in a tpm_chip != NULL, currently fail if the ops are NULL. (If they pass in tpm_chip = NULL, they shouldn't find a chip once ops are null and it has been removed from the IDR). I wouldn't change that since IMA will call in with a tpm_chip != NULL and we want to protect the ops. All existing code within the tpm subsystem does seem to call tpm_chip_find_get() with a NULL pointer, though. Also trusted keys seems to pass in a NULL pointer every time. > > Either by giving them an API to do it, or revising the TPM entry > points to do it. > > I didn't look, but how did the ops semaphore get grabbed in your > revised patches? They do grab it, right? The revised patches do not touch the existing code much but will call tpm_chip_find_get() and get that semaphore every time before the ops are used. IMA is the only caller of tpm_chip_find() that now gets an additional reference to the tpm_chip and these APIs get called like this from IMA: ima init: chip = tpm_chip_find() ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip) ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip) [repeat] ima shutdown: tpm_chip_put(chip) Stefan > > Jason >
On Thu, Jun 21, 2018 at 02:19:44PM -0400, Stefan Berger wrote: > On 06/21/2018 01:56 PM, Jason Gunthorpe wrote: > >On Thu, Jun 21, 2018 at 01:45:03PM -0400, Stefan Berger wrote: > >>On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote: > >>>On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote: > >>>>Implement tpm_chip_find() for other subsystems to find a TPM chip and > >>>>get a reference to that chip. Once done with using the chip, the reference > >>>>is released using tpm_chip_put(). > >>>> > >>>>Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > >>>You should sort this out in a way that we don't end up with duplicate > >>>functions. > >>Do you want me to create a function *like* tpm_chip_find_get() that takes an > >>additional parameter whether to get the ops semaphore and have that function > >>called by the existing tpm_chip_find_get() and the new tpm_chip_find(). The > >>latter would then not get the ops semphore. I didn't want to do this since > >>one time the function returns with a lock held and the other time not. > >Another option, and I haven't looked, is to revise the callers of > >tpm_chip_find_get to not require it to hold the ops semaphore for > >them. > > We have tpm_chip_unregister calling tpm_del_char_device to set the ops to > NULL once a chip is unregistered. All existing callers, if they pass in a > tpm_chip != NULL, currently fail if the ops are NULL. (If they pass in > tpm_chip = NULL, they shouldn't find a chip once ops are null and it has > been removed from the IDR). I wouldn't change that since IMA will call in > with a tpm_chip != NULL and we want to protect the ops. All existing code > within the tpm subsystem does seem to call tpm_chip_find_get() with a NULL > pointer, though. Also trusted keys seems to pass in a NULL pointer every > time. > > > > >Either by giving them an API to do it, or revising the TPM entry > >points to do it. > > > >I didn't look, but how did the ops semaphore get grabbed in your > >revised patches? They do grab it, right? > > The revised patches do not touch the existing code much but will call > tpm_chip_find_get() and get that semaphore every time before the ops are > used. IMA is the only caller of tpm_chip_find() that now gets an additional > reference to the tpm_chip and these APIs get called like this from IMA: > > ima init: chip = tpm_chip_find() > > ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip) > > ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip) > > [repeat] > > ima shutdown: tpm_chip_put(chip) Maybe just change tpm_chip_find_get() into tpm_get_ops(chip) and convert all callers? Jason
On 06/21/2018 03:06 PM, Jason Gunthorpe wrote: > On Thu, Jun 21, 2018 at 02:19:44PM -0400, Stefan Berger wrote: >> On 06/21/2018 01:56 PM, Jason Gunthorpe wrote: >>> On Thu, Jun 21, 2018 at 01:45:03PM -0400, Stefan Berger wrote: >>>> On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote: >>>>> On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote: >>>>>> Implement tpm_chip_find() for other subsystems to find a TPM chip and >>>>>> get a reference to that chip. Once done with using the chip, the reference >>>>>> is released using tpm_chip_put(). >>>>>> >>>>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> >>>>> You should sort this out in a way that we don't end up with duplicate >>>>> functions. >>>> Do you want me to create a function *like* tpm_chip_find_get() that takes an >>>> additional parameter whether to get the ops semaphore and have that function >>>> called by the existing tpm_chip_find_get() and the new tpm_chip_find(). The >>>> latter would then not get the ops semphore. I didn't want to do this since >>>> one time the function returns with a lock held and the other time not. >>> Another option, and I haven't looked, is to revise the callers of >>> tpm_chip_find_get to not require it to hold the ops semaphore for >>> them. >> We have tpm_chip_unregister calling tpm_del_char_device to set the ops to >> NULL once a chip is unregistered. All existing callers, if they pass in a >> tpm_chip != NULL, currently fail if the ops are NULL. (If they pass in >> tpm_chip = NULL, they shouldn't find a chip once ops are null and it has >> been removed from the IDR). I wouldn't change that since IMA will call in >> with a tpm_chip != NULL and we want to protect the ops. All existing code >> within the tpm subsystem does seem to call tpm_chip_find_get() with a NULL >> pointer, though. Also trusted keys seems to pass in a NULL pointer every >> time. >> >>> Either by giving them an API to do it, or revising the TPM entry >>> points to do it. >>> >>> I didn't look, but how did the ops semaphore get grabbed in your >>> revised patches? They do grab it, right? >> The revised patches do not touch the existing code much but will call >> tpm_chip_find_get() and get that semaphore every time before the ops are >> used. IMA is the only caller of tpm_chip_find() that now gets an additional >> reference to the tpm_chip and these APIs get called like this from IMA: >> >> ima init: chip = tpm_chip_find() >> >> ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip) >> >> ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip) >> >> [repeat] >> >> ima shutdown: tpm_chip_put(chip) > Maybe just change tpm_chip_find_get() into tpm_get_ops(chip) and > convert all callers? And then re-introduce tpm_chip_find_get() for IMA to call ?
On Thu, Jun 21, 2018 at 04:14:46PM -0400, Stefan Berger wrote: > On 06/21/2018 03:06 PM, Jason Gunthorpe wrote: > >On Thu, Jun 21, 2018 at 02:19:44PM -0400, Stefan Berger wrote: > >>On 06/21/2018 01:56 PM, Jason Gunthorpe wrote: > >>>On Thu, Jun 21, 2018 at 01:45:03PM -0400, Stefan Berger wrote: > >>>>On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote: > >>>>>On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote: > >>>>>>Implement tpm_chip_find() for other subsystems to find a TPM chip and > >>>>>>get a reference to that chip. Once done with using the chip, the reference > >>>>>>is released using tpm_chip_put(). > >>>>>> > >>>>>>Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > >>>>>You should sort this out in a way that we don't end up with duplicate > >>>>>functions. > >>>>Do you want me to create a function *like* tpm_chip_find_get() that takes an > >>>>additional parameter whether to get the ops semaphore and have that function > >>>>called by the existing tpm_chip_find_get() and the new tpm_chip_find(). The > >>>>latter would then not get the ops semphore. I didn't want to do this since > >>>>one time the function returns with a lock held and the other time not. > >>>Another option, and I haven't looked, is to revise the callers of > >>>tpm_chip_find_get to not require it to hold the ops semaphore for > >>>them. > >>We have tpm_chip_unregister calling tpm_del_char_device to set the ops to > >>NULL once a chip is unregistered. All existing callers, if they pass in a > >>tpm_chip != NULL, currently fail if the ops are NULL. (If they pass in > >>tpm_chip = NULL, they shouldn't find a chip once ops are null and it has > >>been removed from the IDR). I wouldn't change that since IMA will call in > >>with a tpm_chip != NULL and we want to protect the ops. All existing code > >>within the tpm subsystem does seem to call tpm_chip_find_get() with a NULL > >>pointer, though. Also trusted keys seems to pass in a NULL pointer every > >>time. > >> > >>>Either by giving them an API to do it, or revising the TPM entry > >>>points to do it. > >>> > >>>I didn't look, but how did the ops semaphore get grabbed in your > >>>revised patches? They do grab it, right? > >>The revised patches do not touch the existing code much but will call > >>tpm_chip_find_get() and get that semaphore every time before the ops are > >>used. IMA is the only caller of tpm_chip_find() that now gets an additional > >>reference to the tpm_chip and these APIs get called like this from IMA: > >> > >>ima init: chip = tpm_chip_find() > >> > >>ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip) > >> > >>ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip) > >> > >>[repeat] > >> > >>ima shutdown: tpm_chip_put(chip) > >Maybe just change tpm_chip_find_get() into tpm_get_ops(chip) and > >convert all callers? > > And then re-introduce tpm_chip_find_get() for IMA to call ? You could keep it as 'tpm_chip_find', that seems like a fine name to me Jason
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index 0a62c19937b6..eaaf41ce73d8 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -81,6 +81,43 @@ void tpm_put_ops(struct tpm_chip *chip) EXPORT_SYMBOL_GPL(tpm_put_ops); /** + * tpm_chip_put() - Release a ref to the tpm_chip + * @chip: Chip to put + */ +void tpm_chip_put(struct tpm_chip *chip) +{ + if (chip) + put_device(&chip->dev); +} + +/** + * tpm_chip_find() - find a TPM chip and get a reference to it + */ +struct tpm_chip *tpm_chip_find(void) +{ + struct tpm_chip *chip, *res = NULL; + int chip_num = 0; + int chip_prev; + + mutex_lock(&idr_lock); + + do { + chip_prev = chip_num; + chip = idr_get_next(&dev_nums_idr, &chip_num); + if (chip) { + get_device(&chip->dev); + res = chip; + break; + } + } while (chip_prev != chip_num); + + mutex_unlock(&idr_lock); + + return res; +} +EXPORT_SYMBOL_GPL(tpm_chip_find); + +/** * tpm_chip_find_get() - find and reserve a TPM chip * @chip: a &struct tpm_chip instance, %NULL for the default chip * diff --git a/include/linux/tpm.h b/include/linux/tpm.h index 06639fb6ab85..f0d1abcaf4af 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -61,6 +61,8 @@ extern int tpm_seal_trusted(struct tpm_chip *chip, extern int tpm_unseal_trusted(struct tpm_chip *chip, struct trusted_key_payload *payload, struct trusted_key_options *options); +extern struct tpm_chip *tpm_chip_find(void); +extern void tpm_chip_put(struct tpm_chip *chip); #else static inline int tpm_is_tpm2(struct tpm_chip *chip) { @@ -96,5 +98,12 @@ static inline int tpm_unseal_trusted(struct tpm_chip *chip, { return -ENODEV; } +static inline struct tpm_chip *tpm_chip_find(void) +{ + return NULL; +} +static inline void tpm_chip_put(struct tpm_chip *chip) +{ +} #endif #endif
Implement tpm_chip_find() for other subsystems to find a TPM chip and get a reference to that chip. Once done with using the chip, the reference is released using tpm_chip_put(). Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- drivers/char/tpm/tpm-chip.c | 37 +++++++++++++++++++++++++++++++++++++ include/linux/tpm.h | 9 +++++++++ 2 files changed, 46 insertions(+)