[PATCH] tpm: Use the right clean up after cdev_add completes

[PATCH] tpm: Use the right clean up after cdev_add completes

[Jason Gunthorpe]

Once cdev_add is done the device node is visible to user space and
could have been opened. Thus we have to go through the locking
process in tpm_del_char_device if device_add fails.

Fixes: 2c91ce8523a ("tpm: fix the rollback in tpm_chip_register()")
Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 drivers/char/tpm/tpm-chip.c | 43 ++++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

static function moved to avoid a prototype

This was noticed while reviewing the cdev patchset from Logan

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index a77262d31911a1..c82acf0a6e7353 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -226,6 +226,26 @@ struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
 }
 EXPORT_SYMBOL_GPL(tpmm_chip_alloc);
 
+static void tpm_del_char_device(struct tpm_chip *chip, bool with_device)
+{
+	cdev_del(&chip->cdev);
+	if (with_device) {
+		device_del(&chip->dev);
+
+		/* Make the chip unavailable. */
+		mutex_lock(&idr_lock);
+		idr_replace(&dev_nums_idr, NULL, chip->dev_num);
+		mutex_unlock(&idr_lock);
+	}
+
+	/* Make the driver uncallable. */
+	down_write(&chip->ops_sem);
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		tpm2_shutdown(chip, TPM2_SU_CLEAR);
+	chip->ops = NULL;
+	up_write(&chip->ops_sem);
+}
+
 static int tpm_add_char_device(struct tpm_chip *chip)
 {
 	int rc;
@@ -246,8 +266,7 @@ static int tpm_add_char_device(struct tpm_chip *chip)
 			"unable to device_register() %s, major %d, minor %d, err=%d\n",
 			dev_name(&chip->dev), MAJOR(chip->dev.devt),
 			MINOR(chip->dev.devt), rc);
-
-		cdev_del(&chip->cdev);
+		tpm_del_char_device(chip, false);
 		return rc;
 	}
 
@@ -259,24 +278,6 @@ static int tpm_add_char_device(struct tpm_chip *chip)
 	return rc;
 }
 
-static void tpm_del_char_device(struct tpm_chip *chip)
-{
-	cdev_del(&chip->cdev);
-	device_del(&chip->dev);
-
-	/* Make the chip unavailable. */
-	mutex_lock(&idr_lock);
-	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
-	mutex_unlock(&idr_lock);
-
-	/* Make the driver uncallable. */
-	down_write(&chip->ops_sem);
-	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		tpm2_shutdown(chip, TPM2_SU_CLEAR);
-	chip->ops = NULL;
-	up_write(&chip->ops_sem);
-}
-
 static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
 {
 	struct attribute **i;
@@ -384,6 +385,6 @@ void tpm_chip_unregister(struct tpm_chip *chip)
 {
 	tpm_del_legacy_sysfs(chip);
 	tpm_bios_log_teardown(chip);
-	tpm_del_char_device(chip);
+	tpm_del_char_device(chip, true);
 }
 EXPORT_SYMBOL_GPL(tpm_chip_unregister);
-- 
2.7.4

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH] tpm: Use the right clean up after cdev_add completes

[Jarkko Sakkinen]

On Thu, Feb 23, 2017 at 02:19:14PM -0700, Jason Gunthorpe wrote:
> Once cdev_add is done the device node is visible to user space and
> could have been opened. Thus we have to go through the locking
> process in tpm_del_char_device if device_add fails.
> 
> Fixes: 2c91ce8523a ("tpm: fix the rollback in tpm_chip_register()")
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

> ---
>  drivers/char/tpm/tpm-chip.c | 43 ++++++++++++++++++++++---------------------
>  1 file changed, 22 insertions(+), 21 deletions(-)
> 
> static function moved to avoid a prototype
> 
> This was noticed while reviewing the cdev patchset from Logan
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index a77262d31911a1..c82acf0a6e7353 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -226,6 +226,26 @@ struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
>  }
>  EXPORT_SYMBOL_GPL(tpmm_chip_alloc);
>  
> +static void tpm_del_char_device(struct tpm_chip *chip, bool with_device)
> +{
> +	cdev_del(&chip->cdev);
> +	if (with_device) {
> +		device_del(&chip->dev);
> +
> +		/* Make the chip unavailable. */
> +		mutex_lock(&idr_lock);
> +		idr_replace(&dev_nums_idr, NULL, chip->dev_num);
> +		mutex_unlock(&idr_lock);
> +	}
> +
> +	/* Make the driver uncallable. */
> +	down_write(&chip->ops_sem);
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +		tpm2_shutdown(chip, TPM2_SU_CLEAR);
> +	chip->ops = NULL;
> +	up_write(&chip->ops_sem);
> +}
> +
>  static int tpm_add_char_device(struct tpm_chip *chip)
>  {
>  	int rc;
> @@ -246,8 +266,7 @@ static int tpm_add_char_device(struct tpm_chip *chip)
>  			"unable to device_register() %s, major %d, minor %d, err=%d\n",
>  			dev_name(&chip->dev), MAJOR(chip->dev.devt),
>  			MINOR(chip->dev.devt), rc);
> -
> -		cdev_del(&chip->cdev);
> +		tpm_del_char_device(chip, false);
>  		return rc;
>  	}
>  
> @@ -259,24 +278,6 @@ static int tpm_add_char_device(struct tpm_chip *chip)
>  	return rc;
>  }
>  
> -static void tpm_del_char_device(struct tpm_chip *chip)
> -{
> -	cdev_del(&chip->cdev);
> -	device_del(&chip->dev);
> -
> -	/* Make the chip unavailable. */
> -	mutex_lock(&idr_lock);
> -	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
> -	mutex_unlock(&idr_lock);
> -
> -	/* Make the driver uncallable. */
> -	down_write(&chip->ops_sem);
> -	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> -		tpm2_shutdown(chip, TPM2_SU_CLEAR);
> -	chip->ops = NULL;
> -	up_write(&chip->ops_sem);
> -}
> -
>  static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
>  {
>  	struct attribute **i;
> @@ -384,6 +385,6 @@ void tpm_chip_unregister(struct tpm_chip *chip)
>  {
>  	tpm_del_legacy_sysfs(chip);
>  	tpm_bios_log_teardown(chip);
> -	tpm_del_char_device(chip);
> +	tpm_del_char_device(chip, true);
>  }
>  EXPORT_SYMBOL_GPL(tpm_chip_unregister);
> -- 
> 2.7.4

Re: [PATCH] tpm: Use the right clean up after cdev_add completes

[Jarkko Sakkinen]

On Thu, Feb 23, 2017 at 02:19:14PM -0700, Jason Gunthorpe wrote:
> Once cdev_add is done the device node is visible to user space and
> could have been opened. Thus we have to go through the locking
> process in tpm_del_char_device if device_add fails.
> 
> Fixes: 2c91ce8523a ("tpm: fix the rollback in tpm_chip_register()")
> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

Pushed.

/Jarkko

> ---
>  drivers/char/tpm/tpm-chip.c | 43 ++++++++++++++++++++++---------------------
>  1 file changed, 22 insertions(+), 21 deletions(-)
> 
> static function moved to avoid a prototype
> 
> This was noticed while reviewing the cdev patchset from Logan
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index a77262d31911a1..c82acf0a6e7353 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -226,6 +226,26 @@ struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
>  }
>  EXPORT_SYMBOL_GPL(tpmm_chip_alloc);
>  
> +static void tpm_del_char_device(struct tpm_chip *chip, bool with_device)
> +{
> +	cdev_del(&chip->cdev);
> +	if (with_device) {
> +		device_del(&chip->dev);
> +
> +		/* Make the chip unavailable. */
> +		mutex_lock(&idr_lock);
> +		idr_replace(&dev_nums_idr, NULL, chip->dev_num);
> +		mutex_unlock(&idr_lock);
> +	}
> +
> +	/* Make the driver uncallable. */
> +	down_write(&chip->ops_sem);
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +		tpm2_shutdown(chip, TPM2_SU_CLEAR);
> +	chip->ops = NULL;
> +	up_write(&chip->ops_sem);
> +}
> +
>  static int tpm_add_char_device(struct tpm_chip *chip)
>  {
>  	int rc;
> @@ -246,8 +266,7 @@ static int tpm_add_char_device(struct tpm_chip *chip)
>  			"unable to device_register() %s, major %d, minor %d, err=%d\n",
>  			dev_name(&chip->dev), MAJOR(chip->dev.devt),
>  			MINOR(chip->dev.devt), rc);
> -
> -		cdev_del(&chip->cdev);
> +		tpm_del_char_device(chip, false);
>  		return rc;
>  	}
>  
> @@ -259,24 +278,6 @@ static int tpm_add_char_device(struct tpm_chip *chip)
>  	return rc;
>  }
>  
> -static void tpm_del_char_device(struct tpm_chip *chip)
> -{
> -	cdev_del(&chip->cdev);
> -	device_del(&chip->dev);
> -
> -	/* Make the chip unavailable. */
> -	mutex_lock(&idr_lock);
> -	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
> -	mutex_unlock(&idr_lock);
> -
> -	/* Make the driver uncallable. */
> -	down_write(&chip->ops_sem);
> -	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> -		tpm2_shutdown(chip, TPM2_SU_CLEAR);
> -	chip->ops = NULL;
> -	up_write(&chip->ops_sem);
> -}
> -
>  static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
>  {
>  	struct attribute **i;
> @@ -384,6 +385,6 @@ void tpm_chip_unregister(struct tpm_chip *chip)
>  {
>  	tpm_del_legacy_sysfs(chip);
>  	tpm_bios_log_teardown(chip);
> -	tpm_del_char_device(chip);
> +	tpm_del_char_device(chip, true);
>  }
>  EXPORT_SYMBOL_GPL(tpm_chip_unregister);
> -- 
> 2.7.4

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH] tpm: Use the right clean up after cdev_add completes

[Jarkko Sakkinen]

On Fri, Feb 24, 2017 at 07:43:54PM +0200, Jarkko Sakkinen wrote:
> On Thu, Feb 23, 2017 at 02:19:14PM -0700, Jason Gunthorpe wrote:
> > Once cdev_add is done the device node is visible to user space and
> > could have been opened. Thus we have to go through the locking
> > process in tpm_del_char_device if device_add fails.
> > 
> > Fixes: 2c91ce8523a ("tpm: fix the rollback in tpm_chip_register()")
> > Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> 
> Pushed.

It would make easier to merge this with resource manager commits if
there was instead

void tpm_chip_shutdown(struct tpm_chip *chip)
{
	/* Make the driver uncallable. */
	down_write(&chip->ops_sem);
	if (chip->flags & TPM_CHIP_FLAG_TPM2)
		tpm2_shutdown(chip, TPM2_SU_CLEAR);
	chip->ops = NULL;
	up_write(&chip->ops_sem);
}

And you would call this instead of wiring into tpm_del_char_device.

I can update the commit.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH] tpm: Use the right clean up after cdev_add completes

[Jarkko Sakkinen]

On Fri, Feb 24, 2017 at 10:59:49PM +0200, Jarkko Sakkinen wrote:
> On Fri, Feb 24, 2017 at 07:43:54PM +0200, Jarkko Sakkinen wrote:
> > On Thu, Feb 23, 2017 at 02:19:14PM -0700, Jason Gunthorpe wrote:
> > > Once cdev_add is done the device node is visible to user space and
> > > could have been opened. Thus we have to go through the locking
> > > process in tpm_del_char_device if device_add fails.
> > > 
> > > Fixes: 2c91ce8523a ("tpm: fix the rollback in tpm_chip_register()")
> > > Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > 
> > Pushed.
> 
> It would make easier to merge this with resource manager commits if
> there was instead
> 
> void tpm_chip_shutdown(struct tpm_chip *chip)
> {
> 	/* Make the driver uncallable. */
> 	down_write(&chip->ops_sem);
> 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> 		tpm2_shutdown(chip, TPM2_SU_CLEAR);
> 	chip->ops = NULL;
> 	up_write(&chip->ops_sem);
> }
> 
> And you would call this instead of wiring into tpm_del_char_device.
> 
> I can update the commit.
> 
> /Jarkko

Actually lets keep as it is. I'll extend tpm_del_char_device a bit
instead in the RM series.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH] tpm: Use the right clean up after cdev_add completes

[Jason Gunthorpe]

On Fri, Feb 24, 2017 at 10:59:50PM +0200, Jarkko Sakkinen wrote:
> On Fri, Feb 24, 2017 at 07:43:54PM +0200, Jarkko Sakkinen wrote:
> > On Thu, Feb 23, 2017 at 02:19:14PM -0700, Jason Gunthorpe wrote:
> > > Once cdev_add is done the device node is visible to user space and
> > > could have been opened. Thus we have to go through the locking
> > > process in tpm_del_char_device if device_add fails.
> > > 
> > > Fixes: 2c91ce8523a ("tpm: fix the rollback in tpm_chip_register()")
> > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > 
> > Pushed.
> 
> It would make easier to merge this with resource manager commits if
> there was instead
> 
> void tpm_chip_shutdown(struct tpm_chip *chip)
> {
> 	/* Make the driver uncallable. */
> 	down_write(&chip->ops_sem);
> 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> 		tpm2_shutdown(chip, TPM2_SU_CLEAR);
> 	chip->ops = NULL;
> 	up_write(&chip->ops_sem);
> }
> 
> And you would call this instead of wiring into tpm_del_char_device.

Hum.. I think this direction in the RM patch would be more
maintainable.. Otherwise tpm_add_char_device has very complicated
error handling.

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index c82acf0a6e7353..444cf2495ef0e0 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -353,18 +353,26 @@ int tpm_chip_register(struct tpm_chip *chip)
 	tpm_add_ppi(chip);
 
 	rc = tpm_add_char_device(chip);
-	if (rc) {
-		tpm_bios_log_teardown(chip);
-		return rc;
-	}
+	if (rc)
+		goto err_add_char;
+
+	rc = tpm_add_rm_char_device(chip);
+	if (rc)
+		goto err_add_rm_char;
 
 	rc = tpm_add_legacy_sysfs(chip);
-	if (rc) {
-		tpm_chip_unregister(chip);
-		return rc;
-	}
+	if (rc)
+		goto err_add_legacy_sysfs;
 
 	return 0;
+
+err_add_legacy_sysfs:
+	tpm_del_rm_char_device(chip);
+err_add_rm_char:
+	tpm_del_char_device(chip, true);
+err_add_char:
+	tpm_bios_log_teardown(chip);
+	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_chip_register);
 
@@ -384,6 +392,7 @@ EXPORT_SYMBOL_GPL(tpm_chip_register);
 void tpm_chip_unregister(struct tpm_chip *chip)
 {
 	tpm_del_legacy_sysfs(chip);
+	tpm_del_rm_char_device(chip);
 	tpm_bios_log_teardown(chip);
 	tpm_del_char_device(chip, true);
 }