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

[Eddie James <eajames@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxx>
> >    */
> >   
> > +#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