Messages in this thread Patch in this message | | | Date | Mon, 19 Nov 2007 00:13:19 +0100 | From | Marcel Selhorst <> | Subject | Re: [tpmdd-devel] [PATCH] - TPM device driver layer (tpm.c|h) |
| |
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 <tpm@selhorst.net> ---
Richard MUSIL wrote: > 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 > > >> >From bd80b63ca2e1edb761a3ffcf87bd86c30a44ca5f Mon Sep 17 00:00:00 2001 > From: Richard Musil <richard.musil@st.com> > 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(-) > > diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c > index 9bb5429..41eba7e 100644 > --- a/drivers/char/tpm/tpm.c > +++ b/drivers/char/tpm/tpm.c > @@ -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; > } > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index b2e2b00..f1c265e 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -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) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |