- TPM device driver layer (tpm.c|h)

PATCH In review Richard MUSIL 4 participants 12 messages Aug 23, 2007 – Nov 20, 2007 TPM device driver

lore

Share & tools

lore

Richard MUSIL

author

2007-08-23 09:11 UTC #0 lore raw

PATCH 1/1 - TPM device driver layer (tpm.c|h)

Dear all,

I am currently writing virtual TPM device driver. This driver exposes itself and behaves like regular TPM device (i.e. uses TPM layer which is already present in kernel), but instead of talking to hardware it talks to user space.

In my setup, I can create several virtual devices and delete them in runtime. During this I noticed that current implementation is prone to segfault when tpm_remove_hardware is called while the device is in its receiving routine.

The problem is in tpm_remove_hardware which does all necessary steps to remove the device, but it does not wait for the device being actually removed and kfrees tpm_chip struct right away. If the device is at this time in its receiving routine (through read call, for example) it blocks the device removal until the call is completed. The call cannot be completed though because tpm_chip struct was already kfreed.

Since there is significant timeout and it is possible to meet that timeout in normal operation this situation is quite easily reproducible.

What I present below is rather quickfix with least impact on other TPM parts (drivers). The patch uses device->remove callback (of platform_device device) and reroutes this to itself. In this callback it eventually calls vendor callback and finally kfrees all memory resources allocated on its own.

The correct solution for this I believe would require removing TPM hardware (as it is done by tpm_remove_hardware right now) in two steps both called by vendor driver. In the first one vendor driver will just "unregister" the hardware. In the second step vendor driver will call tpm_release_resources routine which kfrees all resources allocated by tpm.c. In order to be able to call tpm_release_resources, vendor driver will have to register as platform driver and handle platform_driver.probe and platform_driver.remove.

This will however require some changes in all drivers, which I cannot test (but I could eventually write the patch). Also I see some "issues" in tpm_tis which will have to be clarified.

--
Richard

Quoted 1 line

From bd80b63ca2e1edb761a3ffcf87bd86c30a44ca5f Mon Sep 17 00:00:00 2001

From: Richard Musil <[email protected]>
Date: Tue, 21 Aug 2007 16:46:06 +0200
Subject: [PATCH] Change in TPM module:

The clean up procedure now uses platform device "release" callback to handle memory clean up. For this purpose "release" function callback was added to struct tpm_vendor_specific, so hw device driver provider can get called when it is safe to remove all allocated resources.

This is supposed to fix a bug in device removal, where device while in receive function (waiting on timeout) was prone to segfault, if the tpm_chip struct was unallocated before the timeout expired (in tpm_remove_hardware).

---
 drivers/char/tpm/tpm.c |   46 +++++++++++++++++++++++++++++-----------------
 drivers/char/tpm/tpm.h |    2 ++
 2 files changed, 31 insertions(+), 17 deletions(-)

drivers/char/tpm/tpm.c +29 ~~-17~~

index 9bb5429..41eba7e 100644
@@ -1031,18 +1031,13 @@ void tpm_remove_hardware(struct device *dev)
 
 	spin_unlock(&driver_lock);
 
-	dev_set_drvdata(dev, NULL);
 	misc_deregister(&chip->vendor.miscdev);
-	kfree(chip->vendor.miscdev.name);
 
 	sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
 	tpm_bios_log_teardown(chip->bios_dir);
 
-	clear_bit(chip->dev_num, dev_mask);
-
-	kfree(chip);
-
-	put_device(dev);
+	/* write it this way to be explicit (chip->dev == dev) */
+	put_device(chip->dev);
 }
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);

@@ -1083,6 +1078,28 @@ int tpm_pm_resume(struct device *dev)
 EXPORT_SYMBOL_GPL(tpm_pm_resume);
 
 /*
+ * Once all references to platform device are down to 0,
+ * release all allocated structures.
+ * In case vendor provided release function,
+ * call it too.
+ */
+static void tpm_dev_release(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	/* call vendor release, if defined */
+	if (chip->vendor.release)
+		chip->vendor.release(dev);
+
+	/* it *should* be: chip->release != NULL */
+	if (likely(chip->release))
+		chip->release(dev);
+
+	clear_bit(chip->dev_num, dev_mask);
+	kfree(chip->vendor.miscdev.name);
+	kfree(chip);
+}
+
+/*
  * Called from tpm_<specific>.c probe function only for devices 
  * the driver has determined it should claim.  Prior to calling
  * this function the specific probe function has called pci_enable_device

@@ -1136,23 +1153,21 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 
 	chip->vendor.miscdev.parent = dev;
 	chip->dev = get_device(dev);
+	chip->release = dev->release;
+	dev->release = tpm_dev_release;
+	dev_set_drvdata(dev, chip);
 
 	if (misc_register(&chip->vendor.miscdev)) {
 		dev_err(chip->dev,
 			"unable to misc_register %s, minor %d\n",
 			chip->vendor.miscdev.name,
 			chip->vendor.miscdev.minor);
-		put_device(dev);
-		clear_bit(chip->dev_num, dev_mask);
-		kfree(chip);
-		kfree(devname);
+		put_device(chip->dev);
 		return NULL;
 	}
 
 	spin_lock(&driver_lock);
 
-	dev_set_drvdata(dev, chip);
-
 	list_add(&chip->list, &tpm_chip_list);
 
 	spin_unlock(&driver_lock);

@@ -1160,10 +1175,7 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 	if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
 		list_del(&chip->list);
 		misc_deregister(&chip->vendor.miscdev);
-		put_device(dev);
-		clear_bit(chip->dev_num, dev_mask);
-		kfree(chip);
-		kfree(devname);
+		put_device(chip->dev);
 		return NULL;
 	}

drivers/char/tpm/tpm.h +2 -0

index b2e2b00..f1c265e 100644
@@ -74,6 +74,7 @@ struct tpm_vendor_specific {
 	int (*send) (struct tpm_chip *, u8 *, size_t);
 	void (*cancel) (struct tpm_chip *);
 	u8 (*status) (struct tpm_chip *);
+	void (*release) (struct device *);
 	struct miscdevice miscdev;
 	struct attribute_group *attr_group;
 	struct list_head list;

@@ -106,6 +107,7 @@ struct tpm_chip {
 	struct dentry **bios_dir;
 
 	struct list_head list;
+	void (*release) (struct device *);
 };
 
 #define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)

Signature · 1 line

1.5.3.rc5

Greg KH

2007-08-23 09:32 UTC #1 lore raw

Quoted 6 lines · Richard MUSIL

Dear all,
I am currently writing virtual TPM device driver. This driver exposes itself and behaves like regular TPM device (i.e. uses TPM layer which is already present in kernel), but instead of talking to hardware it talks to user space.

Heh, I like the idea, I can imagine what it could be used for :)

Quoted 5 lines

What I present below is rather quickfix with least impact on other TPM parts (drivers). The patch uses device->remove callback (of platform_device device) and reroutes this to itself. In this callback it eventually calls vendor callback and finally kfrees all memory resources allocated on its own.

It looks sane to me, nice fixup.

thanks,

greg k-h

Richard MUSIL

author

2007-09-25 13:15 UTC #2 lore raw

Re: [PATCH] - TPM device driver layer (tpm.c|h) - repost

Hello all,

sometime ago I submitted patch to TPM layer, originally I thought this patch could be accepted into kernel (see below). However, since this did not happen, I wonder, if there are some problems with the patch or whether I am expected to do/provide something else, in order to have it accepted.

The patch follows even more below.

Thanks,
Richard

Quoted 16 lines · Greg KH

Quoted 6 lines · Richard MUSIL
Dear all,
I am currently writing virtual TPM device driver. This driver exposes itself and behaves like regular TPM device (i.e. uses TPM layer which is already present in kernel), but instead of talking to hardware it talks to user space.
Heh, I like the idea, I can imagine what it could be used for :)
Quoted 5 lines
What I present below is rather quickfix with least impact on other TPM parts (drivers). The patch uses device->remove callback (of platform_device device) and reroutes this to itself. In this callback it eventually calls vendor callback and finally kfrees all memory resources allocated on its own.

Quoted 8 lines

It looks sane to me, nice fixup.
thanks,
greg k-h
From bd80b63ca2e1edb761a3ffcf87bd86c30a44ca5f Mon Sep 17 00:00:00 2001

From: Richard Musil <[email protected]>
Date: Tue, 21 Aug 2007 16:46:06 +0200
Subject: [PATCH] Change in TPM module:

---
 drivers/char/tpm/tpm.c |   46 +++++++++++++++++++++++++++++-----------------
 drivers/char/tpm/tpm.h |    2 ++
 2 files changed, 31 insertions(+), 17 deletions(-)

drivers/char/tpm/tpm.c +29 ~~-17~~

index 9bb5429..41eba7e 100644
@@ -1031,18 +1031,13 @@ void tpm_remove_hardware(struct device *dev)
 
 	spin_unlock(&driver_lock);
 
-	dev_set_drvdata(dev, NULL);
 	misc_deregister(&chip->vendor.miscdev);
-	kfree(chip->vendor.miscdev.name);
 
 	sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
 	tpm_bios_log_teardown(chip->bios_dir);
 
-	clear_bit(chip->dev_num, dev_mask);
-
-	kfree(chip);
-
-	put_device(dev);
+	/* write it this way to be explicit (chip->dev == dev) */
+	put_device(chip->dev);
 }
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);

@@ -1083,6 +1078,28 @@ int tpm_pm_resume(struct device *dev)
 EXPORT_SYMBOL_GPL(tpm_pm_resume);
 
 /*
+ * Once all references to platform device are down to 0,
+ * release all allocated structures.
+ * In case vendor provided release function,
+ * call it too.
+ */
+static void tpm_dev_release(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	/* call vendor release, if defined */
+	if (chip->vendor.release)
+		chip->vendor.release(dev);
+
+	/* it *should* be: chip->release != NULL */
+	if (likely(chip->release))
+		chip->release(dev);
+
+	clear_bit(chip->dev_num, dev_mask);
+	kfree(chip->vendor.miscdev.name);
+	kfree(chip);
+}
+
+/*
  * Called from tpm_<specific>.c probe function only for devices 
  * the driver has determined it should claim.  Prior to calling
  * this function the specific probe function has called pci_enable_device

@@ -1136,23 +1153,21 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 
 	chip->vendor.miscdev.parent = dev;
 	chip->dev = get_device(dev);
+	chip->release = dev->release;
+	dev->release = tpm_dev_release;
+	dev_set_drvdata(dev, chip);
 
 	if (misc_register(&chip->vendor.miscdev)) {
 		dev_err(chip->dev,
 			"unable to misc_register %s, minor %d\n",
 			chip->vendor.miscdev.name,
 			chip->vendor.miscdev.minor);
-		put_device(dev);
-		clear_bit(chip->dev_num, dev_mask);
-		kfree(chip);
-		kfree(devname);
+		put_device(chip->dev);
 		return NULL;
 	}
 
 	spin_lock(&driver_lock);
 
-	dev_set_drvdata(dev, chip);
-
 	list_add(&chip->list, &tpm_chip_list);
 
 	spin_unlock(&driver_lock);

@@ -1160,10 +1175,7 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 	if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
 		list_del(&chip->list);
 		misc_deregister(&chip->vendor.miscdev);
-		put_device(dev);
-		clear_bit(chip->dev_num, dev_mask);
-		kfree(chip);
-		kfree(devname);
+		put_device(chip->dev);
 		return NULL;
 	}

drivers/char/tpm/tpm.h +2 -0

index b2e2b00..f1c265e 100644
@@ -74,6 +74,7 @@ struct tpm_vendor_specific {
 	int (*send) (struct tpm_chip *, u8 *, size_t);
 	void (*cancel) (struct tpm_chip *);
 	u8 (*status) (struct tpm_chip *);
+	void (*release) (struct device *);
 	struct miscdevice miscdev;
 	struct attribute_group *attr_group;
 	struct list_head list;

@@ -106,6 +107,7 @@ struct tpm_chip {
 	struct dentry **bios_dir;
 
 	struct list_head list;
+	void (*release) (struct device *);
 };
 
 #define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)

-- 1.5.3.rc5

Greg KH

2007-09-25 14:17 UTC #3 lore raw

Re: [PATCH] - TPM device driver layer (tpm.c|h) - repost

Quoted 7 lines · Richard MUSIL

Hello all,
sometime ago I submitted patch to TPM layer, originally I thought this patch could be accepted into kernel (see below). However, since this did not happen, I wonder, if there are some problems with the patch or whether I am expected to do/provide something else, in order to have it accepted.

That would be up to the maintainer of the TPM code, which last I looked was Kylene and Marcel (both of whom should be on the tpmdd-devel list and should have picked this up already...)

thanks,

greg k-h

Marcel Selhorst

2007-09-28 08:17 UTC #4 lore raw

Re: [tpmdd-devel] [PATCH] - TPM device driver layer (tpm.c|h) - repost

Dear Richard, dear Greg,

thanks for the patch, I will have a look at it next week and get back to you as soon as I can acknowledge it.

Thanks,
Marcel

Quoted 26 lines · Greg KH

Quoted 7 lines · Richard MUSIL
Hello all,
sometime ago I submitted patch to TPM layer, originally I thought this patch could be accepted into kernel (see below). However, since this did not happen, I wonder, if there are some problems with the patch or whether I am expected to do/provide something else, in order to have it accepted.
That would be up to the maintainer of the TPM code, which last I looked was Kylene and Marcel (both of whom should be on the tpmdd-devel list and should have picked this up already...)
thanks,
greg k-h
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
tpmdd-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Andrew Morton

2007-11-20 06:38 UTC #5 lore raw

Re: [PATCH] - TPM device driver layer (tpm.c|h) - repost

In reply to Richard MUSIL

Quoted 10 lines · On Tue, 25 Sep 2007 15:14:50 +0200 Richard MUSIL <[email protected]> wrote:

Hello all,
sometime ago I submitted patch to TPM layer, originally I thought this patch could be accepted into kernel (see below). However, since this did not happen, I wonder, if there are some problems with the patch or whether I am expected to do/provide something else, in order to have it accepted.
The patch follows even more below.

Thanks. We prefer that contributors sign off their work as per Documentation/SubmittingPatches. Please review that and if agrereable, send a Signed-off-by: for this work.

drivers/char/tpm/tpm.c int tpm_pm_resume(struct device *dev) · line 1089

Quoted 10 lines

/*
+ * Once all references to platform device are down to 0,
+ * release all allocated structures.
+ * In case vendor provided release function,
+ * call it too.
+ */
drivers/char/tpm/tpm.c +4 -0
+static void tpm_dev_release(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	/* call vendor release, if defined */

That's not the most useful of comments ;)

drivers/char/tpm/tpm.c int tpm_pm_resume(struct device *dev) · line 1093

Quoted 4 lines

drivers/char/tpm/tpm.c +4 -0

+	if (chip->vendor.release)
+		chip->vendor.release(dev);
+
+	/* it *should* be: chip->release != NULL */

And that one's actually wrong in the context of kernel coding practices. But whatever.

Quoted 4 lines

drivers/char/tpm/tpm.c +2 -0
+	if (likely(chip->release))
+		chip->release(dev);
From my reading, neither of these fields can ever be NULL, so the tests

simply aren't needed?

Quoted 69 lines

drivers/char/tpm/tpm.c +11 ~~-10~~

+	clear_bit(chip->dev_num, dev_mask);
+	kfree(chip->vendor.miscdev.name);
+	kfree(chip);
+}
+
+/*
  * Called from tpm_<specific>.c probe function only for devices 
  * the driver has determined it should claim.  Prior to calling
  * this function the specific probe function has called pci_enable_device

@@ -1136,23 +1153,21 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 
 	chip->vendor.miscdev.parent = dev;
 	chip->dev = get_device(dev);
+	chip->release = dev->release;
+	dev->release = tpm_dev_release;
+	dev_set_drvdata(dev, chip);
 
 	if (misc_register(&chip->vendor.miscdev)) {
 		dev_err(chip->dev,
 			"unable to misc_register %s, minor %d\n",
 			chip->vendor.miscdev.name,
 			chip->vendor.miscdev.minor);
-		put_device(dev);
-		clear_bit(chip->dev_num, dev_mask);
-		kfree(chip);
-		kfree(devname);
+		put_device(chip->dev);
 		return NULL;
 	}
 
 	spin_lock(&driver_lock);
 
-	dev_set_drvdata(dev, chip);
-
 	list_add(&chip->list, &tpm_chip_list);
 
 	spin_unlock(&driver_lock);

@@ -1160,10 +1175,7 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 	if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
 		list_del(&chip->list);
 		misc_deregister(&chip->vendor.miscdev);
-		put_device(dev);
-		clear_bit(chip->dev_num, dev_mask);
-		kfree(chip);
-		kfree(devname);
+		put_device(chip->dev);
 		return NULL;
 	}

drivers/char/tpm/tpm.h +2 -0

index b2e2b00..f1c265e 100644
@@ -74,6 +74,7 @@ struct tpm_vendor_specific {
 	int (*send) (struct tpm_chip *, u8 *, size_t);
 	void (*cancel) (struct tpm_chip *);
 	u8 (*status) (struct tpm_chip *);
+	void (*release) (struct device *);
 	struct miscdevice miscdev;
 	struct attribute_group *attr_group;
 	struct list_head list;

@@ -106,6 +107,7 @@ struct tpm_chip {
 	struct dentry **bios_dir;
 
 	struct list_head list;
+	void (*release) (struct device *);
 };
 
 #define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)

Richard MUSIL

author

2007-11-20 12:32 UTC #6 lore raw

Re: [PATCH] - TPM device driver layer (tpm.c|h) - 2nd repost

Hello Andrew,

I am including 2nd version of the patch, slightly modified according to your comments. See inline my response:

Quoted 7 lines · Andrew Morton

Quoted 1 line · On Tue, 25 Sep 2007 15:14:50 +0200 Richard MUSIL <[email protected]> wrote:
The patch follows even more below.
Thanks. We prefer that contributors sign off their work as per Documentation/SubmittingPatches. Please review that and if agrereable, send a Signed-off-by: for this work.

Ok, done in repost.

Quoted 4 lines

Quoted 4 lines
/*
+ * Once all references to platform device are down to 0,
+ * release all allocated structures.
+ * In case vendor provided release function,

Quoted 8 lines

Review
Quoted 6 lines
+ * call it too.
+ */
(quoted diff) +4 -0
+static void tpm_dev_release(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	/* call vendor release, if defined */
That's not the most useful of comments ;)

Agreed and removed :).

Quoted 7 lines

Review
Quoted 4 lines
(quoted diff) +4 -0
+	if (chip->vendor.release)
+		chip->vendor.release(dev);
+
+	/* it *should* be: chip->release != NULL */
And that one's actually wrong in the context of kernel coding practices. But whatever.

Well I am not sure, what is exactly against coding practices (this is my first patch, so bear with me). Was it the comment? Or the "likely". But, anyway, I guess I was a bit paranoic. chip->release is set to original device::release and this should be set to platform_device_release at least (and if someone messed with it, it should not be NULL anyway). So I removed complete condition.

Quoted 6 lines

Quoted 5 lines
(quoted diff) +2 -0
+	if (likely(chip->release))
+		chip->release(dev);
From my reading, neither of these fields can ever be NULL, so the tests
simply aren't needed?

The first condition is needed, since the vendor does not have to define release function for its own purpose. In fact, for example current TIS driver does not even know about it.

--
Richard

Quoted 1 line

From 070bd6e6753ae213f4ebe8367135cc3f4dfcb5ca Mon Sep 17 00:00:00 2001

From: Richard Musil <[email protected]>
Date: Tue, 20 Nov 2007 14:10:48 +0100
Subject: [PATCH] TPM: fix for segfault in device removal

Signed-off-by Richard Musil <[email protected]>

---
 drivers/char/tpm/tpm.c |   42 +++++++++++++++++++++++++-----------------
 drivers/char/tpm/tpm.h |    2 ++
 2 files changed, 27 insertions(+), 17 deletions(-)

drivers/char/tpm/tpm.c +25 ~~-17~~

index 9bb5429..59919b5 100644
@@ -1031,18 +1031,13 @@ void tpm_remove_hardware(struct device *dev)
 
 	spin_unlock(&driver_lock);
 
-	dev_set_drvdata(dev, NULL);
 	misc_deregister(&chip->vendor.miscdev);
-	kfree(chip->vendor.miscdev.name);
 
 	sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
 	tpm_bios_log_teardown(chip->bios_dir);
 
-	clear_bit(chip->dev_num, dev_mask);
-
-	kfree(chip);
-
-	put_device(dev);
+	/* write it this way to be explicit (chip->dev == dev) */
+	put_device(chip->dev);
 }
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);

@@ -1083,6 +1078,24 @@ int tpm_pm_resume(struct device *dev)
 EXPORT_SYMBOL_GPL(tpm_pm_resume);
 
 /*
+ * Once all references to platform device are down to 0,
+ * release all allocated structures.
+ * In case vendor provided release function,
+ * call it too.
+ */
+static void tpm_dev_release(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+
+	if (chip->vendor.release)
+		chip->vendor.release(dev);
+
+	clear_bit(chip->dev_num, dev_mask);
+	kfree(chip->vendor.miscdev.name);
+	kfree(chip);
+}
+
+/*
  * Called from tpm_<specific>.c probe function only for devices 
  * the driver has determined it should claim.  Prior to calling
  * this function the specific probe function has called pci_enable_device

@@ -1136,23 +1149,21 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 
 	chip->vendor.miscdev.parent = dev;
 	chip->dev = get_device(dev);
+	chip->release = dev->release;
+	dev->release = tpm_dev_release;
+	dev_set_drvdata(dev, chip);
 
 	if (misc_register(&chip->vendor.miscdev)) {
 		dev_err(chip->dev,
 			"unable to misc_register %s, minor %d\n",
 			chip->vendor.miscdev.name,
 			chip->vendor.miscdev.minor);
-		put_device(dev);
-		clear_bit(chip->dev_num, dev_mask);
-		kfree(chip);
-		kfree(devname);
+		put_device(chip->dev);
 		return NULL;
 	}
 
 	spin_lock(&driver_lock);
 
-	dev_set_drvdata(dev, chip);
-
 	list_add(&chip->list, &tpm_chip_list);
 
 	spin_unlock(&driver_lock);

@@ -1160,10 +1171,7 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 	if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
 		list_del(&chip->list);
 		misc_deregister(&chip->vendor.miscdev);
-		put_device(dev);
-		clear_bit(chip->dev_num, dev_mask);
-		kfree(chip);
-		kfree(devname);
+		put_device(chip->dev);
 		return NULL;
 	}

drivers/char/tpm/tpm.h +2 -0

index b2e2b00..f1c265e 100644
@@ -74,6 +74,7 @@ struct tpm_vendor_specific {
 	int (*send) (struct tpm_chip *, u8 *, size_t);
 	void (*cancel) (struct tpm_chip *);
 	u8 (*status) (struct tpm_chip *);
+	void (*release) (struct device *);
 	struct miscdevice miscdev;
 	struct attribute_group *attr_group;
 	struct list_head list;

@@ -106,6 +107,7 @@ struct tpm_chip {
 	struct dentry **bios_dir;
 
 	struct list_head list;
+	void (*release) (struct device *);
 };
 
 #define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)

Signature · 1 line

1.5.3.4

Richard MUSIL

author

2007-11-20 12:54 UTC #7 lore raw

Re: [PATCH] - TPM device driver layer (tpm.c|h) - 2nd repost

Gentlemen,

I am sorry for confusion, really do not have my day today :(. In the last patch I mistakenly removed call to original release. Now it should be OK.

Richard

Quoted 1 line

From 208991bcea7034202b9504c2e26c9b2edbf6e31d Mon Sep 17 00:00:00 2001

From: Richard Musil <[email protected]>
Date: Tue, 20 Nov 2007 14:47:27 +0100
Subject: [PATCH] Subject: [PATCH] TPM: fix for segfault in device removal

Signed-off-by Richard Musil <[email protected]>

---
 drivers/char/tpm/tpm.c |   44 +++++++++++++++++++++++++++-----------------
 drivers/char/tpm/tpm.h |    2 ++
 2 files changed, 29 insertions(+), 17 deletions(-)

drivers/char/tpm/tpm.c +27 ~~-17~~

index 9bb5429..ba48a11 100644
@@ -1031,18 +1031,13 @@ void tpm_remove_hardware(struct device *dev)
 
 	spin_unlock(&driver_lock);
 
-	dev_set_drvdata(dev, NULL);
 	misc_deregister(&chip->vendor.miscdev);
-	kfree(chip->vendor.miscdev.name);
 
 	sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
 	tpm_bios_log_teardown(chip->bios_dir);
 
-	clear_bit(chip->dev_num, dev_mask);
-
-	kfree(chip);
-
-	put_device(dev);
+	/* write it this way to be explicit (chip->dev == dev) */
+	put_device(chip->dev);
 }
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);

@@ -1083,6 +1078,26 @@ int tpm_pm_resume(struct device *dev)
 EXPORT_SYMBOL_GPL(tpm_pm_resume);
 
 /*
+ * Once all references to platform device are down to 0,
+ * release all allocated structures.
+ * In case vendor provided release function,
+ * call it too.
+ */
+static void tpm_dev_release(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+
+	if (chip->vendor.release)
+		chip->vendor.release(dev);
+
+	chip->release(dev);
+
+	clear_bit(chip->dev_num, dev_mask);
+	kfree(chip->vendor.miscdev.name);
+	kfree(chip);
+}
+
+/*
  * Called from tpm_<specific>.c probe function only for devices 
  * the driver has determined it should claim.  Prior to calling
  * this function the specific probe function has called pci_enable_device

@@ -1136,23 +1151,21 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 
 	chip->vendor.miscdev.parent = dev;
 	chip->dev = get_device(dev);
+	chip->release = dev->release;
+	dev->release = tpm_dev_release;
+	dev_set_drvdata(dev, chip);
 
 	if (misc_register(&chip->vendor.miscdev)) {
 		dev_err(chip->dev,
 			"unable to misc_register %s, minor %d\n",
 			chip->vendor.miscdev.name,
 			chip->vendor.miscdev.minor);
-		put_device(dev);
-		clear_bit(chip->dev_num, dev_mask);
-		kfree(chip);
-		kfree(devname);
+		put_device(chip->dev);
 		return NULL;
 	}
 
 	spin_lock(&driver_lock);
 
-	dev_set_drvdata(dev, chip);
-
 	list_add(&chip->list, &tpm_chip_list);
 
 	spin_unlock(&driver_lock);

@@ -1160,10 +1173,7 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 	if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
 		list_del(&chip->list);
 		misc_deregister(&chip->vendor.miscdev);
-		put_device(dev);
-		clear_bit(chip->dev_num, dev_mask);
-		kfree(chip);
-		kfree(devname);
+		put_device(chip->dev);
 		return NULL;
 	}

drivers/char/tpm/tpm.h +2 -0

index b2e2b00..f1c265e 100644
@@ -74,6 +74,7 @@ struct tpm_vendor_specific {
 	int (*send) (struct tpm_chip *, u8 *, size_t);
 	void (*cancel) (struct tpm_chip *);
 	u8 (*status) (struct tpm_chip *);
+	void (*release) (struct device *);
 	struct miscdevice miscdev;
 	struct attribute_group *attr_group;
 	struct list_head list;

@@ -106,6 +107,7 @@ struct tpm_chip {
 	struct dentry **bios_dir;
 
 	struct list_head list;
+	void (*release) (struct device *);
 };
 
 #define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)

Signature · 1 line

1.5.3.4

Andrew Morton

2007-11-20 20:07 UTC #8 lore raw

Re: [PATCH] - TPM device driver layer (tpm.c|h) - 2nd repost

In reply to Richard MUSIL

Quoted 10 lines · On Tue, 20 Nov 2007 13:32:06 +0100 Richard MUSIL <[email protected]> wrote:

Quoted 7 lines
drivers/char/tpm/tpm.c int tpm_pm_resume(struct device *dev)
Quoted 4 lines
drivers/char/tpm/tpm.c +4 -0
+	if (chip->vendor.release)
+		chip->vendor.release(dev);
+
+	/* it *should* be: chip->release != NULL */
And that one's actually wrong in the context of kernel coding practices. But whatever.
Well I am not sure, what is exactly against coding practices (this is my first patch, so bear with me). Was it the comment? Or the "likely".

The code was

	/* it *should* be: chip->release != NULL */
	if (chip->release)

and the I took the comment to mean that it should be

	if (chip->release != NULL)

I was just pointing out that the test-pointer-as-truth-value trick is smiled upon in kernel coding.

Quoted 6 lines

But, anyway, I guess I was a bit paranoic. chip->release is set to original device::release and this should be set to platform_device_release at least (and if someone messed with it, it should not be NULL anyway). So I removed complete condition.
From the above it appears that the code comment misled me.

Marcel Selhorst

2007-11-18 23:23 UTC #9 lore raw

In reply to Richard MUSIL

Dear all,

sorry for the delay. The patch below was posted 3 months ago and fixes
possible problems during unloading of TPM device drivers.

Acked by: Marcel Selhorst <[email protected]>
---

Quoted 178 lines · Richard MUSIL

Dear all,
I am currently writing virtual TPM device driver. This driver exposes itself and behaves like regular TPM device (i.e. uses TPM layer which is already present in kernel), but instead of talking to hardware it talks to user space.
In my setup, I can create several virtual devices and delete them in runtime. During this I noticed that current implementation is prone to segfault when tpm_remove_hardware is called while the device is in its receiving routine.
The problem is in tpm_remove_hardware which does all necessary steps to remove the device, but it does not wait for the device being actually removed and kfrees tpm_chip struct right away. If the device is at this time in its receiving routine (through read call, for example) it blocks the device removal until the call is completed. The call cannot be completed though because tpm_chip struct was already kfreed.
Since there is significant timeout and it is possible to meet that timeout in normal operation this situation is quite easily reproducible.
What I present below is rather quickfix with least impact on other TPM parts (drivers). The patch uses device->remove callback (of platform_device device) and reroutes this to itself. In this callback it eventually calls vendor callback and finally kfrees all memory resources allocated on its own.
The correct solution for this I believe would require removing TPM hardware (as it is done by tpm_remove_hardware right now) in two steps both called by vendor driver. In the first one vendor driver will just "unregister" the hardware. In the second step vendor driver will call tpm_release_resources routine which kfrees all resources allocated by tpm.c. In order to be able to call tpm_release_resources, vendor driver will have to register as platform driver and handle platform_driver.probe and platform_driver.remove.
This will however require some changes in all drivers, which I cannot test (but I could eventually write the patch). Also I see some "issues" in tpm_tis which will have to be clarified.
--
Richard
Quoted 1 line
Quoted 1 line
From bd80b63ca2e1edb761a3ffcf87bd86c30a44ca5f Mon Sep 17 00:00:00 2001
From: Richard Musil <[email protected]>
Date: Tue, 21 Aug 2007 16:46:06 +0200
Subject: [PATCH] Change in TPM module:
The clean up procedure now uses platform device "release" callback to handle memory clean up. For this purpose "release" function callback was added to struct tpm_vendor_specific, so hw device driver provider can get called when it is safe to remove all allocated resources.
This is supposed to fix a bug in device removal, where device while in receive function (waiting on timeout) was prone to segfault, if the tpm_chip struct was unallocated before the timeout expired (in tpm_remove_hardware).
---
 drivers/char/tpm/tpm.c |   46 +++++++++++++++++++++++++++++-----------------
 drivers/char/tpm/tpm.h |    2 ++
 2 files changed, 31 insertions(+), 17 deletions(-)
drivers/char/tpm/tpm.c +29 ~~-17~~
index 9bb5429..41eba7e 100644
@@ -1031,18 +1031,13 @@ void tpm_remove_hardware(struct device *dev)
 
 	spin_unlock(&driver_lock);
 
-	dev_set_drvdata(dev, NULL);
 	misc_deregister(&chip->vendor.miscdev);
-	kfree(chip->vendor.miscdev.name);
 
 	sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
 	tpm_bios_log_teardown(chip->bios_dir);
 
-	clear_bit(chip->dev_num, dev_mask);
-
-	kfree(chip);
-
-	put_device(dev);
+	/* write it this way to be explicit (chip->dev == dev) */
+	put_device(chip->dev);
 }
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);
 
@@ -1083,6 +1078,28 @@ int tpm_pm_resume(struct device *dev)
 EXPORT_SYMBOL_GPL(tpm_pm_resume);
 
 /*
+ * Once all references to platform device are down to 0,
+ * release all allocated structures.
+ * In case vendor provided release function,
+ * call it too.
+ */
+static void tpm_dev_release(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	/* call vendor release, if defined */
+	if (chip->vendor.release)
+		chip->vendor.release(dev);
+
+	/* it *should* be: chip->release != NULL */
+	if (likely(chip->release))
+		chip->release(dev);
+
+	clear_bit(chip->dev_num, dev_mask);
+	kfree(chip->vendor.miscdev.name);
+	kfree(chip);
+}
+
+/*
  * Called from tpm_<specific>.c probe function only for devices 
  * the driver has determined it should claim.  Prior to calling
  * this function the specific probe function has called pci_enable_device
@@ -1136,23 +1153,21 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 
 	chip->vendor.miscdev.parent = dev;
 	chip->dev = get_device(dev);
+	chip->release = dev->release;
+	dev->release = tpm_dev_release;
+	dev_set_drvdata(dev, chip);
 
 	if (misc_register(&chip->vendor.miscdev)) {
 		dev_err(chip->dev,
 			"unable to misc_register %s, minor %d\n",
 			chip->vendor.miscdev.name,
 			chip->vendor.miscdev.minor);
-		put_device(dev);
-		clear_bit(chip->dev_num, dev_mask);
-		kfree(chip);
-		kfree(devname);
+		put_device(chip->dev);
 		return NULL;
 	}
 
 	spin_lock(&driver_lock);
 
-	dev_set_drvdata(dev, chip);
-
 	list_add(&chip->list, &tpm_chip_list);
 
 	spin_unlock(&driver_lock);
@@ -1160,10 +1175,7 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 	if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
 		list_del(&chip->list);
 		misc_deregister(&chip->vendor.miscdev);
-		put_device(dev);
-		clear_bit(chip->dev_num, dev_mask);
-		kfree(chip);
-		kfree(devname);
+		put_device(chip->dev);
 		return NULL;
 	}
 
drivers/char/tpm/tpm.h +2 -0
index b2e2b00..f1c265e 100644
@@ -74,6 +74,7 @@ struct tpm_vendor_specific {
 	int (*send) (struct tpm_chip *, u8 *, size_t);
 	void (*cancel) (struct tpm_chip *);
 	u8 (*status) (struct tpm_chip *);
+	void (*release) (struct device *);
 	struct miscdevice miscdev;
 	struct attribute_group *attr_group;
 	struct list_head list;
@@ -106,6 +107,7 @@ struct tpm_chip {
 	struct dentry **bios_dir;
 
 	struct list_head list;
+	void (*release) (struct device *);
 };
 
 #define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)

Greg KH

2007-11-19 05:13 UTC #10 lore raw

Quoted 6 lines · Marcel Selhorst

Dear all,
sorry for the delay. The patch below was posted 3 months ago and fixes
possible problems during unloading of TPM device drivers.
Acked by: Marcel Selhorst <[email protected]>

Ok, but shouldn't you be sending this patch onward to Andrew and/or others?

Who is the TPM driver maintainer these days?

thanks,

greg k-h

Marcel Selhorst

2007-11-19 06:45 UTC #11 lore raw

Hi Greg,

Quoted 3 lines

Quoted 1 line
Acked by: Marcel Selhorst <[email protected]>
Ok, but shouldn't you be sending this patch onward to Andrew and/or others?

You are right, I replied to all but he wasn't in CC... Sorry, I forwarded the patch to him.

Quoted 1 line

Who is the TPM driver maintainer these days?

That would currently be me ;)

Best regards,
Marcel Selhorst