Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1691507 > unrolled thread

[PATCH 4.4 40/57] tpm: Provide strong locking for device removal

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4.4 40/57] tpm: Provide strong locking for device removal Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-07-19 13:20 +0200
    Re: [PATCH 4.4 40/57] tpm: Provide strong locking for device removal Ben Hutchings <ben.hutchings@codethink.co.uk> - 2017-07-26 01:00 +0200
      Re: [PATCH 4.4 40/57] tpm: Provide strong locking for device removal Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-07-26 22:00 +0200
        Re: [PATCH 4.4 40/57] tpm: Provide strong locking for device removal Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-07-26 22:10 +0200
          Re: [PATCH 4.4 40/57] tpm: Provide strong locking for device removal Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-07-29 00:50 +0200
            Re: [PATCH 4.4 40/57] tpm: Provide strong locking for device removal Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-08-01 00:30 +0200
              Re: [PATCH 4.4 40/57] tpm: Provide strong locking for device removal Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-08-04 22:10 +0200
                Re: [PATCH 4.4 40/57] tpm: Provide strong locking for device removal Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-08-04 23:50 +0200
                  Re: [PATCH 4.4 40/57] tpm: Provide strong locking for device removal Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-08-06 14:50 +0200
                    Re: [PATCH 4.4 40/57] tpm: Provide strong locking for device removal Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-08-08 23:10 +0200
                      Re: [PATCH 4.4 40/57] tpm: Provide strong locking for device removal Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-08-08 23:20 +0200

#1691507 — [PATCH 4.4 40/57] tpm: Provide strong locking for device removal

4.4-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

commit 4e26195f240d73150e8308ae42874702e3df8d2c upstream.

Add a read/write semaphore around the ops function pointers so
ops can be set to null when the driver un-registers.

Previously the tpm core expected module locking to be enough to
ensure that tpm_unregister could not be called during certain times,
however that hasn't been sufficient for a long time.

Introduce a read/write semaphore around 'ops' so the core can set
it to null when unregistering. This provides a strong fence around
the driver callbacks, guaranteeing to the driver that no callbacks
are running or will run again.

For now the ops_lock is placed very high in the call stack, it could
be pushed down and made more granular in future if necessary.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>


---
 drivers/char/tpm/tpm-chip.c      |   72 +++++++++++++++++++++++++++++++++++----
 drivers/char/tpm/tpm-dev.c       |   11 +++++
 drivers/char/tpm/tpm-interface.c |   19 +++++-----
 drivers/char/tpm/tpm-sysfs.c     |    5 ++
 drivers/char/tpm/tpm.h           |   14 ++++---
 5 files changed, 100 insertions(+), 21 deletions(-)

--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -36,10 +36,60 @@ static DEFINE_SPINLOCK(driver_lock);
 struct class *tpm_class;
 dev_t tpm_devt;
 
-/*
- * tpm_chip_find_get - return tpm_chip for a given chip number
- * @chip_num the device number for the chip
+/**
+ * tpm_try_get_ops() - Get a ref to the tpm_chip
+ * @chip: Chip to ref
+ *
+ * The caller must already have some kind of locking to ensure that chip is
+ * valid. This function will lock the chip so that the ops member can be
+ * accessed safely. The locking prevents tpm_chip_unregister from
+ * completing, so it should not be held for long periods.
+ *
+ * Returns -ERRNO if the chip could not be got.
  */
+int tpm_try_get_ops(struct tpm_chip *chip)
+{
+	int rc = -EIO;
+
+	get_device(&chip->dev);
+
+	down_read(&chip->ops_sem);
+	if (!chip->ops)
+		goto out_lock;
+
+	if (!try_module_get(chip->dev.parent->driver->owner))
+		goto out_lock;
+
+	return 0;
+out_lock:
+	up_read(&chip->ops_sem);
+	put_device(&chip->dev);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(tpm_try_get_ops);
+
+/**
+ * tpm_put_ops() - Release a ref to the tpm_chip
+ * @chip: Chip to put
+ *
+ * This is the opposite pair to tpm_try_get_ops(). After this returns chip may
+ * be kfree'd.
+ */
+void tpm_put_ops(struct tpm_chip *chip)
+{
+	module_put(chip->dev.parent->driver->owner);
+	up_read(&chip->ops_sem);
+	put_device(&chip->dev);
+}
+EXPORT_SYMBOL_GPL(tpm_put_ops);
+
+/**
+ * tpm_chip_find_get() - return tpm_chip for a given chip number
+ * @chip_num: id to find
+ *
+ * The return'd chip has been tpm_try_get_ops'd and must be released via
+ * tpm_put_ops
+  */
 struct tpm_chip *tpm_chip_find_get(int chip_num)
 {
 	struct tpm_chip *pos, *chip = NULL;
@@ -49,10 +99,10 @@ struct tpm_chip *tpm_chip_find_get(int c
 		if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
 			continue;
 
-		if (try_module_get(pos->dev.parent->driver->owner)) {
+		/* rcu prevents chip from being free'd */
+		if (!tpm_try_get_ops(pos))
 			chip = pos;
-			break;
-		}
+		break;
 	}
 	rcu_read_unlock();
 	return chip;
@@ -94,6 +144,7 @@ struct tpm_chip *tpmm_chip_alloc(struct
 		return ERR_PTR(-ENOMEM);
 
 	mutex_init(&chip->tpm_mutex);
+	init_rwsem(&chip->ops_sem);
 	INIT_LIST_HEAD(&chip->list);
 
 	chip->ops = ops;
@@ -171,6 +222,12 @@ static int tpm_add_char_device(struct tp
 static void tpm_del_char_device(struct tpm_chip *chip)
 {
 	cdev_del(&chip->cdev);
+
+	/* Make the driver uncallable. */
+	down_write(&chip->ops_sem);
+	chip->ops = NULL;
+	up_write(&chip->ops_sem);
+
 	device_del(&chip->dev);
 }
 
@@ -256,6 +313,9 @@ EXPORT_SYMBOL_GPL(tpm_chip_register);
  * Takes the chip first away from the list of available TPM chips and then
  * cleans up all the resources reserved by tpm_chip_register().
  *
+ * Once this function returns the driver call backs in 'op's will not be
+ * running and will no longer start.
+ *
  * NOTE: This function should be only called before deinitializing chip
  * resources.
  */
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -136,9 +136,18 @@ static ssize_t tpm_write(struct file *fi
 		return -EFAULT;
 	}
 
-	/* atomic tpm command send and result receive */
+	/* atomic tpm command send and result receive. We only hold the ops
+	 * lock during this period so that the tpm can be unregistered even if
+	 * the char dev is held open.
+	 */
+	if (tpm_try_get_ops(priv->chip)) {
+		mutex_unlock(&priv->buffer_mutex);
+		return -EPIPE;
+	}
 	out_size = tpm_transmit(priv->chip, priv->data_buffer,
 				sizeof(priv->data_buffer), 0);
+
+	tpm_put_ops(priv->chip);
 	if (out_size < 0) {
 		mutex_unlock(&priv->buffer_mutex);
 		return out_size;
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -687,7 +687,7 @@ int tpm_is_tpm2(u32 chip_num)
 
 	rc = (chip->flags & TPM_CHIP_FLAG_TPM2) != 0;
 
-	tpm_chip_put(chip);
+	tpm_put_ops(chip);
 
 	return rc;
 }
@@ -716,7 +716,7 @@ int tpm_pcr_read(u32 chip_num, int pcr_i
 		rc = tpm2_pcr_read(chip, pcr_idx, res_buf);
 	else
 		rc = tpm_pcr_read_dev(chip, pcr_idx, res_buf);
-	tpm_chip_put(chip);
+	tpm_put_ops(chip);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_pcr_read);
@@ -751,7 +751,7 @@ int tpm_pcr_extend(u32 chip_num, int pcr
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
 		rc = tpm2_pcr_extend(chip, pcr_idx, hash);
-		tpm_chip_put(chip);
+		tpm_put_ops(chip);
 		return rc;
 	}
 
@@ -761,7 +761,7 @@ int tpm_pcr_extend(u32 chip_num, int pcr
 	rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE, 0,
 			      "attempting extend a PCR value");
 
-	tpm_chip_put(chip);
+	tpm_put_ops(chip);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_pcr_extend);
@@ -842,7 +842,7 @@ int tpm_send(u32 chip_num, void *cmd, si
 
 	rc = tpm_transmit_cmd(chip, cmd, buflen, 0, "attempting tpm_cmd");
 
-	tpm_chip_put(chip);
+	tpm_put_ops(chip);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_send);
@@ -1025,7 +1025,7 @@ int tpm_get_random(u32 chip_num, u8 *out
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
 		err = tpm2_get_random(chip, out, max);
-		tpm_chip_put(chip);
+		tpm_put_ops(chip);
 		return err;
 	}
 
@@ -1047,7 +1047,7 @@ int tpm_get_random(u32 chip_num, u8 *out
 		num_bytes -= recd;
 	} while (retries-- && total < max);
 
-	tpm_chip_put(chip);
+	tpm_put_ops(chip);
 	return total ? total : -EIO;
 }
 EXPORT_SYMBOL_GPL(tpm_get_random);
@@ -1073,7 +1073,7 @@ int tpm_seal_trusted(u32 chip_num, struc
 
 	rc = tpm2_seal_trusted(chip, payload, options);
 
-	tpm_chip_put(chip);
+	tpm_put_ops(chip);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_seal_trusted);
@@ -1099,7 +1099,8 @@ int tpm_unseal_trusted(u32 chip_num, str
 
 	rc = tpm2_unseal_trusted(chip, payload, options);
 
-	tpm_chip_put(chip);
+	tpm_put_ops(chip);
+
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_unseal_trusted);
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -295,5 +295,10 @@ int tpm_sysfs_add_device(struct tpm_chip
 
 void tpm_sysfs_del_device(struct tpm_chip *chip)
 {
+	/* The sysfs routines rely on an implicit tpm_try_get_ops, this
+	 * function is called before ops is null'd and the sysfs core
+	 * synchronizes this removal so that no callbacks are running or can
+	 * run again
+	 */
 	sysfs_remove_group(&chip->dev.parent->kobj, &tpm_dev_group);
 }
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -174,7 +174,13 @@ struct tpm_chip {
 	struct device dev;
 	struct cdev cdev;
 
+	/* A driver callback under ops cannot be run unless ops_sem is held
+	 * (sometimes implicitly, eg for the sysfs code). ops becomes null
+	 * when the driver is unregistered, see tpm_try_get_ops.
+	 */
+	struct rw_semaphore ops_sem;
 	const struct tpm_class_ops *ops;
+
 	unsigned int flags;
 
 	int dev_num;		/* /dev/tpm# */
@@ -200,11 +206,6 @@ struct tpm_chip {
 
 #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
 
-static inline void tpm_chip_put(struct tpm_chip *chip)
-{
-	module_put(chip->dev.parent->driver->owner);
-}
-
 static inline int tpm_read_index(int base, int index)
 {
 	outb(index, base);
@@ -516,6 +517,9 @@ extern int wait_for_tpm_stat(struct tpm_
 			     wait_queue_head_t *, bool);
 
 struct tpm_chip *tpm_chip_find_get(int chip_num);
+__must_check int tpm_try_get_ops(struct tpm_chip *chip);
+void tpm_put_ops(struct tpm_chip *chip);
+
 extern struct tpm_chip *tpmm_chip_alloc(struct device *dev,
 				       const struct tpm_class_ops *ops);
 extern int tpm_chip_register(struct tpm_chip *chip);

[toc] | [next] | [standalone]

#1696652

On Wed, 2017-07-19 at 13:12 +0200, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> 
> commit 4e26195f240d73150e8308ae42874702e3df8d2c upstream.
> 
> Add a read/write semaphore around the ops function pointers so
> ops can be set to null when the driver un-registers.
[...]
> @@ -49,10 +99,10 @@ struct tpm_chip *tpm_chip_find_get(int c
>  		if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
>  			continue;
>  
> -		if (try_module_get(pos->dev.parent->driver->owner)) {
> +		/* rcu prevents chip from being free'd */
> +		if (!tpm_try_get_ops(pos))
[...]

But an RCU read-side critical section is an atomic context, and
semaphore operations can block!  Fixed upstream by:

commit 15516788e581eb32ec1c50e5f00aba3faf95d817
Author: Stefan Berger <stefanb@linux.vnet.ibm.com>
Date:   Mon Feb 29 08:53:02 2016 -0500

    tpm: Replace device number bitmap with IDR

Ben.

-- 
Ben Hutchings
Software Developer, Codethink Ltd.

[toc] | [prev] | [next] | [standalone]

#1697524

On Tue, Jul 25, 2017 at 11:56:01PM +0100, Ben Hutchings wrote:
> On Wed, 2017-07-19 at 13:12 +0200, Greg Kroah-Hartman wrote:
> > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > 
> > commit 4e26195f240d73150e8308ae42874702e3df8d2c upstream.
> > 
> > Add a read/write semaphore around the ops function pointers so
> > ops can be set to null when the driver un-registers.
> [...]
> > @@ -49,10 +99,10 @@ struct tpm_chip *tpm_chip_find_get(int c
> >  		if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
> >  			continue;
> >  
> > -		if (try_module_get(pos->dev.parent->driver->owner)) {
> > +		/* rcu prevents chip from being free'd */
> > +		if (!tpm_try_get_ops(pos))
> [...]
> 
> But an RCU read-side critical section is an atomic context, and
> semaphore operations can block!  Fixed upstream by:
> 
> commit 15516788e581eb32ec1c50e5f00aba3faf95d817
> Author: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Date:   Mon Feb 29 08:53:02 2016 -0500
> 
>     tpm: Replace device number bitmap with IDR

Ugh, that's a big patch.

Jason, Stefan, and Jarkko, what do you think?  Should I also take this
for 4.4-stable?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1697530

On Wed, Jul 26, 2017 at 12:56:37PM -0700, Greg Kroah-Hartman wrote:
> On Tue, Jul 25, 2017 at 11:56:01PM +0100, Ben Hutchings wrote:
> > On Wed, 2017-07-19 at 13:12 +0200, Greg Kroah-Hartman wrote:
> > > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > > 
> > > 
> > > From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > 
> > > commit 4e26195f240d73150e8308ae42874702e3df8d2c upstream.
> > > 
> > > Add a read/write semaphore around the ops function pointers so
> > > ops can be set to null when the driver un-registers.
> > [...]
> > > @@ -49,10 +99,10 @@ struct tpm_chip *tpm_chip_find_get(int c
> > >  		if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
> > >  			continue;
> > >  
> > > -		if (try_module_get(pos->dev.parent->driver->owner)) {
> > > +		/* rcu prevents chip from being free'd */
> > > +		if (!tpm_try_get_ops(pos))
> > [...]
> > 
> > But an RCU read-side critical section is an atomic context, and
> > semaphore operations can block!  Fixed upstream by:
> > 
> > commit 15516788e581eb32ec1c50e5f00aba3faf95d817
> > Author: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > Date:   Mon Feb 29 08:53:02 2016 -0500
> > 
> >     tpm: Replace device number bitmap with IDR
> 
> Ugh, that's a big patch.
> 
> Jason, Stefan, and Jarkko, what do you think?  Should I also take this
> for 4.4-stable?

15516 is part of the series that included 4e26, I wouldn't take that
series piecemeal, as Ben observes..

I think it would be safer to avoid all these backport patches and
instead restructure the important TPM shutdown patch so that it is
'less safe'. This would mean there is a chance that the another TPM
user could send a command after shutdown, but realistically that is
not likely to happen.

Jason

[toc] | [prev] | [next] | [standalone]

#1699144

On Wed, Jul 26, 2017 at 02:03:06PM -0600, Jason Gunthorpe wrote:
> On Wed, Jul 26, 2017 at 12:56:37PM -0700, Greg Kroah-Hartman wrote:
> > On Tue, Jul 25, 2017 at 11:56:01PM +0100, Ben Hutchings wrote:
> > > On Wed, 2017-07-19 at 13:12 +0200, Greg Kroah-Hartman wrote:
> > > > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > > > 
> > > > 
> > > > From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > > 
> > > > commit 4e26195f240d73150e8308ae42874702e3df8d2c upstream.
> > > > 
> > > > Add a read/write semaphore around the ops function pointers so
> > > > ops can be set to null when the driver un-registers.
> > > [...]
> > > > @@ -49,10 +99,10 @@ struct tpm_chip *tpm_chip_find_get(int c
> > > >  		if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
> > > >  			continue;
> > > >  
> > > > -		if (try_module_get(pos->dev.parent->driver->owner)) {
> > > > +		/* rcu prevents chip from being free'd */
> > > > +		if (!tpm_try_get_ops(pos))
> > > [...]
> > > 
> > > But an RCU read-side critical section is an atomic context, and
> > > semaphore operations can block!  Fixed upstream by:
> > > 
> > > commit 15516788e581eb32ec1c50e5f00aba3faf95d817
> > > Author: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > Date:   Mon Feb 29 08:53:02 2016 -0500
> > > 
> > >     tpm: Replace device number bitmap with IDR
> > 
> > Ugh, that's a big patch.
> > 
> > Jason, Stefan, and Jarkko, what do you think?  Should I also take this
> > for 4.4-stable?
> 
> 15516 is part of the series that included 4e26, I wouldn't take that
> series piecemeal, as Ben observes..
> 
> I think it would be safer to avoid all these backport patches and
> instead restructure the important TPM shutdown patch so that it is
> 'less safe'. This would mean there is a chance that the another TPM
> user could send a command after shutdown, but realistically that is
> not likely to happen.

Ok, so what do you want me to do here?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1700441

On Fri, Jul 28, 2017 at 03:42:18PM -0700, Greg Kroah-Hartman wrote:
> On Wed, Jul 26, 2017 at 02:03:06PM -0600, Jason Gunthorpe wrote:
> > On Wed, Jul 26, 2017 at 12:56:37PM -0700, Greg Kroah-Hartman wrote:
> > > On Tue, Jul 25, 2017 at 11:56:01PM +0100, Ben Hutchings wrote:
> > > > On Wed, 2017-07-19 at 13:12 +0200, Greg Kroah-Hartman wrote:
> > > > > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > > > > 
> > > > > 
> > > > > From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > > > 
> > > > > commit 4e26195f240d73150e8308ae42874702e3df8d2c upstream.
> > > > > 
> > > > > Add a read/write semaphore around the ops function pointers so
> > > > > ops can be set to null when the driver un-registers.
> > > > [...]
> > > > > @@ -49,10 +99,10 @@ struct tpm_chip *tpm_chip_find_get(int c
> > > > >  		if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
> > > > >  			continue;
> > > > >  
> > > > > -		if (try_module_get(pos->dev.parent->driver->owner)) {
> > > > > +		/* rcu prevents chip from being free'd */
> > > > > +		if (!tpm_try_get_ops(pos))
> > > > [...]
> > > > 
> > > > But an RCU read-side critical section is an atomic context, and
> > > > semaphore operations can block!  Fixed upstream by:
> > > > 
> > > > commit 15516788e581eb32ec1c50e5f00aba3faf95d817
> > > > Author: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > > Date:   Mon Feb 29 08:53:02 2016 -0500
> > > > 
> > > >     tpm: Replace device number bitmap with IDR
> > > 
> > > Ugh, that's a big patch.
> > > 
> > > Jason, Stefan, and Jarkko, what do you think?  Should I also take this
> > > for 4.4-stable?
> > 
> > 15516 is part of the series that included 4e26, I wouldn't take that
> > series piecemeal, as Ben observes..
> > 
> > I think it would be safer to avoid all these backport patches and
> > instead restructure the important TPM shutdown patch so that it is
> > 'less safe'. This would mean there is a chance that the another TPM
> > user could send a command after shutdown, but realistically that is
> > not likely to happen.
> 
> Ok, so what do you want me to do here?
> 
> thanks,
> 
> greg k-h

Sorry for late response. I just came from four week leave (have been
watching kernel mails only 1-2 times a week and missed this thread).

I would actually think that taking this patch would make sense as the
changes are trivial and also because this code has reminded almost
unchanged after it was added.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1704189

On Tue, Aug 01, 2017 at 01:22:54AM +0300, Jarkko Sakkinen wrote:
> On Fri, Jul 28, 2017 at 03:42:18PM -0700, Greg Kroah-Hartman wrote:
> > On Wed, Jul 26, 2017 at 02:03:06PM -0600, Jason Gunthorpe wrote:
> > > On Wed, Jul 26, 2017 at 12:56:37PM -0700, Greg Kroah-Hartman wrote:
> > > > On Tue, Jul 25, 2017 at 11:56:01PM +0100, Ben Hutchings wrote:
> > > > > On Wed, 2017-07-19 at 13:12 +0200, Greg Kroah-Hartman wrote:
> > > > > > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > > > > > 
> > > > > > 
> > > > > > From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > > > > 
> > > > > > commit 4e26195f240d73150e8308ae42874702e3df8d2c upstream.
> > > > > > 
> > > > > > Add a read/write semaphore around the ops function pointers so
> > > > > > ops can be set to null when the driver un-registers.
> > > > > [...]
> > > > > > @@ -49,10 +99,10 @@ struct tpm_chip *tpm_chip_find_get(int c
> > > > > >  		if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
> > > > > >  			continue;
> > > > > >  
> > > > > > -		if (try_module_get(pos->dev.parent->driver->owner)) {
> > > > > > +		/* rcu prevents chip from being free'd */
> > > > > > +		if (!tpm_try_get_ops(pos))
> > > > > [...]
> > > > > 
> > > > > But an RCU read-side critical section is an atomic context, and
> > > > > semaphore operations can block!  Fixed upstream by:
> > > > > 
> > > > > commit 15516788e581eb32ec1c50e5f00aba3faf95d817
> > > > > Author: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > > > Date:   Mon Feb 29 08:53:02 2016 -0500
> > > > > 
> > > > >     tpm: Replace device number bitmap with IDR
> > > > 
> > > > Ugh, that's a big patch.
> > > > 
> > > > Jason, Stefan, and Jarkko, what do you think?  Should I also take this
> > > > for 4.4-stable?
> > > 
> > > 15516 is part of the series that included 4e26, I wouldn't take that
> > > series piecemeal, as Ben observes..
> > > 
> > > I think it would be safer to avoid all these backport patches and
> > > instead restructure the important TPM shutdown patch so that it is
> > > 'less safe'. This would mean there is a chance that the another TPM
> > > user could send a command after shutdown, but realistically that is
> > > not likely to happen.
> > 
> > Ok, so what do you want me to do here?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Sorry for late response. I just came from four week leave (have been
> watching kernel mails only 1-2 times a week and missed this thread).
> 
> I would actually think that taking this patch would make sense as the
> changes are trivial and also because this code has reminded almost
> unchanged after it was added.

Ok, now queued up, thanks.

greg k-h

[toc] | [prev] | [next] | [standalone]

#1704237

On Fri, Aug 04, 2017 at 12:59:56PM -0700, Greg Kroah-Hartman wrote:
> On Tue, Aug 01, 2017 at 01:22:54AM +0300, Jarkko Sakkinen wrote:
> > On Fri, Jul 28, 2017 at 03:42:18PM -0700, Greg Kroah-Hartman wrote:
> > > On Wed, Jul 26, 2017 at 02:03:06PM -0600, Jason Gunthorpe wrote:
> > > > On Wed, Jul 26, 2017 at 12:56:37PM -0700, Greg Kroah-Hartman wrote:
> > > > > On Tue, Jul 25, 2017 at 11:56:01PM +0100, Ben Hutchings wrote:
> > > > > > On Wed, 2017-07-19 at 13:12 +0200, Greg Kroah-Hartman wrote:
> > > > > > > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > > > > > > 
> > > > > > > 
> > > > > > > From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > > > > > 
> > > > > > > commit 4e26195f240d73150e8308ae42874702e3df8d2c upstream.
> > > > > > > 
> > > > > > > Add a read/write semaphore around the ops function pointers so
> > > > > > > ops can be set to null when the driver un-registers.
> > > > > > [...]
> > > > > > > @@ -49,10 +99,10 @@ struct tpm_chip *tpm_chip_find_get(int c
> > > > > > >  		if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
> > > > > > >  			continue;
> > > > > > >  
> > > > > > > -		if (try_module_get(pos->dev.parent->driver->owner)) {
> > > > > > > +		/* rcu prevents chip from being free'd */
> > > > > > > +		if (!tpm_try_get_ops(pos))
> > > > > > [...]
> > > > > > 
> > > > > > But an RCU read-side critical section is an atomic context, and
> > > > > > semaphore operations can block!  Fixed upstream by:
> > > > > > 
> > > > > > commit 15516788e581eb32ec1c50e5f00aba3faf95d817
> > > > > > Author: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > > > > Date:   Mon Feb 29 08:53:02 2016 -0500
> > > > > > 
> > > > > >     tpm: Replace device number bitmap with IDR
> > > > > 
> > > > > Ugh, that's a big patch.
> > > > > 
> > > > > Jason, Stefan, and Jarkko, what do you think?  Should I also take this
> > > > > for 4.4-stable?
> > > > 
> > > > 15516 is part of the series that included 4e26, I wouldn't take that
> > > > series piecemeal, as Ben observes..
> > > > 
> > > > I think it would be safer to avoid all these backport patches and
> > > > instead restructure the important TPM shutdown patch so that it is
> > > > 'less safe'. This would mean there is a chance that the another TPM
> > > > user could send a command after shutdown, but realistically that is
> > > > not likely to happen.
> > > 
> > > Ok, so what do you want me to do here?
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > Sorry for late response. I just came from four week leave (have been
> > watching kernel mails only 1-2 times a week and missed this thread).
> > 
> > I would actually think that taking this patch would make sense as the
> > changes are trivial and also because this code has reminded almost
> > unchanged after it was added.
> 
> Ok, now queued up, thanks.

But it's messy, if someone could verify that this is all working
properly on the 4.4-stable kernel with the next -rc release, I would
really appreciate it.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1704837

On Fri, Aug 04, 2017 at 02:44:18PM -0700, Greg Kroah-Hartman wrote:
> On Fri, Aug 04, 2017 at 12:59:56PM -0700, Greg Kroah-Hartman wrote:
> > On Tue, Aug 01, 2017 at 01:22:54AM +0300, Jarkko Sakkinen wrote:
> > > On Fri, Jul 28, 2017 at 03:42:18PM -0700, Greg Kroah-Hartman wrote:
> > > > On Wed, Jul 26, 2017 at 02:03:06PM -0600, Jason Gunthorpe wrote:
> > > > > On Wed, Jul 26, 2017 at 12:56:37PM -0700, Greg Kroah-Hartman wrote:
> > > > > > On Tue, Jul 25, 2017 at 11:56:01PM +0100, Ben Hutchings wrote:
> > > > > > > On Wed, 2017-07-19 at 13:12 +0200, Greg Kroah-Hartman wrote:
> > > > > > > > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > > > > > > 
> > > > > > > > commit 4e26195f240d73150e8308ae42874702e3df8d2c upstream.
> > > > > > > > 
> > > > > > > > Add a read/write semaphore around the ops function pointers so
> > > > > > > > ops can be set to null when the driver un-registers.
> > > > > > > [...]
> > > > > > > > @@ -49,10 +99,10 @@ struct tpm_chip *tpm_chip_find_get(int c
> > > > > > > >  		if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
> > > > > > > >  			continue;
> > > > > > > >  
> > > > > > > > -		if (try_module_get(pos->dev.parent->driver->owner)) {
> > > > > > > > +		/* rcu prevents chip from being free'd */
> > > > > > > > +		if (!tpm_try_get_ops(pos))
> > > > > > > [...]
> > > > > > > 
> > > > > > > But an RCU read-side critical section is an atomic context, and
> > > > > > > semaphore operations can block!  Fixed upstream by:
> > > > > > > 
> > > > > > > commit 15516788e581eb32ec1c50e5f00aba3faf95d817
> > > > > > > Author: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > > > > > Date:   Mon Feb 29 08:53:02 2016 -0500
> > > > > > > 
> > > > > > >     tpm: Replace device number bitmap with IDR
> > > > > > 
> > > > > > Ugh, that's a big patch.
> > > > > > 
> > > > > > Jason, Stefan, and Jarkko, what do you think?  Should I also take this
> > > > > > for 4.4-stable?
> > > > > 
> > > > > 15516 is part of the series that included 4e26, I wouldn't take that
> > > > > series piecemeal, as Ben observes..
> > > > > 
> > > > > I think it would be safer to avoid all these backport patches and
> > > > > instead restructure the important TPM shutdown patch so that it is
> > > > > 'less safe'. This would mean there is a chance that the another TPM
> > > > > user could send a command after shutdown, but realistically that is
> > > > > not likely to happen.
> > > > 
> > > > Ok, so what do you want me to do here?
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > 
> > > Sorry for late response. I just came from four week leave (have been
> > > watching kernel mails only 1-2 times a week and missed this thread).
> > > 
> > > I would actually think that taking this patch would make sense as the
> > > changes are trivial and also because this code has reminded almost
> > > unchanged after it was added.
> > 
> > Ok, now queued up, thanks.
> 
> But it's messy, if someone could verify that this is all working
> properly on the 4.4-stable kernel with the next -rc release, I would
> really appreciate it.
> 
> thanks,
> 
> greg k-h

I will take care of that. Thank you.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1706888

On Sun, Aug 06, 2017 at 03:47:49PM +0300, Jarkko Sakkinen wrote:
> On Fri, Aug 04, 2017 at 02:44:18PM -0700, Greg Kroah-Hartman wrote:
> > On Fri, Aug 04, 2017 at 12:59:56PM -0700, Greg Kroah-Hartman wrote:
> > > On Tue, Aug 01, 2017 at 01:22:54AM +0300, Jarkko Sakkinen wrote:
> > > > On Fri, Jul 28, 2017 at 03:42:18PM -0700, Greg Kroah-Hartman wrote:
> > > > > On Wed, Jul 26, 2017 at 02:03:06PM -0600, Jason Gunthorpe wrote:
> > > > > > On Wed, Jul 26, 2017 at 12:56:37PM -0700, Greg Kroah-Hartman wrote:
> > > > > > > On Tue, Jul 25, 2017 at 11:56:01PM +0100, Ben Hutchings wrote:
> > > > > > > > On Wed, 2017-07-19 at 13:12 +0200, Greg Kroah-Hartman wrote:
> > > > > > > > > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > > > > > > > 
> > > > > > > > > commit 4e26195f240d73150e8308ae42874702e3df8d2c upstream.
> > > > > > > > > 
> > > > > > > > > Add a read/write semaphore around the ops function pointers so
> > > > > > > > > ops can be set to null when the driver un-registers.
> > > > > > > > [...]
> > > > > > > > > @@ -49,10 +99,10 @@ struct tpm_chip *tpm_chip_find_get(int c
> > > > > > > > >  		if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
> > > > > > > > >  			continue;
> > > > > > > > >  
> > > > > > > > > -		if (try_module_get(pos->dev.parent->driver->owner)) {
> > > > > > > > > +		/* rcu prevents chip from being free'd */
> > > > > > > > > +		if (!tpm_try_get_ops(pos))
> > > > > > > > [...]
> > > > > > > > 
> > > > > > > > But an RCU read-side critical section is an atomic context, and
> > > > > > > > semaphore operations can block!  Fixed upstream by:
> > > > > > > > 
> > > > > > > > commit 15516788e581eb32ec1c50e5f00aba3faf95d817
> > > > > > > > Author: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > > > > > > Date:   Mon Feb 29 08:53:02 2016 -0500
> > > > > > > > 
> > > > > > > >     tpm: Replace device number bitmap with IDR
> > > > > > > 
> > > > > > > Ugh, that's a big patch.
> > > > > > > 
> > > > > > > Jason, Stefan, and Jarkko, what do you think?  Should I also take this
> > > > > > > for 4.4-stable?
> > > > > > 
> > > > > > 15516 is part of the series that included 4e26, I wouldn't take that
> > > > > > series piecemeal, as Ben observes..
> > > > > > 
> > > > > > I think it would be safer to avoid all these backport patches and
> > > > > > instead restructure the important TPM shutdown patch so that it is
> > > > > > 'less safe'. This would mean there is a chance that the another TPM
> > > > > > user could send a command after shutdown, but realistically that is
> > > > > > not likely to happen.
> > > > > 
> > > > > Ok, so what do you want me to do here?
> > > > > 
> > > > > thanks,
> > > > > 
> > > > > greg k-h
> > > > 
> > > > Sorry for late response. I just came from four week leave (have been
> > > > watching kernel mails only 1-2 times a week and missed this thread).
> > > > 
> > > > I would actually think that taking this patch would make sense as the
> > > > changes are trivial and also because this code has reminded almost
> > > > unchanged after it was added.
> > > 
> > > Ok, now queued up, thanks.
> > 
> > But it's messy, if someone could verify that this is all working
> > properly on the 4.4-stable kernel with the next -rc release, I would
> > really appreciate it.
> > 
> > thanks,
> > 
> > greg k-h
> 
> I will take care of that. Thank you.
> 
> /Jarkko

I tried 4.4.80 kernel (which has the IDR patc) and everything seems to
work just fine.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1706889

On Wed, Aug 09, 2017 at 12:05:38AM +0300, Jarkko Sakkinen wrote:
> On Sun, Aug 06, 2017 at 03:47:49PM +0300, Jarkko Sakkinen wrote:
> > On Fri, Aug 04, 2017 at 02:44:18PM -0700, Greg Kroah-Hartman wrote:
> > > On Fri, Aug 04, 2017 at 12:59:56PM -0700, Greg Kroah-Hartman wrote:
> > > > On Tue, Aug 01, 2017 at 01:22:54AM +0300, Jarkko Sakkinen wrote:
> > > > > On Fri, Jul 28, 2017 at 03:42:18PM -0700, Greg Kroah-Hartman wrote:
> > > > > > On Wed, Jul 26, 2017 at 02:03:06PM -0600, Jason Gunthorpe wrote:
> > > > > > > On Wed, Jul 26, 2017 at 12:56:37PM -0700, Greg Kroah-Hartman wrote:
> > > > > > > > On Tue, Jul 25, 2017 at 11:56:01PM +0100, Ben Hutchings wrote:
> > > > > > > > > On Wed, 2017-07-19 at 13:12 +0200, Greg Kroah-Hartman wrote:
> > > > > > > > > > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > > > > > > > > 
> > > > > > > > > > commit 4e26195f240d73150e8308ae42874702e3df8d2c upstream.
> > > > > > > > > > 
> > > > > > > > > > Add a read/write semaphore around the ops function pointers so
> > > > > > > > > > ops can be set to null when the driver un-registers.
> > > > > > > > > [...]
> > > > > > > > > > @@ -49,10 +99,10 @@ struct tpm_chip *tpm_chip_find_get(int c
> > > > > > > > > >  		if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
> > > > > > > > > >  			continue;
> > > > > > > > > >  
> > > > > > > > > > -		if (try_module_get(pos->dev.parent->driver->owner)) {
> > > > > > > > > > +		/* rcu prevents chip from being free'd */
> > > > > > > > > > +		if (!tpm_try_get_ops(pos))
> > > > > > > > > [...]
> > > > > > > > > 
> > > > > > > > > But an RCU read-side critical section is an atomic context, and
> > > > > > > > > semaphore operations can block!  Fixed upstream by:
> > > > > > > > > 
> > > > > > > > > commit 15516788e581eb32ec1c50e5f00aba3faf95d817
> > > > > > > > > Author: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > > > > > > > Date:   Mon Feb 29 08:53:02 2016 -0500
> > > > > > > > > 
> > > > > > > > >     tpm: Replace device number bitmap with IDR
> > > > > > > > 
> > > > > > > > Ugh, that's a big patch.
> > > > > > > > 
> > > > > > > > Jason, Stefan, and Jarkko, what do you think?  Should I also take this
> > > > > > > > for 4.4-stable?
> > > > > > > 
> > > > > > > 15516 is part of the series that included 4e26, I wouldn't take that
> > > > > > > series piecemeal, as Ben observes..
> > > > > > > 
> > > > > > > I think it would be safer to avoid all these backport patches and
> > > > > > > instead restructure the important TPM shutdown patch so that it is
> > > > > > > 'less safe'. This would mean there is a chance that the another TPM
> > > > > > > user could send a command after shutdown, but realistically that is
> > > > > > > not likely to happen.
> > > > > > 
> > > > > > Ok, so what do you want me to do here?
> > > > > > 
> > > > > > thanks,
> > > > > > 
> > > > > > greg k-h
> > > > > 
> > > > > Sorry for late response. I just came from four week leave (have been
> > > > > watching kernel mails only 1-2 times a week and missed this thread).
> > > > > 
> > > > > I would actually think that taking this patch would make sense as the
> > > > > changes are trivial and also because this code has reminded almost
> > > > > unchanged after it was added.
> > > > 
> > > > Ok, now queued up, thanks.
> > > 
> > > But it's messy, if someone could verify that this is all working
> > > properly on the 4.4-stable kernel with the next -rc release, I would
> > > really appreciate it.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > I will take care of that. Thank you.
> > 
> > /Jarkko
> 
> I tried 4.4.80 kernel (which has the IDR patc) and everything seems to
> work just fine.

Great, thanks for testing and letting me know.

greg k-h

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web