Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1521470 > unrolled thread

[PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with a static array

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with a static array Nayna Jain <nayna@linux.vnet.ibm.com> - 2016-11-14 11:10 +0100
    Re: [PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with  a static array Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-11-22 12:30 +0100
      Re: [PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with  a static array Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-11-22 18:00 +0100
        Re: [PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with  a static array Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-11-24 15:00 +0100
          Re: [PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with  a static array Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-11-24 18:00 +0100
            Re: [PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with  a static array Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-11-25 09:10 +0100
      Re: [PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with a  static array Nayna <nayna@linux.vnet.ibm.com> - 2016-11-22 20:30 +0100
        Re: [PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with  a static array Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-11-24 15:10 +0100

#1521470 — [PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with a static array

This commit is based on a commit by Nayna Jain. Replaced dynamically
allocated bios_dir with a static array as the size is always constant.

Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-chip.c     |  9 ++++---
 drivers/char/tpm/tpm.h          |  3 ++-
 drivers/char/tpm/tpm_eventlog.c | 59 ++++++++++++++++++-----------------------
 drivers/char/tpm/tpm_eventlog.h | 10 +++----
 4 files changed, 38 insertions(+), 43 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 836f056..d0c1872 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -278,14 +278,16 @@ static void tpm_del_char_device(struct tpm_chip *chip)
 
 static int tpm1_chip_register(struct tpm_chip *chip)
 {
+	int rc;
+
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		return 0;
 
 	tpm_sysfs_add_device(chip);
 
-	chip->bios_dir = tpm_bios_log_setup(dev_name(&chip->dev));
+	rc = tpm_bios_log_setup(chip);
 
-	return 0;
+	return rc;
 }
 
 static void tpm1_chip_unregister(struct tpm_chip *chip)
@@ -293,8 +295,7 @@ static void tpm1_chip_unregister(struct tpm_chip *chip)
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		return;
 
-	if (chip->bios_dir)
-		tpm_bios_log_teardown(chip->bios_dir);
+	tpm_bios_log_teardown(chip);
 }
 
 static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f9401ca..9d69580 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -40,6 +40,7 @@ enum tpm_const {
 	TPM_BUFSIZE = 4096,
 	TPM_NUM_DEVICES = 65536,
 	TPM_RETRY = 50,		/* 5 seconds */
+	TPM_NUM_EVENT_LOG_FILES = 3,
 };
 
 enum tpm_timeout {
@@ -171,7 +172,7 @@ struct tpm_chip {
 	unsigned long duration[3]; /* jiffies */
 	bool duration_adjusted;
 
-	struct dentry **bios_dir;
+	struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES];
 
 	const struct attribute_group *groups[3];
 	unsigned int groups_cnt;
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index 9467e31..62e9da6 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -368,54 +368,47 @@ static int is_bad(void *p)
 	return 0;
 }
 
-struct dentry **tpm_bios_log_setup(const char *name)
+int tpm_bios_log_setup(struct tpm_chip *chip)
 {
-	struct dentry **ret = NULL, *tpm_dir, *bin_file, *ascii_file;
+	const char *name = dev_name(&chip->dev);
+	unsigned int cnt;
 
-	tpm_dir = securityfs_create_dir(name, NULL);
-	if (is_bad(tpm_dir))
-		goto out;
+	cnt = 0;
+	chip->bios_dir[cnt] = securityfs_create_dir(name, NULL);
+	if (is_bad(chip->bios_dir[cnt]))
+		goto err;
+	cnt++;
 
-	bin_file =
+	chip->bios_dir[cnt] =
 	    securityfs_create_file("binary_bios_measurements",
-				   0440, tpm_dir,
+				   0440, chip->bios_dir[0],
 				   (void *)&tpm_binary_b_measurements_seqops,
 				   &tpm_bios_measurements_ops);
-	if (is_bad(bin_file))
-		goto out_tpm;
+	if (is_bad(chip->bios_dir[cnt]))
+		goto err;
+	cnt++;
 
-	ascii_file =
+	chip->bios_dir[cnt] =
 	    securityfs_create_file("ascii_bios_measurements",
-				   0440, tpm_dir,
+				   0440, chip->bios_dir[0],
 				   (void *)&tpm_ascii_b_measurements_seqops,
 				   &tpm_bios_measurements_ops);
-	if (is_bad(ascii_file))
-		goto out_bin;
+	if (is_bad(chip->bios_dir[cnt]))
+		goto err;
+	cnt++;
 
-	ret = kmalloc(3 * sizeof(struct dentry *), GFP_KERNEL);
-	if (!ret)
-		goto out_ascii;
-
-	ret[0] = ascii_file;
-	ret[1] = bin_file;
-	ret[2] = tpm_dir;
-
-	return ret;
+	return 0;
 
-out_ascii:
-	securityfs_remove(ascii_file);
-out_bin:
-	securityfs_remove(bin_file);
-out_tpm:
-	securityfs_remove(tpm_dir);
-out:
-	return NULL;
+err:
+	chip->bios_dir[cnt] = NULL;
+	tpm_bios_log_teardown(chip);
+	return -EIO;
 }
 
-void tpm_bios_log_teardown(struct dentry **lst)
+void tpm_bios_log_teardown(struct tpm_chip *chip)
 {
 	int i;
 
-	for (i = 0; i < 3; i++)
-		securityfs_remove(lst[i]);
+	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--)
+		securityfs_remove(chip->bios_dir[i]);
 }
diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
index 8de62b0..fd3357e 100644
--- a/drivers/char/tpm/tpm_eventlog.h
+++ b/drivers/char/tpm/tpm_eventlog.h
@@ -77,14 +77,14 @@ int read_log(struct tpm_bios_log *log);
 
 #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
 	defined(CONFIG_ACPI)
-extern struct dentry **tpm_bios_log_setup(const char *);
-extern void tpm_bios_log_teardown(struct dentry **);
+extern int tpm_bios_log_setup(struct tpm_chip *chip);
+extern void tpm_bios_log_teardown(struct tpm_chip *chip);
 #else
-static inline struct dentry **tpm_bios_log_setup(const char *name)
+static inline int tpm_bios_log_setup(struct tpm_chip *chip)
 {
-	return NULL;
+	return 0;
 }
-static inline void tpm_bios_log_teardown(struct dentry **dir)
+static inline void tpm_bios_log_teardown(struct tpm_chip *chip)
 {
 }
 #endif
-- 
2.5.0

[toc] | [next] | [standalone]

#1527431 — Re: [PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with a static array

On Mon, Nov 14, 2016 at 05:00:50AM -0500, Nayna Jain wrote:
> This commit is based on a commit by Nayna Jain. Replaced dynamically
> allocated bios_dir with a static array as the size is always constant.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

This commit remains unreviewed and tested. I'm in the author role here
so I cannot help with this. If that does not happen soon I cannot put
this into the pull request.

/Jarkko


> ---
>  drivers/char/tpm/tpm-chip.c     |  9 ++++---
>  drivers/char/tpm/tpm.h          |  3 ++-
>  drivers/char/tpm/tpm_eventlog.c | 59 ++++++++++++++++++-----------------------
>  drivers/char/tpm/tpm_eventlog.h | 10 +++----
>  4 files changed, 38 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 836f056..d0c1872 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -278,14 +278,16 @@ static void tpm_del_char_device(struct tpm_chip *chip)
>  
>  static int tpm1_chip_register(struct tpm_chip *chip)
>  {
> +	int rc;
> +
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		return 0;
>  
>  	tpm_sysfs_add_device(chip);
>  
> -	chip->bios_dir = tpm_bios_log_setup(dev_name(&chip->dev));
> +	rc = tpm_bios_log_setup(chip);
>  
> -	return 0;
> +	return rc;
>  }
>  
>  static void tpm1_chip_unregister(struct tpm_chip *chip)
> @@ -293,8 +295,7 @@ static void tpm1_chip_unregister(struct tpm_chip *chip)
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		return;
>  
> -	if (chip->bios_dir)
> -		tpm_bios_log_teardown(chip->bios_dir);
> +	tpm_bios_log_teardown(chip);
>  }
>  
>  static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f9401ca..9d69580 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -40,6 +40,7 @@ enum tpm_const {
>  	TPM_BUFSIZE = 4096,
>  	TPM_NUM_DEVICES = 65536,
>  	TPM_RETRY = 50,		/* 5 seconds */
> +	TPM_NUM_EVENT_LOG_FILES = 3,
>  };
>  
>  enum tpm_timeout {
> @@ -171,7 +172,7 @@ struct tpm_chip {
>  	unsigned long duration[3]; /* jiffies */
>  	bool duration_adjusted;
>  
> -	struct dentry **bios_dir;
> +	struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES];
>  
>  	const struct attribute_group *groups[3];
>  	unsigned int groups_cnt;
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index 9467e31..62e9da6 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -368,54 +368,47 @@ static int is_bad(void *p)
>  	return 0;
>  }
>  
> -struct dentry **tpm_bios_log_setup(const char *name)
> +int tpm_bios_log_setup(struct tpm_chip *chip)
>  {
> -	struct dentry **ret = NULL, *tpm_dir, *bin_file, *ascii_file;
> +	const char *name = dev_name(&chip->dev);
> +	unsigned int cnt;
>  
> -	tpm_dir = securityfs_create_dir(name, NULL);
> -	if (is_bad(tpm_dir))
> -		goto out;
> +	cnt = 0;
> +	chip->bios_dir[cnt] = securityfs_create_dir(name, NULL);
> +	if (is_bad(chip->bios_dir[cnt]))
> +		goto err;
> +	cnt++;
>  
> -	bin_file =
> +	chip->bios_dir[cnt] =
>  	    securityfs_create_file("binary_bios_measurements",
> -				   0440, tpm_dir,
> +				   0440, chip->bios_dir[0],
>  				   (void *)&tpm_binary_b_measurements_seqops,
>  				   &tpm_bios_measurements_ops);
> -	if (is_bad(bin_file))
> -		goto out_tpm;
> +	if (is_bad(chip->bios_dir[cnt]))
> +		goto err;
> +	cnt++;
>  
> -	ascii_file =
> +	chip->bios_dir[cnt] =
>  	    securityfs_create_file("ascii_bios_measurements",
> -				   0440, tpm_dir,
> +				   0440, chip->bios_dir[0],
>  				   (void *)&tpm_ascii_b_measurements_seqops,
>  				   &tpm_bios_measurements_ops);
> -	if (is_bad(ascii_file))
> -		goto out_bin;
> +	if (is_bad(chip->bios_dir[cnt]))
> +		goto err;
> +	cnt++;
>  
> -	ret = kmalloc(3 * sizeof(struct dentry *), GFP_KERNEL);
> -	if (!ret)
> -		goto out_ascii;
> -
> -	ret[0] = ascii_file;
> -	ret[1] = bin_file;
> -	ret[2] = tpm_dir;
> -
> -	return ret;
> +	return 0;
>  
> -out_ascii:
> -	securityfs_remove(ascii_file);
> -out_bin:
> -	securityfs_remove(bin_file);
> -out_tpm:
> -	securityfs_remove(tpm_dir);
> -out:
> -	return NULL;
> +err:
> +	chip->bios_dir[cnt] = NULL;
> +	tpm_bios_log_teardown(chip);
> +	return -EIO;
>  }
>  
> -void tpm_bios_log_teardown(struct dentry **lst)
> +void tpm_bios_log_teardown(struct tpm_chip *chip)
>  {
>  	int i;
>  
> -	for (i = 0; i < 3; i++)
> -		securityfs_remove(lst[i]);
> +	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--)
> +		securityfs_remove(chip->bios_dir[i]);
>  }
> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
> index 8de62b0..fd3357e 100644
> --- a/drivers/char/tpm/tpm_eventlog.h
> +++ b/drivers/char/tpm/tpm_eventlog.h
> @@ -77,14 +77,14 @@ int read_log(struct tpm_bios_log *log);
>  
>  #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
>  	defined(CONFIG_ACPI)
> -extern struct dentry **tpm_bios_log_setup(const char *);
> -extern void tpm_bios_log_teardown(struct dentry **);
> +extern int tpm_bios_log_setup(struct tpm_chip *chip);
> +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
>  #else
> -static inline struct dentry **tpm_bios_log_setup(const char *name)
> +static inline int tpm_bios_log_setup(struct tpm_chip *chip)
>  {
> -	return NULL;
> +	return 0;
>  }
> -static inline void tpm_bios_log_teardown(struct dentry **dir)
> +static inline void tpm_bios_log_teardown(struct tpm_chip *chip)
>  {
>  }
>  #endif
> -- 
> 2.5.0
> 

[toc] | [prev] | [next] | [standalone]

#1527709 — Re: [PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with a static array

On Tue, Nov 22, 2016 at 01:23:33PM +0200, Jarkko Sakkinen wrote:
> On Mon, Nov 14, 2016 at 05:00:50AM -0500, Nayna Jain wrote:
> > This commit is based on a commit by Nayna Jain. Replaced dynamically
> > allocated bios_dir with a static array as the size is always constant.
> > 
> > Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> This commit remains unreviewed and tested. I'm in the author role here
> so I cannot help with this. If that does not happen soon I cannot put
> this into the pull request.

Nayna must have tested it, looks OK to me..

> > +err:
> > +	chip->bios_dir[cnt] = NULL;
> > +	tpm_bios_log_teardown(chip);
> > +	return -EIO;

Except that return should ideally be PTR_ERR(chip->bios_dir[cnt])

.. and we still set ERR_PTR into bios_dir in the ENODEV case, so the
overall series is still broken if securityfs is compiled out.

Lets fix this all like this - which is a good enough reason to leave the
ENODEV detect alone - just squash this into your patch:

diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index 2a15b866ac257a..11bb1138a8282e 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -356,15 +356,6 @@ static const struct file_operations tpm_bios_measurements_ops = {
 	.release = tpm_bios_measurements_release,
 };
 
-static int is_bad(void *p)
-{
-	if (!p)
-		return 1;
-	if (IS_ERR(p) && (PTR_ERR(p) != -ENODEV))
-		return 1;
-	return 0;
-}
-
 static int tpm_read_log(struct tpm_chip *chip)
 {
 	int rc;
@@ -390,7 +381,8 @@ static int tpm_read_log(struct tpm_chip *chip)
  * If an event log is found then the securityfs files are setup to
  * export it to userspace, otherwise nothing is done.
  *
- * Returns -ENODEV if the firmware has no event log.
+ * Returns -ENODEV if the firmware has no event log or securityfs is not
+ * supported.
  */
 int tpm_bios_log_setup(struct tpm_chip *chip)
 {
@@ -407,7 +399,10 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
 
 	cnt = 0;
 	chip->bios_dir[cnt] = securityfs_create_dir(name, NULL);
-	if (is_bad(chip->bios_dir[cnt]))
+	/* NOTE: securityfs_create_dir can return ENODEV if securityfs is
+	 * compiled out. The caller should ignore the ENODEV return code.
+	 */
+	if (IS_ERR(chip->bios_dir[cnt]))
 		goto err;
 	cnt++;
 
@@ -419,7 +414,7 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
 				   0440, chip->bios_dir[0],
 				   (void *)&chip->bin_log_seqops,
 				   &tpm_bios_measurements_ops);
-	if (is_bad(chip->bios_dir[cnt]))
+	if (IS_ERR(chip->bios_dir[cnt]))
 		goto err;
 	cnt++;
 
@@ -431,16 +426,17 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
 				   0440, chip->bios_dir[0],
 				   (void *)&chip->ascii_log_seqops,
 				   &tpm_bios_measurements_ops);
-	if (is_bad(chip->bios_dir[cnt]))
+	if (IS_ERR(chip->bios_dir[cnt]))
 		goto err;
 	cnt++;
 
 	return 0;
 
 err:
+	rc = PTR_ERR(chip->bios_dir[cnt]);
 	chip->bios_dir[cnt] = NULL;
 	tpm_bios_log_teardown(chip);
-	return -EIO;
+	return rc;
 }
 
 void tpm_bios_log_teardown(struct tpm_chip *chip)

[toc] | [prev] | [next] | [standalone]

#1529311 — Re: [PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with a static array

On Tue, Nov 22, 2016 at 09:58:56AM -0700, Jason Gunthorpe wrote:
> On Tue, Nov 22, 2016 at 01:23:33PM +0200, Jarkko Sakkinen wrote:
> > On Mon, Nov 14, 2016 at 05:00:50AM -0500, Nayna Jain wrote:
> > > This commit is based on a commit by Nayna Jain. Replaced dynamically
> > > allocated bios_dir with a static array as the size is always constant.
> > > 
> > > Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 
> > This commit remains unreviewed and tested. I'm in the author role here
> > so I cannot help with this. If that does not happen soon I cannot put
> > this into the pull request.
> 
> Nayna must have tested it, looks OK to me..
> 
> > > +err:
> > > +	chip->bios_dir[cnt] = NULL;
> > > +	tpm_bios_log_teardown(chip);
> > > +	return -EIO;
> 
> Except that return should ideally be PTR_ERR(chip->bios_dir[cnt])
> 
> .. and we still set ERR_PTR into bios_dir in the ENODEV case, so the
> overall series is still broken if securityfs is compiled out.
> 
> Lets fix this all like this - which is a good enough reason to leave the
> ENODEV detect alone - just squash this into your patch:

That was a great catch. Thank you.

> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index 2a15b866ac257a..11bb1138a8282e 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -356,15 +356,6 @@ static const struct file_operations tpm_bios_measurements_ops = {
>  	.release = tpm_bios_measurements_release,
>  };
>  
> -static int is_bad(void *p)
> -{
> -	if (!p)
> -		return 1;
> -	if (IS_ERR(p) && (PTR_ERR(p) != -ENODEV))
> -		return 1;
> -	return 0;
> -}
> -

This function is only confusing indirection anyway. Does not serve
really any justifiable purpose.

>  static int tpm_read_log(struct tpm_chip *chip)
>  {
>  	int rc;
> @@ -390,7 +381,8 @@ static int tpm_read_log(struct tpm_chip *chip)
>   * If an event log is found then the securityfs files are setup to
>   * export it to userspace, otherwise nothing is done.
>   *
> - * Returns -ENODEV if the firmware has no event log.
> + * Returns -ENODEV if the firmware has no event log or securityfs is not
> + * supported.
>   */
>  int tpm_bios_log_setup(struct tpm_chip *chip)
>  {
> @@ -407,7 +399,10 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>  
>  	cnt = 0;
>  	chip->bios_dir[cnt] = securityfs_create_dir(name, NULL);
> -	if (is_bad(chip->bios_dir[cnt]))
> +	/* NOTE: securityfs_create_dir can return ENODEV if securityfs is
> +	 * compiled out. The caller should ignore the ENODEV return code.
> +	 */
> +	if (IS_ERR(chip->bios_dir[cnt]))
>  		goto err;
>  	cnt++;
>  
> @@ -419,7 +414,7 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>  				   0440, chip->bios_dir[0],
>  				   (void *)&chip->bin_log_seqops,
>  				   &tpm_bios_measurements_ops);
> -	if (is_bad(chip->bios_dir[cnt]))
> +	if (IS_ERR(chip->bios_dir[cnt]))
>  		goto err;
>  	cnt++;
>  
> @@ -431,16 +426,17 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>  				   0440, chip->bios_dir[0],
>  				   (void *)&chip->ascii_log_seqops,
>  				   &tpm_bios_measurements_ops);
> -	if (is_bad(chip->bios_dir[cnt]))
> +	if (IS_ERR(chip->bios_dir[cnt]))
>  		goto err;
>  	cnt++;
>  
>  	return 0;
>  
>  err:
> +	rc = PTR_ERR(chip->bios_dir[cnt]);
>  	chip->bios_dir[cnt] = NULL;
>  	tpm_bios_log_teardown(chip);
> -	return -EIO;
> +	return rc;
>  }
>  
>  void tpm_bios_log_teardown(struct tpm_chip *chip)

I manually added the changes to:

  tpm: replace dynamically allocated bios_dir with a static array

The code was changed so radically after that patch so it was the
cleanest way.

I need to declare 'rc' in that patch.  I changed also this "int rc = 0;"
to "int rc;" as it does not need to be initialized in the declaration.
This affects:

  tpm: have event log use the tpm_chip

And finally I needed the update this patch too to accomadate the doc
change:

  tpm: Fix handling of missing event log

Could you check through those patches that I didn't blow things up,
which could easily happen given that I needed to update three patches
and give your final reviewed-by if it looks good to you?

In the meanwhile I'll start running sparse, coccicheck etc. for the
release content.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1529571 — Re: [PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with a static array

On Thu, Nov 24, 2016 at 03:57:23PM +0200, Jarkko Sakkinen wrote:
> I manually added the changes to:
> 
>   tpm: replace dynamically allocated bios_dir with a static array

For this patch..

Could drop 'int rc' from tpm1_chip_register, but it will come back in
a later patch

Could dump TPM_NUM_EVENT_LOG_FILES and just use
ARRAY_SIZE(chip->bios_dir)

Now the the stub for tpm_bios_log_setup can properly return -ENODEV

This is no good at this point in the series - we need the ENODEV
detection in tpm_chip_register() from the 'Fix handle of missing event
log' moved into this patch, because it now returns ENODEV due to
sercurityfs

The commit 'tpm: vtpm_proxy: Do not access host's event log' still
needs a proper commit message - it doesn't match what the patch is
doing at all.

If you are going to be editing the patches I'd suggest squashing all
the bug fix ones with proper credit so it at least makes sense to
read..

Jason

[toc] | [prev] | [next] | [standalone]

#1529852 — Re: [PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with a static array

On Thu, Nov 24, 2016 at 09:53:13AM -0700, Jason Gunthorpe wrote:
> On Thu, Nov 24, 2016 at 03:57:23PM +0200, Jarkko Sakkinen wrote:
> > I manually added the changes to:
> > 
> >   tpm: replace dynamically allocated bios_dir with a static array
> 
> For this patch..
> 
> Could drop 'int rc' from tpm1_chip_register, but it will come back in
> a later patch
> 
> Could dump TPM_NUM_EVENT_LOG_FILES and just use
> ARRAY_SIZE(chip->bios_dir)

Not a bug fix. Happy take a patch after the pull request.

> Now the the stub for tpm_bios_log_setup can properly return -ENODEV
> 
> This is no good at this point in the series - we need the ENODEV
> detection in tpm_chip_register() from the 'Fix handle of missing event
> log' moved into this patch, because it now returns ENODEV due to
> sercurityfs

Sure it would be cleaner but not really necessary. Do you really think
this is mandatory? No matter how I reorder patches this will require
time and effort to fix various merge conflicts because of the replacemnt
of event log. After that I have to test everything.

Not doing this for light reasons...

> The commit 'tpm: vtpm_proxy: Do not access host's event log' still
> needs a proper commit message - it doesn't match what the patch is
> doing at all.

The commit message otherwise great except for the short summary, which
is now fixed.

> If you are going to be editing the patches I'd suggest squashing all
> the bug fix ones with proper credit so it at least makes sense to
> read..
> 
> Jason

I do not have interest to edit patches more than I have to.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1527866 — Re: [PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with a static array

On 11/22/2016 04:53 PM, Jarkko Sakkinen wrote:
> On Mon, Nov 14, 2016 at 05:00:50AM -0500, Nayna Jain wrote:
>> This commit is based on a commit by Nayna Jain. Replaced dynamically
>> allocated bios_dir with a static array as the size is always constant.
>>
>> Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
>> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>
> This commit remains unreviewed and tested. I'm in the author role here
> so I cannot help with this. If that does not happen soon I cannot put
> this into the pull request.

Tested-By: Nayna Jain <nayna@linux.vnet.ibm.com>

Thanks & Regards,
   - Nayna

>
> /Jarkko
>
>
>> ---
>>   drivers/char/tpm/tpm-chip.c     |  9 ++++---
>>   drivers/char/tpm/tpm.h          |  3 ++-
>>   drivers/char/tpm/tpm_eventlog.c | 59 ++++++++++++++++++-----------------------
>>   drivers/char/tpm/tpm_eventlog.h | 10 +++----
>>   4 files changed, 38 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 836f056..d0c1872 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -278,14 +278,16 @@ static void tpm_del_char_device(struct tpm_chip *chip)
>>
>>   static int tpm1_chip_register(struct tpm_chip *chip)
>>   {
>> +	int rc;
>> +
>>   	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>>   		return 0;
>>
>>   	tpm_sysfs_add_device(chip);
>>
>> -	chip->bios_dir = tpm_bios_log_setup(dev_name(&chip->dev));
>> +	rc = tpm_bios_log_setup(chip);
>>
>> -	return 0;
>> +	return rc;
>>   }
>>
>>   static void tpm1_chip_unregister(struct tpm_chip *chip)
>> @@ -293,8 +295,7 @@ static void tpm1_chip_unregister(struct tpm_chip *chip)
>>   	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>>   		return;
>>
>> -	if (chip->bios_dir)
>> -		tpm_bios_log_teardown(chip->bios_dir);
>> +	tpm_bios_log_teardown(chip);
>>   }
>>
>>   static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index f9401ca..9d69580 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -40,6 +40,7 @@ enum tpm_const {
>>   	TPM_BUFSIZE = 4096,
>>   	TPM_NUM_DEVICES = 65536,
>>   	TPM_RETRY = 50,		/* 5 seconds */
>> +	TPM_NUM_EVENT_LOG_FILES = 3,
>>   };
>>
>>   enum tpm_timeout {
>> @@ -171,7 +172,7 @@ struct tpm_chip {
>>   	unsigned long duration[3]; /* jiffies */
>>   	bool duration_adjusted;
>>
>> -	struct dentry **bios_dir;
>> +	struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES];
>>
>>   	const struct attribute_group *groups[3];
>>   	unsigned int groups_cnt;
>> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
>> index 9467e31..62e9da6 100644
>> --- a/drivers/char/tpm/tpm_eventlog.c
>> +++ b/drivers/char/tpm/tpm_eventlog.c
>> @@ -368,54 +368,47 @@ static int is_bad(void *p)
>>   	return 0;
>>   }
>>
>> -struct dentry **tpm_bios_log_setup(const char *name)
>> +int tpm_bios_log_setup(struct tpm_chip *chip)
>>   {
>> -	struct dentry **ret = NULL, *tpm_dir, *bin_file, *ascii_file;
>> +	const char *name = dev_name(&chip->dev);
>> +	unsigned int cnt;
>>
>> -	tpm_dir = securityfs_create_dir(name, NULL);
>> -	if (is_bad(tpm_dir))
>> -		goto out;
>> +	cnt = 0;
>> +	chip->bios_dir[cnt] = securityfs_create_dir(name, NULL);
>> +	if (is_bad(chip->bios_dir[cnt]))
>> +		goto err;
>> +	cnt++;
>>
>> -	bin_file =
>> +	chip->bios_dir[cnt] =
>>   	    securityfs_create_file("binary_bios_measurements",
>> -				   0440, tpm_dir,
>> +				   0440, chip->bios_dir[0],
>>   				   (void *)&tpm_binary_b_measurements_seqops,
>>   				   &tpm_bios_measurements_ops);
>> -	if (is_bad(bin_file))
>> -		goto out_tpm;
>> +	if (is_bad(chip->bios_dir[cnt]))
>> +		goto err;
>> +	cnt++;
>>
>> -	ascii_file =
>> +	chip->bios_dir[cnt] =
>>   	    securityfs_create_file("ascii_bios_measurements",
>> -				   0440, tpm_dir,
>> +				   0440, chip->bios_dir[0],
>>   				   (void *)&tpm_ascii_b_measurements_seqops,
>>   				   &tpm_bios_measurements_ops);
>> -	if (is_bad(ascii_file))
>> -		goto out_bin;
>> +	if (is_bad(chip->bios_dir[cnt]))
>> +		goto err;
>> +	cnt++;
>>
>> -	ret = kmalloc(3 * sizeof(struct dentry *), GFP_KERNEL);
>> -	if (!ret)
>> -		goto out_ascii;
>> -
>> -	ret[0] = ascii_file;
>> -	ret[1] = bin_file;
>> -	ret[2] = tpm_dir;
>> -
>> -	return ret;
>> +	return 0;
>>
>> -out_ascii:
>> -	securityfs_remove(ascii_file);
>> -out_bin:
>> -	securityfs_remove(bin_file);
>> -out_tpm:
>> -	securityfs_remove(tpm_dir);
>> -out:
>> -	return NULL;
>> +err:
>> +	chip->bios_dir[cnt] = NULL;
>> +	tpm_bios_log_teardown(chip);
>> +	return -EIO;
>>   }
>>
>> -void tpm_bios_log_teardown(struct dentry **lst)
>> +void tpm_bios_log_teardown(struct tpm_chip *chip)
>>   {
>>   	int i;
>>
>> -	for (i = 0; i < 3; i++)
>> -		securityfs_remove(lst[i]);
>> +	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--)
>> +		securityfs_remove(chip->bios_dir[i]);
>>   }
>> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
>> index 8de62b0..fd3357e 100644
>> --- a/drivers/char/tpm/tpm_eventlog.h
>> +++ b/drivers/char/tpm/tpm_eventlog.h
>> @@ -77,14 +77,14 @@ int read_log(struct tpm_bios_log *log);
>>
>>   #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
>>   	defined(CONFIG_ACPI)
>> -extern struct dentry **tpm_bios_log_setup(const char *);
>> -extern void tpm_bios_log_teardown(struct dentry **);
>> +extern int tpm_bios_log_setup(struct tpm_chip *chip);
>> +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
>>   #else
>> -static inline struct dentry **tpm_bios_log_setup(const char *name)
>> +static inline int tpm_bios_log_setup(struct tpm_chip *chip)
>>   {
>> -	return NULL;
>> +	return 0;
>>   }
>> -static inline void tpm_bios_log_teardown(struct dentry **dir)
>> +static inline void tpm_bios_log_teardown(struct tpm_chip *chip)
>>   {
>>   }
>>   #endif
>> --
>> 2.5.0
>>
>

[toc] | [prev] | [next] | [standalone]

#1529316 — Re: [PATCH v6 3/9] tpm: replace dynamically allocated bios_dir with a static array

On Wed, Nov 23, 2016 at 12:56:19AM +0530, Nayna wrote:
> 
> 
> On 11/22/2016 04:53 PM, Jarkko Sakkinen wrote:
> > On Mon, Nov 14, 2016 at 05:00:50AM -0500, Nayna Jain wrote:
> > > This commit is based on a commit by Nayna Jain. Replaced dynamically
> > > allocated bios_dir with a static array as the size is always constant.
> > > 
> > > Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 
> > This commit remains unreviewed and tested. I'm in the author role here
> > so I cannot help with this. If that does not happen soon I cannot put
> > this into the pull request.
> 
> Tested-By: Nayna Jain <nayna@linux.vnet.ibm.com>

Thanks.

/Jarkko

> 
> Thanks & Regards,
>   - Nayna
> 
> > 
> > /Jarkko
> > 
> > 
> > > ---
> > >   drivers/char/tpm/tpm-chip.c     |  9 ++++---
> > >   drivers/char/tpm/tpm.h          |  3 ++-
> > >   drivers/char/tpm/tpm_eventlog.c | 59 ++++++++++++++++++-----------------------
> > >   drivers/char/tpm/tpm_eventlog.h | 10 +++----
> > >   4 files changed, 38 insertions(+), 43 deletions(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > > index 836f056..d0c1872 100644
> > > --- a/drivers/char/tpm/tpm-chip.c
> > > +++ b/drivers/char/tpm/tpm-chip.c
> > > @@ -278,14 +278,16 @@ static void tpm_del_char_device(struct tpm_chip *chip)
> > > 
> > >   static int tpm1_chip_register(struct tpm_chip *chip)
> > >   {
> > > +	int rc;
> > > +
> > >   	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> > >   		return 0;
> > > 
> > >   	tpm_sysfs_add_device(chip);
> > > 
> > > -	chip->bios_dir = tpm_bios_log_setup(dev_name(&chip->dev));
> > > +	rc = tpm_bios_log_setup(chip);
> > > 
> > > -	return 0;
> > > +	return rc;
> > >   }
> > > 
> > >   static void tpm1_chip_unregister(struct tpm_chip *chip)
> > > @@ -293,8 +295,7 @@ static void tpm1_chip_unregister(struct tpm_chip *chip)
> > >   	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> > >   		return;
> > > 
> > > -	if (chip->bios_dir)
> > > -		tpm_bios_log_teardown(chip->bios_dir);
> > > +	tpm_bios_log_teardown(chip);
> > >   }
> > > 
> > >   static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
> > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > > index f9401ca..9d69580 100644
> > > --- a/drivers/char/tpm/tpm.h
> > > +++ b/drivers/char/tpm/tpm.h
> > > @@ -40,6 +40,7 @@ enum tpm_const {
> > >   	TPM_BUFSIZE = 4096,
> > >   	TPM_NUM_DEVICES = 65536,
> > >   	TPM_RETRY = 50,		/* 5 seconds */
> > > +	TPM_NUM_EVENT_LOG_FILES = 3,
> > >   };
> > > 
> > >   enum tpm_timeout {
> > > @@ -171,7 +172,7 @@ struct tpm_chip {
> > >   	unsigned long duration[3]; /* jiffies */
> > >   	bool duration_adjusted;
> > > 
> > > -	struct dentry **bios_dir;
> > > +	struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES];
> > > 
> > >   	const struct attribute_group *groups[3];
> > >   	unsigned int groups_cnt;
> > > diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> > > index 9467e31..62e9da6 100644
> > > --- a/drivers/char/tpm/tpm_eventlog.c
> > > +++ b/drivers/char/tpm/tpm_eventlog.c
> > > @@ -368,54 +368,47 @@ static int is_bad(void *p)
> > >   	return 0;
> > >   }
> > > 
> > > -struct dentry **tpm_bios_log_setup(const char *name)
> > > +int tpm_bios_log_setup(struct tpm_chip *chip)
> > >   {
> > > -	struct dentry **ret = NULL, *tpm_dir, *bin_file, *ascii_file;
> > > +	const char *name = dev_name(&chip->dev);
> > > +	unsigned int cnt;
> > > 
> > > -	tpm_dir = securityfs_create_dir(name, NULL);
> > > -	if (is_bad(tpm_dir))
> > > -		goto out;
> > > +	cnt = 0;
> > > +	chip->bios_dir[cnt] = securityfs_create_dir(name, NULL);
> > > +	if (is_bad(chip->bios_dir[cnt]))
> > > +		goto err;
> > > +	cnt++;
> > > 
> > > -	bin_file =
> > > +	chip->bios_dir[cnt] =
> > >   	    securityfs_create_file("binary_bios_measurements",
> > > -				   0440, tpm_dir,
> > > +				   0440, chip->bios_dir[0],
> > >   				   (void *)&tpm_binary_b_measurements_seqops,
> > >   				   &tpm_bios_measurements_ops);
> > > -	if (is_bad(bin_file))
> > > -		goto out_tpm;
> > > +	if (is_bad(chip->bios_dir[cnt]))
> > > +		goto err;
> > > +	cnt++;
> > > 
> > > -	ascii_file =
> > > +	chip->bios_dir[cnt] =
> > >   	    securityfs_create_file("ascii_bios_measurements",
> > > -				   0440, tpm_dir,
> > > +				   0440, chip->bios_dir[0],
> > >   				   (void *)&tpm_ascii_b_measurements_seqops,
> > >   				   &tpm_bios_measurements_ops);
> > > -	if (is_bad(ascii_file))
> > > -		goto out_bin;
> > > +	if (is_bad(chip->bios_dir[cnt]))
> > > +		goto err;
> > > +	cnt++;
> > > 
> > > -	ret = kmalloc(3 * sizeof(struct dentry *), GFP_KERNEL);
> > > -	if (!ret)
> > > -		goto out_ascii;
> > > -
> > > -	ret[0] = ascii_file;
> > > -	ret[1] = bin_file;
> > > -	ret[2] = tpm_dir;
> > > -
> > > -	return ret;
> > > +	return 0;
> > > 
> > > -out_ascii:
> > > -	securityfs_remove(ascii_file);
> > > -out_bin:
> > > -	securityfs_remove(bin_file);
> > > -out_tpm:
> > > -	securityfs_remove(tpm_dir);
> > > -out:
> > > -	return NULL;
> > > +err:
> > > +	chip->bios_dir[cnt] = NULL;
> > > +	tpm_bios_log_teardown(chip);
> > > +	return -EIO;
> > >   }
> > > 
> > > -void tpm_bios_log_teardown(struct dentry **lst)
> > > +void tpm_bios_log_teardown(struct tpm_chip *chip)
> > >   {
> > >   	int i;
> > > 
> > > -	for (i = 0; i < 3; i++)
> > > -		securityfs_remove(lst[i]);
> > > +	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--)
> > > +		securityfs_remove(chip->bios_dir[i]);
> > >   }
> > > diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
> > > index 8de62b0..fd3357e 100644
> > > --- a/drivers/char/tpm/tpm_eventlog.h
> > > +++ b/drivers/char/tpm/tpm_eventlog.h
> > > @@ -77,14 +77,14 @@ int read_log(struct tpm_bios_log *log);
> > > 
> > >   #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
> > >   	defined(CONFIG_ACPI)
> > > -extern struct dentry **tpm_bios_log_setup(const char *);
> > > -extern void tpm_bios_log_teardown(struct dentry **);
> > > +extern int tpm_bios_log_setup(struct tpm_chip *chip);
> > > +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
> > >   #else
> > > -static inline struct dentry **tpm_bios_log_setup(const char *name)
> > > +static inline int tpm_bios_log_setup(struct tpm_chip *chip)
> > >   {
> > > -	return NULL;
> > > +	return 0;
> > >   }
> > > -static inline void tpm_bios_log_teardown(struct dentry **dir)
> > > +static inline void tpm_bios_log_teardown(struct tpm_chip *chip)
> > >   {
> > >   }
> > >   #endif
> > > --
> > > 2.5.0
> > > 
> > 
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web