Jason Gunthorpe <[email protected]> wrote on 01/26/2016
01:22:48 PM:
>
> On Tue, Jan 26, 2016 at 09:21:42AM -0500, Stefan Berger wrote:
> > > So, in an ideal world vtpms0 wouldn't even exist. The need for it
is
> > > more tpm core breakage. You could avoid this by having the tpm
core
> > > not create sysfs files on the parent for the vtpms. I think we
are
> > > very close to being able to do that.
>
> > We're not explicitly registering the parent with sysfs, yet it
appears
> > there. How do you prevent it from appearing there?
>
> static struct kobject *get_device_parent(struct device *dev,
> struct device *parent)
>
> /*
> * If we have no parent, we live in "virtual".
> * Class-devices with a non class-device as parent, live
> * in a "glue" directory to prevent namespace
collisions.
> */
>
> The vtpm should be under virtual, the question is if we need vtpm and
> and tpm devices, ie should the vtpm driver be calling class_create or
> not.
>
> I don't have an answer to that, are there other virtualization devices
> you can reference as an example?
I don't have a virtual device example that doesn't have a parent. I had
looked at virtio_console for, but that one also has a parent.
http://lxr.free-electrons.com/source/drivers/char/virtio_console.c#L1439
>
> Ideally you'd get someone familiar with sysfs to confirm these details.
>
> > The only other solution I could think of is being able to pass a
> > NULL for 'dev' into the existing tpmm_chip_alloc() and avoid the
> > parent device altogether.
>
> This would drop the vtpm and put tpm directly under virtual, however I
> think the tpm core currently will segfault if parent is null, that
> could be fixed if that is the way this should go.
Likely the parent isn't needed if the vtpm driver calls it. So a couple of
checks for dev being NULL could solve that. But I rather leave it as is
for now before I tear code out that I may have to re-add later on.
>
> >> put_device should be last because it can kfree(vtpm_dev)
> > Yes. Fixed.
>
> Not on your github...
>
> +static int vtpm_fops_release(struct inode *inode, struct file *filp)
> +{
> + struct vtpm_dev *vtpm_dev = filp->private_data;
> +
> + filp->private_data = NULL;
> +
> + put_device(&vtpm_dev->dev);
> +
> + vtpm_delete_device_pair(vtpm_dev);
> +
> + return 0;
> +}
>
> Don't put something then continue to reference it after.
>
> Also, I would probably drop the get_device in vtpm_fops_open, use the
> implicit get_device of device_initialize instead. Then drop the
> put_device in vtpm_destroy_vtpm_dev and use only one at the end of the
> fops release.
We have the implicit get_device in vtpm_create_vtpm_dev and would
therefore try to keep the put_device in vtpm_destroy_vtpm_dev.
Besides that a device_initialize + device_add = device_register and
device_del + put_device = device_unregister, which could go into
vtpm_create_vtpm_dev and vtpm_destroy_vtpm_dev respectively.
>
> So the flow is, device_initialize [...] create a flip, transfer the
> kref from device_initialize to the flip [..]
>
> Which makes the error flow very sane, once the flip is created,
> deleting the flip will clean up everything.
>
> > I put the fixed version of the driver here:
> > https://github.com/stefanberger/linux/commits/vtpm-driver.v2
>
> Please go through all the error handling, it still looks wrong:
>
> +static long vtpmx_fops_ioctl(struct file *f, unsigned int ioctl,
> + unsigned long arg)
> + vtpm_dev = vtpm_create_device_pair(&vtpm_new_pair);
> [..]
> + if (copy_to_user(vtpm_new_pair_p, &vtpm_new_pair,
> + sizeof(vtpm_new_pair)))
> + return -EFAULT;
>
> Can't be right, that fails the ioctl but still creates the FD and TPM.
True. I have not been able to undo fd_install so far and cannot find any
other code that does that. So, I pulled that out and put it after the
successful copy_to_user().
>
> The goto unwind of vtpm_create_device_pair looks wrong too, double
> calls vtpm_destroy_vtpm_dev and other. See above on how the flip
> release should be relied upon to do that once the flip is created.
>
> Also I'm wondering about the placement of tpm_chip_register, long
> term we are going to be moving tpm setup commands under
> tpm_chip_register, so the driver must be fully ready to handle
> commands when it is called, so I think vtpm calls it too early.
re-ordered
>
> For instance, get_timeouts is never called in vtpm, which is not
> correct. The soft tpm should have an opportunity to tell the kernel
> the timeout values.
That's seems a chick-and-egg problem. The vTPM can only be started once
the ioctl has provided the fd.
>
> I think you have to fix this by calling tpm_get_timeouts/chip_register
> from a work queue thread created before the ioctl returns. That should
> avoid deadlock..
We'll only ever be able to call that once the vTPM has been started on the
fd... otherwise the default timeout values will have to do it.
>
> This is also very wrong:
>
> +struct vtpm_dev {
> + struct kref kref;
> +
> + struct device dev;
>
> Structs should only ever have one kref. Get rid of 'kref' and use the
> kref embedded in 'dev' via get_device/put_device.
Yes, sir.
>
> Also, what is the point of 'vtpm_dev_get_by_chip' ? Just store the
> vtpm_dev in the chip's private data pointer like every other driver.
Introduced chip->priv.
>
> No special locking is needed, tpm core guarentees no op is running or
> will ever be called again once tpm_unregister returns.
>
> Jason
>
cheers!
Stefan
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=267308311&iu=/4140
_______________________________________________
tpmdd-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
