[PATCH v5 0/5] Multi-instance vTPM driver

[PATCH v5 0/5] Multi-instance vTPM driver

[Stefan Berger]

The following series of patches implements a multi-instance vTPM driver
that can dynamically create TPM 'server' and client device pairs.

Using an ioctl on the provided /dev/vtpmx, a client-side vTPM device
and a server side file descriptor is created. The file descriptor must
be passed to a TPM emulator. The device driver will initialize the
emulated TPM using TPM 1.2 or TPM 2 startup commands and it will read
the command durations from the device in case of a TPM 1.2. The choice
of emulated TPM device (1.2 or 2) must be provided with a flag in
the ioctl.

   Stefan


Stefan Berger (5):
  Implement tpm_chip_free
  Implement driver for supporting multiple emulated TPMs
  Make tpm_startup() available
  Initialize TPM and get durations and timeouts
  A test program for vTPM device creation

 drivers/char/tpm/Kconfig         |  10 +
 drivers/char/tpm/Makefile        |   1 +
 drivers/char/tpm/tpm-chip.c      |  14 +-
 drivers/char/tpm/tpm-interface.c |   8 +-
 drivers/char/tpm/tpm-vtpm.c      | 664 +++++++++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm.h           |  10 +
 include/uapi/linux/Kbuild        |   1 +
 include/uapi/linux/vtpm.h        |  38 +++
 vtpmctrl.c                       | 117 +++++++
 9 files changed, 854 insertions(+), 9 deletions(-)
 create mode 100644 drivers/char/tpm/tpm-vtpm.c
 create mode 100644 include/uapi/linux/vtpm.h
 create mode 100644 vtpmctrl.c

-- 
2.4.3


------------------------------------------------------------------------------
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

[PATCH v5 1/5] Implement tpm_chip_free

[Stefan Berger]

Implement the public function tpm_chip_free to undo tpmm_chip_alloc.

Signed-off-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/tpm-chip.c | 14 ++++++++++----
 drivers/char/tpm/tpm.h      |  1 +
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index fbd75c5..361cee8 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -59,6 +59,15 @@ struct tpm_chip *tpm_chip_find_get(int chip_num)
 }
 EXPORT_SYMBOL_GPL(tpm_chip_find_get);
 
+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);
+
 /**
  * tpm_dev_release() - free chip memory and the device number
  * @dev: the character device for the TPM chip
@@ -69,10 +78,7 @@ static void tpm_dev_release(struct device *dev)
 {
 	struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev);
 
-	spin_lock(&driver_lock);
-	clear_bit(chip->dev_num, dev_mask);
-	spin_unlock(&driver_lock);
-	kfree(chip);
+	tpm_chip_free(chip);
 }
 
 /**
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a4257a3..440a167 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -515,6 +515,7 @@ extern int wait_for_tpm_stat(struct tpm_chip *, u8, unsigned long,
 struct tpm_chip *tpm_chip_find_get(int chip_num);
 extern struct tpm_chip *tpmm_chip_alloc(struct device *dev,
 				       const struct tpm_class_ops *ops);
+extern void tpm_chip_free(struct tpm_chip *chip);
 extern int tpm_chip_register(struct tpm_chip *chip);
 extern void tpm_chip_unregister(struct tpm_chip *chip);
 
-- 
2.4.3


------------------------------------------------------------------------------
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

[PATCH v5 2/5] Implement driver for supporting multiple emulated TPMs

[Stefan Berger]

This patch implements a driver for supporting multiple emulated TPMs in a
system.

The driver implements a device /dev/vtpmx that is used to created
a client device pair /dev/tpmX (e.g., /dev/tpm10) and a server side that
is accessed using a file descriptor returned by an ioctl.
The device /dev/tpmX is the usual TPM device created by the core TPM
driver. Applications or kernel subsystems can send TPM commands to it
and the corresponding server-side file descriptor receives these
commands and delivers them to an emulated TPM. The device /dev/tpmX can
be moved into a container and appear there as /dev/tpm0.

Signed-off-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/Kconfig    |  10 +
 drivers/char/tpm/Makefile   |   1 +
 drivers/char/tpm/tpm-vtpm.c | 606 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm.h      |   2 +
 include/uapi/linux/Kbuild   |   1 +
 include/uapi/linux/vtpm.h   |  38 +++
 6 files changed, 658 insertions(+)
 create mode 100644 drivers/char/tpm/tpm-vtpm.c
 create mode 100644 include/uapi/linux/vtpm.h

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 3b84a8b..4c4e843 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -122,5 +122,15 @@ config TCG_CRB
 	  from within Linux.  To compile this driver as a module, choose
 	  M here; the module will be called tpm_crb.
 
+config TCG_VTPM
+	tristate "VTPM Interface"
+	depends on TCG_TPM
+	---help---
+	  This driver supports an emulated TPM (vTPM) running in userspace.
+	  A device /dev/vtpmx is provided that creates a device pair
+	  /dev/vtpmX and a server-side file descriptor on which the vTPM
+	  can receive commands.
+
+
 source "drivers/char/tpm/st33zp24/Kconfig"
 endif # TCG_TPM
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 56e8f1f..d947db2 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -23,3 +23,4 @@ obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
 obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
 obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
 obj-$(CONFIG_TCG_CRB) += tpm_crb.o
+obj-$(CONFIG_TCG_VTPM) += tpm-vtpm.o
diff --git a/drivers/char/tpm/tpm-vtpm.c b/drivers/char/tpm/tpm-vtpm.c
new file mode 100644
index 0000000..6ee1e44
--- /dev/null
+++ b/drivers/char/tpm/tpm-vtpm.c
@@ -0,0 +1,606 @@
+/*
+ * Copyright (C) 2015, 2016 IBM Corporation
+ *
+ * Author: Stefan Berger <stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
+ *
+ * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
+ *
+ * Device driver for vTPM.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the
+ * License.
+ *
+ */
+
+#include <linux/types.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/uaccess.h>
+#include <linux/wait.h>
+#include <linux/miscdevice.h>
+#include <linux/vtpm.h>
+#include <linux/file.h>
+#include <linux/anon_inodes.h>
+#include <linux/poll.h>
+#include <linux/compat.h>
+
+#include "tpm.h"
+
+#define VTPM_NUM_DEVICES TPM_NUM_DEVICES
+
+struct vtpm_dev {
+	struct device dev;
+
+	struct tpm_chip *chip;
+
+	u32 flags;                   /* public API flags */
+
+	int dev_num;
+
+	long state;
+#define STATE_OPENED_BIT   0
+
+	spinlock_t buf_lock;         /* lock for buffers */
+
+	wait_queue_head_t wq;
+
+	size_t req_len;              /* length of queued TPM request */
+	u8 req_buf[TPM_BUFSIZE];     /* request buffer */
+
+	size_t resp_len;             /* length of queued TPM response */
+	u8 resp_buf[TPM_BUFSIZE];    /* response buffer */
+
+	struct list_head list;
+};
+
+static DECLARE_BITMAP(dev_mask, VTPM_NUM_DEVICES);
+static LIST_HEAD(vtpm_list);
+static DEFINE_SPINLOCK(driver_lock);
+
+static struct class *vtpm_class;
+
+static void vtpm_delete_device_pair(struct vtpm_dev *vtpm_dev);
+
+/*
+ * Functions related to 'server side'
+ */
+
+/**
+ * vtpm_fops_read - Read TPM commands on 'server side'
+ *
+ * Return value:
+ *	Number of bytes read or negative error code
+ */
+static ssize_t vtpm_fops_read(struct file *filp, char __user *buf,
+			      size_t count, loff_t *off)
+{
+	struct vtpm_dev *vtpm_dev = filp->private_data;
+	size_t len;
+	int sig, rc;
+
+	sig = wait_event_interruptible(vtpm_dev->wq, vtpm_dev->req_len != 0);
+	if (sig)
+		return -EINTR;
+
+	len = vtpm_dev->req_len;
+
+	if (count < len) {
+		dev_err(&vtpm_dev->dev,
+			"Invalid size in recv: count=%zd, req_len=%zd\n",
+			count, len);
+		return -EIO;
+	}
+
+	spin_lock(&vtpm_dev->buf_lock);
+
+	rc = copy_to_user(buf, vtpm_dev->req_buf, len);
+	memset(vtpm_dev->req_buf, 0, len);
+	vtpm_dev->req_len = 0;
+
+	spin_unlock(&vtpm_dev->buf_lock);
+
+	if (rc)
+		return -EFAULT;
+
+	return len;
+}
+
+/**
+ * vtpm_fops_write - Write TPM responses on 'server side'
+ *
+ * Return value:
+ *	Number of bytes read or negative error value
+ */
+static ssize_t vtpm_fops_write(struct file *filp, const char __user *buf,
+			       size_t count, loff_t *off)
+{
+	struct vtpm_dev *vtpm_dev = filp->private_data;
+
+	if (count > sizeof(vtpm_dev->resp_buf))
+		return -EIO;
+
+	spin_lock(&vtpm_dev->buf_lock);
+
+	vtpm_dev->req_len = 0;
+
+	if (copy_from_user(vtpm_dev->resp_buf, buf, count)) {
+		spin_unlock(&vtpm_dev->buf_lock);
+		return -EFAULT;
+	}
+
+	vtpm_dev->resp_len = count;
+
+	spin_unlock(&vtpm_dev->buf_lock);
+
+	wake_up_interruptible(&vtpm_dev->wq);
+
+	return count;
+}
+
+/*
+ * vtpm_fops_poll: Poll status on 'server side'
+ *
+ * Return value:
+ *      Poll flags
+ */
+static unsigned int vtpm_fops_poll(struct file *filp, poll_table *wait)
+{
+	struct vtpm_dev *vtpm_dev = filp->private_data;
+	unsigned ret;
+
+	poll_wait(filp, &vtpm_dev->wq, wait);
+
+	ret = POLLOUT;
+	if (vtpm_dev->req_len)
+		ret |= POLLIN | POLLRDNORM;
+
+	return ret;
+}
+
+/*
+ * vtpm_fops_open - Open vTPM device on 'server side'
+ *
+ * Called when setting up the anonymous file descriptor
+ */
+static void vtpm_fops_open(struct file *filp)
+{
+	struct vtpm_dev *vtpm_dev = filp->private_data;
+
+	set_bit(STATE_OPENED_BIT, &vtpm_dev->state);
+}
+
+/**
+ * vtpm_fops_undo_open - counter-part to vtpm_fops_open
+ *
+ * Call to undo vtpm_fops_open
+ */
+static void vtpm_fops_undo_open(struct vtpm_dev *vtpm_dev)
+{
+	clear_bit(STATE_OPENED_BIT, &vtpm_dev->state);
+
+	/* no more TPM responses -- wake up anyone waiting for them */
+	wake_up_interruptible(&vtpm_dev->wq);
+}
+
+/*
+ * vtpm_fops_release: Close 'server side'
+ *
+ * Return value:
+ *      Always returns 0.
+ */
+static int vtpm_fops_release(struct inode *inode, struct file *filp)
+{
+	struct vtpm_dev *vtpm_dev = filp->private_data;
+
+	filp->private_data = NULL;
+
+	vtpm_delete_device_pair(vtpm_dev);
+
+	return 0;
+}
+
+static const struct file_operations vtpm_fops = {
+	.owner = THIS_MODULE,
+	.llseek = no_llseek,
+	.read = vtpm_fops_read,
+	.write = vtpm_fops_write,
+	.poll = vtpm_fops_poll,
+	.release = vtpm_fops_release,
+};
+
+/*
+ * Functions invoked by the core TPM driver to send TPM commands to
+ * 'server side' and receive responses from there.
+ */
+
+/*
+ * Called when core TPM driver reads TPM responses from 'server side'
+ *
+ * Return value:
+ *      Number of TPM response bytes read, negative error value otherwise
+ */
+static int vtpm_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count)
+{
+	struct vtpm_dev *vtpm_dev = chip->priv;
+	int sig;
+	size_t len;
+
+	if (!vtpm_dev)
+		return -EIO;
+
+	/* wait for response or responder gone */
+	sig = wait_event_interruptible(vtpm_dev->wq,
+		(vtpm_dev->resp_len != 0
+		|| !test_bit(STATE_OPENED_BIT, &vtpm_dev->state)));
+	if (sig)
+		return -EINTR;
+
+	/* process gone ? */
+	if (!test_bit(STATE_OPENED_BIT, &vtpm_dev->state))
+		return -EIO;
+
+	len = vtpm_dev->resp_len;
+	if (count < len) {
+		dev_err(&vtpm_dev->dev,
+			"Invalid size in recv: count=%zd, resp_len=%zd\n",
+			count, len);
+		return -EIO;
+	}
+
+	spin_lock(&vtpm_dev->buf_lock);
+
+	memcpy(buf, vtpm_dev->resp_buf, len);
+	vtpm_dev->resp_len = 0;
+
+	spin_unlock(&vtpm_dev->buf_lock);
+
+	return len;
+}
+
+/*
+ * Called when core TPM driver forwards TPM requests to 'server side'.
+ *
+ * Return value:
+ *      0 in case of success, negative error value otherwise.
+ */
+static int vtpm_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t count)
+{
+	struct vtpm_dev *vtpm_dev = chip->priv;
+	int rc = 0;
+
+	if (!vtpm_dev)
+		return -EIO;
+
+	if (!test_bit(STATE_OPENED_BIT, &vtpm_dev->state))
+		return -EINVAL;
+
+	if (count > sizeof(vtpm_dev->req_buf)) {
+		dev_err(&vtpm_dev->dev,
+			"Invalid size in send: count=%zd, buffer size=%zd\n",
+			count, sizeof(vtpm_dev->req_buf));
+		return -EIO;
+	}
+
+	spin_lock(&vtpm_dev->buf_lock);
+
+	vtpm_dev->resp_len = 0;
+
+	vtpm_dev->req_len = count;
+	memcpy(vtpm_dev->req_buf, buf, count);
+
+	spin_unlock(&vtpm_dev->buf_lock);
+
+	wake_up_interruptible(&vtpm_dev->wq);
+
+	return rc;
+}
+
+static void vtpm_tpm_op_cancel(struct tpm_chip *chip)
+{
+	/* not supported */
+}
+
+static u8 vtpm_tpm_op_status(struct tpm_chip *chip)
+{
+	return 0;
+}
+
+static bool vtpm_tpm_req_canceled(struct tpm_chip  *chip, u8 status)
+{
+	return (status == 0);
+}
+
+static const struct tpm_class_ops vtpm_tpm_ops = {
+	.recv = vtpm_tpm_op_recv,
+	.send = vtpm_tpm_op_send,
+	.cancel = vtpm_tpm_op_cancel,
+	.status = vtpm_tpm_op_status,
+	.req_complete_mask = 0,
+	.req_complete_val = 0,
+	.req_canceled = vtpm_tpm_req_canceled,
+};
+
+/*
+ * Code related to creation and deletion of device pairs
+ */
+static void vtpm_dev_release(struct device *dev)
+{
+	struct vtpm_dev *vtpm_dev = container_of(dev, struct vtpm_dev, dev);
+
+	spin_lock(&driver_lock);
+	clear_bit(vtpm_dev->dev_num, dev_mask);
+	spin_unlock(&driver_lock);
+
+	kfree(vtpm_dev);
+}
+
+static struct device_driver vtpm_driver = {
+	.name = "tpm-vtpm",
+	.owner = THIS_MODULE,
+};
+
+struct vtpm_dev *vtpm_create_vtpm_dev(void)
+{
+	struct vtpm_dev *vtpm_dev;
+	int err;
+
+	vtpm_dev = kzalloc(sizeof(*vtpm_dev), GFP_KERNEL);
+	if (vtpm_dev == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	init_waitqueue_head(&vtpm_dev->wq);
+	spin_lock_init(&vtpm_dev->buf_lock);
+
+	spin_lock(&driver_lock);
+	vtpm_dev->dev_num = find_first_zero_bit(dev_mask, VTPM_NUM_DEVICES);
+
+	if (vtpm_dev->dev_num >= VTPM_NUM_DEVICES) {
+		spin_unlock(&driver_lock);
+		kfree(vtpm_dev);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	/* device is needed for core TPM driver */
+	vtpm_dev->dev.class = vtpm_class;
+	vtpm_dev->dev.release = vtpm_dev_release;
+	vtpm_dev->dev.driver = &vtpm_driver;
+	dev_set_name(&vtpm_dev->dev, "vtpms%d", vtpm_dev->dev_num);
+
+	err = device_register(&vtpm_dev->dev); /* does get_device */
+	if (err) {
+		spin_unlock(&driver_lock);
+		kfree(vtpm_dev);
+		return ERR_PTR(err);
+	}
+
+	set_bit(vtpm_dev->dev_num, dev_mask);
+
+	spin_unlock(&driver_lock);
+
+	return vtpm_dev;
+}
+
+/*
+ * Undo what has been done in vtpm_create_vtpm_dev
+ */
+void vtpm_delete_vtpm_dev(struct vtpm_dev *vtpm_dev)
+{
+	device_unregister(&vtpm_dev->dev); /* does put_device */
+}
+
+/*
+ * Create a /dev/tpm%d and 'server side' file descriptor pair
+ *
+ * Return value:
+ *      Returns file pointer on success, an error value otherwise
+ */
+static struct file *vtpm_create_device_pair(
+				       struct vtpm_new_pair *vtpm_new_pair)
+{
+	struct vtpm_dev *vtpm_dev;
+	int rc, fd;
+	struct file *file;
+	struct tpm_chip *chip;
+
+	vtpm_dev = vtpm_create_vtpm_dev();
+	if (IS_ERR(vtpm_dev))
+		return ERR_CAST(vtpm_dev);
+
+	vtpm_dev->flags = vtpm_new_pair->flags;
+
+	/* setup an anonymous file for the server-side */
+	fd = get_unused_fd_flags(O_RDWR);
+	if (fd < 0) {
+		rc = fd;
+		goto err_delete_vtpm_dev;
+	}
+
+	file = anon_inode_getfile("[vtpms]", &vtpm_fops, vtpm_dev, O_RDWR);
+	if (IS_ERR(file)) {
+		rc = PTR_ERR(file);
+		goto err_put_unused_fd;
+	}
+
+	spin_lock(&driver_lock);
+	list_add_rcu(&vtpm_dev->list, &vtpm_list);
+	spin_unlock(&driver_lock);
+
+	/* from now on we can unwind with put_unused_fd() + fput() */
+	/* simulate an open() on the server side */
+	vtpm_fops_open(file);
+
+	chip = tpmm_chip_alloc(&vtpm_dev->dev, &vtpm_tpm_ops);
+	if (IS_ERR(chip)) {
+		rc = PTR_ERR(chip);
+		goto err_vtpm_fput;
+	}
+
+	chip->priv = vtpm_dev;
+
+	if (vtpm_dev->flags & VTPM_FLAG_TPM2)
+		chip->flags |= TPM_CHIP_FLAG_TPM2;
+
+	rc = tpm_chip_register(chip);
+	if (rc) {
+		tpm_chip_free(chip);
+		goto err_vtpm_fput;
+	}
+	vtpm_dev->chip = chip;
+
+	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);
+	vtpm_new_pair->tpm_dev_num = vtpm_dev->chip->dev_num;
+
+	return file;
+
+err_vtpm_fput:
+	put_unused_fd(fd);
+	fput(file);
+
+	return ERR_PTR(rc);
+
+err_put_unused_fd:
+	put_unused_fd(fd);
+
+err_delete_vtpm_dev:
+	vtpm_delete_vtpm_dev(vtpm_dev);
+
+	return ERR_PTR(rc);
+}
+
+/*
+ * Counter part to vtpm_create_device_pair.
+ */
+static void vtpm_delete_device_pair(struct vtpm_dev *vtpm_dev)
+{
+	spin_lock(&driver_lock);
+	list_del_rcu(&vtpm_dev->list);
+	spin_unlock(&driver_lock);
+
+	synchronize_rcu();
+
+	if (vtpm_dev->chip)
+		tpm_chip_unregister(vtpm_dev->chip);
+
+	vtpm_fops_undo_open(vtpm_dev);
+
+	vtpm_delete_vtpm_dev(vtpm_dev);
+}
+
+/*
+ * Code related to the control device /dev/vtpmx
+ */
+
+/*
+ * vtpmx_fops_ioctl: ioctl on /dev/vtpmx
+ *
+ * Return value:
+ *      Returns 0 on success, a negative error code otherwise.
+ */
+static long vtpmx_fops_ioctl(struct file *f, unsigned int ioctl,
+			    unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	struct vtpm_new_pair *vtpm_new_pair_p;
+	struct vtpm_new_pair vtpm_new_pair;
+	struct file *file;
+
+	switch (ioctl) {
+	case VTPM_NEW_DEV:
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+		vtpm_new_pair_p = argp;
+		if (copy_from_user(&vtpm_new_pair, vtpm_new_pair_p,
+				   sizeof(vtpm_new_pair)))
+			return -EFAULT;
+		file = vtpm_create_device_pair(&vtpm_new_pair);
+		if (IS_ERR(file))
+			return PTR_ERR(file);
+		if (copy_to_user(vtpm_new_pair_p, &vtpm_new_pair,
+				 sizeof(vtpm_new_pair))) {
+			put_unused_fd(vtpm_new_pair.fd);
+			fput(file);
+			return -EFAULT;
+		}
+
+		fd_install(vtpm_new_pair.fd, file);
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+#ifdef CONFIG_COMPAT
+static long vtpmx_fops_compat_ioctl(struct file *f, unsigned int ioctl,
+				    unsigned long arg)
+{
+	return vtpmx_fops_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
+}
+#endif
+
+static const struct file_operations vtpmx_fops = {
+	.owner = THIS_MODULE,
+	.unlocked_ioctl = vtpmx_fops_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = vtpmx_fops_compat_ioctl,
+#endif
+	.llseek = noop_llseek,
+};
+
+static struct miscdevice vtpmx_miscdev = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "vtpmx",
+	.fops = &vtpmx_fops,
+};
+
+static int vtpmx_init(void)
+{
+	return misc_register(&vtpmx_miscdev);
+}
+
+static void vtpmx_cleanup(void)
+{
+	misc_deregister(&vtpmx_miscdev);
+}
+
+static int __init vtpm_module_init(void)
+{
+	int rc;
+
+	vtpm_class = class_create(THIS_MODULE, "vtpm");
+	if (IS_ERR(vtpm_class)) {
+		pr_err("couldn't create vtpm class\n");
+		return PTR_ERR(vtpm_class);
+	}
+
+	rc = vtpmx_init();
+	if (rc) {
+		pr_err("couldn't create vtpmx device\n");
+		goto err_vtpmx;
+	}
+
+	return 0;
+
+err_vtpmx:
+	class_destroy(vtpm_class);
+
+	return rc;
+}
+
+static void __exit vtpm_module_exit(void)
+{
+	vtpmx_cleanup();
+	class_destroy(vtpm_class);
+}
+
+module_init(vtpm_module_init);
+module_exit(vtpm_module_exit);
+
+MODULE_AUTHOR("Stefan Berger (stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org)");
+MODULE_DESCRIPTION("vTPM Driver");
+MODULE_VERSION("0.1");
+MODULE_LICENSE("GPL");
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 440a167..2fb59f8 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -197,6 +197,8 @@ struct tpm_chip {
 #endif /* CONFIG_ACPI */
 
 	struct list_head list;
+
+	void *priv;
 };
 
 #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index c2e5d6c..c194d61 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -449,6 +449,7 @@ header-y += virtio_scsi.h
 header-y += virtio_types.h
 header-y += vm_sockets.h
 header-y += vt.h
+header-y += vtpm.h
 header-y += wait.h
 header-y += wanrouter.h
 header-y += watchdog.h
diff --git a/include/uapi/linux/vtpm.h b/include/uapi/linux/vtpm.h
new file mode 100644
index 0000000..aef8733
--- /dev/null
+++ b/include/uapi/linux/vtpm.h
@@ -0,0 +1,38 @@
+/*
+ * Definitions for the VTPM interface
+ * Copyright (c) 2015, 2016, IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#ifndef _UAPI_LINUX_VTPM_H
+#define _UAPI_LINUX_VTPM_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+/* ioctls */
+
+struct vtpm_new_pair {
+	__u32 flags;         /* input */
+	__u32 tpm_dev_num;   /* output */
+	__u32 fd;            /* output */
+	__u32 major;         /* output */
+	__u32 minor;         /* output */
+};
+
+/* above flags */
+#define VTPM_FLAG_TPM2           1  /* emulator is TPM 2 */
+
+#define VTPM_TPM 0xa0
+
+#define VTPM_NEW_DEV         _IOW(VTPM_TPM, 0x00, struct vtpm_new_pair)
+
+#endif /* _UAPI_LINUX_VTPM_H */
-- 
2.4.3


------------------------------------------------------------------------------
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

[PATCH v5 3/5] Make tpm_startup() available

[Stefan Berger]

Make tpm_startup() available for others to call.

Signed-off-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/tpm-interface.c | 8 +++-----
 drivers/char/tpm/tpm.h           | 7 +++++++
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 792731e..cda8ef6 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -474,24 +474,22 @@ void tpm_gen_interrupt(struct tpm_chip *chip)
 EXPORT_SYMBOL_GPL(tpm_gen_interrupt);
 
 #define TPM_ORD_STARTUP cpu_to_be32(153)
-#define TPM_ST_CLEAR cpu_to_be16(1)
-#define TPM_ST_STATE cpu_to_be16(2)
-#define TPM_ST_DEACTIVATED cpu_to_be16(3)
 static const struct tpm_input_header tpm_startup_header = {
 	.tag = TPM_TAG_RQU_COMMAND,
 	.length = cpu_to_be32(12),
 	.ordinal = TPM_ORD_STARTUP
 };
 
-static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
+int tpm_startup(struct tpm_chip *chip, u16 startup_type)
 {
 	struct tpm_cmd_t start_cmd;
 	start_cmd.header.in = tpm_startup_header;
 
-	start_cmd.params.startup_in.startup_type = startup_type;
+	start_cmd.params.startup_in.startup_type = cpu_to_be16(startup_type);
 	return tpm_transmit_cmd(chip, &start_cmd, TPM_INTERNAL_RESULT_SIZE,
 				"attempting to start the TPM");
 }
+EXPORT_SYMBOL_GPL(tpm_startup);
 
 int tpm_get_timeouts(struct tpm_chip *chip)
 {
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2fb59f8..68510d8 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -383,6 +383,12 @@ struct tpm_startup_in {
 	__be16	startup_type;
 } __packed;
 
+enum tpm_startup_types {
+	TPM_ST_CLEAR		= 0x0001,
+	TPM_ST_STATE		= 0x0002,
+	TPM_ST_DEACTIVATED	= 0x0003,
+};
+
 typedef union {
 	struct	tpm_getcap_params_out getcap_out;
 	struct	tpm_readpubek_params_out readpubek_out;
@@ -505,6 +511,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
 		     size_t bufsiz);
 ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
 			 const char *desc);
+extern int tpm_startup(struct tpm_chip *, u16 startup_type);
 extern int tpm_get_timeouts(struct tpm_chip *);
 extern void tpm_gen_interrupt(struct tpm_chip *);
 extern int tpm_do_selftest(struct tpm_chip *);
-- 
2.4.3


------------------------------------------------------------------------------
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

[PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

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-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/tpm-vtpm.c | 58 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

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);
-- 
2.4.3


------------------------------------------------------------------------------
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

[PATCH v5 5/5] A test program for vTPM device creation

[Stefan Berger]

This patch provides a program that is for testing purposes only.

Build it using the following commands:

make headers_install ARCH=x86_64  INSTALL_HDR_PATH=/usr

gcc vtpmctrl.c -o vtpmctrl


To use it:

To create a device pair and have vtpmctrl listen for commands, display
them and respond with TPM success messages do:

#> ./vtpmctrl
Created TPM device /dev/tpm0; vTPM device has fd 4, major/minor = 10/224.

In another shell do

#> exec 100<>/dev/tpm0
#> /bin/echo -en '\x00\xc1\x00\x00\x00\x0a\x00\x00\x00\x00' >&100
#> od -t x1 <&100
00000000 00 c4 00 00 00 0a 00 00 00 00
00000012
#> exec 100>&-

Signed-off-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 vtpmctrl.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 117 insertions(+)
 create mode 100644 vtpmctrl.c

diff --git a/vtpmctrl.c b/vtpmctrl.c
new file mode 100644
index 0000000..d39cb23
--- /dev/null
+++ b/vtpmctrl.c
@@ -0,0 +1,117 @@
+/*
+ * vtpmctrl.c -- Linux vTPM driver control program
+ *
+ * (c) Copyright IBM Corporation 2015.
+ *
+ * Author: Stefan Berger <stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * Redistributions of source code must retain the above copyright notice,
+ * this list of conditions and the following disclaimer.
+ *
+ * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * Neither the names of the IBM Corporation nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+
+#include <linux/vtpm.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <string.h>
+
+int vtpmctrl_create(void)
+{
+	int fd, n, option, li, serverfd, nn;
+	struct vtpm_new_pair vtpm_new_pair = {
+		.flags = 0,
+	};
+	char tpmdev[16];
+	unsigned char buffer[4096];
+	const unsigned char response[] = {
+		0x00, 0xc4, 0x00, 0x00, 0x00, 0x0a, 0x00, 0x00, 0x00, 0x00
+	};
+
+	fd = open("/dev/vtpmx", O_RDWR);
+	if (fd < 0) {
+		perror("Could not open /dev/vtpmx");
+		return 1;
+	}
+
+	n = ioctl(fd, VTPM_NEW_DEV, &vtpm_new_pair);
+	if (n != 0) {
+		perror("ioctl to create dev pair failed");
+		close(fd);
+		return 1;
+	}
+
+	snprintf(tpmdev, sizeof(tpmdev), "/dev/tpm%u",
+		 vtpm_new_pair.tpm_dev_num);
+
+	serverfd = vtpm_new_pair.fd;
+
+	printf("Created TPM device %s; vTPM device has fd %d, "
+	       "major/minor = %u/%u.\n",
+	       tpmdev, serverfd, vtpm_new_pair.major, vtpm_new_pair.minor);
+
+	close(fd);
+
+	while (1) {
+		n = read(serverfd, buffer, sizeof(buffer));
+		if (n > 0) {
+			printf("Request with %d bytes:\n", n);
+			nn = 0;
+			while (nn < n) {
+				printf("0x%02x ", buffer[nn]);
+				nn++;
+				if (nn % 16 == 0)
+					printf("\n");
+			}
+			printf("\n");
+			n = write(serverfd, response, sizeof(response));
+			if (n < 0) {
+				printf("Error from writing the response: %s\n",
+				       strerror(errno));
+				break;
+			} else {
+				printf("Sent response with %d bytes.\n", n);
+			}
+		} else {
+			break;
+		}
+	}
+
+	return 0;
+}
+
+int main(int argc, char *argv[])
+{
+	return vtpmctrl_create();
+}
-- 
2.4.3


------------------------------------------------------------------------------
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

Re: [PATCH v5 1/5] Implement tpm_chip_free

[Jason Gunthorpe]

On Mon, Feb 08, 2016 at 02:27:04PM -0500, Stefan Berger wrote:
> Implement the public function tpm_chip_free to undo tpmm_chip_alloc.

No, this is wrong, chip cannot be kfree'd like that.

tpmm needs to rely on devm for cleanup.

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

Re: [PATCH v5 3/5] Make tpm_startup() available

[Jason Gunthorpe]

On Mon, Feb 08, 2016 at 02:27:06PM -0500, Stefan Berger wrote:
> Make tpm_startup() available for others to call.

Why? Just call tpm_get_timeouts.

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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

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

Re: [PATCH v5 3/5] Make tpm_startup() available

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 783 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/09/2016 
12:29:57 AM:

> 
> On Mon, Feb 08, 2016 at 02:27:06PM -0500, Stefan Berger wrote:
> > Make tpm_startup() available for others to call.
> 
> Why? Just call tpm_get_timeouts.

See the subsequent patch.

tpm_get_timeouts will first try to get the timeouts and will fail since 
the TPM hasn't been initialized
and only then it will try a tpm_startup.

http://lxr.free-electrons.com/source/drivers/char/tpm/tpm-interface.c#L508

We want to prevent that user space is already waiting in the transmission 
lock waiting to send its first
command and ends up sending a command before the tpm_startup.

http://lxr.free-electrons.com/ident?i=mutex_lock


   Stefan



[-- Attachment #1.2: Type: text/html, Size: 1293 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 2054 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
> 



[-- Attachment #1.2: Type: text/html, Size: 2746 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

On Tue, Feb 09, 2016 at 11:19:15AM -0500, Stefan Berger wrote:
> Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 1843 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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
> 



[-- Attachment #1.2: Type: text/html, Size: 2481 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 1402 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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



[-- Attachment #1.2: Type: text/html, Size: 1822 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 1455 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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



[-- Attachment #1.2: Type: text/html, Size: 1782 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jarkko Sakkinen]

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-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 1962 bytes --]

Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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
> 



[-- Attachment #1.2: Type: text/html, Size: 2761 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jarkko Sakkinen]

On Wed, Feb 10, 2016 at 12:15:44AM -0500, Stefan Berger wrote:
>    Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 5276 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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
> 


[-- Attachment #1.2: Type: text/html, Size: 10283 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

>    > 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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 2855 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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
> 



[-- Attachment #1.2: Type: text/html, Size: 4163 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jarkko Sakkinen]

On Wed, Feb 10, 2016 at 07:38:52PM -0500, Stefan Berger wrote:
>    Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 2354 bytes --]

Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote on 02/11/2016 
02:04:26 AM:

> From: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> To: Stefan Berger/Watson/IBM@IBMUS
> Cc: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>, 
> dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org, 
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> 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-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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
> 



[-- Attachment #1.2: Type: text/html, Size: 3323 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 919 bytes --]

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



[-- Attachment #1.2: Type: text/html, Size: 1285 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 2805 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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
> 


[-- Attachment #1.2: Type: text/html, Size: 6248 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

On Thu, Feb 11, 2016 at 02:10:56PM -0500, Stefan Berger wrote:

>    > 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);

No, just call the one devm_ function here (Based this work on top of
Jarkko's patch that adds the devm function)

>    > 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.

No, once a chip is created we want the kref to be functional, that
means it can only ever be free'd by put_device. Do not create a
tpm_chip_free.

>     * 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);

I see, why you got confused - don't try and remove device_initialize,
it doesn't care about the parent pointer.

Move the dev_set_drvdata into tpmm_chip_alloc
Move the cdev.owner to before cdev_add

A tpm_chip should never be in a state where the kref doesn't work.

Something like this [be careful merging this on top of Jarkko's final
patch adding the devm call].

pdev could even be passed as null for tpm_chip_alloc, as long as it is
properly set before registration, but I'd probably init you vtpm
device, then alloc the chip copy the id, then register the vtpm.

>From 4b1b31c351f838d608644660eb178b4c1f92bb7c Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Date: Thu, 11 Feb 2016 12:45:48 -0700
Subject: [PATCH] tpm: Split out the devm stuff from tpmm_chip_alloc

tpm_chip_alloc becomes a typical subsystem allocate call.

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 drivers/char/tpm/tpm-chip.c | 46 +++++++++++++++++++++++++++++++++++----------
 drivers/char/tpm/tpm.h      |  2 ++
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 999cbd9b388c..8134003b023c 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -77,15 +77,15 @@ static void tpm_dev_release(struct device *dev)
 /**
  * tpmm_chip_alloc() - allocate a new struct tpm_chip instance
  * @dev: device to which the chip is associated
+ *       At this point pdev must be initalized, but does not have to be
+ *       registered.
  * @ops: struct tpm_class_ops instance
  *
  * Allocates a new struct tpm_chip instance and assigns a free
- * device number for it. Caller does not have to worry about
- * freeing the allocated resources. When the devices is removed
- * devres calls tpmm_chip_remove() to do the job.
+ * device number for it. Must be pair'd with put_device(&chip->dev).
  */
-struct tpm_chip *tpmm_chip_alloc(struct device *dev,
-				 const struct tpm_class_ops *ops)
+struct tpm_chip *tpm_chip_alloc(struct device *dev,
+				const struct tpm_class_ops *ops)
 {
 	struct tpm_chip *chip;
 
@@ -109,13 +109,12 @@ struct tpm_chip *tpmm_chip_alloc(struct device *dev,
 	}
 
 	set_bit(chip->dev_num, dev_mask);
+	device_initialize(&chip->dev);
 
 	scnprintf(chip->devname, sizeof(chip->devname), "tpm%d", chip->dev_num);
 
 	chip->pdev = dev;
 
-	dev_set_drvdata(dev, chip);
-
 	chip->dev.class = tpm_class;
 	chip->dev.release = tpm_dev_release;
 	chip->dev.parent = chip->pdev;
@@ -130,20 +129,47 @@ struct tpm_chip *tpmm_chip_alloc(struct device *dev,
 
 	dev_set_name(&chip->dev, "%s", chip->devname);
 
-	device_initialize(&chip->dev);
-
 	cdev_init(&chip->cdev, &tpm_fops);
-	chip->cdev.owner = chip->pdev->driver->owner;
 	chip->cdev.kobj.parent = &chip->dev.kobj;
 
 	return chip;
 }
+EXPORT_SYMBOL_GPL(tpm_chip_alloc);
+
+/**
+ * tpmm_chip_alloc() - allocate a new struct tpm_chip instance
+ * @dev: device to which the chip is associated
+ *       Must be fully registered and able to handle devm.
+ * @ops: struct tpm_class_ops instance
+ *
+ * Same as tpm_chip_alloc except devm is used to do the put_device
+ */
+struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
+				 const struct tpm_class_ops *ops)
+{
+	struct tpm_chip *chip;
+	int rc;
+
+	chip = tpm_chip_alloc(pdev, ops);
+	if (IS_ERR(chip))
+		return chip;
+	rc = devm_add_action(pdev, (void (*)(void *)) put_device, &chip->dev);
+	if (rc) {
+		put_device(&chip->dev);
+		return ERR_PTR(rc);
+	}
+
+	dev_set_drvdata(pdev, chip);
+
+	return chip;
+}
 EXPORT_SYMBOL_GPL(tpmm_chip_alloc);
 
 static int tpm_dev_add_device(struct tpm_chip *chip)
 {
 	int rc;
 
+	chip->cdev.owner = chip->pdev->driver->owner;
 	rc = cdev_add(&chip->cdev, chip->dev.devt, 1);
 	if (rc) {
 		dev_err(&chip->dev,
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2fd5ee2ed7ad..3a5452f09b96 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -508,6 +508,8 @@ extern int wait_for_tpm_stat(struct tpm_chip *, u8, unsigned long,
 			     wait_queue_head_t *, bool);
 
 struct tpm_chip *tpm_chip_find_get(int chip_num);
+extern struct tpm_chip *tpm_chip_alloc(struct device *dev,
+				       const struct tpm_class_ops *ops);
 extern struct tpm_chip *tpmm_chip_alloc(struct device *dev,
 				       const struct tpm_class_ops *ops);
 extern int tpm_chip_register(struct tpm_chip *chip);
-- 
2.1.4


------------------------------------------------------------------------------
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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 1091 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/11/2016 
02:48:10 PM:

> 
> On Thu, Feb 11, 2016 at 02:10:56PM -0500, Stefan Berger wrote:
> 
> >    > 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);
> 
> No, just call the one devm_ function here (Based this work on top of
> Jarkko's patch that adds the devm function)

Ok.

So here's the new patch.

https://github.com/stefanberger/linux/commit/a1037db7fb239c46449b054bfd1a1d18b54179f3

I removed dev from parameter to tpm_chip_alloc. We don't need to provide 
it here but it has to be a parameter to tpmm_chip_dev.

   Stefan


[-- Attachment #1.2: Type: text/html, Size: 1893 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

On Thu, Feb 11, 2016 at 05:10:28PM -0500, Stefan Berger wrote:
>    Ok.
>    So here's the new patch.
>    [1]https://github.com/stefanberger/linux/commit/a1037db7fb239c46449b054
>    bfd1a1d18b54179f3

What is the point of tpmm_chip_dev?

Just have the vtpm driver do device_initialize, then tpm_alloc and
have the release function do put_device on the chip. No need for devm
at all

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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 966 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/11/2016 
05:18:22 PM:


> 
> On Thu, Feb 11, 2016 at 05:10:28PM -0500, Stefan Berger wrote:
> >    Ok.
> >    So here's the new patch.
> >    [1]
https://github.com/stefanberger/linux/commit/a1037db7fb239c46449b054
> >    bfd1a1d18b54179f3
> 
> What is the point of tpmm_chip_dev?

So that the usage model of the chip is the same. We get this in the 
tpm-vtpm.c with tpm_alloc_chip + tpmm_chip_dev while all others can call 
tpmm_chip_alloc, which combines the two.

> 
> Just have the vtpm driver do device_initialize, then tpm_alloc and
> have the release function do put_device on the chip. No need for devm
> at all

I would also like to have a tpm_chip_put (static inline in tpm.h ?) that 
wraps the put_device. To me this is more intuitive than calling 
put_device() as a counter-part to tpm_chip_alloc.

    Stefan


> 
> Jason
> 



[-- Attachment #1.2: Type: text/html, Size: 1468 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

On Thu, Feb 11, 2016 at 05:26:24PM -0500, Stefan Berger wrote:

>    > What is the point of tpmm_chip_dev?
>    So that the usage model of the chip is the same. We get this in the
>    tpm-vtpm.c with tpm_alloc_chip + tpmm_chip_dev while all others can
>    call tpmm_chip_alloc, which combines the two.

No need, just don't use devm in vtpm, that is even better. The
standard devm idiom is a with and without version.

>    > Just have the vtpm driver do device_initialize, then tpm_alloc and
>    > have the release function do put_device on the chip. No need for devm
>    > at all

>    I would also like to have a tpm_chip_put (static inline in tpm.h ?)
>    that wraps the put_device. To me this is more intuitive than calling
>    put_device() as a counter-part to tpm_chip_alloc.

Many in the kernel community would call this sort of wrapping
obfuscation.. We don't have a put_platform_device, etc for
instance. Naked put_device in an error path is fine.

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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 1480 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/11/2016 
06:56:11 PM:


> 
> On Thu, Feb 11, 2016 at 05:26:24PM -0500, Stefan Berger wrote:
> 
> >    > What is the point of tpmm_chip_dev?
> >    So that the usage model of the chip is the same. We get this in the
> >    tpm-vtpm.c with tpm_alloc_chip + tpmm_chip_dev while all others can
> >    call tpmm_chip_alloc, which combines the two.
> 
> No need, just don't use devm in vtpm, that is even better. The
> standard devm idiom is a with and without version.

Updated the branch. Are you going to upstream your patch? Otherwise I 
would just add your Signed-off-by to it if that's ok ?

https://github.com/stefanberger/linux/tree/vtpm-driver.v3

> 
> >    > Just have the vtpm driver do device_initialize, then tpm_alloc 
and
> >    > have the release function do put_device on the chip. No need for 
devm
> >    > at all
> 
> >    I would also like to have a tpm_chip_put (static inline in tpm.h ?)
> >    that wraps the put_device. To me this is more intuitive than 
calling
> >    put_device() as a counter-part to tpm_chip_alloc.
> 
> Many in the kernel community would call this sort of wrapping
> obfuscation.. We don't have a put_platform_device, etc for
> instance. Naked put_device in an error path is fine.

I guess it's a matter of getting used to. I still like being 'guided' by 
function names...

   Stefan

> 
> Jason
> 



[-- Attachment #1.2: Type: text/html, Size: 2102 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 1026 bytes --]

Stefan Berger/Watson/IBM@IBMUS wrote on 02/11/2016 10:56:47 PM:

> 
> Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/11/
> 2016 06:56:11 PM:
> 
> 
> > 
> > On Thu, Feb 11, 2016 at 05:26:24PM -0500, Stefan Berger wrote:
> > 
> > >    > What is the point of tpmm_chip_dev?
> > >    So that the usage model of the chip is the same. We get this in 
the
> > >    tpm-vtpm.c with tpm_alloc_chip + tpmm_chip_dev while all others 
can
> > >    call tpmm_chip_alloc, which combines the two.
> > 
> > No need, just don't use devm in vtpm, that is even better. The
> > standard devm idiom is a with and without version.
> 
> Updated the branch. Are you going to upstream your patch? Otherwise 
> I would just add your Signed-off-by to it if that's ok ?
> 
> https://github.com/stefanberger/linux/tree/vtpm-driver.v3

I converted tpm-chip.c to use IDA as well. 

A test for 4096 vTPM instances:

for ((i = 0; i < 4096; i++)); do ./vtpmctrl & done

   Stefan



[-- Attachment #1.2: Type: text/html, Size: 1513 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

On Fri, Feb 12, 2016 at 01:13:39PM -0500, Stefan Berger wrote:
>    Stefan Berger/Watson/IBM@IBMUS wrote on 02/11/2016 10:56:47 PM:
>    >
>    > Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/11/
>    > 2016 06:56:11 PM:
>    >
>    >
>    > >
>    > > On Thu, Feb 11, 2016 at 05:26:24PM -0500, Stefan Berger wrote:
>    > >
>    > > >    > What is the point of tpmm_chip_dev?
>    > > >    So that the usage model of the chip is the same. We get this
>    in the
>    > > >    tpm-vtpm.c with tpm_alloc_chip + tpmm_chip_dev while all
>    others can
>    > > >    call tpmm_chip_alloc, which combines the two.
>    > >
>    > > No need, just don't use devm in vtpm, that is even better. The
>    > > standard devm idiom is a with and without version.
>    >
>    > Updated the branch. Are you going to upstream your patch? Otherwise
>    > I would just add your Signed-off-by to it if that's ok ?
>    >
>    > [1]https://github.com/stefanberger/linux/tree/vtpm-driver.v3
>    I converted tpm-chip.c to use IDA as well.
>    A test for 4096 vTPM instances:
>    for ((i = 0; i < 4096; i++)); do ./vtpmctrl & done

Yeah, that looks good, thanks for doing this

Do other places using ida for this still limit the number of char
devices? It feels strange to pre allocate so many, but I don't know
off hand a solution.

Please also get rid of tpm_chip_list, just use the ida lookup in
tpm_chip_find_get.

Use err here, dev_num really should be unsigned:

+	chip->dev_num = ida_simple_get(&dev_nums_ida, 0, TPM_NUM_DEVICES, GFP_KERNEL);
+	if (chip->dev_num < 0) {

And use something like this to deal with the size of devname:

>From afc11daa8d0762c4ad0315b762bf1eab95e46ee2 Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Date: Fri, 12 Feb 2016 11:32:37 -0700
Subject: [PATCH] tpm: Get rid of devname

Now that we have a proper struct device just use dev_name() to
access this value instead of keeping two copies.

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 drivers/char/tpm/tpm-chip.c        | 17 +++++++++++------
 drivers/char/tpm/tpm.h             |  1 -
 drivers/char/tpm/tpm_eventlog.c    |  2 +-
 drivers/char/tpm/tpm_eventlog.h    |  2 +-
 drivers/char/tpm/tpm_i2c_nuvoton.c |  2 +-
 drivers/char/tpm/tpm_tis.c         |  2 +-
 6 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 8134003b023c..c07e397bfddd 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -88,6 +88,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
 				const struct tpm_class_ops *ops)
 {
 	struct tpm_chip *chip;
+	int err;
 
 	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
 	if (chip == NULL)
@@ -111,8 +112,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
 	set_bit(chip->dev_num, dev_mask);
 	device_initialize(&chip->dev);
 
-	scnprintf(chip->devname, sizeof(chip->devname), "tpm%d", chip->dev_num);
-
 	chip->pdev = dev;
 
 	chip->dev.class = tpm_class;
@@ -127,12 +126,18 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
 	else
 		chip->dev.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num);
 
-	dev_set_name(&chip->dev, "%s", chip->devname);
+	err = dev_set_name(&chip->dev, "tpm%d", chip->dev_num);
+	if (err)
+		goto out;
 
 	cdev_init(&chip->cdev, &tpm_fops);
 	chip->cdev.kobj.parent = &chip->dev.kobj;
 
 	return chip;
+
+out:
+	device_put(&chip->dev);
+	return PTR_ERR(err);
 }
 EXPORT_SYMBOL_GPL(tpm_chip_alloc);
 
@@ -174,7 +179,7 @@ static int tpm_dev_add_device(struct tpm_chip *chip)
 	if (rc) {
 		dev_err(&chip->dev,
 			"unable to cdev_add() %s, major %d, minor %d, err=%d\n",
-			chip->devname, MAJOR(chip->dev.devt),
+			dev_name(&chip->dev), MAJOR(chip->dev.devt),
 			MINOR(chip->dev.devt), rc);
 
 		device_unregister(&chip->dev);
@@ -185,7 +190,7 @@ static int tpm_dev_add_device(struct tpm_chip *chip)
 	if (rc) {
 		dev_err(&chip->dev,
 			"unable to device_register() %s, major %d, minor %d, err=%d\n",
-			chip->devname, MAJOR(chip->dev.devt),
+			dev_name(&chip->dev), MAJOR(chip->dev.devt),
 			MINOR(chip->dev.devt), rc);
 
 		return rc;
@@ -211,7 +216,7 @@ static int tpm1_chip_register(struct tpm_chip *chip)
 	if (rc)
 		return rc;
 
-	chip->bios_dir = tpm_bios_log_setup(chip->devname);
+	chip->bios_dir = tpm_bios_log_setup(dev_name(&chip->dev));
 
 	return 0;
 }
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 3a5452f09b96..d51ac7f0da01 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -174,7 +174,6 @@ struct tpm_chip {
 	unsigned int flags;
 
 	int dev_num;		/* /dev/tpm# */
-	char devname[7];
 	unsigned long is_open;	/* only one allowed */
 	int time_expired;
 
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index bd72fb04225e..49e50976efc8 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -397,7 +397,7 @@ static int is_bad(void *p)
 	return 0;
 }
 
-struct dentry **tpm_bios_log_setup(char *name)
+struct dentry **tpm_bios_log_setup(const char *name)
 {
 	struct dentry **ret = NULL, *tpm_dir, *bin_file, *ascii_file;
 
diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
index 267bfbd1b7bb..f072a1a1d5cc 100644
--- a/drivers/char/tpm/tpm_eventlog.h
+++ b/drivers/char/tpm/tpm_eventlog.h
@@ -77,7 +77,7 @@ int read_log(struct tpm_bios_log *log);
 
 #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
 	defined(CONFIG_ACPI)
-extern struct dentry **tpm_bios_log_setup(char *);
+extern struct dentry **tpm_bios_log_setup(const char *name);
 extern void tpm_bios_log_teardown(struct dentry **);
 #else
 static inline struct dentry **tpm_bios_log_setup(char *name)
diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c b/drivers/char/tpm/tpm_i2c_nuvoton.c
index 847f1597fe9b..02f2b35ad753 100644
--- a/drivers/char/tpm/tpm_i2c_nuvoton.c
+++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
@@ -560,7 +560,7 @@ static int i2c_nuvoton_probe(struct i2c_client *client,
 		rc = devm_request_irq(dev, chip->vendor.irq,
 				      i2c_nuvoton_int_handler,
 				      IRQF_TRIGGER_LOW,
-				      chip->devname,
+				      dev_name(&chip->dev),
 				      chip);
 		if (rc) {
 			dev_err(dev, "%s() Unable to request irq: %d for use\n",
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 12aa96a69b6c..39bc0e908e38 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -578,7 +578,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
 	u8 original_int_vec;
 
 	if (devm_request_irq(chip->pdev, irq, tis_int_handler, flags,
-			     chip->devname, chip) != 0) {
+			     dev_name(&chip->dev), chip) != 0) {
 		dev_info(chip->pdev, "Unable to request irq: %d for probe\n",
 			 irq);
 		return -1;
-- 
2.1.4


------------------------------------------------------------------------------
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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

On Thu, Feb 11, 2016 at 10:56:47PM -0500, Stefan Berger wrote:
>    Updated the branch. Are you going to upstream your patch? Otherwise I
>    would just add your Signed-off-by to it if that's ok ?

Yes, sorry about that. Just add it to your series with your
tested-by/reviewed-by. It will need some rebasing to go on top of
Jarkko's final patch that adds the devm_ (just delete it from
tpm_alloc and use my version for tpmm_alloc)

The other thing that has come to my mind is device removal. Right now
the tpm core is still fairly broken in that regard, and we have
largely got away with ignoring that because drviers are rarely
removed.

However, you are adding something that actually requires removal to
work and be race free, so we'll also have to fix core removal :(

The basic approach I'd suggest would be the same as what the rtc
subsystem does.

Add a mutex around 'chip->ops' and every place that
uses ops has to hold the lock.

Upon removal grab the mutex and null ops.

Every place that graps the lock for ops has to check for null. Null
means the device is being destroyed and the call must fail.

Make changes to tpm_chip_find_get to check for null, and make it do
put_device. Reconsider the use of module locking there, not sure it
makes any sense to do that if we have proper safe removal support.

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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 2276 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/12/2016 
01:40:51 PM:


> 
> On Thu, Feb 11, 2016 at 10:56:47PM -0500, Stefan Berger wrote:
> >    Updated the branch. Are you going to upstream your patch? Otherwise 
I
> >    would just add your Signed-off-by to it if that's ok ?
> 
> Yes, sorry about that. Just add it to your series with your
> tested-by/reviewed-by. It will need some rebasing to go on top of
> Jarkko's final patch that adds the devm_ (just delete it from
> tpm_alloc and use my version for tpmm_alloc)
> 
> The other thing that has come to my mind is device removal. Right now
> the tpm core is still fairly broken in that regard, and we have
> largely got away with ignoring that because drviers are rarely
> removed.
> 
> However, you are adding something that actually requires removal to
> work and be race free, so we'll also have to fix core removal :(

Where is the race? I tested the following:

The tpm_vtpm module use counter increases with every server file 
descriptor being handed out, so every run of ./vtpmctrl increases it by 1.

Every opening of /dev/tpm%d (exec 1XY<>/dev/tpm%d) increases the tpm_vtpm 
module use counter also by 1.

If the vtpmctrl's die, the module use counter decreases for every vtpmctrl 
termiating. However, the use counter is still at the number of open 
/dev/tpm%d devices.

vtpm_dev and chip structures only get free'd once the file descriptor is 
close. So it looks like expected good behavior to me. We cannot remove the 
'backend' module following the usage counter increase, so this is good 
too.

     Stefan


> 
> The basic approach I'd suggest would be the same as what the rtc
> subsystem does.
> 
> Add a mutex around 'chip->ops' and every place that
> uses ops has to hold the lock.
> 
> Upon removal grab the mutex and null ops.
> 
> Every place that graps the lock for ops has to check for null. Null
> means the device is being destroyed and the call must fail.
> 
> Make changes to tpm_chip_find_get to check for null, and make it do
> put_device. Reconsider the use of module locking there, not sure it
> makes any sense to do that if we have proper safe removal support.
> 
> Jason
> 



[-- Attachment #1.2: Type: text/html, Size: 2796 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

On Fri, Feb 12, 2016 at 03:31:13PM -0500, Stefan Berger wrote:

>    Where is the race? I tested the following:
>    The tpm_vtpm module use counter increases with every server file
>    descriptor being handed out, so every run of ./vtpmctrl increases it by
>    1.
>    Every opening of /dev/tpm%d (exec 1XY<>/dev/tpm%d) increases the
>    tpm_vtpm module use counter also by 1.
>    If the vtpmctrl's die, the module use counter decreases for every
>    vtpmctrl termiating. However, the use counter is still at the number of
>    open /dev/tpm%d devices.
>    vtpm_dev and chip structures only get free'd once the file descriptor
>    is close. So it looks like expected good behavior to me. We cannot
>    remove the 'backend' module following the usage counter increase, so
>    this is good too.

We don't expect tpm_chip_unregister to run concurrently with any
in-progress operation, that isn't properly synchronized.

So your driver will call tpm_chip_unregister and then put_device it's
structure, but the tpm_chip is still active and could still generate a
callback to vtpm code resulting in use-after-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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 1647 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/12/2016 
03:39:56 PM:

> 
> On Fri, Feb 12, 2016 at 03:31:13PM -0500, Stefan Berger wrote:
> 
> >    Where is the race? I tested the following:
> >    The tpm_vtpm module use counter increases with every server file
> >    descriptor being handed out, so every run of ./vtpmctrl increases 
it by
> >    1.
> >    Every opening of /dev/tpm%d (exec 1XY<>/dev/tpm%d) increases the
> >    tpm_vtpm module use counter also by 1.
> >    If the vtpmctrl's die, the module use counter decreases for every
> >    vtpmctrl termiating. However, the use counter is still at the 
number of
> >    open /dev/tpm%d devices.
> >    vtpm_dev and chip structures only get free'd once the file 
descriptor
> >    is close. So it looks like expected good behavior to me. We cannot
> >    remove the 'backend' module following the usage counter increase, 
so
> >    this is good too.
> 
> We don't expect tpm_chip_unregister to run concurrently with any
> in-progress operation, that isn't properly synchronized.
> 
> So your driver will call tpm_chip_unregister and then put_device it's
> structure, but the tpm_chip is still active and could still generate a
> callback to vtpm code resulting in use-after-free.

What I observed is that both tpm_chip and vtpm_dev structures are freed 
once the last one of two sides (/dev/tpmX or server side file descriptor) 
closes. Closing means there's no more code being executed by an 
application. So how can we have something still executing in the driver 
when both sides closed?

  Stefan



[-- Attachment #1.2: Type: text/html, Size: 2072 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

On Fri, Feb 12, 2016 at 03:44:14PM -0500, Stefan Berger wrote:
>    What I observed is that both tpm_chip and vtpm_dev structures are freed
>    once the last one of two sides (/dev/tpmX or server side file
>    descriptor) closes.

Hmmm...

I don't see how that can happen. Looking at the tpm cdev, it is
continues to exist even after tpm_unregister returns (cdev_del does
not force close existing opens). Certainly the kAPI (ie
tpm_chip_find_get) will continue to use the chip without blocking
tpm_unregister.

I see no mechanism for the cdev/kAPI to continue to hold a kref on the
vtpm struct.

The obvious one would be because the vtpm struct is a parent of the
chip, but that kref is let go during device_del.

So, we have a situation where tpm_unregister can return, release the
kref on the vtpm and still have outstanding users, which will result
in a use after-free.

[BTW, your lastest vtpm on github still has a problem with error
 unwind. Move the put_device(&vtpm_dev->chip->dev);  from
 vtpm_delete_vtpm_dev() and put it in vtpm_dev_release() with a NULL
 test.

 The put_device is missing after the tpm_chip_unregister call, the
 above is the safest way to fix it.
 
 This is why you shouldn't wrapper put_device, anything but naked
 put_device is probably wrong]

[Also, err_kfree should not exist in vtpm_create_vtpm_dev, always
 put_device after device_initialize returns, the comment near the device_add
 is wrong, it is using the get_device done implicitly by
 device_initialize]

[Don't forget to error check dev_set_name]

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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 3524 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/12/2016 
04:15:38 PM:

> From: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> To: Stefan Berger/Watson/IBM@IBMUS
> Cc: dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org, Jarkko Sakkinen 
> <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>, tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> Date: 02/12/2016 04:15 PM
> Subject: Re: [tpmdd-devel] [PATCH v5 4/5] Initialize TPM and get 
> durations and timeouts
> 
> On Fri, Feb 12, 2016 at 03:44:14PM -0500, Stefan Berger wrote:
> >    What I observed is that both tpm_chip and vtpm_dev structures are 
freed
> >    once the last one of two sides (/dev/tpmX or server side file
> >    descriptor) closes.
> 
> Hmmm...
> 
> I don't see how that can happen. Looking at the tpm cdev, it is

Well, following my tests, it does happen.

> continues to exist even after tpm_unregister returns (cdev_del does
> not force close existing opens). Certainly the kAPI (ie
> tpm_chip_find_get) will continue to use the chip without blocking
> tpm_unregister.
> 
> I see no mechanism for the cdev/kAPI to continue to hold a kref on the
> vtpm struct.
> 
> The obvious one would be because the vtpm struct is a parent of the
> chip, but that kref is let go during device_del.
> 
> So, we have a situation where tpm_unregister can return, release the
> kref on the vtpm and still have outstanding users, which will result
> in a use after-free.

Maybe you should give it a try ... I don't see these problems and any 
change seems like ill-fixing it. What will be accessed after 
tpm_unregister? The only case we have tpm_unregister is here:

static void vtpm_delete_device_pair(struct vtpm_dev *vtpm_dev)
{
        tpm_chip_unregister(vtpm_dev->chip);

        vtpm_fops_undo_open(vtpm_dev);

        vtpm_delete_vtpm_dev(vtpm_dev);
}

followed by:

static inline void vtpm_delete_vtpm_dev(struct vtpm_dev *vtpm_dev)
{
        put_device(&vtpm_dev->chip->dev); /* frees chip */
        device_unregister(&vtpm_dev->dev); /* does put_device */
}

I don't see a problem with that.

> 
> [BTW, your lastest vtpm on github still has a problem with error
>  unwind. Move the put_device(&vtpm_dev->chip->dev);  from
>  vtpm_delete_vtpm_dev() and put it in vtpm_dev_release() with a NULL
>  test.

I tested all the error paths. The vtpm_delete_vtpm_dev is the counter-part 
to vtpm_create_vtpm_dev and only ever gets called if vtpm_create_vtpm_dev 
ran successfully and is the function to call to unwind 
vtpm_create_vtpm_dev. So if we unwind there in reverse order then we 
should be ok. If not, why not ?

> 
>  The put_device is missing after the tpm_chip_unregister call, the
>  above is the safest way to fix it.

No, it's the vtpm_delete_vtpm_dev function that unwinds it. I tested this 
and structures get freed properly.

> 
>  This is why you shouldn't wrapper put_device, anything but naked
>  put_device is probably wrong]
> 
> [Also, err_kfree should not exist in vtpm_create_vtpm_dev, always
>  put_device after device_initialize returns, the comment near the 
device_add
>  is wrong, it is using the get_device done implicitly by
>  device_initialize]

Fixed.
> 
> [Don't forget to error check dev_set_name]

Fixed. Looks like 80% of the drivers don't check here.

   Stefan
> 
> Jason
> 



[-- Attachment #1.2: Type: text/html, Size: 5343 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

On Fri, Feb 12, 2016 at 05:23:16PM -0500, Stefan Berger wrote:

>    > I don't see how that can happen. Looking at the tpm cdev, it is

>    Well, following my tests, it does happen.

How are you testing for use-after-free bugs? Did you test the kapi
interface? Did you get it in *exactly* the right configuration to
cause this?

>    > So, we have a situation where tpm_unregister can return, release the
>    > kref on the vtpm and still have outstanding users, which will result
>    > in a use after-free.

>    Maybe you should give it a try ... I don't see these problems and any
>    change seems like ill-fixing it. What will be accessed after
>    tpm_unregister? The only case we have tpm_unregister is here:

>    static void vtpm_delete_device_pair(structvtpm_dev *vtpm_dev)
>    {
>            tpm_chip_unregister(vtpm_dev->chip);
>            vtpm_fops_undo_open(vtpm_dev);
>            vtpm_delete_vtpm_dev(vtpm_dev);
>    }

>    followed by:

>    static inline voidvtpm_delete_vtpm_dev(struct vtpm_dev *vtpm_dev)
>    {
>            put_device(&vtpm_dev->chip->dev); /* frees chip */
>            device_unregister(&vtpm_dev->dev); /* does put_device */
>    }

>    I don't see a problem with that.

device_unregister will put_device(vtpmt_dev) which will kfree it since
no refs are left.

tpm_chip is still alive because the cdev/kapi are still holding a ref
on it.

The cdev/kapi can still call vtpm_tpm_op_send which will then deref
chip->priv which is the free'd vtpm_dev.

Any attempt to test for this scenario is very likely to hit the
STATE_OPENED_BIT test and exit without crashing. However that is just
a user-after free getting lucky. Testing is not a substitute for
proper analysis.

Show me an analysis of what in cdev/kapi is holding the kref on
vtpm_dev if you still think this can't happen.

As I said, the only kref the tpm core takes on vtpm_dev is dropped
by tpm_chip_unregister.

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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 765 bytes --]

Stefan Berger/Watson/IBM@IBMUS wrote on 02/12/2016 05:23:16 PM:

> 
> Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/12/
> 2016 04:15:38 PM:

> > 
> > On Fri, Feb 12, 2016 at 03:44:14PM -0500, Stefan Berger wrote:
> > >    What I observed is that both tpm_chip and vtpm_dev structuresare 
freed
> > >    once the last one of two sides (/dev/tpmX or server side file
> > >    descriptor) closes.
> > 
> > Hmmm...
> > 
> > I don't see how that can happen. Looking at the tpm cdev, it is
> 
> Well, following my tests, it does happen.

Also I am zeroing tpm_chip and vtpm_dev structures before the free. 
Nothing bad happens in any combination of device opening / closing tests I 
did.

    Stefan


[-- Attachment #1.2: Type: text/html, Size: 1045 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 3150 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/12/2016 
05:46:42 PM:

> 
> On Fri, Feb 12, 2016 at 05:23:16PM -0500, Stefan Berger wrote:
> 
> >    > I don't see how that can happen. Looking at the tpm cdev, it is
> 
> >    Well, following my tests, it does happen.
> 
> How are you testing for use-after-free bugs? Did you test the kapi
> interface? Did you get it in *exactly* the right configuration to
> cause this?

I think there are two cases that matter for the final unwind:

1) server side closes last
2) client side closes last

Any other cases?

Here's what happens in these cases:

1) we unwind with vtpm_delete_device_pair + vtpm_delete_vtpm_dev [see 
below]. The chip an vtpm_dev are freed, in that order by just these 
functions.

2) we unwind in tpm_release and the following kicks it off:

http://lxr.free-electrons.com/source/drivers/char/tpm/tpm-dev.c#L169

169         put_device(priv->chip->pdev);
170         kfree(priv);
171         return 0;
172 }
173 

The above put_device (in the front-end) releases vtpm_dev first and then 
after tpm_release finishes the chip is released (following the cdev's 
end). 

Once tpm_release happened, we closed the file descriptor and there's no 
more code doing other business in either the front or backend drivers 
other than unwinding.

   Stefan

> 
> >    > So, we have a situation where tpm_unregister can return, release 
the
> >    > kref on the vtpm and still have outstanding users, which will 
result
> >    > in a use after-free.
> 
> >    Maybe you should give it a try ... I don't see these problems and 
any
> >    change seems like ill-fixing it. What will be accessed after
> >    tpm_unregister? The only case we have tpm_unregister is here:
> 
> >    static void vtpm_delete_device_pair(structvtpm_dev *vtpm_dev)
> >    {
> >            tpm_chip_unregister(vtpm_dev->chip);
> >            vtpm_fops_undo_open(vtpm_dev);
> >            vtpm_delete_vtpm_dev(vtpm_dev);
> >    }
> 
> >    followed by:
> 
> >    static inline voidvtpm_delete_vtpm_dev(struct vtpm_dev *vtpm_dev)
> >    {
> >            put_device(&vtpm_dev->chip->dev); /* frees chip */
> >            device_unregister(&vtpm_dev->dev); /* does put_device */
> >    }
> 
> >    I don't see a problem with that.
> 
> device_unregister will put_device(vtpmt_dev) which will kfree it since
> no refs are left.
> 
> tpm_chip is still alive because the cdev/kapi are still holding a ref
> on it.
> 
> The cdev/kapi can still call vtpm_tpm_op_send which will then deref
> chip->priv which is the free'd vtpm_dev.
> 
> Any attempt to test for this scenario is very likely to hit the
> STATE_OPENED_BIT test and exit without crashing. However that is just
> a user-after free getting lucky. Testing is not a substitute for
> proper analysis.
> 
> Show me an analysis of what in cdev/kapi is holding the kref on
> vtpm_dev if you still think this can't happen.
> 
> As I said, the only kref the tpm core takes on vtpm_dev is dropped
> by tpm_chip_unregister.
> 
> Jason
> 



[-- Attachment #1.2: Type: text/html, Size: 6115 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

On Fri, Feb 12, 2016 at 05:47:11PM -0500, Stefan Berger wrote:

>    Also I am zeroing tpm_chip and vtpm_dev structures before the free.
>    Nothing bad happens in any combination of device opening / closing
>    tests I did.

That won't help detect use after free.

You won't be able to find this with open/close testing, a RPC has to
be done on /dev/tpmX at the right time, and even if there is some
tricky reason why cdev works, kapi doesn't have any protection.

Try this, lets make the user-after-free into a
null-pointer-deref. Much easier to spot.

--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -305,6 +305,8 @@ void tpm_chip_unregister(struct tpm_chip *chip)
 		sysfs_remove_link(&chip->pdev->kobj, "ppi");
 
 	tpm1_chip_unregister(chip);
+	chip->priv = NULL;
+	chip->ops = NULL;
 	tpm_dev_del_device(chip);
 }
 EXPORT_SYMBOL_GPL(tpm_chip_unregister);

------------------------------------------------------------------------------
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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

On Fri, Feb 12, 2016 at 06:14:42PM -0500, Stefan Berger wrote:
>    169        put_device(priv->chip->pdev);
>    170        kfree(priv);

Okay, I thought I took that bogus thing out a long time ago, but that
is why you can't show this with the cdev.

But as I keep saying the kapi doesn't have anything like that.

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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 2476 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/12/2016 
01:34:15 PM:

> 
> On Fri, Feb 12, 2016 at 01:13:39PM -0500, Stefan Berger wrote:
> >    Stefan Berger/Watson/IBM@IBMUS wrote on 02/11/2016 10:56:47 PM:
> >    >
> >    > Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/11/
> >    > 2016 06:56:11 PM:
> >    >
> >    >
> >    > >
> >    > > On Thu, Feb 11, 2016 at 05:26:24PM -0500, Stefan Berger wrote:
> >    > >
> >    > > >    > What is the point of tpmm_chip_dev?
> >    > > >    So that the usage model of the chip is the same. We get 
this
> >    in the
> >    > > >    tpm-vtpm.c with tpm_alloc_chip + tpmm_chip_dev while all
> >    others can
> >    > > >    call tpmm_chip_alloc, which combines the two.
> >    > >
> >    > > No need, just don't use devm in vtpm, that is even better. The
> >    > > standard devm idiom is a with and without version.
> >    >
> >    > Updated the branch. Are you going to upstream your patch? 
Otherwise
> >    > I would just add your Signed-off-by to it if that's ok ?
> >    >
> >    > [1]https://github.com/stefanberger/linux/tree/vtpm-driver.v3
> >    I converted tpm-chip.c to use IDA as well.
> >    A test for 4096 vTPM instances:
> >    for ((i = 0; i < 4096; i++)); do ./vtpmctrl & done
> 
> Yeah, that looks good, thanks for doing this
> 

With the IDR I ran into the problem that TPM core driver and backend now 
share the ID and create their device names with it. What can happen is 
that the core driver gives up ID 123, while the vTPM driver still has the 
device name vtpms123 registered with sysfs. Now the next device is 
created, ID 123 is recycled by the core driver, and it bombs while the 
vTPM driver again tries to register vtpm123 that still hasn't been 
unregistered. One can trigger this problem with lots of concurrency (see 
below) and then the whole system even locks up. I don't know how to solve 
this ID issue in an easier way than having an IDR in the core driver and 
an IDA in the vTPM driver and so handling the IDs independently. This in 
turn makes the split of tpmm_chip_alloc / tpm_chip_alloc unnecessary since 
we don't need the ID from the chip anymore. Now the below test runs 
stable.


for pid in $(ps aux | grep vtpm | gawk '{print $2}'); do kill -9 $pid; 
done ; for ((i =0; i< 8192; i++)); do ./vtpmctrl &>/dev/null & done

 Stefan



[-- Attachment #1.2: Type: text/html, Size: 3424 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 3410 bytes --]

Stefan Berger/Watson/IBM@IBMUS wrote on 02/14/2016 01:37:08 AM:

> 
> Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/12/
> 2016 01:34:15 PM:
> 
> > 
> > On Fri, Feb 12, 2016 at 01:13:39PM -0500, Stefan Berger wrote:
> > >    Stefan Berger/Watson/IBM@IBMUS wrote on 02/11/2016 10:56:47 PM:
> > >    >
> > >    > Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 
02/11/
> > >    > 2016 06:56:11 PM:
> > >    >
> > >    >
> > >    > >
> > >    > > On Thu, Feb 11, 2016 at 05:26:24PM -0500, Stefan Berger 
wrote:
> > >    > >
> > >    > > >    > What is the point of tpmm_chip_dev?
> > >    > > >    So that the usage model of the chip is the same. We get 
this
> > >    in the
> > >    > > >    tpm-vtpm.c with tpm_alloc_chip + tpmm_chip_dev while all
> > >    others can
> > >    > > >    call tpmm_chip_alloc, which combines the two.
> > >    > >
> > >    > > No need, just don't use devm in vtpm, that is even better. 
The
> > >    > > standard devm idiom is a with and without version.
> > >    >
> > >    > Updated the branch. Are you going to upstream your patch? 
Otherwise
> > >    > I would just add your Signed-off-by to it if that's ok ?
> > >    >
> > >    > [1]https://github.com/stefanberger/linux/tree/vtpm-driver.v3
> > >    I converted tpm-chip.c to use IDA as well.
> > >    A test for 4096 vTPM instances:
> > >    for ((i = 0; i < 4096; i++)); do ./vtpmctrl & done
> > 
> > Yeah, that looks good, thanks for doing this
> > 
> 
> With the IDR I ran into the problem that TPM core driver and backend
> now share the ID and create their device names with it. What can 
> happen is that the core driver gives up ID 123, while the vTPM 
> driver still has the device name vtpms123 registered with sysfs. Now
> the next device is created, ID 123 is recycled by the core driver, 
> and it bombs while the vTPM driver again tries to register vtpm123 
> that still hasn't been unregistered. One can trigger this problem 
> with lots of concurrency (see below) and then the whole system even 
> locks up. I don't know how to solve this ID issue in an easier way 
> than having an IDR in the core driver and an IDA in the vTPM driver 
> and so handling the IDs independently. This in turn makes the split 
> of tpmm_chip_alloc / tpm_chip_alloc unnecessary since we don't need 
> the ID from the chip anymore. Now the below test runs stable.

Scratch that. The easier way to fix this was to release the tpm_chip after 
the vtpm_dev.

   Stefan


> 
> 
> for pid in $(ps aux | grep vtpm | gawk '{print $2}'); do kill -9 
> $pid; done ; for ((i =0; i< 8192; i++)); do ./vtpmctrl &>/dev/null & 
done
> 
>  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
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel



[-- Attachment #1.2: Type: text/html, Size: 4931 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

On Tue, Feb 16, 2016 at 11:44:06AM -0500, Stefan Berger wrote:

>    Scratch that. The easier way to fix this was to release the tpm_chip
>    after the vtpm_dev.

Right, that is nicer

Did you solve the lock ups when register fails?

One other thing you can look at is ditching the vtpm struct
device. With the last patches I sent the tpm core would only need two
more lines to work with a null parent device, which would be even
simpler for vtpm.

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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 969 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/16/2016 
01:00:28 PM:

> 
> On Tue, Feb 16, 2016 at 11:44:06AM -0500, Stefan Berger wrote:
> 
> >    Scratch that. The easier way to fix this was to release the 
tpm_chip
> >    after the vtpm_dev.
> 
> Right, that is nicer
> 
> Did you solve the lock ups when register fails?

The re-ordering of the tpm_chip and vtpm_dev release actually solved that 
problem.

[My feeling is, something outside of the vtpm driver doesn't unroll 
properly and locks up if there's lots of concurrency].

> 
> One other thing you can look at is ditching the vtpm struct
> device. With the last patches I sent the tpm core would only need two
> more lines to work with a null parent device, which would be even
> simpler for vtpm.

Not having a parent device isn't really necessary, except for not having 
it appear in sysfs maybe?

   Stefan

> 
> Jason
> 



[-- Attachment #1.2: Type: text/html, Size: 1319 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Jason Gunthorpe]

On Tue, Feb 16, 2016 at 02:26:56PM -0500, Stefan Berger wrote:
> The re-ordering of the tpm_chip and vtpm_dev release actually solved that
> problem.

? You mean you don't see the lock up because there is no error unwind?

> [My feeling is, something outside of the vtpm driver doesn't unroll properly
> and locks up if there's lots of concurrency].

Hum, I'd be surprised if this is not caused by something outside tpm/vtpm..
It would be very nice to know, but perhaps not essential. I didn't see
anything obvious in vtpm either.

> > One other thing you can look at is ditching the vtpm struct
> > device. With the last patches I sent the tpm core would only need two
> > more lines to work with a null parent device, which would be even
> > simpler for vtpm.
> 
> Not having a parent device isn't really necessary, except for not having it
> appear in sysfs maybe?

Dropping the dummy parent out of vtpm makes it work a little more more
like the rest of the virtual stuff and slightly changes the sysfs
paths visble to user space. If this is the right way to go it would be
great to do that at the start. It wasn't worth bringing up before, but
now that I wrote the parent removal patch it is pretty trivial.

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

Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 1660 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/16/2016 
11:47:50 PM:


> 
> On Tue, Feb 16, 2016 at 02:26:56PM -0500, Stefan Berger wrote:
> > The re-ordering of the tpm_chip and vtpm_dev release actually solved 
that
> > problem.
> 
> ? You mean you don't see the lock up because there is no error unwind?
> 
> > [My feeling is, something outside of the vtpm driver doesn't unroll 
properly
> > and locks up if there's lots of concurrency].
> 
> Hum, I'd be surprised if this is not caused by something outside 
tpm/vtpm..
> It would be very nice to know, but perhaps not essential. I didn't see
> anything obvious in vtpm either.
> 
> > > One other thing you can look at is ditching the vtpm struct
> > > device. With the last patches I sent the tpm core would only need 
two
> > > more lines to work with a null parent device, which would be even
> > > simpler for vtpm.
> > 
> > Not having a parent device isn't really necessary, except for not 
having it
> > appear in sysfs maybe?
> 
> Dropping the dummy parent out of vtpm makes it work a little more more
> like the rest of the virtual stuff and slightly changes the sysfs
> paths visble to user space. If this is the right way to go it would be
> great to do that at the start. It wasn't worth bringing up before, but
> now that I wrote the parent removal patch it is pretty trivial.

Beside the code to lock the module with the parent device, the 
chip->dev.parent is used in sysfs API calls. Using the chip->dev.kobj 
there in case chip->dev.parent is NULL also seems a way to hang the 
system.

   Stefan




[-- Attachment #1.2: Type: text/html, Size: 1992 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel