diff mbox series

[v2,1/2] tpm: Use managed allocation for bios event log

Message ID 20230113161017.1079299-2-eajames@linux.ibm.com
State New
Headers
Series tpm: Add reserved memory event log |

Commit Message

Eddie James Jan. 13, 2023, 4:10 p.m. UTC 
  Since the bios event log is freed in the device release function,
let devres handle the deallocation. This will allow other memory
allocation/mapping functions to be used for the bios event log.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/char/tpm/eventlog/acpi.c |  5 +++--
 drivers/char/tpm/eventlog/efi.c  | 13 +++++++------
 drivers/char/tpm/eventlog/of.c   |  3 ++-
 drivers/char/tpm/tpm-chip.c      |  1 -
 4 files changed, 12 insertions(+), 10 deletions(-)

Comments

Jarkko Sakkinen Jan. 21, 2023, 12:11 a.m. UTC | #1 
On Fri, Jan 13, 2023 at 10:10:16AM -0600, Eddie James wrote:
> Since the bios event log is freed in the device release function,
> let devres handle the deallocation. This will allow other memory
> allocation/mapping functions to be used for the bios event log.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  drivers/char/tpm/eventlog/acpi.c |  5 +++--
>  drivers/char/tpm/eventlog/efi.c  | 13 +++++++------
>  drivers/char/tpm/eventlog/of.c   |  3 ++-
>  drivers/char/tpm/tpm-chip.c      |  1 -
>  4 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c
> index 0913d3eb8d51..40360e599bc3 100644
> --- a/drivers/char/tpm/eventlog/acpi.c
> +++ b/drivers/char/tpm/eventlog/acpi.c
> @@ -14,6 +14,7 @@
>   * Access to the event log extended by the TCG BIOS of PC platform
>   */
>  
> +#include <linux/device.h>
>  #include <linux/seq_file.h>
>  #include <linux/fs.h>
>  #include <linux/security.h>
> @@ -135,7 +136,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
>  	}
>  
>  	/* malloc EventLog space */
> -	log->bios_event_log = kmalloc(len, GFP_KERNEL);
> +	log->bios_event_log = devm_kmalloc(&chip->dev, len, GFP_KERNEL);
>  	if (!log->bios_event_log)
>  		return -ENOMEM;
>  
> @@ -160,7 +161,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
>  	return format;
>  
>  err:
> -	kfree(log->bios_event_log);
> +	devm_kfree(&chip->dev, log->bios_event_log);

I wonder do we want to do devm_kfree's at all as the memory is freed during
detach, i.e. taken care by devres.

>  	log->bios_event_log = NULL;
>  	return ret;
>  }
> diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
> index e6cb9d525e30..4e9d7c2bf32e 100644
> --- a/drivers/char/tpm/eventlog/efi.c
> +++ b/drivers/char/tpm/eventlog/efi.c
> @@ -6,6 +6,7 @@
>   *      Thiebaud Weksteen <tweek@google.com>
>   */
>  
> +#include <linux/device.h>
>  #include <linux/efi.h>
>  #include <linux/tpm_eventlog.h>
>  
> @@ -55,7 +56,7 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>  	}
>  
>  	/* malloc EventLog space */
> -	log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL);
> +	log->bios_event_log = devm_kmemdup(&chip->dev, log_tbl->log, log_size, GFP_KERNEL);
>  	if (!log->bios_event_log) {
>  		ret = -ENOMEM;
>  		goto out;
> @@ -76,7 +77,7 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>  			     MEMREMAP_WB);
>  	if (!final_tbl) {
>  		pr_err("Could not map UEFI TPM final log\n");
> -		kfree(log->bios_event_log);
> +		devm_kfree(&chip->dev, log->bios_event_log);
>  		ret = -ENOMEM;
>  		goto out;
>  	}
> @@ -91,11 +92,11 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>  	 * Allocate memory for the 'combined log' where we will append the
>  	 * 'final events log' to.
>  	 */
> -	tmp = krealloc(log->bios_event_log,
> -		       log_size + final_events_log_size,
> -		       GFP_KERNEL);
> +	tmp = devm_krealloc(&chip->dev, log->bios_event_log,
> +			    log_size + final_events_log_size,
> +			    GFP_KERNEL);
>  	if (!tmp) {
> -		kfree(log->bios_event_log);
> +		devm_kfree(&chip->dev, log->bios_event_log);
>  		ret = -ENOMEM;
>  		goto out;
>  	}
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index a9ce66d09a75..741ab2204b11 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -10,6 +10,7 @@
>   * Read the event log created by the firmware on PPC64
>   */
>  
> +#include <linux/device.h>
>  #include <linux/slab.h>
>  #include <linux/of.h>
>  #include <linux/tpm_eventlog.h>
> @@ -65,7 +66,7 @@ int tpm_read_log_of(struct tpm_chip *chip)
>  		return -EIO;
>  	}
>  
> -	log->bios_event_log = kmemdup(__va(base), size, GFP_KERNEL);
> +	log->bios_event_log = devm_kmemdup(&chip->dev, __va(base), size, GFP_KERNEL);
>  	if (!log->bios_event_log)
>  		return -ENOMEM;
>  
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 741d8f3e8fb3..b99f55f2d4fd 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -267,7 +267,6 @@ static void tpm_dev_release(struct device *dev)
>  	idr_remove(&dev_nums_idr, chip->dev_num);
>  	mutex_unlock(&idr_lock);
>  
> -	kfree(chip->log.bios_event_log);
>  	kfree(chip->work_space.context_buf);
>  	kfree(chip->work_space.session_buf);
>  	kfree(chip->allocated_banks);
> -- 
> 2.31.1
> 

BR, Jarkko
Eddie James Jan. 25, 2023, 7:06 p.m. UTC | #2 
On 1/20/23 18:11, Jarkko Sakkinen wrote:
> On Fri, Jan 13, 2023 at 10:10:16AM -0600, Eddie James wrote:
>> Since the bios event log is freed in the device release function,
>> let devres handle the deallocation. This will allow other memory
>> allocation/mapping functions to be used for the bios event log.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>>   drivers/char/tpm/eventlog/acpi.c |  5 +++--
>>   drivers/char/tpm/eventlog/efi.c  | 13 +++++++------
>>   drivers/char/tpm/eventlog/of.c   |  3 ++-
>>   drivers/char/tpm/tpm-chip.c      |  1 -
>>   4 files changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c
>> index 0913d3eb8d51..40360e599bc3 100644
>> --- a/drivers/char/tpm/eventlog/acpi.c
>> +++ b/drivers/char/tpm/eventlog/acpi.c
>> @@ -14,6 +14,7 @@
>>    * Access to the event log extended by the TCG BIOS of PC platform
>>    */
>>   
>> +#include <linux/device.h>
>>   #include <linux/seq_file.h>
>>   #include <linux/fs.h>
>>   #include <linux/security.h>
>> @@ -135,7 +136,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
>>   	}
>>   
>>   	/* malloc EventLog space */
>> -	log->bios_event_log = kmalloc(len, GFP_KERNEL);
>> +	log->bios_event_log = devm_kmalloc(&chip->dev, len, GFP_KERNEL);
>>   	if (!log->bios_event_log)
>>   		return -ENOMEM;
>>   
>> @@ -160,7 +161,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
>>   	return format;
>>   
>>   err:
>> -	kfree(log->bios_event_log);
>> +	devm_kfree(&chip->dev, log->bios_event_log);
> I wonder do we want to do devm_kfree's at all as the memory is freed during
> detach, i.e. taken care by devres.


I think we should since the chip/tpm driver will continue to probe 
without the bios event log. Therefore that memory will be wasted if 
there is some error during bios log setup.


Thanks,

Eddie


>
>>   	log->bios_event_log = NULL;
>>   	return ret;
>>   }
>> diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
>> index e6cb9d525e30..4e9d7c2bf32e 100644
>> --- a/drivers/char/tpm/eventlog/efi.c
>> +++ b/drivers/char/tpm/eventlog/efi.c
>> @@ -6,6 +6,7 @@
>>    *      Thiebaud Weksteen <tweek@google.com>
>>    */
>>   
>> +#include <linux/device.h>
>>   #include <linux/efi.h>
>>   #include <linux/tpm_eventlog.h>
>>   
>> @@ -55,7 +56,7 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>>   	}
>>   
>>   	/* malloc EventLog space */
>> -	log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL);
>> +	log->bios_event_log = devm_kmemdup(&chip->dev, log_tbl->log, log_size, GFP_KERNEL);
>>   	if (!log->bios_event_log) {
>>   		ret = -ENOMEM;
>>   		goto out;
>> @@ -76,7 +77,7 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>>   			     MEMREMAP_WB);
>>   	if (!final_tbl) {
>>   		pr_err("Could not map UEFI TPM final log\n");
>> -		kfree(log->bios_event_log);
>> +		devm_kfree(&chip->dev, log->bios_event_log);
>>   		ret = -ENOMEM;
>>   		goto out;
>>   	}
>> @@ -91,11 +92,11 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>>   	 * Allocate memory for the 'combined log' where we will append the
>>   	 * 'final events log' to.
>>   	 */
>> -	tmp = krealloc(log->bios_event_log,
>> -		       log_size + final_events_log_size,
>> -		       GFP_KERNEL);
>> +	tmp = devm_krealloc(&chip->dev, log->bios_event_log,
>> +			    log_size + final_events_log_size,
>> +			    GFP_KERNEL);
>>   	if (!tmp) {
>> -		kfree(log->bios_event_log);
>> +		devm_kfree(&chip->dev, log->bios_event_log);
>>   		ret = -ENOMEM;
>>   		goto out;
>>   	}
>> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
>> index a9ce66d09a75..741ab2204b11 100644
>> --- a/drivers/char/tpm/eventlog/of.c
>> +++ b/drivers/char/tpm/eventlog/of.c
>> @@ -10,6 +10,7 @@
>>    * Read the event log created by the firmware on PPC64
>>    */
>>   
>> +#include <linux/device.h>
>>   #include <linux/slab.h>
>>   #include <linux/of.h>
>>   #include <linux/tpm_eventlog.h>
>> @@ -65,7 +66,7 @@ int tpm_read_log_of(struct tpm_chip *chip)
>>   		return -EIO;
>>   	}
>>   
>> -	log->bios_event_log = kmemdup(__va(base), size, GFP_KERNEL);
>> +	log->bios_event_log = devm_kmemdup(&chip->dev, __va(base), size, GFP_KERNEL);
>>   	if (!log->bios_event_log)
>>   		return -ENOMEM;
>>   
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 741d8f3e8fb3..b99f55f2d4fd 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -267,7 +267,6 @@ static void tpm_dev_release(struct device *dev)
>>   	idr_remove(&dev_nums_idr, chip->dev_num);
>>   	mutex_unlock(&idr_lock);
>>   
>> -	kfree(chip->log.bios_event_log);
>>   	kfree(chip->work_space.context_buf);
>>   	kfree(chip->work_space.session_buf);
>>   	kfree(chip->allocated_banks);
>> -- 
>> 2.31.1
>>
> BR, Jarkko
Jarkko Sakkinen Jan. 26, 2023, 5:31 p.m. UTC | #3 
On Wed, Jan 25, 2023 at 01:06:31PM -0600, Eddie James wrote:
> 
> On 1/20/23 18:11, Jarkko Sakkinen wrote:
> > On Fri, Jan 13, 2023 at 10:10:16AM -0600, Eddie James wrote:
> > > Since the bios event log is freed in the device release function,
> > > let devres handle the deallocation. This will allow other memory
> > > allocation/mapping functions to be used for the bios event log.
> > > 
> > > Signed-off-by: Eddie James <eajames@linux.ibm.com>
> > > ---
> > >   drivers/char/tpm/eventlog/acpi.c |  5 +++--
> > >   drivers/char/tpm/eventlog/efi.c  | 13 +++++++------
> > >   drivers/char/tpm/eventlog/of.c   |  3 ++-
> > >   drivers/char/tpm/tpm-chip.c      |  1 -
> > >   4 files changed, 12 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c
> > > index 0913d3eb8d51..40360e599bc3 100644
> > > --- a/drivers/char/tpm/eventlog/acpi.c
> > > +++ b/drivers/char/tpm/eventlog/acpi.c
> > > @@ -14,6 +14,7 @@
> > >    * Access to the event log extended by the TCG BIOS of PC platform
> > >    */
> > > +#include <linux/device.h>
> > >   #include <linux/seq_file.h>
> > >   #include <linux/fs.h>
> > >   #include <linux/security.h>
> > > @@ -135,7 +136,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
> > >   	}
> > >   	/* malloc EventLog space */
> > > -	log->bios_event_log = kmalloc(len, GFP_KERNEL);
> > > +	log->bios_event_log = devm_kmalloc(&chip->dev, len, GFP_KERNEL);
> > >   	if (!log->bios_event_log)
> > >   		return -ENOMEM;
> > > @@ -160,7 +161,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
> > >   	return format;
> > >   err:
> > > -	kfree(log->bios_event_log);
> > > +	devm_kfree(&chip->dev, log->bios_event_log);
> > I wonder do we want to do devm_kfree's at all as the memory is freed during
> > detach, i.e. taken care by devres.
> 
> 
> I think we should since the chip/tpm driver will continue to probe without
> the bios event log. Therefore that memory will be wasted if there is some
> error during bios log setup.

OK, I buy this!

For this patch:

Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

If you could refine the description for the 2nd, then these would be ready
to be picked.

> Thanks,
> 
> Eddie

BR, Jarkko
diff mbox series

Patch

diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c
index 0913d3eb8d51..40360e599bc3 100644
--- a/drivers/char/tpm/eventlog/acpi.c
+++ b/drivers/char/tpm/eventlog/acpi.c
@@ -14,6 +14,7 @@ 
  * Access to the event log extended by the TCG BIOS of PC platform
  */
 
+#include <linux/device.h>
 #include <linux/seq_file.h>
 #include <linux/fs.h>
 #include <linux/security.h>
@@ -135,7 +136,7 @@  int tpm_read_log_acpi(struct tpm_chip *chip)
 	}
 
 	/* malloc EventLog space */
-	log->bios_event_log = kmalloc(len, GFP_KERNEL);
+	log->bios_event_log = devm_kmalloc(&chip->dev, len, GFP_KERNEL);
 	if (!log->bios_event_log)
 		return -ENOMEM;
 
@@ -160,7 +161,7 @@  int tpm_read_log_acpi(struct tpm_chip *chip)
 	return format;
 
 err:
-	kfree(log->bios_event_log);
+	devm_kfree(&chip->dev, log->bios_event_log);
 	log->bios_event_log = NULL;
 	return ret;
 }
diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
index e6cb9d525e30..4e9d7c2bf32e 100644
--- a/drivers/char/tpm/eventlog/efi.c
+++ b/drivers/char/tpm/eventlog/efi.c
@@ -6,6 +6,7 @@ 
  *      Thiebaud Weksteen <tweek@google.com>
  */
 
+#include <linux/device.h>
 #include <linux/efi.h>
 #include <linux/tpm_eventlog.h>
 
@@ -55,7 +56,7 @@  int tpm_read_log_efi(struct tpm_chip *chip)
 	}
 
 	/* malloc EventLog space */
-	log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL);
+	log->bios_event_log = devm_kmemdup(&chip->dev, log_tbl->log, log_size, GFP_KERNEL);
 	if (!log->bios_event_log) {
 		ret = -ENOMEM;
 		goto out;
@@ -76,7 +77,7 @@  int tpm_read_log_efi(struct tpm_chip *chip)
 			     MEMREMAP_WB);
 	if (!final_tbl) {
 		pr_err("Could not map UEFI TPM final log\n");
-		kfree(log->bios_event_log);
+		devm_kfree(&chip->dev, log->bios_event_log);
 		ret = -ENOMEM;
 		goto out;
 	}
@@ -91,11 +92,11 @@  int tpm_read_log_efi(struct tpm_chip *chip)
 	 * Allocate memory for the 'combined log' where we will append the
 	 * 'final events log' to.
 	 */
-	tmp = krealloc(log->bios_event_log,
-		       log_size + final_events_log_size,
-		       GFP_KERNEL);
+	tmp = devm_krealloc(&chip->dev, log->bios_event_log,
+			    log_size + final_events_log_size,
+			    GFP_KERNEL);
 	if (!tmp) {
-		kfree(log->bios_event_log);
+		devm_kfree(&chip->dev, log->bios_event_log);
 		ret = -ENOMEM;
 		goto out;
 	}
diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
index a9ce66d09a75..741ab2204b11 100644
--- a/drivers/char/tpm/eventlog/of.c
+++ b/drivers/char/tpm/eventlog/of.c
@@ -10,6 +10,7 @@ 
  * Read the event log created by the firmware on PPC64
  */
 
+#include <linux/device.h>
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/tpm_eventlog.h>
@@ -65,7 +66,7 @@  int tpm_read_log_of(struct tpm_chip *chip)
 		return -EIO;
 	}
 
-	log->bios_event_log = kmemdup(__va(base), size, GFP_KERNEL);
+	log->bios_event_log = devm_kmemdup(&chip->dev, __va(base), size, GFP_KERNEL);
 	if (!log->bios_event_log)
 		return -ENOMEM;
 
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 741d8f3e8fb3..b99f55f2d4fd 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -267,7 +267,6 @@  static void tpm_dev_release(struct device *dev)
 	idr_remove(&dev_nums_idr, chip->dev_num);
 	mutex_unlock(&idr_lock);
 
-	kfree(chip->log.bios_event_log);
 	kfree(chip->work_space.context_buf);
 	kfree(chip->work_space.session_buf);
 	kfree(chip->allocated_banks);