Re: [PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get()

[Jarkko Sakkinen]

On Wed, Oct 25, 2017 at 02:17:44PM -0600, Jason Gunthorpe wrote:
> On Wed, Oct 25, 2017 at 10:07:46PM +0200, Jarkko Sakkinen wrote:
> 
> > The id has a nice feature that it is unique for one boot cycle you can
> > even try to get a chip that has been deleted. It has the most stable
> > properties in the long run.
> 
> It isn't unique, we can re-use ids them via idr_alloc(). We should
> never use index inside the kernel.
> 
> > Address is a reusable identifier in one boot cycle.
> 
> It is invalid to pass in a chip for which the caller does not hold a
> kref, so address is the safest argument.
> 
> Jason

I'll buy this too (sending update today).

/Jarkko

Re: [PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get()

[Jason Gunthorpe]

On Wed, Oct 25, 2017 at 10:07:46PM +0200, Jarkko Sakkinen wrote:

> The id has a nice feature that it is unique for one boot cycle you can
> even try to get a chip that has been deleted. It has the most stable
> properties in the long run.

It isn't unique, we can re-use ids them via idr_alloc(). We should
never use index inside the kernel.

> Address is a reusable identifier in one boot cycle.

It is invalid to pass in a chip for which the caller does not hold a
kref, so address is the safest argument.

Jason

Re: [PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get()

[Jarkko Sakkinen]

On Wed, Oct 25, 2017 at 01:46:33PM -0600, Jason Gunthorpe wrote:
> > struct tpm_chip *tpm_chip_find_get(u64 id)
> > {
> > struct tpm_chup *chip;
> > struct tpm_chip *res = NULL;
> > int chip_num = 0;
> > int chip_prev;
> > 
> > mutex_lock(_lock);
> > 
> > do {
> > chip_prev = chip_num;
> > 
> > chip = idr_get_next(_nums_idr, _num);
> > 
> > if (chip && (!id || id == chip->id) && !tpm_try_get_ops(chip)) {
> > res = chip;
> > break;
> > }
> > } while (chip_prev != chip_num);
> > 
> > mutex_unlock(_lock);
> > 
> > return res;
> > }
> 
> ?? The old version was correct, idr_find_slowpath is better than an
> idr_get_next serach if you already know id.
> 
> PrasannaKumar's solution seems right, if we already have chip, then we
> just need to lock it again:
> 
> struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
> {
>   struct tpm_chip *res = NULL;
> 
>   mutex_lock(_lock);
> 
>   if (!chip) {
>   int chip_num = 0;
>   int chip_prev;
> 
>   do {
>   chip_prev = chip_num;
>   chip = idr_get_next(_nums_idr, _num);
>   if (chip && !tpm_try_get_ops(chip)) {
>   res = chip;
>   break;
>   }
>   } while (chip_prev != chip_num);
>   } else {
>   if (!tpm_try_get_ops(chip))
>   res = chip;
>   }
> 
>   mutex_unlock(_lock);
> 
>   return res;
> }
> 
> Jason

The id has a nice feature that it is unique for one boot cycle you can
even try to get a chip that has been deleted. It has the most stable
properties in the long run.

Address is a reusable identifier in one boot cycle.

/Jarkko

Re: [PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get()

[Jason Gunthorpe]

> struct tpm_chip *tpm_chip_find_get(u64 id)
> {
>   struct tpm_chup *chip;
>   struct tpm_chip *res = NULL;
>   int chip_num = 0;
>   int chip_prev;
> 
>   mutex_lock(_lock);
> 
>   do {
>   chip_prev = chip_num;
> 
>   chip = idr_get_next(_nums_idr, _num);
> 
>   if (chip && (!id || id == chip->id) && !tpm_try_get_ops(chip)) {
>   res = chip;
>   break;
>   }
>   } while (chip_prev != chip_num);
> 
>   mutex_unlock(_lock);
> 
>   return res;
> }

?? The old version was correct, idr_find_slowpath is better than an
idr_get_next serach if you already know id.

PrasannaKumar's solution seems right, if we already have chip, then we
just need to lock it again:

struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
{
struct tpm_chip *res = NULL;

mutex_lock(_lock);

if (!chip) {
int chip_num = 0;
int chip_prev;

do {
chip_prev = chip_num;
chip = idr_get_next(_nums_idr, _num);
if (chip && !tpm_try_get_ops(chip)) {
res = chip;
break;
}
} while (chip_prev != chip_num);
} else {
if (!tpm_try_get_ops(chip))
res = chip;
}

mutex_unlock(_lock);

return res;
}

Jason

Re: [PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get()

[Jarkko Sakkinen]

On Wed, Oct 25, 2017 at 08:40:26PM +0530, PrasannaKumar Muralidharan wrote:
> > -struct tpm_chip *tpm_chip_find_get(int chip_num)
> > +struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
> >  {
> > -   struct tpm_chip *chip, *res = NULL;
> > +   struct tpm_chip *res = NULL;
> > +   int chip_num = 0;
> > int chip_prev;
> >
> > mutex_lock(_lock);
> >
> > -   if (chip_num == TPM_ANY_NUM) {
> > -   chip_num = 0;
> > +   if (!chip) {
> > do {
> > chip_prev = chip_num;
> > chip = idr_get_next(_nums_idr, _num);
> 
> When chip is not NULL just do tpm_try_get_ops(chip). Current code does
> more things which are not required.

Your observation is right that there is something wrong but conclusions
are incorrect.

It's actually a regression.

If @chip has a value, the code does one iteration of what it is doing in
the first branch of the condition. That is completely bogus semantics to
say the least.

To sort that out I'll introduce a new field to struct tpm_chip:

  u64 id;

This gets a value from a global count every time a chip is created.

The function will become then:

struct tpm_chip *tpm_chip_find_get(u64 id)
{
struct tpm_chup *chip;
struct tpm_chip *res = NULL;
int chip_num = 0;
int chip_prev;

mutex_lock(_lock);

do {
chip_prev = chip_num;

chip = idr_get_next(_nums_idr, _num);

if (chip && (!id || id == chip->id) && !tpm_try_get_ops(chip)) {
res = chip;
break;
}
} while (chip_prev != chip_num);

mutex_unlock(_lock);

return res;
}

Thanks for spotting this out. I'll refine the patch.

/Jarkko

Re: [PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get()

[Jason Gunthorpe]

On Wed, Oct 25, 2017 at 01:55:04PM +0200, Jarkko Sakkinen wrote:
> Device number (the character device index) is not a stable identifier
> for a TPM chip. That is the reason why every call site passes
> TPM_ANY_NUM to tpm_chip_find_get().
> 
> This commit changes the API in a way that instead a struct tpm_chip
> instance is given and NULL means the default chip. In addition, this
> commit refines the documentation to be up to date with the
> implementation.
> 
> Suggested-by: Jason Gunthorpe  (@chip_num -> 
> @chip)
> Signed-off-by: Jarkko Sakkinen 
> v2:
> * Further defined function documentation.
> * Changed @chip_num to @chip instead of removing the parameter as suggested by
>   Jason Gunthorpe.

Reviewed-by: Jason Gunthorpe 

Jason

Re: [PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get()

[PrasannaKumar Muralidharan]

Hi Jarkko,

On 25 October 2017 at 17:25, Jarkko Sakkinen
 wrote:
> Device number (the character device index) is not a stable identifier
> for a TPM chip. That is the reason why every call site passes
> TPM_ANY_NUM to tpm_chip_find_get().
>
> This commit changes the API in a way that instead a struct tpm_chip
> instance is given and NULL means the default chip. In addition, this
> commit refines the documentation to be up to date with the
> implementation.
>
> Suggested-by: Jason Gunthorpe  (@chip_num -> 
> @chip)
> Signed-off-by: Jarkko Sakkinen 
> ---
> v2:
> * Further defined function documentation.
> * Changed @chip_num to @chip instead of removing the parameter as suggested by
>   Jason Gunthorpe.
>  drivers/char/hw_random/tpm-rng.c|   2 +-
>  drivers/char/tpm/tpm-chip.c |  21 +++---
>  drivers/char/tpm/tpm-interface.c| 135 
> +++-
>  drivers/char/tpm/tpm.h  |   2 +-
>  include/linux/tpm.h |  38 +-
>  security/integrity/ima/ima_crypto.c |   2 +-
>  security/integrity/ima/ima_init.c   |   2 +-
>  security/integrity/ima/ima_queue.c  |   2 +-
>  security/keys/trusted.c |  35 +-
>  9 files changed, 125 insertions(+), 114 deletions(-)
>
> diff --git a/drivers/char/hw_random/tpm-rng.c 
> b/drivers/char/hw_random/tpm-rng.c
> index d6d448266f07..c5e363825af0 100644
> --- a/drivers/char/hw_random/tpm-rng.c
> +++ b/drivers/char/hw_random/tpm-rng.c
> @@ -25,7 +25,7 @@
>
>  static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
>  {
> -   return tpm_get_random(TPM_ANY_NUM, data, max);
> +   return tpm_get_random(NULL, data, max);
>  }
>
>  static struct hwrng tpm_rng = {
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index a114e8f7fb90..c7a4e7fb424d 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -81,21 +81,26 @@ void tpm_put_ops(struct tpm_chip *chip)
>  EXPORT_SYMBOL_GPL(tpm_put_ops);
>
>  /**
> - * tpm_chip_find_get() - return tpm_chip for a given chip number
> - * @chip_num: id to find
> + * tpm_chip_find_get() - find and reserve a TPM chip
> + * @chip:  a  tpm_chip instance, %NULL for the default chip
>   *
> - * The return'd chip has been tpm_try_get_ops'd and must be released via
> - * tpm_put_ops
> + * Finds a TPM chip and reserves its class device and operations. The chip 
> must
> + * be released with tpm_chip_put_ops() after use.
> + *
> + * Return:
> + * A reserved  tpm_chip instance.
> + * %NULL if a chip is not found.
> + * %NULL if the chip is not available.
>   */
> -struct tpm_chip *tpm_chip_find_get(int chip_num)
> +struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
>  {
> -   struct tpm_chip *chip, *res = NULL;
> +   struct tpm_chip *res = NULL;
> +   int chip_num = 0;
> int chip_prev;
>
> mutex_lock(_lock);
>
> -   if (chip_num == TPM_ANY_NUM) {
> -   chip_num = 0;
> +   if (!chip) {
> do {
> chip_prev = chip_num;
> chip = idr_get_next(_nums_idr, _num);

When chip is not NULL just do tpm_try_get_ops(chip). Current code does
more things which are not required.

> diff --git a/drivers/char/tpm/tpm-interface.c 
> b/drivers/char/tpm/tpm-interface.c
> index ebe0a1d36d8c..19f820f775b5 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -809,19 +809,20 @@ int tpm_pcr_read_dev(struct tpm_chip *chip, int 
> pcr_idx, u8 *res_buf)
>  }
>
>  /**
> - * tpm_is_tpm2 - is the chip a TPM2 chip?
> - * @chip_num:  tpm idx # or ANY
> + * tpm_is_tpm2 - do we a have a TPM2 chip?
> + * @chip:  a  tpm_chip instance, %NULL for the default chip
>   *
> - * Returns < 0 on error, and 1 or 0 on success depending whether the chip
> - * is a TPM2 chip.
> + * Return:
> + * 1 if we have a TPM2 chip.
> + * 0 if we don't have a TPM2 chip.
> + * A negative number for system errors (errno).
>   */
> -int tpm_is_tpm2(u32 chip_num)
> +int tpm_is_tpm2(struct tpm_chip *chip)
>  {
> -   struct tpm_chip *chip;
> int rc;
>
> -   chip = tpm_chip_find_get(chip_num);
> -   if (chip == NULL)
> +   chip = tpm_chip_find_get(chip);
> +   if (!chip)
> return -ENODEV;
>
> rc = (chip->flags & TPM_CHIP_FLAG_TPM2) != 0;
> @@ -833,23 +834,19 @@ int tpm_is_tpm2(u32 chip_num)
>  EXPORT_SYMBOL_GPL(tpm_is_tpm2);
>
>  /**
> - * tpm_pcr_read - read a pcr value
> - * @chip_num:  tpm idx # or ANY
> - * @pcr_idx:   pcr idx to retrieve
> - * @res_buf:   TPM_PCR value
> - * size of res_buf is 20 bytes (or NULL if you don't care)
> + * tpm_pcr_read - read a PCR value from SHA1 bank
> + * @chip:  a  tpm_chip instance, %NULL for the default chip
> + * @pcr_idx:   the PCR to be retrieved
> + * @res_buf:   the value of the PCR
>   *
> - * 

[PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get()

[Jarkko Sakkinen]

Device number (the character device index) is not a stable identifier
for a TPM chip. That is the reason why every call site passes
TPM_ANY_NUM to tpm_chip_find_get().

This commit changes the API in a way that instead a struct tpm_chip
instance is given and NULL means the default chip. In addition, this
commit refines the documentation to be up to date with the
implementation.

Suggested-by: Jason Gunthorpe  (@chip_num -> 
@chip)
Signed-off-by: Jarkko Sakkinen 
---
v2:
* Further defined function documentation.
* Changed @chip_num to @chip instead of removing the parameter as suggested by
  Jason Gunthorpe.
 drivers/char/hw_random/tpm-rng.c|   2 +-
 drivers/char/tpm/tpm-chip.c |  21 +++---
 drivers/char/tpm/tpm-interface.c| 135 +++-
 drivers/char/tpm/tpm.h  |   2 +-
 include/linux/tpm.h |  38 +-
 security/integrity/ima/ima_crypto.c |   2 +-
 security/integrity/ima/ima_init.c   |   2 +-
 security/integrity/ima/ima_queue.c  |   2 +-
 security/keys/trusted.c |  35 +-
 9 files changed, 125 insertions(+), 114 deletions(-)

diff --git a/drivers/char/hw_random/tpm-rng.c b/drivers/char/hw_random/tpm-rng.c
index d6d448266f07..c5e363825af0 100644
--- a/drivers/char/hw_random/tpm-rng.c
+++ b/drivers/char/hw_random/tpm-rng.c
@@ -25,7 +25,7 @@
 
 static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
-   return tpm_get_random(TPM_ANY_NUM, data, max);
+   return tpm_get_random(NULL, data, max);
 }
 
 static struct hwrng tpm_rng = {
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index a114e8f7fb90..c7a4e7fb424d 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -81,21 +81,26 @@ void tpm_put_ops(struct tpm_chip *chip)
 EXPORT_SYMBOL_GPL(tpm_put_ops);
 
 /**
- * tpm_chip_find_get() - return tpm_chip for a given chip number
- * @chip_num: id to find
+ * tpm_chip_find_get() - find and reserve a TPM chip
+ * @chip:  a  tpm_chip instance, %NULL for the default chip
  *
- * The return'd chip has been tpm_try_get_ops'd and must be released via
- * tpm_put_ops
+ * Finds a TPM chip and reserves its class device and operations. The chip must
+ * be released with tpm_chip_put_ops() after use.
+ *
+ * Return:
+ * A reserved  tpm_chip instance.
+ * %NULL if a chip is not found.
+ * %NULL if the chip is not available.
  */
-struct tpm_chip *tpm_chip_find_get(int chip_num)
+struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
 {
-   struct tpm_chip *chip, *res = NULL;
+   struct tpm_chip *res = NULL;
+   int chip_num = 0;
int chip_prev;
 
mutex_lock(_lock);
 
-   if (chip_num == TPM_ANY_NUM) {
-   chip_num = 0;
+   if (!chip) {
do {
chip_prev = chip_num;
chip = idr_get_next(_nums_idr, _num);
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index ebe0a1d36d8c..19f820f775b5 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -809,19 +809,20 @@ int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, 
u8 *res_buf)
 }
 
 /**
- * tpm_is_tpm2 - is the chip a TPM2 chip?
- * @chip_num:  tpm idx # or ANY
+ * tpm_is_tpm2 - do we a have a TPM2 chip?
+ * @chip:  a  tpm_chip instance, %NULL for the default chip
  *
- * Returns < 0 on error, and 1 or 0 on success depending whether the chip
- * is a TPM2 chip.
+ * Return:
+ * 1 if we have a TPM2 chip.
+ * 0 if we don't have a TPM2 chip.
+ * A negative number for system errors (errno).
  */
-int tpm_is_tpm2(u32 chip_num)
+int tpm_is_tpm2(struct tpm_chip *chip)
 {
-   struct tpm_chip *chip;
int rc;
 
-   chip = tpm_chip_find_get(chip_num);
-   if (chip == NULL)
+   chip = tpm_chip_find_get(chip);
+   if (!chip)
return -ENODEV;
 
rc = (chip->flags & TPM_CHIP_FLAG_TPM2) != 0;
@@ -833,23 +834,19 @@ int tpm_is_tpm2(u32 chip_num)
 EXPORT_SYMBOL_GPL(tpm_is_tpm2);
 
 /**
- * tpm_pcr_read - read a pcr value
- * @chip_num:  tpm idx # or ANY
- * @pcr_idx:   pcr idx to retrieve
- * @res_buf:   TPM_PCR value
- * size of res_buf is 20 bytes (or NULL if you don't care)
+ * tpm_pcr_read - read a PCR value from SHA1 bank
+ * @chip:  a  tpm_chip instance, %NULL for the default chip
+ * @pcr_idx:   the PCR to be retrieved
+ * @res_buf:   the value of the PCR
  *
- * The TPM driver should be built-in, but for whatever reason it
- * isn't, protect against the chip disappearing, by incrementing
- * the module usage count.
+ * Return: same as with tpm_transmit_cmd()
  */
-int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf)
+int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
 {
-   struct tpm_chip *chip;
int rc;
 
-   chip = tpm_chip_find_get(chip_num);
-   if (chip == NULL)
+