[PATCH RESEND 0/3] Clean up handling of event log files

[PATCH RESEND 0/3] Clean up handling of event log files

[Jarkko Sakkinen]

Meld a common function for opening measurements files and keep the
dentries in a static array.

Jarkko Sakkinen (2):
  tpm: replace dynamically allocated bios_dir with a static array
  tpm: drop tpm1_chip_register(/unregister)

Nayna Jain (1):
  tpm: define a generic open() method for ascii & bios measurements

 drivers/char/tpm/tpm-chip.c     |  30 ++--------
 drivers/char/tpm/tpm-sysfs.c    |   3 +
 drivers/char/tpm/tpm.h          |   3 +-
 drivers/char/tpm/tpm_eventlog.c | 129 +++++++++++++++-------------------------
 drivers/char/tpm/tpm_eventlog.h |  10 ++--
 5 files changed, 64 insertions(+), 111 deletions(-)

-- 
2.7.4

[PATCH RESEND 1/3] tpm: define a generic open() method for ascii & bios measurements

[Jarkko Sakkinen]

From: Nayna Jain <nayna@linux.vnet.ibm.com>

open() method for event log ascii and binary bios measurements file
operations are very similar. This patch refactors the code into a
single open() call by passing seq_operations as i_node->private data.

Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/char/tpm/tpm_eventlog.c | 59 +++++++++--------------------------------
 1 file changed, 13 insertions(+), 46 deletions(-)

diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index e722886..75e6644 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -7,6 +7,7 @@
  *	Stefan Berger <stefanb@us.ibm.com>
  *	Reiner Sailer <sailer@watson.ibm.com>
  *	Kylene Hall <kjhall@us.ibm.com>
+ *	Nayna Jain <nayna@linux.vnet.ibm.com>
  *
  * Maintained by: <tpmdd-devel@lists.sourceforge.net>
  *
@@ -318,12 +319,14 @@ static const struct seq_operations tpm_binary_b_measurments_seqops = {
 	.show = tpm_binary_bios_measurements_show,
 };
 
-static int tpm_ascii_bios_measurements_open(struct inode *inode,
+static int tpm_bios_measurements_open(struct inode *inode,
 					    struct file *file)
 {
 	int err;
 	struct tpm_bios_log *log;
 	struct seq_file *seq;
+	const struct seq_operations *seqops =
+		(const struct seq_operations *)inode->i_private;
 
 	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
 	if (!log)
@@ -333,7 +336,7 @@ static int tpm_ascii_bios_measurements_open(struct inode *inode,
 		goto out_free;
 
 	/* now register seq file */
-	err = seq_open(file, &tpm_ascii_b_measurments_seqops);
+	err = seq_open(file, seqops);
 	if (!err) {
 		seq = file->private_data;
 		seq->private = log;
@@ -349,46 +352,8 @@ out_free:
 	goto out;
 }
 
-static const struct file_operations tpm_ascii_bios_measurements_ops = {
-	.open = tpm_ascii_bios_measurements_open,
-	.read = seq_read,
-	.llseek = seq_lseek,
-	.release = tpm_bios_measurements_release,
-};
-
-static int tpm_binary_bios_measurements_open(struct inode *inode,
-					     struct file *file)
-{
-	int err;
-	struct tpm_bios_log *log;
-	struct seq_file *seq;
-
-	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
-	if (!log)
-		return -ENOMEM;
-
-	if ((err = read_log(log)))
-		goto out_free;
-
-	/* now register seq file */
-	err = seq_open(file, &tpm_binary_b_measurments_seqops);
-	if (!err) {
-		seq = file->private_data;
-		seq->private = log;
-	} else {
-		goto out_free;
-	}
-
-out:
-	return err;
-out_free:
-	kfree(log->bios_event_log);
-	kfree(log);
-	goto out;
-}
-
-static const struct file_operations tpm_binary_bios_measurements_ops = {
-	.open = tpm_binary_bios_measurements_open,
+static const struct file_operations tpm_bios_measurements_ops = {
+	.open = tpm_bios_measurements_open,
 	.read = seq_read,
 	.llseek = seq_lseek,
 	.release = tpm_bios_measurements_release,
@@ -413,15 +378,17 @@ struct dentry **tpm_bios_log_setup(const char *name)
 
 	bin_file =
 	    securityfs_create_file("binary_bios_measurements",
-				   S_IRUSR | S_IRGRP, tpm_dir, NULL,
-				   &tpm_binary_bios_measurements_ops);
+				   S_IRUSR | S_IRGRP, tpm_dir,
+				   (void *)&tpm_binary_b_measurments_seqops,
+				   &tpm_bios_measurements_ops);
 	if (is_bad(bin_file))
 		goto out_tpm;
 
 	ascii_file =
 	    securityfs_create_file("ascii_bios_measurements",
-				   S_IRUSR | S_IRGRP, tpm_dir, NULL,
-				   &tpm_ascii_bios_measurements_ops);
+				   S_IRUSR | S_IRGRP, tpm_dir,
+				   (void *)&tpm_ascii_b_measurments_seqops,
+				   &tpm_bios_measurements_ops);
 	if (is_bad(ascii_file))
 		goto out_bin;
 
-- 
2.7.4

[PATCH RESEND 2/3] tpm: replace dynamically allocated bios_dir with a static array

[Jarkko Sakkinen]

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 | 62 +++++++++++++++++++----------------------
 drivers/char/tpm/tpm_eventlog.h | 10 +++----
 4 files changed, 41 insertions(+), 43 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index e595013..a56b609 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 4d183c9..4c118a4 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 75e6644..f5e1d06 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -368,54 +368,50 @@ 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",
-				   S_IRUSR | S_IRGRP, tpm_dir,
+				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
 				   (void *)&tpm_binary_b_measurments_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",
-				   S_IRUSR | S_IRGRP, tpm_dir,
+				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
 				   (void *)&tpm_ascii_b_measurments_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--) {
+		if (chip->bios_dir[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.7.4

[PATCH RESEND 3/3] tpm: drop tpm1_chip_register(/unregister)

[Jarkko Sakkinen]

Check for TPM2 chip in tpm_sysfs_add_device, tpm_bios_log_setup and
tpm_bios_log_teardown in order to make code flow cleaner and to enable
to implement TPM 2.0 support later on. This is partially derived from
the commit by Nayna Jain with the extension that also tpm1_chip_register
is dropped.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-chip.c     | 31 +++++--------------------------
 drivers/char/tpm/tpm-sysfs.c    |  3 +++
 drivers/char/tpm/tpm_eventlog.c |  6 ++++++
 3 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index a56b609..eac1f10 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -276,28 +276,6 @@ static void tpm_del_char_device(struct tpm_chip *chip)
 	up_write(&chip->ops_sem);
 }
 
-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);
-
-	rc = tpm_bios_log_setup(chip);
-
-	return rc;
-}
-
-static void tpm1_chip_unregister(struct tpm_chip *chip)
-{
-	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		return;
-
-	tpm_bios_log_teardown(chip);
-}
-
 static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
 {
 	struct attribute **i;
@@ -364,7 +342,9 @@ int tpm_chip_register(struct tpm_chip *chip)
 			return rc;
 	}
 
-	rc = tpm1_chip_register(chip);
+	tpm_sysfs_add_device(chip);
+
+	rc = tpm_bios_log_setup(chip);
 	if (rc)
 		return rc;
 
@@ -372,7 +352,7 @@ int tpm_chip_register(struct tpm_chip *chip)
 
 	rc = tpm_add_char_device(chip);
 	if (rc) {
-		tpm1_chip_unregister(chip);
+		tpm_bios_log_teardown(chip);
 		return rc;
 	}
 
@@ -407,8 +387,7 @@ void tpm_chip_unregister(struct tpm_chip *chip)
 		return;
 
 	tpm_del_legacy_sysfs(chip);
-
-	tpm1_chip_unregister(chip);
+	tpm_bios_log_teardown(chip);
 	tpm_del_char_device(chip);
 }
 EXPORT_SYMBOL_GPL(tpm_chip_unregister);
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index a76ab4a..1eca5ec 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -284,6 +284,9 @@ static const struct attribute_group tpm_dev_group = {
 
 void tpm_sysfs_add_device(struct tpm_chip *chip)
 {
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		return;
+
 	/* The sysfs routines rely on an implicit tpm_try_get_ops, device_del
 	 * is called before ops is null'd and the sysfs core synchronizes this
 	 * removal so that no callbacks are running or can run again
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index f5e1d06..c57b981 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -373,6 +373,9 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
 	const char *name = dev_name(&chip->dev);
 	unsigned int cnt;
 
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		return 0;
+
 	cnt = 0;
 	chip->bios_dir[cnt] =
 		securityfs_create_dir(name, NULL);
@@ -410,6 +413,9 @@ void tpm_bios_log_teardown(struct tpm_chip *chip)
 {
 	int i;
 
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		return;
+
 	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
 		if (chip->bios_dir[i])
 			securityfs_remove(chip->bios_dir[i]);
-- 
2.7.4

Re: [PATCH RESEND 2/3] tpm: replace dynamically allocated bios_dir with a static array

[Jason Gunthorpe]

On Sat, Oct 01, 2016 at 10:25:25PM +0300, Jarkko Sakkinen wrote:

> +	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
> +		if (chip->bios_dir[i])

The entries can't actually be null here, right?

Jason

Re: [PATCH RESEND 2/3] tpm: replace dynamically allocated bios_dir with a static array

[Jarkko Sakkinen]

On Sun, Oct 02, 2016 at 03:28:01PM -0600, Jason Gunthorpe wrote:
> On Sat, Oct 01, 2016 at 10:25:25PM +0300, Jarkko Sakkinen wrote:
> 
> > +	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
> > +		if (chip->bios_dir[i])
> 
> The entries can't actually be null here, right?

They can because this function is called as a rollbcak procedure for
tpm_bios_log_setup.

> Jason

/Jarkko

Re: [PATCH RESEND 1/3] tpm: define a generic open() method for ascii & bios measurements

[Jarkko Sakkinen]

On Sat, Oct 01, 2016 at 10:25:24PM +0300, Jarkko Sakkinen wrote:
> From: Nayna Jain <nayna@linux.vnet.ibm.com>
> 
> open() method for event log ascii and binary bios measurements file
> operations are very similar. This patch refactors the code into a
> single open() call by passing seq_operations as i_node->private data.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

Re: [PATCH RESEND 2/3] tpm: replace dynamically allocated bios_dir with a static array

[Jarkko Sakkinen]

On Mon, Oct 03, 2016 at 03:21:35PM +0300, Jarkko Sakkinen wrote:
> On Sun, Oct 02, 2016 at 03:28:01PM -0600, Jason Gunthorpe wrote:
> > On Sat, Oct 01, 2016 at 10:25:25PM +0300, Jarkko Sakkinen wrote:
> > 
> > > +	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
> > > +		if (chip->bios_dir[i])
> > 
> > The entries can't actually be null here, right?
> 
> They can because this function is called as a rollbcak procedure for
> tpm_bios_log_setup.

I've tested these with Dell E6400 laptop and IvyBridge NUC both with TPM
1.2 chip by repeating this a few iterations:

1. modprobe tpm_tis
2. Outputted sysfs attributes
3. Outputted measurement files.
4. rmmod tpm_tis

I think these three commits could be applied at any point and would help
with the event log series (shrunk it a bit).

/Jarkko

Re: [PATCH RESEND 2/3] tpm: replace dynamically allocated bios_dir with a static array

[Nayna]

On 10/02/2016 12:55 AM, Jarkko Sakkinen 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>
> ---
>   drivers/char/tpm/tpm-chip.c     |  9 +++---
>   drivers/char/tpm/tpm.h          |  3 +-
>   drivers/char/tpm/tpm_eventlog.c | 62 +++++++++++++++++++----------------------
>   drivers/char/tpm/tpm_eventlog.h | 10 +++----
>   4 files changed, 41 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index e595013..a56b609 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 4d183c9..4c118a4 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 75e6644..f5e1d06 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -368,54 +368,50 @@ 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",
> -				   S_IRUSR | S_IRGRP, tpm_dir,
> +				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
>   				   (void *)&tpm_binary_b_measurments_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",
> -				   S_IRUSR | S_IRGRP, tpm_dir,
> +				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
>   				   (void *)&tpm_ascii_b_measurments_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;

The updated patch looks fine.
Just, I am not sure if NULL assignment is needed.

> +	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--) {
> +		if (chip->bios_dir[i])

Probably, this check is not required because securityfs_remove() takes 
care of checking both NULL and err dentry.

Thanks & Regards,
    - Nayna

> +			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
>

Re: [PATCH RESEND 2/3] tpm: replace dynamically allocated bios_dir with a static array

[Jarkko Sakkinen]

On Tue, Oct 11, 2016 at 12:23:15AM +0530, Nayna wrote:
> 
> 
> On 10/02/2016 12:55 AM, Jarkko Sakkinen 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>
> >---
> >  drivers/char/tpm/tpm-chip.c     |  9 +++---
> >  drivers/char/tpm/tpm.h          |  3 +-
> >  drivers/char/tpm/tpm_eventlog.c | 62 +++++++++++++++++++----------------------
> >  drivers/char/tpm/tpm_eventlog.h | 10 +++----
> >  4 files changed, 41 insertions(+), 43 deletions(-)
> >
> >diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> >index e595013..a56b609 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 4d183c9..4c118a4 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 75e6644..f5e1d06 100644
> >--- a/drivers/char/tpm/tpm_eventlog.c
> >+++ b/drivers/char/tpm/tpm_eventlog.c
> >@@ -368,54 +368,50 @@ 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",
> >-				   S_IRUSR | S_IRGRP, tpm_dir,
> >+				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> >  				   (void *)&tpm_binary_b_measurments_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",
> >-				   S_IRUSR | S_IRGRP, tpm_dir,
> >+				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> >  				   (void *)&tpm_ascii_b_measurments_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;
> 
> The updated patch looks fine.
> Just, I am not sure if NULL assignment is needed.

It's not needed.

> >+	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--) {
> >+		if (chip->bios_dir[i])
> 
> Probably, this check is not required because securityfs_remove() takes care
> of checking both NULL and err dentry.

Thanks. I did the adjustments to the eventlog branch. Please use these
three patches as "head" for your series. I'll apply the rest of patches
to this branch when you post the next version of the series.

Please also include these three patches to the next version when you
post even if they are applied there. The topic branch is there to make
the testing easier.

> Thanks & Regards,
>    - Nayna

/Jarkko

Re: [PATCH RESEND 2/3] tpm: replace dynamically allocated bios_dir with a static array

[Jason Gunthorpe]

On Tue, Oct 11, 2016 at 02:23:15PM +0300, Jarkko Sakkinen wrote:
> > >+	chip->bios_dir[cnt] =
> > >  	    securityfs_create_file("ascii_bios_measurements",
> > >-				   S_IRUSR | S_IRGRP, tpm_dir,
> > >+				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> > >  				   (void *)&tpm_ascii_b_measurments_seqops,
> > >  				   &tpm_bios_measurements_ops);

> > >+	if (is_bad(chip->bios_dir[cnt]))
> > >+		goto err;

> > >+err:
> > >+	chip->bios_dir[cnt] = NULL;
> > 
> > The updated patch looks fine.
> > Just, I am not sure if NULL assignment is needed.
> 
> It's not needed.

It is required to switch an ERR_PTR to NULL, see is_bad()

The original version that used a counter in chip did not need it.

Jason

Re: [PATCH RESEND 2/3] tpm: replace dynamically allocated bios_dir with a static array

[Nayna]

On 10/11/2016 10:23 PM, Jason Gunthorpe wrote:
> On Tue, Oct 11, 2016 at 02:23:15PM +0300, Jarkko Sakkinen wrote:
>>>> +	chip->bios_dir[cnt] =
>>>>   	    securityfs_create_file("ascii_bios_measurements",
>>>> -				   S_IRUSR | S_IRGRP, tpm_dir,
>>>> +				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
>>>>   				   (void *)&tpm_ascii_b_measurments_seqops,
>>>>   				   &tpm_bios_measurements_ops);
>
>>>> +	if (is_bad(chip->bios_dir[cnt]))
>>>> +		goto err;
>
>>>> +err:
>>>> +	chip->bios_dir[cnt] = NULL;
>>>
>>> The updated patch looks fine.
>>> Just, I am not sure if NULL assignment is needed.
>>
>> It's not needed.
>
> It is required to switch an ERR_PTR to NULL, see is_bad()

My understanding is that securityfs_remove() takes care of both NULL and 
ERR_PTR().

 From securityfs_remove():

  if (!dentry || IS_ERR(dentry))
                 return;

Thanks & Regards,
   - Nayna

>
> The original version that used a counter in chip did not need it.
>
> Jason
>

Re: [PATCH RESEND 2/3] tpm: replace dynamically allocated bios_dir with a static array

[Jarkko Sakkinen]

On Tue, Oct 11, 2016 at 10:53:55AM -0600, Jason Gunthorpe wrote:
> On Tue, Oct 11, 2016 at 02:23:15PM +0300, Jarkko Sakkinen wrote:
> > > >+	chip->bios_dir[cnt] =
> > > >  	    securityfs_create_file("ascii_bios_measurements",
> > > >-				   S_IRUSR | S_IRGRP, tpm_dir,
> > > >+				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> > > >  				   (void *)&tpm_ascii_b_measurments_seqops,
> > > >  				   &tpm_bios_measurements_ops);
> 
> > > >+	if (is_bad(chip->bios_dir[cnt]))
> > > >+		goto err;
> 
> > > >+err:
> > > >+	chip->bios_dir[cnt] = NULL;
> > > 
> > > The updated patch looks fine.
> > > Just, I am not sure if NULL assignment is needed.
> > 
> > It's not needed.
> 
> It is required to switch an ERR_PTR to NULL, see is_bad()
> 
> The original version that used a counter in chip did not need it.

Oops, sorry. I added it back. Thanks for pointing this out.

> Jason

/Jarkko

Re: [PATCH RESEND 2/3] tpm: replace dynamically allocated bios_dir with a static array

[Jarkko Sakkinen]

On Tue, Oct 11, 2016 at 10:49:56PM +0530, Nayna wrote:
> 
> 
> On 10/11/2016 10:23 PM, Jason Gunthorpe wrote:
> >On Tue, Oct 11, 2016 at 02:23:15PM +0300, Jarkko Sakkinen wrote:
> >>>>+	chip->bios_dir[cnt] =
> >>>>  	    securityfs_create_file("ascii_bios_measurements",
> >>>>-				   S_IRUSR | S_IRGRP, tpm_dir,
> >>>>+				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> >>>>  				   (void *)&tpm_ascii_b_measurments_seqops,
> >>>>  				   &tpm_bios_measurements_ops);
> >
> >>>>+	if (is_bad(chip->bios_dir[cnt]))
> >>>>+		goto err;
> >
> >>>>+err:
> >>>>+	chip->bios_dir[cnt] = NULL;
> >>>
> >>>The updated patch looks fine.
> >>>Just, I am not sure if NULL assignment is needed.
> >>
> >>It's not needed.
> >
> >It is required to switch an ERR_PTR to NULL, see is_bad()
> 
> My understanding is that securityfs_remove() takes care of both NULL and
> ERR_PTR().
> 
> From securityfs_remove():
> 
>  if (!dentry || IS_ERR(dentry))
>                 return;

Right. I just checked from LXR. Seems weird that they have that kind of
special handling for IS_ERR. Checking for NULL is more usual.  I think I
keep setting NULL anyway if nothing else for robustness.  Anyway, thanks
for pointing this out.

> Thanks & Regards,
>   - Nayna

/Jarkko