On 10/14/2016 12:21 AM, Nayna wrote: > > > On 10/01/2016 05:31 PM, Jarkko Sakkinen wrote: >> On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote: >>> Currently, the securityfs pseudo files for obtaining the firmware >>> event log are created whether the event log properties exist or not. >>> This patch creates ascii and bios measurements pseudo files >>> only if read_log() is successful. >> >> Re-reviewing this. The commit message should mention about preventing >> a race condition. >> >> I think Jason was right. It makes code much more manageable with a >> small price of memory consumption. >> >>> Suggested-by: Jason Gunthorpe <jguntho...@obsidianresearch.com> >>> Signed-off-by: Nayna Jain <na...@linux.vnet.ibm.com> >>> --- >>> drivers/char/tpm/tpm.h | 6 +++++ >>> drivers/char/tpm/tpm_acpi.c | 12 +++++++--- >>> drivers/char/tpm/tpm_eventlog.c | 53 >>> +++++++++++++++++++---------------------- >>> drivers/char/tpm/tpm_eventlog.h | 7 +++++- >>> drivers/char/tpm/tpm_of.c | 4 +++- >>> 5 files changed, 48 insertions(+), 34 deletions(-) >>> >>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h >>> index b5866bb..68630cd 100644 >>> --- a/drivers/char/tpm/tpm.h >>> +++ b/drivers/char/tpm/tpm.h >>> @@ -35,6 +35,8 @@ >>> #include <linux/cdev.h> >>> #include <linux/highmem.h> >>> >>> +#include "tpm_eventlog.h" >>> + >>> enum tpm_const { >>> TPM_MINOR = 224, /* officially assigned */ >>> TPM_BUFSIZE = 4096, >>> @@ -156,6 +158,10 @@ struct tpm_chip { >>> struct rw_semaphore ops_sem; >>> const struct tpm_class_ops *ops; >>> >>> + struct tpm_bios_log log; >> >> struct tpm_bios_log should be renamed as struct tpm_event_log in some >> commit of this patch set as tpm_bios_log is a misleading name. > > My understanding is that other event log functions are also named in > consistent with tpm_bios_log naming.. for eg.. > tpm_bios_log_setup(/teardown), tpm_bios_measurements_open,etc. So, > wanted to understand if idea is only to change the struct name to > tpm_event_log ? > > Thanks & Regards, > - Nayna
I have not modified the tpm_bios_log naming in my new patch set for the above reason. But if we think that it is appropriate to change the data type (and functions ?) naming, I will post it as separate single patch. Thanks & Regards, - Nayna > >> >>> + struct tpm_securityfs_data bin_sfs_data; >>> + struct tpm_securityfs_data ascii_sfs_data; >> >> I think this is otherwise right but the struct name is very clunky. >> First of all it doesn't own the data and IMHO now it kind of implies >> of owning. >> >> Maybe something like tpm_event_log_fd would a better name. It's a >> description of the event log file essentially. >> >>> + >>> unsigned int flags; >>> >>> int dev_num; /* /dev/tpm# */ >>> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c >>> index 565a947..4d6c2d7 100644 >>> --- a/drivers/char/tpm/tpm_acpi.c >>> +++ b/drivers/char/tpm/tpm_acpi.c >>> @@ -45,13 +45,15 @@ struct acpi_tcpa { >>> }; >>> >>> /* read binary bios log */ >>> -int read_log(struct tpm_bios_log *log) >>> +int read_log(struct tpm_chip *chip) >>> { >>> struct acpi_tcpa *buff; >>> acpi_status status; >>> void __iomem *virt; >>> u64 len, start; >>> + struct tpm_bios_log *log; >>> >>> + log = &chip->log; >>> if (log->bios_event_log != NULL) { >>> printk(KERN_ERR >>> "%s: ERROR - Eventlog already initialized\n", >>> @@ -97,13 +99,17 @@ int read_log(struct tpm_bios_log *log) >>> >>> virt = acpi_os_map_iomem(start, len); >>> if (!virt) { >>> - kfree(log->bios_event_log); >>> printk("%s: ERROR - Unable to map memory\n", __func__); >>> - return -EIO; >>> + goto err; >>> } >>> >>> memcpy_fromio(log->bios_event_log, virt, len); >>> >>> acpi_os_unmap_iomem(virt, len); >>> return 0; >>> + >>> +err: >>> + kfree(log->bios_event_log); >>> + return -EIO; >>> + >>> } >>> diff --git a/drivers/char/tpm/tpm_eventlog.c >>> b/drivers/char/tpm/tpm_eventlog.c >>> index f1df782..a8cd4a1 100644 >>> --- a/drivers/char/tpm/tpm_eventlog.c >>> +++ b/drivers/char/tpm/tpm_eventlog.c >>> @@ -261,14 +261,6 @@ static int tpm_binary_bios_measurements_show(struct >>> seq_file *m, void *v) >>> static int tpm_bios_measurements_release(struct inode *inode, >>> struct file *file) >>> { >>> - struct seq_file *seq = file->private_data; >>> - struct tpm_bios_log *log = seq->private; >>> - >>> - if (log) { >>> - kfree(log->bios_event_log); >>> - kfree(log); >>> - } >>> - >>> return seq_release(inode, file); >>> } >>> >>> @@ -323,34 +315,19 @@ 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) >>> - return -ENOMEM; >>> - >>> - err = read_log(log); >>> - if (err) >>> - goto out_free; >>> + const struct tpm_securityfs_data *sfs_data = >>> + (const struct tpm_securityfs_data *)inode->i_private; >>> + const struct seq_operations *seqops = sfs_data->seqops; >>> >>> /* now register seq file */ >>> err = seq_open(file, seqops); >>> if (!err) { >>> seq = file->private_data; >>> - seq->private = log; >>> - } else { >>> - goto out_free; >>> + seq->private = sfs_data->log; >>> } >>> >>> -out: >>> return err; >>> -out_free: >>> - kfree(log->bios_event_log); >>> - kfree(log); >>> - goto out; >>> } >>> >>> static const struct file_operations tpm_bios_measurements_ops = { >>> @@ -372,6 +349,18 @@ static int is_bad(void *p) >>> int tpm_bios_log_setup(struct tpm_chip *chip) >>> { >>> const char *name = dev_name(&chip->dev); >>> + int rc = 0; >>> + >>> + rc = read_log(chip); >>> + /* >>> + * read_log failure means event log is not supported except for ENOMEM >>> + */ >>> + if (rc < 0) { >>> + if (rc == -ENOMEM) >>> + return rc; >>> + else >>> + return 0; >>> + } >>> >>> chip->bios_dir_count = 0; >>> chip->bios_dir[chip->bios_dir_count] = >>> @@ -380,19 +369,24 @@ int tpm_bios_log_setup(struct tpm_chip *chip) >>> goto err; >>> chip->bios_dir_count++; >>> >>> + chip->bin_sfs_data.log = &chip->log; >>> + chip->bin_sfs_data.seqops = &tpm_binary_b_measurments_seqops; >>> + >>> chip->bios_dir[chip->bios_dir_count] = >>> securityfs_create_file("binary_bios_measurements", >>> S_IRUSR | S_IRGRP, chip->bios_dir[0], >>> - (void *)&tpm_binary_b_measurments_seqops, >>> + (void *)&chip->bin_sfs_data, >>> &tpm_bios_measurements_ops); >>> if (is_bad(chip->bios_dir[chip->bios_dir_count])) >>> goto err; >>> chip->bios_dir_count++; >>> >>> + chip->ascii_sfs_data.log = &chip->log; >>> + chip->ascii_sfs_data.seqops = &tpm_ascii_b_measurments_seqops; >>> chip->bios_dir[chip->bios_dir_count] = >>> securityfs_create_file("ascii_bios_measurements", >>> S_IRUSR | S_IRGRP, chip->bios_dir[0], >>> - (void *)&tpm_ascii_b_measurments_seqops, >>> + (void *)&chip->ascii_sfs_data, >>> &tpm_bios_measurements_ops); >>> if (is_bad(chip->bios_dir[chip->bios_dir_count])) >>> goto err; >>> @@ -413,4 +407,5 @@ void tpm_bios_log_teardown(struct tpm_chip *chip) >>> securityfs_remove(chip->bios_dir[i-1]); >>> chip->bios_dir_count = i; >>> >>> + kfree(chip->log.bios_event_log); >>> } >>> diff --git a/drivers/char/tpm/tpm_eventlog.h >>> b/drivers/char/tpm/tpm_eventlog.h >>> index fd3357e..7ea066c 100644 >>> --- a/drivers/char/tpm/tpm_eventlog.h >>> +++ b/drivers/char/tpm/tpm_eventlog.h >>> @@ -22,6 +22,11 @@ struct tpm_bios_log { >>> void *bios_event_log_end; >>> }; >>> >>> +struct tpm_securityfs_data { >>> + struct tpm_bios_log *log; >>> + const struct seq_operations *seqops; >>> +}; >>> + >>> struct tcpa_event { >>> u32 pcr_index; >>> u32 event_type; >>> @@ -73,7 +78,7 @@ enum tcpa_pc_event_ids { >>> HOST_TABLE_OF_DEVICES, >>> }; >>> >>> -int read_log(struct tpm_bios_log *log); >>> +int read_log(struct tpm_chip *chip); >>> >>> #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || >>> \ >>> defined(CONFIG_ACPI) >>> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c >>> index 570f30c..68d891a 100644 >>> --- a/drivers/char/tpm/tpm_of.c >>> +++ b/drivers/char/tpm/tpm_of.c >>> @@ -20,12 +20,14 @@ >>> #include "tpm.h" >>> #include "tpm_eventlog.h" >>> >>> -int read_log(struct tpm_bios_log *log) >>> +int read_log(struct tpm_chip *chip) >>> { >>> struct device_node *np; >>> const u32 *sizep; >>> const u64 *basep; >>> + struct tpm_bios_log *log; >>> >>> + log = &chip->log; >>> if (log->bios_event_log != NULL) { >>> pr_err("%s: ERROR - Eventlog already initialized\n", __func__); >>> return -EFAULT; >>> -- >>> 2.5.0 >>> >> >> /Jarkko >> > > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > _______________________________________________ > tpmdd-devel mailing list > tpmdd-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot _______________________________________________ tpmdd-devel mailing list tpmdd-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tpmdd-devel