| Message ID | 1455321871-28296-2-git-send-email-jgunthorpe@obsidianresearch.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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)
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(+)