[tpmdd-devel,1/3] tpm: Hold the kref during tpm_chip_find_get
diff mbox

Message ID 1455321871-28296-2-git-send-email-jgunthorpe@obsidianresearch.com
State New
Headers show

Commit Message

Jason Gunthorpe Feb. 13, 2016, 12:04 a.m. UTC
This was missed during the struct device conversion, we
need to hold a kref on the chip to make sure it isn't freed.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/char/tpm/tpm-chip.c | 2 ++
 drivers/char/tpm/tpm.h      | 1 +
 2 files changed, 3 insertions(+)

Comments

Jarkko Sakkinen Feb. 13, 2016, 10:08 a.m. UTC | #1
On Fri, Feb 12, 2016 at 05:04:29PM -0700, Jason Gunthorpe wrote:
> This was missed during the struct device conversion, we
> need to hold a kref on the chip to make sure it isn't freed.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

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

For rest of the patches I'll look at them post-v4.5. I'm still waiting
for my first pull request containing fixes for that release to be
taken.

James, would it be possible to take the pull request containing fixes
soon that I origianally sent already in Jan and bit updated version
few days ago [1]? I still have one more pull request coming after that. It will
contain only few line changes:

* Missing put_device() in two places.
* There is a small change to ABI behavior (I'll post it
  soon) of policy sealing so I really need it for this release because
  4.5 is the first release containing this feature and I cannot change
  it when the release is tagged.

Thank you.

[1] http://www.gossamer-threads.com/lists/linux/kernel/2366902

/Jarkko


> ---
>  drivers/char/tpm/tpm-chip.c | 2 ++
>  drivers/char/tpm/tpm.h      | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 45cc39aabeee..ae2fed8a162b 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -53,6 +53,8 @@ struct tpm_chip *tpm_chip_find_get(int chip_num)
>  			chip = pos;
>  			break;
>  		}
> +
> +		get_device(&chip->dev);
>  	}
>  	rcu_read_unlock();
>  	return chip;
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 542a80cbfd9c..f6ba79d91857 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -207,6 +207,7 @@ struct tpm_chip {
>  static inline void tpm_chip_put(struct tpm_chip *chip)
>  {
>  	module_put(chip->pdev->driver->owner);
> +	put_device(&chip->dev);
>  }
>  
>  static inline int tpm_read_index(int base, int index)
> -- 
> 2.1.4
> 

------------------------------------------------------------------------------
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
Jason Gunthorpe Feb. 13, 2016, 3:45 p.m. UTC | #2
Hi Jarkko,

I found a bug in this last night, please hold off.

Jason


On Feb 13, 2016 3:08 AM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
>
> On Fri, Feb 12, 2016 at 05:04:29PM -0700, Jason Gunthorpe wrote: 
> > This was missed during the struct device conversion, we 
> > need to hold a kref on the chip to make sure it isn't freed. 
> > 
> > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> 
>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.cm> 
>
> For rest of the patches I'll look at them post-v4.5. I'm still waiting 
> for my first pull request containing fixes for that release to be 
> taken. 
>
> James, would it be possible to take the pull request containing fixes 
> soon that I origianally sent already in Jan and bit updated version 
> few days ago [1]? I still have one more pull request coming after that. It will 
> contain only few line changes: 
>
> * Missing put_device() in two places. 
> * There is a small change to ABI behavior (I'll post it 
>   soon) of policy sealing so I really need it for this release because 
>   4.5 is the first release containing this feature and I cannot change 
>   it when the release is tagged. 
>
> Thank you. 
>
> [1] http://www.gossamer-threads.com/lists/linux/kernel/2366902 
>
> /Jarkko 
>
>
> > --- 
> >  drivers/char/tpm/tpm-chip.c | 2 ++ 
> >  drivers/char/tpm/tpm.h      | 1 + 
> >  2 files changed, 3 insertions(+) 
> > 
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c 
> > index 45cc39aabeee..ae2fed8a162b 100644 
> > --- a/drivers/char/tpm/tpm-chip.c 
> > +++ b/drivers/char/tpm/tpm-chip.c 
> > @@ -53,6 +53,8 @@ struct tpm_chip *tpm_chip_find_get(int chip_num) 
> >  chip = pos; 
> >  break; 
> >  } 
> > + 
> > + get_device(&chip->dev); 
> >  } 
> >  rcu_read_unlock(); 
> >  return chip; 
> > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h 
> > index 542a80cbfd9c..f6ba79d91857 100644 
> > --- a/drivers/char/tpm/tpm.h 
> > +++ b/drivers/char/tpm/tpm.h 
> > @@ -207,6 +207,7 @@ struct tpm_chip { 
> >  static inline void tpm_chip_put(struct tpm_chip *chip) 
> >  { 
> >  module_put(chip->pdev->driver->owner); 
> > + put_device(&chip->dev); 
> >  } 
> >  
> >  static inline int tpm_read_index(int base, int index) 
> > -- 
> > 2.1.4 
> > 
------------------------------------------------------------------------------
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
Jarkko Sakkinen Feb. 14, 2016, 4:55 a.m. UTC | #3
On Fri, Feb 12, 2016 at 05:04:29PM -0700, Jason Gunthorpe wrote:
> This was missed during the struct device conversion, we
> need to hold a kref on the chip to make sure it isn't freed.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

I'm bit confused about this patch. What is the regression if this needs
to be dropped from my last pull request for 4.5 (that is the only one
I'm planning to include from this patch set, rest are definitely
post-4.5)?

If there is clear regression in this patch, I can do it. I didn't
reproduce any.

/Jarkko

> ---
>  drivers/char/tpm/tpm-chip.c | 2 ++
>  drivers/char/tpm/tpm.h      | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 45cc39aabeee..ae2fed8a162b 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -53,6 +53,8 @@ struct tpm_chip *tpm_chip_find_get(int chip_num)
>  			chip = pos;
>  			break;
>  		}
> +
> +		get_device(&chip->dev);
>  	}
>  	rcu_read_unlock();
>  	return chip;
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 542a80cbfd9c..f6ba79d91857 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -207,6 +207,7 @@ struct tpm_chip {
>  static inline void tpm_chip_put(struct tpm_chip *chip)
>  {
>  	module_put(chip->pdev->driver->owner);
> +	put_device(&chip->dev);
>  }
>  
>  static inline int tpm_read_index(int base, int index)
> -- 
> 2.1.4
> 

------------------------------------------------------------------------------
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
Jason Gunthorpe Feb. 14, 2016, 6:50 a.m. UTC | #4
On Sun, Feb 14, 2016 at 06:55:12AM +0200, Jarkko Sakkinen wrote:
> On Fri, Feb 12, 2016 at 05:04:29PM -0700, Jason Gunthorpe wrote:
> > This was missed during the struct device conversion, we
> > need to hold a kref on the chip to make sure it isn't freed.
> > 
> > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> 
> I'm bit confused about this patch. What is the regression if this
> needs

The patch is simply totally broken, the placement of the get_device is
wrong:

> > @@ -53,6 +53,8 @@ struct tpm_chip *tpm_chip_find_get(int chip_num)
> >  			chip = pos;
> >  			break;
> >  		}
> > +
> > +		get_device(&chip->dev);

It needs to be moved up two lines before the break, into the if
statement.

As for the urgency - today the tpm core relies on module locking to
try and prevent tpm_chip_unregister from racing with stuff like the
above. That is totally broken in modern kernels, but it is what the
core tries to do. Within that framework the get/put are not needed
because of the module locking.

The only time these additional get/put do anything is when we are
racing with tpm_unregister, but if we are racing with unregister then
there are much bigger problems and things will crash anyhow.

So, this patch is just a tiny step.

The revised version of this patch with the rw_sem attempts to address
the complete race.

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
Jarkko Sakkinen Feb. 14, 2016, 8:02 a.m. UTC | #5
On Sat, Feb 13, 2016 at 11:50:08PM -0700, Jason Gunthorpe wrote:
> On Sun, Feb 14, 2016 at 06:55:12AM +0200, Jarkko Sakkinen wrote:
> > On Fri, Feb 12, 2016 at 05:04:29PM -0700, Jason Gunthorpe wrote:
> > > This was missed during the struct device conversion, we
> > > need to hold a kref on the chip to make sure it isn't freed.
> > > 
> > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > 
> > I'm bit confused about this patch. What is the regression if this
> > needs
> 
> The patch is simply totally broken, the placement of the get_device is
> wrong:
> 
> > > @@ -53,6 +53,8 @@ struct tpm_chip *tpm_chip_find_get(int chip_num)
> > >  			chip = pos;
> > >  			break;
> > >  		}
> > > +
> > > +		get_device(&chip->dev);
> 
> It needs to be moved up two lines before the break, into the if
> statement.

Right.

> As for the urgency - today the tpm core relies on module locking to
> try and prevent tpm_chip_unregister from racing with stuff like the
> above. That is totally broken in modern kernels, but it is what the
> core tries to do. Within that framework the get/put are not needed
> because of the module locking.

Right, because that gives the guarantee that device has refcount of
at least one.

> The only time these additional get/put do anything is when we are
> racing with tpm_unregister, but if we are racing with unregister then
> there are much bigger problems and things will crash anyhow.
> 
> So, this patch is just a tiny step.
> 
> The revised version of this patch with the rw_sem attempts to address
> the complete race.

Got it. Yeah, I'll drop this from my next pull request. Thanks for
the explanation.

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

Patch
diff mbox

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 45cc39aabeee..ae2fed8a162b 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -53,6 +53,8 @@  struct tpm_chip *tpm_chip_find_get(int chip_num)
 			chip = pos;
 			break;
 		}
+
+		get_device(&chip->dev);
 	}
 	rcu_read_unlock();
 	return chip;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 542a80cbfd9c..f6ba79d91857 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -207,6 +207,7 @@  struct tpm_chip {
 static inline void tpm_chip_put(struct tpm_chip *chip)
 {
 	module_put(chip->pdev->driver->owner);
+	put_device(&chip->dev);
 }
 
 static inline int tpm_read_index(int base, int index)