Re: [PATCH v5 4/7] tpm: fix the race condition between event log access and chip getting unregistered

[Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

On Mon, Nov 07, 2016 at 05:29:43PM -0700, Jason Gunthorpe wrote:
> On Mon, Nov 07, 2016 at 04:26:43PM -0800, Jarkko Sakkinen wrote:
> > On Mon, Nov 07, 2016 at 04:48:39PM -0700, Jason Gunthorpe wrote:
> > > On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote:
> > > > On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> > > > > Currently, the event log file operations are not serialized with
> > > > > tpm_chip_unregister(), which can possibly cause a race condition.
> > > > > 
> > > > > This patch fixes the race condition by:
> > > > >  - moving read_log() from fops to chip register.
> > > > 
> > > > What is "chip register"? Please use exact names.
> > > > 
> > > > >  - disallowing event log file operations when chip unregister is in
> > > > >    progress.
> > > > 
> > > > Could you elaborate this sentence?
> > > > 
> > > > >  - guarding event log memory using chip krefs.
> > > > 
> > > > Could you elaborate this sentence?
> > > > 
> > > > Please describe how the race condition could happen and provide the
> > > > "Fixes:" line for the commit ID that caused it. Otherwise, your commit
> > > > message won't make any sense. I cannot apply this commit with this
> > > > commit message.
> > > > 
> > > > The commit message does not make much sense...
> > > 
> > > Lets get this moving along, it is hard to keep everything straight
> > > over months..
> > > 
> > > Nayna: This commit message should work:
> > > 
> > > tpm: Have eventlog use the tpm_chip
> > > 
> > > Move the backing memory for the eventlog into tpm_chip and push
> > > the tpm_chip into read_log. This optimizes read_log processing by
> > > only doing it once and prepares things for the next patches in the
> > > series which require the tpm_chip to locate the event log via
> > > ACPI and OF handles instead of searching.
> > > 
> > > This is straightfoward except for the issue of passing a kref through
> > > i_private with securityfs. Since securityfs_remove does not have any
> > > removal fencing like sysfs we use the inode lock to safely get a
> > > kref on the tpm_chip.
> > 
> > Perfect. Thank you.
> > 
> > With this
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > 
> > No need for resend.
> 
> The missing null still needs to be fixed, and the inode lock needs to
> be held only over get_device in open()

Right, I'm sorry I forgot that... Another reason for one more resend.

/Jarkko

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi