| Message ID | 1454959628-30582-5-git-send-email-stefanb@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 08, 2016 at 02:27:07PM -0500, Stefan Berger wrote: > Add the retrieval of TPM 1.2 durations and timeouts. Since this requires > the startup of the TPM, do this for TPM 1.2 and TPM 2. > > To not allow to interleave with commands from user space, so send the > TPM_Startup as the first command. The timeouts can then be gotten at any > later time, even interleaved with commands from user space. Do not call tpm_register until get_timeouts has completed and this will naturally be avoided the same way every other TPM driver does. The long term goal is to move the get_timeouts call into tpm_register. Jason ------------------------------------------------------------------------------ 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=272487151&iu=/4140
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/09/2016 12:33:23 AM: > > On Mon, Feb 08, 2016 at 02:27:07PM -0500, Stefan Berger wrote: > > Add the retrieval of TPM 1.2 durations and timeouts. Since this requires > > the startup of the TPM, do this for TPM 1.2 and TPM 2. > > > > To not allow to interleave with commands from user space, so send the > > TPM_Startup as the first command. The timeouts can then be gotten at any > > later time, even interleaved with commands from user space. > > Do not call tpm_register until get_timeouts has completed and this > will naturally be avoided the same way every other TPM driver does. Getting the timeouts cannot complete before the TPM emulator has started, which in turn cannot start before the ioctl returns. We don't know whether user space starts a client first on /dev/tpm1 or the TPM emulator starts first on the file descriptor. We can control which command gets *queued* for the TPM emulator and that's what I am doing by having the kernel queue the startup command first. I moved the start of the work call before the tpm_chip_register, so that we *queue* the TPM_Startup before user space can send its first command. > > The long term goal is to move the get_timeouts call into tpm_register. That may become a problem then. Stefan > > Jason > > ------------------------------------------------------------------------------ > 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=272487151&iu=/4140 > _______________________________________________ > tpmdd-devel mailing list > tpmdd-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel > ------------------------------------------------------------------------------ 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=272487151&iu=/4140
On Tue, Feb 09, 2016 at 11:19:15AM -0500, Stefan Berger wrote: > Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/09/2016 12:33:23 > AM: > > > > > > On Mon, Feb 08, 2016 at 02:27:07PM -0500, Stefan Berger wrote: > > > Add the retrieval of TPM 1.2 durations and timeouts. Since this requires > > > the startup of the TPM, do this for TPM 1.2 and TPM 2. > > > > > > To not allow to interleave with commands from user space, so send the > > > TPM_Startup as the first command. The timeouts can then be gotten at any > > > later time, even interleaved with commands from user space. > > > > Do not call tpm_register until get_timeouts has completed and this > > will naturally be avoided the same way every other TPM driver does. > > Getting the timeouts cannot complete before the TPM emulator has started, which > in turn cannot start before the ioctl returns. We don't know whether user space > starts a client first on /dev/tpm1 or the TPM emulator starts first on the file > descriptor. We can control which command gets *queued* for the TPM emulator and > that's what I am doing by having the kernel queue the startup command first. I keep saying this same solution - start a work queue just before the ioctl returns and do the get_timeouts and registration in there. Jason ------------------------------------------------------------------------------ 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=272487151&iu=/4140
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/09/2016 11:52:28 AM: > > On Tue, Feb 09, 2016 at 11:19:15AM -0500, Stefan Berger wrote: > > Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/09/ > 2016 12:33:23 > > AM: > > > > > > > > > > On Mon, Feb 08, 2016 at 02:27:07PM -0500, Stefan Berger wrote: > > > > Add the retrieval of TPM 1.2 durations and timeouts. Since this requires > > > > the startup of the TPM, do this for TPM 1.2 and TPM 2. > > > > > > > > To not allow to interleave with commands from user space, so send the > > > > TPM_Startup as the first command. The timeouts can then be gotten at any > > > > later time, even interleaved with commands from user space. > > > > > > Do not call tpm_register until get_timeouts has completed and this > > > will naturally be avoided the same way every other TPM driver does. > > > > Getting the timeouts cannot complete before the TPM emulator has > started, which > > in turn cannot start before the ioctl returns. We don't know > whether user space > > starts a client first on /dev/tpm1 or the TPM emulator starts > first on the file > > descriptor. We can control which command gets *queued* for the TPM > emulator and > > that's what I am doing by having the kernel queue the startup command first. > > I keep saying this same solution - start a work queue just before the > ioctl returns and do the get_timeouts and registration in there. And how do we unroll if tpm_chip_register fails? We just close the fd that we gave to user space? I also don't see why it is a better solution than what I have here now: https://github.com/stefanberger/linux/commit/954c7dece4a5c44525ca07a9569dc2f3b2b3d174 Stefan > > Jason > ------------------------------------------------------------------------------ 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=272487151&iu=/4140
On Tue, Feb 09, 2016 at 12:45:37PM -0500, Stefan Berger wrote: > And how do we unroll if tpm_chip_register fails? We just close the fd that we > gave to user space? Arrange for the fd to become permanently readable and read returns error, user space will close the fd. > I also don't see why it is a better solution than what I have here now: > > https://github.com/stefanberger/linux/commit/ > 954c7dece4a5c44525ca07a9569dc2f3b2b3d174 As I've said, this mucks up where we want to go with the core code, and breaks our current invariants - the TPM hardware must be fully operational before tpm_register is called. Jason ------------------------------------------------------------------------------ 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=272487151&iu=/4140
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/09/2016 01:01:52 PM: > > On Tue, Feb 09, 2016 at 12:45:37PM -0500, Stefan Berger wrote: > > And how do we unroll if tpm_chip_register fails? We just close thefd that we > > gave to user space? > > Arrange for the fd to become permanently readable and read returns > error, user space will close the fd. :-( User space will just pass the fd to another process, which is the TPM emulator, and that thing then starts failing on the broken file descriptor. > > > I also don't see why it is a better solution than what I have here now: > > > > https://github.com/stefanberger/linux/commit/ > > 954c7dece4a5c44525ca07a9569dc2f3b2b3d174 > > As I've said, this mucks up where we want to go with the core code, > and breaks our current invariants - the TPM hardware must be fully > operational before tpm_register is called. Either way, I don't see how the TPM emulator can be fully operational before tpm_chip_register() is called. So far the code allows to start a TPM emulator on the file descript 'late'. The possibility for accomodating the solution in the future is there by leaving the possibility for a boolean where the driver can choose whether timeout retrieval is done as part of tpm_chip_register or it has to take care of this itself. 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=272487151&iu=/4140
On Tue, Feb 09, 2016 at 01:11:58PM -0500, Stefan Berger wrote: > :-( User space will just pass the fd to another process, which is the TPM > emulator, and that thing then starts failing on the broken file descriptor. .. and then it exits like any other failure and the caller has to sort it out, which it already has to handle. > Either way, I don't see how the TPM emulator can be fully operational before > tpm_chip_register() is called. You still haven't said what is wrong with doing all the work in a work queue so user space can respond normally without all this ugly hacking. > So far the code allows to start a TPM emulator on the file descript > 'late'. That breaks our invariants to user space. The /dev/tpmX, etc cannot appear until get_timeouts/etc complete. This isn't optional. Ordering startup isn't sufficient. Jason ------------------------------------------------------------------------------ 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=272487151&iu=/4140
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/09/2016 01:20:13 PM: > Subject: Re: [tpmdd-devel] [PATCH v5 4/5] Initialize TPM and get > durations and timeouts > > On Tue, Feb 09, 2016 at 01:11:58PM -0500, Stefan Berger wrote: > > > :-( User space will just pass the fd to another process, which is the TPM > > emulator, and that thing then starts failing on the broken file descriptor. > > .. and then it exits like any other failure and the caller has to sort > it out, which it already has to handle. > > > Either way, I don't see how the TPM emulator can be fully operational before > > tpm_chip_register() is called. > > You still haven't said what is wrong with doing all the work in a work > queue so user space can respond normally without all this ugly hacking. It doesn't seem right to simply close the file descriptor on the process if something goes wrong. The process couldn't close() on it since another thread could have gotten the same fd number for something else. I haven't seen a file descriptor that one must not close() in user space. It doesn't seem to work to only close the file (fput()) in case something goes wrong while sending the commands to the TPM and before the tpm_chip_register(). In that case I see the message 'VFS: Close: file count is 0' once the application closes the fd. Do you have any insights? 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=272487151&iu=/4140
On Tue, Feb 09, 2016 at 02:22:21PM -0500, Stefan Berger wrote: > It doesn't seem right to simply close the file descriptor on the > process if something goes wrong. The process couldn't close() on it > since another thread could have gotten the same fd number for something > else. I haven't seen a file descriptor that one must not close() in > user space. The kernel should never unilaterally close a fd, just make it permanently error, like a far-end closed socket. Read fails, write fails, and poll returns readable. User space will close it when it detects read failure. Jason ------------------------------------------------------------------------------ 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=272487151&iu=/4140
On Tue, Feb 09, 2016 at 09:52:28AM -0700, Jason Gunthorpe wrote: > On Tue, Feb 09, 2016 at 11:19:15AM -0500, Stefan Berger wrote: > > Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/09/2016 12:33:23 > > AM: > > > > > > > > > > On Mon, Feb 08, 2016 at 02:27:07PM -0500, Stefan Berger wrote: > > > > Add the retrieval of TPM 1.2 durations and timeouts. Since this requires > > > > the startup of the TPM, do this for TPM 1.2 and TPM 2. > > > > > > > > To not allow to interleave with commands from user space, so send the > > > > TPM_Startup as the first command. The timeouts can then be gotten at any > > > > later time, even interleaved with commands from user space. > > > > > > Do not call tpm_register until get_timeouts has completed and this > > > will naturally be avoided the same way every other TPM driver does. > > > > Getting the timeouts cannot complete before the TPM emulator has started, which > > in turn cannot start before the ioctl returns. We don't know whether user space > > starts a client first on /dev/tpm1 or the TPM emulator starts first on the file > > descriptor. We can control which command gets *queued* for the TPM emulator and > > that's what I am doing by having the kernel queue the startup command first. > > I keep saying this same solution - start a work queue just before the > ioctl returns and do the get_timeouts and registration in there. *Maybe* it would be worth to check David's patch: https://github.com/PeterHuewe/linux-tpmdd/commit/9329f13c403daf1f4bd1e715d2ba0866e089fb1d /Jarkko ------------------------------------------------------------------------------ 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=272487151&iu=/4140
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 02/09/2016 10:56:20 PM: > > On Tue, Feb 09, 2016 at 09:52:28AM -0700, Jason Gunthorpe wrote: > > On Tue, Feb 09, 2016 at 11:19:15AM -0500, Stefan Berger wrote: > > > Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/ > 09/2016 12:33:23 > > > AM: > > > > > > > > > > > > > > On Mon, Feb 08, 2016 at 02:27:07PM -0500, Stefan Berger wrote: > > > > > Add the retrieval of TPM 1.2 durations and timeouts. Since > this requires > > > > > the startup of the TPM, do this for TPM 1.2 and TPM 2. > > > > > > > > > > To not allow to interleave with commands from user space, so send the > > > > > TPM_Startup as the first command. The timeouts can then be > gotten at any > > > > > later time, even interleaved with commands from user space. > > > > > > > > Do not call tpm_register until get_timeouts has completed and this > > > > will naturally be avoided the same way every other TPM driver does. > > > > > > Getting the timeouts cannot complete before the TPM emulator has > started, which > > > in turn cannot start before the ioctl returns. We don't know > whether user space > > > starts a client first on /dev/tpm1 or the TPM emulator starts > first on the file > > > descriptor. We can control which command gets *queued* for the > TPM emulator and > > > that's what I am doing by having the kernel queue the startup > command first. > > > > I keep saying this same solution - start a work queue just before the > > ioctl returns and do the get_timeouts and registration in there. > > *Maybe* it would be worth to check David's patch: > > https://github.com/PeterHuewe/linux-tpmdd/commit/ > 9329f13c403daf1f4bd1e715d2ba0866e089fb1d Redid that now. https://github.com/stefanberger/linux/commit/83019eaab2cf5eb33f2665efdf9d2a117ed703b2 Stefan > > /Jarkko > ------------------------------------------------------------------------------ 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=272487151&iu=/4140
On Wed, Feb 10, 2016 at 12:15:44AM -0500, Stefan Berger wrote: > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 02/09/2016 > 10:56:20 PM: > > > > > On Tue, Feb 09, 2016 at 09:52:28AM -0700, Jason Gunthorpe wrote: > > > On Tue, Feb 09, 2016 at 11:19:15AM -0500, Stefan Berger wrote: > > > > Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/ > > 09/2016 12:33:23 > > > > AM: > > > > > > > > > > > > > > > > > > On Mon, Feb 08, 2016 at 02:27:07PM -0500, Stefan Berger wrote: > > > > > > Add the retrieval of TPM 1.2 durations and timeouts. Since > > this requires > > > > > > the startup of the TPM, do this for TPM 1.2 and TPM 2. > > > > > > > > > > > > To not allow to interleave with commands from user space, so > send the > > > > > > TPM_Startup as the first command. The timeouts can then be > > gotten at any > > > > > > later time, even interleaved with commands from user space. > > > > > > > > > > Do not call tpm_register until get_timeouts has completed and this > > > > > will naturally be avoided the same way every other TPM driver > does. > > > > > > > > Getting the timeouts cannot complete before the TPM emulator has > > started, which > > > > in turn cannot start before the ioctl returns. We don't know > > whether user space > > > > starts a client first on /dev/tpm1 or the TPM emulator starts > > first on the file > > > > descriptor. We can control which command gets *queued* for the > > TPM emulator and > > > > that's what I am doing by having the kernel queue the startup > > command first. > > > > > > I keep saying this same solution - start a work queue just before the > > > ioctl returns and do the get_timeouts and registration in there. > > > > *Maybe* it would be worth to check David's patch: > > > > https://github.com/PeterHuewe/linux-tpmdd/commit/ > > 9329f13c403daf1f4bd1e715d2ba0866e089fb1d > > Redid that now. > > https://github.com/stefanberger/linux/commit/83019eaab2cf5eb33f2665efdf9d2a117ed703b2 With very quick skim looks very similar, which is good because this approach is tested and known to work thanks to David. /Jarkko ------------------------------------------------------------------------------ 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=272487151&iu=/4140
On Wed, Feb 10, 2016 at 12:15:44AM -0500, Stefan Berger wrote: > Redid that now. > https://github.com/stefanberger/linux/commit/83019eaab2cf5eb33f2665efdf9d2a117ed703b2 Yeah, I think you've got that now. Just a few other random commments.. This isn't great: vtpm_dev->dev_num = find_first_zero_bit(dev_mask, VTPM_NUM_DEVICES); We shouldn't artificially limit the number of devices if virtualization is the target. Use an idr, or figure out how to get rid of it. Since it is only used here: dev_set_name(&vtpm_dev->dev, "vtpms%d", vtpm_dev->dev_num); Maybe it could be adjusted to use chip->dev_num instead. This: INIT_WORK(&vtpm_dev->work, vtpm_dev_work); should be near the kzalloc Drop the 1 line functions (vtpm_dev_work_start, vtpm_delete_vtpm_dev, Use u32's here: u8 req_buf[TPM_BUFSIZE]; /* request buffer */ and related to ensure the buffer is sensibly aligned Don't log on user space inputs, use debug or something: dev_err(&vtpm_dev->dev, "Invalid size in recv: count=%zd, req_len=%zd\n", count, len); This: vtpm_dev_work_stop(vtpm_dev); Doesn't kill a running work thread - more synchronization is needed for that corner case Make sure that devm is working for your specific case when this happens: err: /* did not register chip */ vtpm_dev->chip = NULL; Instrument the core, devm should free the tpm chip when user space closes the fd. If not the core needs to provide a non-devm alloc for this driver. (easy) This spin lock: spin_lock(&driver_lock); vtpm_dev->dev_num = find_first_zero_bit(dev_mask, VTPM_NUM_DEVICES); [..] err = device_register(&vtpm_dev->dev); /* does get_device */ spin_unlock(&driver_lock); I'm not comfortable with how big a region that is. If you keep dev_mask, then only lock around the dev_mask manipulation not other stuff. Relock to undo fops_write should fail if op_recv isn't waiting for a buffer. Looks pretty good really Jason ------------------------------------------------------------------------------ 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=272487151&iu=/4140
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/10/2016 11:28:09 AM: > > On Wed, Feb 10, 2016 at 12:15:44AM -0500, Stefan Berger wrote: > > Redid that now. > > https://github.com/stefanberger/linux/commit/ > 83019eaab2cf5eb33f2665efdf9d2a117ed703b2 > > Yeah, I think you've got that now. > > Just a few other random commments.. > > This isn't great: > > vtpm_dev->dev_num = find_first_zero_bit(dev_mask, VTPM_NUM_DEVICES); > > We shouldn't artificially limit the number of devices if > virtualization is the target. Use an idr, or figure out how to get rid > of it. Since it is only used here: > > dev_set_name(&vtpm_dev->dev, "vtpms%d", vtpm_dev->dev_num); > > Maybe it could be adjusted to use chip->dev_num instead. The chip->dev_num comes back from tpmm_chip_alloc() which is called with the device as a parameter. That's the device we're trying to create. So it seems we need to have two independency ways to generate unique device names. My suggestion would have been to increase the bitmap to the maximum size of the minor numbers but that's 20 bits, 1Mb. This would need to be done for both sides. I don't know how idr applies here from what I read in an article. https://lwn.net/Articles/103209/ > > This: > INIT_WORK(&vtpm_dev->work, vtpm_dev_work); > should be near the kzalloc Ok, I'll move that. > > Drop the 1 line functions (vtpm_dev_work_start, vtpm_delete_vtpm_dev, We have these function pairs here: vtpm_dev_work_start vtpm_dev_work_stop and vtpm_create_vtpm_dev vtpm_delete_vtpm_dev Can we just keep them as pairs with the one function undoing what the other one has done. I prefix the one-liners with a 'static inline' and let the compiler optimize the code. > > Use u32's here: > > u8 req_buf[TPM_BUFSIZE]; /* request buffer */ > > and related to ensure the buffer is sensibly aligned May I ask why use u32's for a byte buffer? I am not sure what alignment we need here. 4 byte boundary for maximum memcpy performance? http://lxr.free-electrons.com/source/drivers/char/tpm/tpm-dev.c#L34 http://lxr.free-electrons.com/source/drivers/char/tpm/tpm_i2c_infineon.c#L67 > > Don't log on user space inputs, use debug or something: > > dev_err(&vtpm_dev->dev, > "Invalid size in recv: count=%zd, req_len=%zd\n", > count, len); > > This: > > vtpm_dev_work_stop(vtpm_dev); > > Doesn't kill a running work thread - more synchronization is needed > for that corner case Do you mean another test for STATE_OPEN bit is needed before it enters the wait_event_interruptible in the code below? Otherwise I tested this. What can happen is that the worker thread is stuck in the wait function: static int vtpm_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count) [...] /* wait for response or responder gone */ sig = wait_event_interruptible(vtpm_dev->wq, (vtpm_dev->resp_len != 0 || !test_bit(STATE_OPENED, &vtpm_dev->state))); if (sig) return -EINTR; /* process gone ? */ if (!test_bit(STATE_OPENED, &vtpm_dev->state)) return -EIO; [...] We loop while the task is still busy and kick it out of the above wait queue with this function here. Ok, so far we may kick multiple times since the condition may only be tested after waiting. static void vtpm_fops_undo_open(struct vtpm_dev *vtpm_dev) { clear_bit(STATE_OPENED, &vtpm_dev->state); /* no more TPM responses -- wake up anyone waiting for them */ wake_up_interruptible(&vtpm_dev->wq); } > > Make sure that devm is working for your specific case when this > happens: > Just tested this and it seems to work. Chip is kfree'd once user space closes the fd. > err: > /* did not register chip */ > vtpm_dev->chip = NULL; > > Instrument the core, devm should free the tpm chip when user space > closes the fd. If not the core needs to provide a non-devm alloc for > this driver. (easy) It does that. We only partly unwind in the thread by letting the file descriptor fail upon read and write and do the full unwinding upon file descriptor closure. > > This spin lock: > > spin_lock(&driver_lock); > vtpm_dev->dev_num = find_first_zero_bit(dev_mask, VTPM_NUM_DEVICES); > [..] > err = device_register(&vtpm_dev->dev); /* does get_device */ > spin_unlock(&driver_lock); > > I'm not comfortable with how big a region that is. If you keep > dev_mask, then only lock around the dev_mask manipulation not other > stuff. Relock to undo Ok. > > fops_write should fail if op_recv isn't waiting for a buffer. So we introduce another state flag whether we are in receiving or sending mode ? Return -EBADF in the case of an unexpected write() ? Though -EBADF indicates "fd is not a valid file descriptor or is not open for writing." So return -EIO? > > Looks pretty good really Yes. Thanks. And let's just keep the function pairs for the humans and let the compiler use the 'static inline' . Stefan > > Jason > ------------------------------------------------------------------------------ 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=272487151&iu=/4140
> > We shouldn't artificially limit the number of devices if > > virtualization is the target. Use an idr, or figure out how to get rid > > of it. Since it is only used here: > > > > dev_set_name(&vtpm_dev->dev, "vtpms%d", vtpm_dev->dev_num); > > > > Maybe it could be adjusted to use chip->dev_num instead. > The chip->dev_num comes back from tpmm_chip_alloc() which is called > with the device as a parameter. That's the device we're trying to Hm, that is only needed for devm, could also do the tpm/tpmm split and avoid needing a registered dev. > create. So it seems we need to have two independency ways to generate > unique device names. My suggestion would have been to increase the > bitmap to the maximum size of the minor numbers but that's 20 bits, > 1Mb. This would need to be done for both sides. No way > I don't know how idr applies here from what I read in an article. > [2]https://lwn.net/Articles/103209/ Most subsystems will use a idr to map the index number back to their private number. idr efficiently allocates these numbers. Eg look at how drivers/rtc/class.c uses the ida_ functions. BTW, get rid of vtpm_list - it seems totally unusued. > > > > Drop the 1 line functions (vtpm_dev_work_start, vtpm_delete_vtpm_dev, > We have these function pairs here: > vtpm_dev_work_start > vtpm_dev_work_stop > and > vtpm_create_vtpm_dev > vtpm_delete_vtpm_dev > Can we just keep them as pairs with the one function undoing what the > other one has done. I prefix the one-liners with a 'static inline' and > let the compiler optimize the code. I don't care that much either way.. But these sorts of obfuscating wrappers are not really in line with good kernel practices. Especially if they can only legally be called from one place. Ie the work cleanup can only be properly called from fops release.. > > Use u32's here: > > > > u8 req_buf[TPM_BUFSIZE]; /* request buffer */ > > > > and related to ensure the buffer is sensibly aligned > May I ask why use u32's for a byte buffer? I am not sure what alignment > we need here. 4 byte boundary for maximum memcpy performance? Yes, it is nice to see the alignment guarenteed if memcpy is the primary use of this memory. It almost always happens naturally.. > > Doesn't kill a running work thread - more synchronization is needed > > for that corner case > Do you mean another test for STATE_OPEN bit is needed before it enters > the wait_event_interruptible in the code below? No, just use a typical kernel idiom: clear_bit(STATE_OPENED, &vtpm_dev->state); wake_up_interruptible(&vtpm_dev->wq); flush_workqueue(&vtpm_dev->work); And it would be typical/desirable to see that open coded in the one and only cleanup routine. That is what I usually look for when looking at work queues. I can't rember off hand if you need locking around dev->state to make the above work, IIRC the bit ops are special. Don't use work_busy in production code. > > > > fops_write should fail if op_recv isn't waiting for a buffer. > So we introduce another state flag whether we are in receiving or > sending mode ? Return -EBADF in the case of an unexpected write() ? > Though -EBADF indicates "fd is not a valid file descriptor or is not > open for writing." So return -EIO? Probably yeah, pick an error code makes sense. EIO is probably OK, but maybe shift the 'STATE_OPEN' code to EPIPE or something. Jason ------------------------------------------------------------------------------ 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=272487151&iu=/4140
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/10/2016 05:23:13 PM: > > > > > We shouldn't artificially limit the number of devices if > > > virtualization is the target. Use an idr, or figure out how to get rid > > > of it. Since it is only used here: > > > > > > dev_set_name(&vtpm_dev->dev, "vtpms%d", vtpm_dev->dev_num); > > > > > > Maybe it could be adjusted to use chip->dev_num instead. > > > The chip->dev_num comes back from tpmm_chip_alloc() which is called > > with the device as a parameter. That's the device we're trying to > > Hm, that is only needed for devm, could also do the tpm/tpmm split and > avoid needing a registered dev. > How about another patch on top of the ones I have so far only solving this problem ? > > > > > Use u32's here: > > > > > > u8 req_buf[TPM_BUFSIZE]; /* request buffer */ > > > > > > and related to ensure the buffer is sensibly aligned > > May I ask why use u32's for a byte buffer? I am not sure what alignment > > we need here. 4 byte boundary for maximum memcpy performance? > > Yes, it is nice to see the alignment guarenteed if memcpy is the > primary use of this memory. It almost always happens naturally.. We now have only one buffer and there's a size_t in front of it. So alignment will be at 4 bytes. > > > > Doesn't kill a running work thread - more synchronization is needed > > > for that corner case > > Do you mean another test for STATE_OPEN bit is needed before it enters > > the wait_event_interruptible in the code below? > > No, just use a typical kernel idiom: > > clear_bit(STATE_OPENED, &vtpm_dev->state); > wake_up_interruptible(&vtpm_dev->wq); > flush_workqueue(&vtpm_dev->work); > > And it would be typical/desirable to see that open coded in the > one and only cleanup routine. That is what I usually look for when > looking at work queues. > > I can't rember off hand if you need locking around dev->state to make > the above work, IIRC the bit ops are special. These ops are atomic: http://lxr.free-electrons.com/source/include/asm-generic/bitops/atomic.h#L86 Stefan > > Don't use work_busy in production code. > > > > > > > fops_write should fail if op_recv isn't waiting for a buffer. > > So we introduce another state flag whether we are in receiving or > > sending mode ? Return -EBADF in the case of an unexpected write() ? > > Though -EBADF indicates "fd is not a valid file descriptor or is not > > open for writing." So return -EIO? > > Probably yeah, pick an error code makes sense. EIO is probably OK, but > maybe shift the 'STATE_OPEN' code to EPIPE or something. > > Jason > ------------------------------------------------------------------------------ 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=272487151&iu=/4140
On Wed, Feb 10, 2016 at 07:38:52PM -0500, Stefan Berger wrote: > Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/10/2016 > 05:23:13 PM: > > > > > > > > > We shouldn't artificially limit the number of devices if > > > > virtualization is the target. Use an idr, or figure out how to > get rid > > > > of it. Since it is only used here: > > > > > > > > dev_set_name(&vtpm_dev->dev, "vtpms%d", vtpm_dev->dev_num); > > > > > > > > Maybe it could be adjusted to use chip->dev_num instead. > > > > > The chip->dev_num comes back from tpmm_chip_alloc() which is called > > > with the device as a parameter. That's the device we're trying to > > > > Hm, that is only needed for devm, could also do the tpm/tpmm split and > > avoid needing a registered dev. > > > > How about another patch on top of the ones I have so far only solving this > problem ? IDR has been on my backlog for a long time (over a year now). If you'd create a patch that would migrate the subsystem to that and do also do this split, I could take that patch as an independent contribution and you would have a better baseline to develop on top of. What do you think? /Jarkko ------------------------------------------------------------------------------ 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=272487151&iu=/4140
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 02/11/2016 02:04:26 AM: > From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > To: Stefan Berger/Watson/IBM@IBMUS > Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>, > dhowells@redhat.com, dwmw2@infradead.org, tpmdd-devel@lists.sourceforge.net > Date: 02/11/2016 02:04 AM > Subject: Re: [tpmdd-devel] [PATCH v5 4/5] Initialize TPM and get > durations and timeouts > > On Wed, Feb 10, 2016 at 07:38:52PM -0500, Stefan Berger wrote: > > Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/10/2016 > > 05:23:13 PM: > > > > > > > > > > > > > We shouldn't artificially limit the number of devices if > > > > > virtualization is the target. Use an idr, or figure out how to > > get rid > > > > > of it. Since it is only used here: > > > > > > > > > > dev_set_name(&vtpm_dev->dev, "vtpms%d", vtpm_dev->dev_num); > > > > > > > > > > Maybe it could be adjusted to use chip->dev_num instead. > > > > > > > The chip->dev_num comes back from tpmm_chip_alloc() > which is called > > > > with the device as a parameter. That's the device we're trying to > > > > > > Hm, that is only needed for devm, could also do the tpm/tpmm split and > > > avoid needing a registered dev. > > > > > > > How about another patch on top of the ones I have so far only > solving this > > problem ? > > IDR has been on my backlog for a long time (over a year now). > > If you'd create a patch that would migrate the subsystem to that and > do also do this split, I could take that patch as an independent > contribution and you would have a better baseline to develop on top > of. What do you think? We need two steps here. One to let tpmm_chip_alloc call two functions tpm_chip_alloc + tpmm_chip_dev (better name?), which are both going to be public and called by tpm-vtpm.c, and provide tpm_chip_free. Let me do that. Then we can get rid of the bitmap for the chip->dev_num independently and use idr there. Stefan > > /Jarkko > ------------------------------------------------------------------------------ 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=272487151&iu=/4140
Stefan Berger/Watson/IBM@IBMUS wrote on 02/11/2016 10:24:18 AM: > > If you'd create a patch that would migrate the subsystem to that and > > do also do this split, I could take that patch as an independent > > contribution and you would have a better baseline to develop on top > > of. What do you think? > > We need two steps here. One to let tpmm_chip_alloc call two > functions tpm_chip_alloc + tpmm_chip_dev (better name?), which are > both going to be public and called by tpm-vtpm.c, and provide > tpm_chip_free. Let me do that. Then we can get rid of the bitmap for > the chip->dev_num independently and use idr there. > Updated my branch now and based the driver on this function. https://github.com/stefanberger/linux/commits/vtpm-driver.v3 No current driver needs this additional patch I think. So it can wait. I fixed a locking problem on the way, though... 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=272487151&iu=/4140
On Thu, Feb 11, 2016 at 10:24:18AM -0500, Stefan Berger wrote: > We need two steps here. One to let tpmm_chip_alloc call two functions > tpm_chip_alloc + tpmm_chip_dev (better name?), which are both going to > be public and called by tpm-vtpm.c, and provide tpm_chip_free. Let me > do that. Then we can get rid of the bitmap for the chip->dev_num > independently and use idr there. Make sense. Don't change the names all the drivers would have to be churn'd. tpm_chip_alloc, tpmm_chip_alloc. It is probably OK just to use put_device(&chip->dev), tpm_chip_put is already taken for something else. Don't call it free, it isn't free. Jason ------------------------------------------------------------------------------ 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=272487151&iu=/4140
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/11/2016 01:12:08 PM: > > On Thu, Feb 11, 2016 at 10:24:18AM -0500, Stefan Berger wrote: > > We need two steps here. One to let tpmm_chip_alloc call two functions > > tpm_chip_alloc + tpmm_chip_dev (better name?), which are both going to > > be public and called by tpm-vtpm.c, and provide tpm_chip_free. Let me > > do that. Then we can get rid of the bitmap for the chip->dev_num > > independently and use idr there. > > Make sense. Don't change the names all the drivers would have to be > churn'd. tpm_chip_alloc, tpmm_chip_alloc. > That's right: struct tpm_chip *tpmm_chip_alloc(struct device *dev, const struct tpm_class_ops *ops) { struct tpm_chip *chip; chip = tpm_chip_alloc(ops); if (IS_ERR(chip)) return chip; tpmm_chip_dev(chip, dev); return chip; } EXPORT_SYMBOL_GPL(tpmm_chip_alloc); > It is probably OK just to use put_device(&chip->dev), tpm_chip_put is > already taken for something else. Don't call it free, it isn't free. tpm_chip_free undoes what tpm_chip_alloc did. /** * tpm_chip_alloc() - allocate a new struct tpm_chip instance * @dev: device to which the chip is associated * @ops: struct tpm_class_ops instance * * Allocates a new struct tpm_chip instance and assigns a free * device number for it. */ struct tpm_chip *tpm_chip_alloc(const struct tpm_class_ops *ops) { struct tpm_chip *chip; chip = kzalloc(sizeof(*chip), GFP_KERNEL); if (chip == NULL) return ERR_PTR(-ENOMEM); mutex_init(&chip->tpm_mutex); INIT_LIST_HEAD(&chip->list); chip->ops = ops; spin_lock(&driver_lock); chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES); if (chip->dev_num < TPM_NUM_DEVICES) set_bit(chip->dev_num, dev_mask); spin_unlock(&driver_lock); if (chip->dev_num >= TPM_NUM_DEVICES) { pr_err("No available tpm device numbers\n"); kfree(chip); return ERR_PTR(-ENOMEM); } scnprintf(chip->devname, sizeof(chip->devname), "tpm%d", chip->dev_num); return chip; } EXPORT_SYMBOL_GPL(tpm_chip_alloc); /** * tpm_chip_free() - free tpm_chip previously allocated with tpm_chip_alloc * @chip: the tpm_chip to free */ void tpm_chip_free(struct tpm_chip *chip) { spin_lock(&driver_lock); clear_bit(chip->dev_num, dev_mask); spin_unlock(&driver_lock); kfree(chip); } EXPORT_SYMBOL_GPL(tpm_chip_free); Good? Stefan > > Jason > ------------------------------------------------------------------------------ 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=272487151&iu=/4140
diff --git a/drivers/char/tpm/tpm-vtpm.c b/drivers/char/tpm/tpm-vtpm.c index 6ee1e44..d9fd797 100644 --- a/drivers/char/tpm/tpm-vtpm.c +++ b/drivers/char/tpm/tpm-vtpm.c @@ -41,6 +41,7 @@ struct vtpm_dev { long state; #define STATE_OPENED_BIT 0 +#define STATE_INIT_VTPM 1 spinlock_t buf_lock; /* lock for buffers */ @@ -52,6 +53,8 @@ struct vtpm_dev { size_t resp_len; /* length of queued TPM response */ u8 resp_buf[TPM_BUFSIZE]; /* response buffer */ + struct work_struct work; /* task that retrieves TPM timeouts */ + struct list_head list; }; @@ -292,6 +295,9 @@ static int vtpm_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t count) spin_unlock(&vtpm_dev->buf_lock); + /* sync for first startup command */ + set_bit(STATE_INIT_VTPM, &vtpm_dev->state); + wake_up_interruptible(&vtpm_dev->wq); return rc; @@ -323,6 +329,52 @@ static const struct tpm_class_ops vtpm_tpm_ops = { }; /* + * Code related to the startup of the TPM 2 and startup of TPM 1.2 + + * retrieval of timeouts and durations. + */ + +static void vtpm_dev_work(struct work_struct *work) +{ + struct vtpm_dev *vtpm_dev = container_of(work, struct vtpm_dev, work); + + if (vtpm_dev->flags & VTPM_FLAG_TPM2) + tpm2_startup(vtpm_dev->chip, TPM2_SU_CLEAR); + else { + /* we must send TPM_Startup() first */ + tpm_startup(vtpm_dev->chip, TPM_ST_CLEAR); + tpm_get_timeouts(vtpm_dev->chip); + } +} + +/* + * vtpm_dev_start_work: Schedule the work for TPM 1.2 & 2 initialization + */ +static int vtpm_dev_start_work(struct vtpm_dev *vtpm_dev) +{ + int sig; + + INIT_WORK(&vtpm_dev->work, vtpm_dev_work); + schedule_work(&vtpm_dev->work); + + /* make sure we send the 1st command before user space can */ + sig = wait_event_interruptible(vtpm_dev->wq, + test_bit(STATE_INIT_VTPM, &vtpm_dev->state)); + if (sig) { + cancel_work_sync(&vtpm_dev->work); + return -EINTR; + } + return 0; +} + +/* + * vtpm_dev_stop_work: prevent the scheduled work from running + */ +static void vtpm_dev_stop_work(struct vtpm_dev *vtpm_dev) +{ + cancel_work_sync(&vtpm_dev->work); +} + +/* * Code related to creation and deletion of device pairs */ static void vtpm_dev_release(struct device *dev) @@ -449,6 +501,10 @@ static struct file *vtpm_create_device_pair( } vtpm_dev->chip = chip; + rc = vtpm_dev_start_work(vtpm_dev); + if (rc) + goto err_vtpm_fput; + vtpm_new_pair->fd = fd; vtpm_new_pair->major = MAJOR(vtpm_dev->chip->dev.devt); vtpm_new_pair->minor = MINOR(vtpm_dev->chip->dev.devt); @@ -476,6 +532,8 @@ err_delete_vtpm_dev: */ static void vtpm_delete_device_pair(struct vtpm_dev *vtpm_dev) { + vtpm_dev_stop_work(vtpm_dev); + spin_lock(&driver_lock); list_del_rcu(&vtpm_dev->list); spin_unlock(&driver_lock);
Add the retrieval of TPM 1.2 durations and timeouts. Since this requires the startup of the TPM, do this for TPM 1.2 and TPM 2. To not allow to interleave with commands from user space, so send the TPM_Startup as the first command. The timeouts can then be gotten at any later time, even interleaved with commands from user space. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- drivers/char/tpm/tpm-vtpm.c | 58 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)