[PATCH] tpm: fix rollback/cleanup before tpm_chip_register()

[PATCH] tpm: fix rollback/cleanup before tpm_chip_register()

[Jarkko Sakkinen]

The release-callback is not used before the device is attached to the
device hierarchy. This caused resources not to cleanup properly if the
device driver initialization failed before tpm_chip_register().

This patch fixes the issue by adding the cleanup function to the devres
list of the platform device in tpmm_chip_alloc().

Fixes: 313d21eeab ("tpm: device class for tpm")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 drivers/char/tpm/tpm-chip.c | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 1a9dcee..cf2b351 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -75,16 +75,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
+ * tpmm_chip_alloc() - allocate and initialize a TPM chip
+ * @pdev: the platform device who is the parent of the chip
  * @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.
+ * Allocates a new struct tpm_chip instance, prepares the character device and
+ * assigns a free device number for it. The memory is freed automatically when
+ * the platform device is detached.
  */
-struct tpm_chip *tpmm_chip_alloc(struct device *dev,
+struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
 				 const struct tpm_class_ops *ops)
 {
 	struct tpm_chip *chip;
@@ -103,7 +102,7 @@ struct tpm_chip *tpmm_chip_alloc(struct device *dev,
 	spin_unlock(&driver_lock);
 
 	if (chip->dev_num >= TPM_NUM_DEVICES) {
-		dev_err(dev, "No available tpm device numbers\n");
+		dev_err(pdev, "No available tpm device numbers\n");
 		kfree(chip);
 		return ERR_PTR(-ENOMEM);
 	}
@@ -112,9 +111,7 @@ struct tpm_chip *tpmm_chip_alloc(struct device *dev,
 
 	scnprintf(chip->devname, sizeof(chip->devname), "tpm%d", chip->dev_num);
 
-	chip->pdev = dev;
-
-	dev_set_drvdata(dev, chip);
+	chip->pdev = pdev;
 
 	chip->dev.class = tpm_class;
 	chip->dev.release = tpm_dev_release;
@@ -136,6 +133,12 @@ struct tpm_chip *tpmm_chip_alloc(struct device *dev,
 	chip->cdev.owner = chip->pdev->driver->owner;
 	chip->cdev.kobj.parent = &chip->dev.kobj;
 
+	/* Associate character device with the platform device only after
+	 * it is properly initialized.
+	 */
+	dev_set_drvdata(pdev, chip);
+	devm_add_action(pdev, (void (*)(void *)) tpm_dev_release, &chip->dev);
+
 	return chip;
 }
 EXPORT_SYMBOL_GPL(tpmm_chip_alloc);
@@ -162,7 +165,10 @@ static int tpm_add_char_device(struct tpm_chip *chip)
 			MINOR(chip->dev.devt), rc);
 
 		cdev_del(&chip->cdev);
-		return rc;
+	} else {
+		devm_remove_action(chip->dev.parent,
+				   (void (*)(void *)) tpm_dev_release,
+				   &chip->dev);
 	}
 
 	return rc;
@@ -202,15 +208,14 @@ static void tpm1_chip_unregister(struct tpm_chip *chip)
 }
 
 /*
- * tpm_chip_register() - create a character device for the TPM chip
- * @chip: TPM chip to use.
+ * tpm_chip_register() - add a TPM chip to the device hierarchy
+ * @chip: the TPM chip to be added
  *
- * Creates a character device for the TPM chip and adds sysfs attributes for
- * the device. As the last step this function adds the chip to the list of TPM
- * chips available for in-kernel use.
+ * Adds a TPM chip to the device hierarchy and makes it available to for
+ * in-kernel use.
  *
- * This function should be only called after the chip initialization is
- * complete.
+ * This function should be called only after the device driver is otherwise
+ * initialized because it will become available for client access.
  */
 int tpm_chip_register(struct tpm_chip *chip)
 {
-- 
2.7.0


------------------------------------------------------------------------------
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=267308311&iu=/4140

Re: [PATCH] tpm: fix rollback/cleanup before tpm_chip_register()

[Jason Gunthorpe]

On Sat, Jan 30, 2016 at 06:05:42PM -0800, Jarkko Sakkinen wrote:
> The release-callback is not used before the device is attached to the
> device hierarchy. This caused resources not to cleanup properly if the
> device driver initialization failed before tpm_chip_register().

This commentary is not right, the release callback is callable
immediately after device_initialize returns, it will be called by the
last put_device().

> - * tpmm_chip_alloc() - allocate a new struct tpm_chip instance
> - * @dev: device to which the chip is associated
> + * tpmm_chip_alloc() - allocate and initialize a TPM chip
> + * @pdev: the platform device who is the parent of the chip

? A platform device is not required, just something in a state that
can handle devm.

> +	/* Associate character device with the platform device only after
> +	 * it is properly initialized.
> +	 */
> +	dev_set_drvdata(pdev, chip);
> +	devm_add_action(pdev, (void (*)(void *)) tpm_dev_release,
> &chip->dev);

No, a release function can never be called naked. The action needs
to do put_device, which is the error unwind for device_initialize().

> @@ -162,7 +165,10 @@ static int tpm_add_char_device(struct tpm_chip *chip)
>  			MINOR(chip->dev.devt), rc);
>  
>  		cdev_del(&chip->cdev);
> -		return rc;
> +	} else {
> +		devm_remove_action(chip->dev.parent,
> +				   (void (*)(void *)) tpm_dev_release,
> +				   &chip->dev);

This is in the wrong place, the devm should be canceled only if
tpm_chip_register returns success, at that point the caller's contract
is to guarentee a call to tpm_chip_unregister, and
tpm_chip_unregister does the put_device that calls the release
function.

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=267308311&iu=/4140

Re: [PATCH] tpm: fix rollback/cleanup before tpm_chip_register()

[Jarkko Sakkinen]

On Tue, Feb 02, 2016 at 04:13:53PM -0700, Jason Gunthorpe wrote:
> On Sat, Jan 30, 2016 at 06:05:42PM -0800, Jarkko Sakkinen wrote:
> > The release-callback is not used before the device is attached to the
> > device hierarchy. This caused resources not to cleanup properly if the
> > device driver initialization failed before tpm_chip_register().
> 
> This commentary is not right, the release callback is callable
> immediately after device_initialize returns, it will be called by the
> last put_device().

Ah, right.

> > - * tpmm_chip_alloc() - allocate a new struct tpm_chip instance
> > - * @dev: device to which the chip is associated
> > + * tpmm_chip_alloc() - allocate and initialize a TPM chip
> > + * @pdev: the platform device who is the parent of the chip
> 
> ? A platform device is not required, just something in a state that
> can handle devm.

Platform device in a generic sense like like ACPI or PNP device or
something else. How would you call it instead? I want to call the
parameter something else than 'dev' solely for readability.

Would s/the platform device/the parent device/ be better?

> > +	/* Associate character device with the platform device only after
> > +	 * it is properly initialized.
> > +	 */
> > +	dev_set_drvdata(pdev, chip);
> > +	devm_add_action(pdev, (void (*)(void *)) tpm_dev_release,
> > &chip->dev);
> 
> No, a release function can never be called naked. The action needs
> to do put_device, which is the error unwind for device_initialize().

Got it (already from your first comment)!

What does "called naked" even mean? I just don't understand the
english here and want to be sure that I understand what you are saying
and not make false assumptions.


> > @@ -162,7 +165,10 @@ static int tpm_add_char_device(struct tpm_chip *chip)
> >  			MINOR(chip->dev.devt), rc);
> >  
> >  		cdev_del(&chip->cdev);
> > -		return rc;
> > +	} else {
> > +		devm_remove_action(chip->dev.parent,
> > +				   (void (*)(void *)) tpm_dev_release,
> > +				   &chip->dev);
> 
> This is in the wrong place, the devm should be canceled only if
> tpm_chip_register returns success, at that point the caller's contract
> is to guarentee a call to tpm_chip_unregister, and
> tpm_chip_unregister does the put_device that calls the release
> function.

rc == 0 at that point i.e. success. I don't see the problem here.

> Jason

/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=267308311&iu=/4140

Re: [PATCH] tpm: fix rollback/cleanup before tpm_chip_register()

[Jason Gunthorpe]

On Wed, Feb 03, 2016 at 08:02:35AM -0800, Jarkko Sakkinen wrote:

> Would s/the platform device/the parent device/ be better?

Yes

> > > +	/* Associate character device with the platform device only after
> > > +	 * it is properly initialized.
> > > +	 */
> > > +	dev_set_drvdata(pdev, chip);
> > > +	devm_add_action(pdev, (void (*)(void *)) tpm_dev_release,
> > > &chip->dev);
> > 
> > No, a release function can never be called naked. The action needs
> > to do put_device, which is the error unwind for device_initialize().
> 
> Got it (already from your first comment)!
> 
> What does "called naked" even mean? I just don't understand the
> english here and want to be sure that I understand what you are saying
> and not make false assumptions.

'called naked' would refer to just open coding a call to
tpm_dev_release, using it as a devm_add_action is the same as open
coding.

The function must only ever be called by put_device.

> > > @@ -162,7 +165,10 @@ static int tpm_add_char_device(struct tpm_chip *chip)
> > >  			MINOR(chip->dev.devt), rc);
> > >  
> > >  		cdev_del(&chip->cdev);
> > > -		return rc;
> > > +	} else {
> > > +		devm_remove_action(chip->dev.parent,
> > > +				   (void (*)(void *)) tpm_dev_release,
> > > +				   &chip->dev);
> > 
> > This is in the wrong place, the devm should be canceled only if
> > tpm_chip_register returns success, at that point the caller's contract
> > is to guarentee a call to tpm_chip_unregister, and
> > tpm_chip_unregister does the put_device that calls the release
> > function.
> 
> rc == 0 at that point i.e. success. I don't see the problem here.

It should not be in tpm_add_char_device

I'm also not completely sure about the error handling around
tpm_register - if it fails the tpm_chip should not be destroyed, and I
think it does..

It would probably be ideal to just rely on devm to always do the final
put_device and avoid the devm_remove_action entirely. I think this
means a get_device will be needed in tpm_register after device_add ?
Didn't look closely at how all the refs balance.

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: [tpmdd-devel] [PATCH] tpm: fix rollback/cleanup before tpm_chip_register()

[Jarkko Sakkinen]

On Wed, Feb 03, 2016 at 05:34:37PM -0700, Jason Gunthorpe wrote:
> On Wed, Feb 03, 2016 at 08:02:35AM -0800, Jarkko Sakkinen wrote:
> 
> > Would s/the platform device/the parent device/ be better?
> 
> Yes
> 
> > > > +	/* Associate character device with the platform device only after
> > > > +	 * it is properly initialized.
> > > > +	 */
> > > > +	dev_set_drvdata(pdev, chip);
> > > > +	devm_add_action(pdev, (void (*)(void *)) tpm_dev_release,
> > > > &chip->dev);
> > > 
> > > No, a release function can never be called naked. The action needs
> > > to do put_device, which is the error unwind for device_initialize().
> > 
> > Got it (already from your first comment)!
> > 
> > What does "called naked" even mean? I just don't understand the
> > english here and want to be sure that I understand what you are saying
> > and not make false assumptions.
> 
> 'called naked' would refer to just open coding a call to
> tpm_dev_release, using it as a devm_add_action is the same as open
> coding.
> 
> The function must only ever be called by put_device.
> 
> > > > @@ -162,7 +165,10 @@ static int tpm_add_char_device(struct tpm_chip *chip)
> > > >  			MINOR(chip->dev.devt), rc);
> > > >  
> > > >  		cdev_del(&chip->cdev);
> > > > -		return rc;
> > > > +	} else {
> > > > +		devm_remove_action(chip->dev.parent,
> > > > +				   (void (*)(void *)) tpm_dev_release,
> > > > +				   &chip->dev);
> > > 
> > > This is in the wrong place, the devm should be canceled only if
> > > tpm_chip_register returns success, at that point the caller's contract
> > > is to guarentee a call to tpm_chip_unregister, and
> > > tpm_chip_unregister does the put_device that calls the release
> > > function.
> > 
> > rc == 0 at that point i.e. success. I don't see the problem here.
> 
> It should not be in tpm_add_char_device
> 
> I'm also not completely sure about the error handling around
> tpm_register - if it fails the tpm_chip should not be destroyed, and I
> think it does..
> 
> It would probably be ideal to just rely on devm to always do the final
> put_device and avoid the devm_remove_action entirely. I think this
> means a get_device will be needed in tpm_register after device_add ?
> Didn't look closely at how all the refs balance.

I'm OK with letting devm always do the job. Then there needs to be
just dummy calback for release because the framework complains about
it otherwise.

> Jason

/Jarkko