Based on Christoph Hellwig's TPM internal kernel interface 
locking question, the following TPM changes were made: 
        - removal of the BKL calls from the TPM driver, which were
          added in the overall misc-char-dev-BKL-pushdown.patch
        - continue to protect the tpm_chip_list using the 
          driver_lock, and add an rcu to protect readers.

The TPM internal kernel interface patch will be posted as 
part of the integrity patchset.

 drivers/char/tpm/tpm.c |  285 +++++++++++++++++++++++++++++++-----------------
 drivers/char/tpm/tpm.h |    2 +-
 2 files changed, 188 insertions(+), 99 deletions(-)

--

[ message continues ]

This patch removes the BKL calls from the TPM driver, which
were added in the overall misc-char-dev-BKL-pushdown.patch,
as they are not needed.  In addition, renames num_open to
is_open, as only one process can open the file at a time.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm.c |   35 ++++++++++++++++++-----------------
 drivers/char/tpm/tpm.h |    2 +-
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index ae766d8..59b31e1 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -954,13 +954,16 @@ EXPORT_SYMBOL_GPL(tpm_store_cancel);
 
 /*
  * Device file system interface to the TPM
+ *
+ * It's assured that the chip will be opened just once,
+ * by the check of is_open variable, which is protected
+ * by driver_lock.
  */
 int tpm_open(struct inode *inode, struct file *file)
 {
 	int rc = 0, minor = iminor(inode);
 	struct tpm_chip *chip = NULL, *pos;
 
-	lock_kernel();
 	spin_lock(&driver_lock);
 
 	list_for_each_entry(pos, &tpm_chip_list, list) {
@@ -975,34 +978,31 @@ int tpm_open(struct inode *inode, struct file *file)
 		goto err_out;
 	}
 
-	if (chip->num_opens) {
+	if (chip->is_open) {
 		dev_dbg(chip->dev, "Another process owns this TPM\n");
 		rc = -EBUSY;
 		goto err_out;
 	}
 
-	chip->num_opens++;
-	get_device(chip->dev);
+	chip->is_open = 1;
+	get_device(chip->dev); /* protect from chip disappearing */
 
 	spin_unlock(&driver_lock);
 
 	chip->data_buffer = kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
 	if (chip->data_buffer == NULL) {
-		chip->num_opens--;
+		chip->is_open = 0;
 		put_device(chip->dev);
-		unlock_kernel();
 		return -ENOMEM;
 	}
 
 	atomic_set(&chip->data_pending, 0);
 
 	file->private_data = chip;
-	unlock_kernel();
 	return 0;
 
 err_out:
 	spin_unlock(&driver_lock);
-	unlock_kernel();
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_open);
@@ ...
[ message continues ]

--

[ message continues ]

Continue to protect the tpm_chip_list using the driver_lock
as before, and add an rcu to protect readers.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Acked-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm.c |  268 ++++++++++++++++++++++++++++++++----------------
 1 files changed, 178 insertions(+), 90 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 59b31e1..704ab01 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -587,14 +587,22 @@ ssize_t tpm_show_enabled(struct device * dev, struct device_attribute * attr,
 {
 	u8 *data;
 	ssize_t rc;
+	struct tpm_chip *chip;
+
+	rcu_read_lock();
+	chip = dev_get_drvdata(dev);
+	if (chip)
+		get_device(chip->dev);	/* protect from disappearing */
+	rcu_read_unlock();
 
-	struct tpm_chip *chip = dev_get_drvdata(dev);
 	if (chip == NULL)
 		return -ENODEV;
 
 	data = kzalloc(TPM_INTERNAL_RESULT_SIZE, GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
+	if (!data) {
+		rc = -ENOMEM;
+		goto out_alloc;
+	}
 
 	memcpy(data, tpm_cap, sizeof(tpm_cap));
 	data[TPM_CAP_IDX] = TPM_CAP_FLAG;
@@ -603,13 +611,15 @@ ssize_t tpm_show_enabled(struct device * dev, struct device_attribute * attr,
 	rc = transmit_cmd(chip, data, TPM_INTERNAL_RESULT_SIZE,
 			"attemtping to determine the permanent enabled state");
 	if (rc) {
-		kfree(data);
-		return 0;
+		rc = 0;
+		goto out;
 	}
 
 	rc = sprintf(buf, "%d\n", !data[TPM_GET_CAP_PERM_DISABLE_IDX]);
-
+out:
 	kfree(data);
+out_alloc:
+	put_device(chip->dev);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_show_enabled);
@@ -619,14 +629,24 @@ ssize_t tpm_show_active(struct device * dev, struct device_attribute * attr,
 {
 	u8 *data;
 	ssize_t rc;
+	struct tpm_chip *chip;
 
-	struct tpm_chip *chip = dev_get_drvdata(dev);
-	if (chip == NULL)
-		return -ENODEV;
+	rcu_read_lock();
+	chip = dev_get_drvdata(dev);
+	if (chip)
+		get_device(chip->dev);	/* protect from disappearing */
+	rcu_read_unlock();
+
+	if ...
[ message continues ]

A few questions interspersed below...


The following five lines of code appear repeatedly.  Why not put them
into an inline function or a macro?

Also, doesn't there need to be an rcu_dereference() somewhere either in
this sequence of code or in either dev_get_drvdata() or get_device()?
Or am I missing something subtle here?

From what I see of the code, I actually don't understand why the
rcu_read_lock() is needed here -- only tpm_chip_list is covered by
the RCU list addition/deletion primitives in this patch.  If you are
adding a multilinked data structure into an RCU-protected list, you
need RCU traversal primitives only when traversing the list, not the
data structure contained in the list.  (Of course, any changes to the
data structure contained in the list must be protected somehow or other,

The above synchronize_rcu() is not needed -- unless it is required that
tpm_register_hardware() not return until it can be guaranteed that all
subsequent readers will see the new entry.  (Of course, the current
CPU/task is guaranteed to see it in any subsequent call, even without
--

[ message continues ]

Yes, a good idea, because in the second repetition of this code sequence
you have the wrong set of gotos, and can end up doing

Good point.  Which I missed entirely when I saw the patch before.

Taking the case of tpm_show_enabled(), it is called as a sysfs file
callback, right?  So the sysfs/kobject infrastructure will pin the
--

[ message continues ]

Ok, the rcu_read_lock is only used when accessing an rcu protected
variable, such as when walking the tpm_chip_list. When registering
the tpm chip, a pointer to the chip is stored in device->driver_data. 
The tpm_show_XXX() functions dereference and use it, which is safe if
the device is pinned when the sysfs file is opened. Documentation 
indicates that the kobject is protected from going away when attribute
files are opened. I'm not seeing the kobject refcount being incremented, 
when the attribute file is opened. How is the device being pinned?

Thanks!

Mimi Zohar

--

[ message continues ]