On 10/07/2016 01:40 AM, Jason Gunthorpe wrote:
> On Fri, Oct 07, 2016 at 01:26:45AM +0530, Nayna wrote:
>
>> - there is no kref increment during eventlog fops or seq_ops operations.
>> - fops and seq ops are parsing over memory buffer. fops->open() returns
>> after giving the memory buffer(log) to seq->open(). And, seq ops on reading
>> of log memory are not bound to any locks or krefs.
>
> I sent a email about this, you are missing a get_device(chip) in open.
> (see entry Jason Gunthorpe - Oct. 3, 2016, 5:14 p.m.
> https://patchwork.kernel.org/patch/9353259/ )
>
>> - once securityfs_remove() is done, there are no more files accessible to
>> user to do open(). Which implies, there can't be any new open() after chip
>> unregister, but existing open() might continue to work(this is I expecting
>> for now).
>
> Right..
>
>> However, I do see one issue as I am freeing log.bios_event_log during
>> teardown().
>
> Correct, I thought I pointed that out last round, that kfree must be
> moved to tpm_dev_release
>
>> which implies seq_ops might fail if there is no proper null
>> checks for log.bios_event_log.
>
> No, hold the chip mutex between fops->open() -> release() which will
> ensure that the log memory continues exists.
>
>> #1. Do we want securityfs_remove() to wait till all the opened eventlog
>> files are closed().
>
> That is not simple, or necessary..
>
>> #2 Are we ok with securityfs_remove() being done and files are removed, but
>> existing seq ops, eventlog parsing should continue to work till closed by
>> user. That implies, we should not unknowingly nullify log pointers which are
>> used by parser.
>
> This is simpler, moving the kfree to tpm_dev_release and holding the
> chip kref for the lifetime of the filp is easy and safe.
>
>> I took sometime to understand how is kref getting accessed for tpm_chip. And
>> I tried tracing krefs. I have been looking at
>> chip->dev.kobj.kref.. Please
>
> Right, it used with get_device/put_device
>
> The model to go for is that open() acquires a get_device() kref on
> the chip and that kref is held for the duration of the lifetime of the
> filp.
>
> The log and data members of chip must remain allocated and unchanged until
> tpm_dev_release.
>
> Try something like the algorithm I gave in 'Jason Gunthorpe - Oct. 4,
> 2016, 5:12 p.m.' to solve the remove/open race for now with a big fat
> comment.
Sure, will try this and will also include all other feedbacks.
Thanks & Regards,
- Nayna
>
> Jason
>
------------------------------------------------------------------------------
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
