linux-kernel - RE: [RFC][PATCH 1/2] efi_pstore: hold multiple logs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <A5ED84D3BB3A384992CBB9C77DEDA4D4138796E1@USINDEM103.corp.hds.com>
Date:	Fri, 19 Oct 2012 14:26:33 +0000
From:	Seiji Aguchi <seiji.aguchi@....com>
To:	Mike Waychison <mikew@...gle.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
	"Luck, Tony (tony.luck@...el.com)" <tony.luck@...el.com>,
	"Matthew Garrett (mjg@...hat.com)" <mjg@...hat.com>,
	"dzickus@...hat.com" <dzickus@...hat.com>,
	"dle-develop@...ts.sourceforge.net" 
	<dle-develop@...ts.sourceforge.net>,
	Satoru Moriya <satoru.moriya@....com>
Subject: RE: [RFC][PATCH 1/2] efi_pstore: hold multiple logs

Mike,

Thank you for reviewing my patch.
Here is my comment.

> >   - Write callback
> >     - Check if there are enough spaces to write logs with QueryVariableInfo()
> >       to avoid handling out of space situation. (It is suggested by
> > Matthew Garrett.)
> >
> 
> I would prefer to see the exposing query_variable_info callback as a separate patch.  The check in the patch looks good.
>

OK. I will make a separate patch.

 
> >     - Remove a logic erasing existing entries.
> >
> >   - Erase callback
> >     - Freshly create a logic rather than sharing it with a write callback because
> >       erasing entries doesn't need in a write callback.
> >
> 
> This sort of change is a lot easier to review if you copy and paste the routine in a patch separate from this one.
> 

I will separate it as well.


> > +       /*
> > +        * Check if there is a space enough to log.
> > +        * size: a size of logging data
> > +        * DUMP_NAME_LEN * 2: a maximum size of variable name
> > +        *
> 
> extra line
> 

Will fix.

> > +
> > +       efivars->ops->set_variable(efi_name, &vendor, PSTORE_EFI_ATTRIBUTES,
> > +                                  size, psi->buf);
> > +
> > +       spin_unlock(&efivars->lock);
> > +
> > +       if (size)
> > +               ret = efivar_create_sysfs_entry(efivars,
> > +                                         utf16_strsize(efi_name,
> > +                                                       DUMP_NAME_LEN * 2),
> > +                                         efi_name, &vendor);
> 
> What is happening here?  Why is size checked for non-zero?
> 

I just copied an original code sharing a logic of write callback and erase callback.. 
But, if size is zero, we don't need to create a sysfs file because a set_variable service erases the entry.
It is defined in EFI specification.
So, I think this code is right.

> > +++ b/fs/pstore/inode.c
> > @@ -175,7 +175,8 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
> >         struct pstore_private *p = dentry->d_inode->i_private;
> >
> >         if (p->psi->erase)
> > -               p->psi->erase(p->type, p->id, p->psi);
> > +               p->psi->erase(p->type, p->id, dentry->d_inode->i_ctime,
> > +                             p->psi);
> 
> This doesn't look right.  What guarantees that the i_ctime means anything across reboots?
> 

Ctime is persistent across reboots because ctime of pstore means the date that the recode was
originally stored. (See below.)

To do this, efi_pstore saves the ctime to variable name at writing time and passes it to pstore at reading time.

fs/pstore/inode.c:
<snip>
/*
  * Make a regular file in the root directory of our file system.
  * Load it up with "size" bytes of data from "buf".
  * Set the mtime & ctime to the date that this record was originally stored.
  */
int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
                   char *data, size_t size, struct timespec time,
                   struct pstore_info *psi) {
         struct dentry           *root = pstore_sb->s_root;
         struct dentry           *dentry;
         struct inode            *inode;
         int                     rc = 0;
         char                    name[PSTORE_NAMELEN];
<snip>

Seiji
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/