[tpmdd-devel,RESEND,2/3] tpm: replace dynamically allocated bios_dir with a static array
diff mbox

Message ID 1475349926-476-3-git-send-email-jarkko.sakkinen@linux.intel.com
State New
Headers show

Commit Message

Jarkko Sakkinen Oct. 1, 2016, 7:25 p.m. UTC
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(-)

Comments

Jason Gunthorpe Oct. 2, 2016, 9:28 p.m. UTC | #1
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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jarkko Sakkinen Oct. 3, 2016, 12:21 p.m. UTC | #2
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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jarkko Sakkinen Oct. 8, 2016, 11:01 a.m. UTC | #3
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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Nayna Jain Oct. 10, 2016, 6:53 p.m. UTC | #4
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
>


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jarkko Sakkinen Oct. 11, 2016, 11:23 a.m. UTC | #5
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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jason Gunthorpe Oct. 11, 2016, 4:53 p.m. UTC | #6
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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Nayna Jain Oct. 11, 2016, 5:19 p.m. UTC | #7
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
>


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jarkko Sakkinen Oct. 11, 2016, 6:09 p.m. UTC | #8
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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jarkko Sakkinen Oct. 11, 2016, 6:12 p.m. UTC | #9
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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Patch
diff mbox

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