[tpmdd-devel,GIT,PULL] remaining tpmdd fixes for Linux 4.5
mbox

Message ID 20160220081705.GA12981@intel.com
State New
Headers show

Pull-request

https://github.com/jsakkine/linux-tpmdd.git tags/tpmdd-next-20160220

Message

Jarkko Sakkinen Feb. 20, 2016, 8:17 a.m. UTC
Hi James,

I'm sorry for the late pull request for 4.5. The reason for this was
the latency in my previous one. I picked with care the absolutely
critical fixes so that we can make a sound tpmdd release.

I really hope you can still pick these as one of them is absolutely
critical to get authorization policy sealing API right (kernel keeps
it finger out of user space created objects).

BR,
/Jarkko

The following changes since commit 8e0ee3c9faed7ca68807ea45141775856c438ac0:

  tpm: fix the cleanup of struct tpm_chip (2016-02-10 04:12:08 +0200)

are available in the git repository at:

  https://github.com/jsakkine/linux-tpmdd.git tags/tpmdd-next-20160220

for you to fetch changes up to 99cda8cb4639de81cde785b5bab9bc52e916e594:

  tpm_crb: tpm2_shutdown() must be called before tpm_chip_unregister() (2016-02-20 09:59:33 +0200)

----------------------------------------------------------------
remaining tpmdd fixes for Linux 4.5

----------------------------------------------------------------
Harald Hoyer (1):
      tpm_eventlog.c: fix binary_bios_measurements

Jarkko Sakkinen (4):
      tpm: fix: set continueSession attribute for the unseal operation
      tpm: fix: return rc when devm_add_action() fails
      tpm_crb/tis: fix: use dev_name() for /proc/iomem
      tpm_crb: tpm2_shutdown() must be called before tpm_chip_unregister()

 drivers/char/tpm/tpm-chip.c     |  7 ++++++-
 drivers/char/tpm/tpm2-cmd.c     | 10 +++++++---
 drivers/char/tpm/tpm_crb.c      |  8 +++++---
 drivers/char/tpm/tpm_eventlog.c | 14 ++++++++++----
 drivers/char/tpm/tpm_tis.c      |  4 +++-
 5 files changed, 31 insertions(+), 12 deletions(-)

------------------------------------------------------------------------------
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

Comments

James Morris Feb. 22, 2016, 1:56 a.m. UTC | #1
On Sat, 20 Feb 2016, Jarkko Sakkinen wrote:

> Hi James,
> 
> I'm sorry for the late pull request for 4.5. The reason for this was
> the latency in my previous one. I picked with care the absolutely
> critical fixes so that we can make a sound tpmdd release.
> 
> I really hope you can still pick these as one of them is absolutely
> critical to get authorization policy sealing API right (kernel keeps
> it finger out of user space created objects).

Pushed to next for more testing and review.

This really is getting too late in the development cycle for so many 
fixes.  It means the code was not ready to be merged in the first place.

Also, any idea why I'm seeing this:

drivers/char/tpm/tpm_tis.c:838: warning: ‘tpm_tis_resume’ defined but not 
used
Jarkko Sakkinen Feb. 22, 2016, 2:50 p.m. UTC | #2
On Mon, Feb 22, 2016 at 12:56:53PM +1100, James Morris wrote:
> On Sat, 20 Feb 2016, Jarkko Sakkinen wrote:
> 
> > Hi James,
> > 
> > I'm sorry for the late pull request for 4.5. The reason for this was
> > the latency in my previous one. I picked with care the absolutely
> > critical fixes so that we can make a sound tpmdd release.
> > 
> > I really hope you can still pick these as one of them is absolutely
> > critical to get authorization policy sealing API right (kernel keeps
> > it finger out of user space created objects).
> 
> Pushed to next for more testing and review.
> 
> This really is getting too late in the development cycle for so many 
> fixes.  It means the code was not ready to be merged in the first place.

I fully agree what you're saying. I'll learn the lesson here and take
factors more conservative attitude from now on. No excuses. I'm sorry
about this.

Partly the reason for recent increase in regressions has been
increased real-world use of TPM2 and thus issues have started to pop
up that's a lame excuse anyway.

> Also, any idea why I'm seeing this:
> 
> drivers/char/tpm/tpm_tis.c:838: warning: ‘tpm_tis_resume’ defined but not 
> used

Bisected the patch: 00194826e6be

Do you want me to send a pull request containing a fix for the build
warning or reverting the whole commit? My call would be to apply the
fix because this commit has been tested both TPM 1.2 by Martin and
with TPM 2.0 by me and things have worked well.

I can live with either option.

I already pushed a fix to my master for this issue:

https://github.com/jsakkine/linux-tpmdd/commit/6386544ad7bceb3d0248b85da29d4d99eebe9161

> -- 
> James Morris
> <jmorris@namei.org>

I've been recently working on a custom BR environment that bundles my
latest master with initramfs user space [1]. At minimum I'll start
using this environment to create builds of this env with and without
PM for release testing and run the images both 1.2 and 2.0 HW.

This should prevent the warning you experienced never happening again.

[1]  http://git.infradead.org/users/jjs/buildroot-tpmdd.git

/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
Jason Gunthorpe Feb. 22, 2016, 5:52 p.m. UTC | #3
On Mon, Feb 22, 2016 at 04:50:23PM +0200, Jarkko Sakkinen wrote:

> I already pushed a fix to my master for this issue:
> 
> https://github.com/jsakkine/linux-tpmdd/commit/6386544ad7bceb3d0248b85da29d4d99eebe9161

The goal is to reduce the number of #ifdef'd code segments so we have
fewer problems in future with a large .config test matrix.

I'd rather see a __maybe_unused annotation instead.

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. 22, 2016, 7:08 p.m. UTC | #4
On Mon, Feb 22, 2016 at 10:52:45AM -0700, Jason Gunthorpe wrote:
> On Mon, Feb 22, 2016 at 04:50:23PM +0200, Jarkko Sakkinen wrote:
> 
> > I already pushed a fix to my master for this issue:
> > 
> > https://github.com/jsakkine/linux-tpmdd/commit/6386544ad7bceb3d0248b85da29d4d99eebe9161
> 
> The goal is to reduce the number of #ifdef'd code segments so we have
> fewer problems in future with a large .config test matrix.
> 
> I'd rather see a __maybe_unused annotation instead.

Agreed that it's a better form but at this point it's probably revert
the breaking change and move to that later on. Otherwise, I don't see
reason not to include the patch that you authored to the release. I've
used it in my test kernels for quite some time now and it has worked
without issues.

I sent my fix for review now.

> 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
Jason Gunthorpe Feb. 22, 2016, 7:11 p.m. UTC | #5
On Mon, Feb 22, 2016 at 09:08:28PM +0200, Jarkko Sakkinen wrote:
> On Mon, Feb 22, 2016 at 10:52:45AM -0700, Jason Gunthorpe wrote:
> > On Mon, Feb 22, 2016 at 04:50:23PM +0200, Jarkko Sakkinen wrote:
> > 
> > > I already pushed a fix to my master for this issue:
> > > 
> > > https://github.com/jsakkine/linux-tpmdd/commit/6386544ad7bceb3d0248b85da29d4d99eebe9161
> > 
> > The goal is to reduce the number of #ifdef'd code segments so we have
> > fewer problems in future with a large .config test matrix.
> > 
> > I'd rather see a __maybe_unused annotation instead.
> 
> Agreed that it's a better form but at this point it's probably revert
> the breaking change and move to that later on. Otherwise, I don't see
> reason not to include the patch that you authored to the release. I've
> used it in my test kernels for quite some time now and it has worked
> without issues.
> 
> I sent my fix for review now.

A warning with some kconfigs is very minor, we can take the time to
fix it properly for 4.6.

I am surprised the 0day -next builds/etc didn't notice this - Jarkko is
your tree included in that process somehow? (sorry, I don't remember
the details)

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. 22, 2016, 9:23 p.m. UTC | #6
On Mon, Feb 22, 2016 at 12:11:48PM -0700, Jason Gunthorpe wrote:
> On Mon, Feb 22, 2016 at 09:08:28PM +0200, Jarkko Sakkinen wrote:
> > On Mon, Feb 22, 2016 at 10:52:45AM -0700, Jason Gunthorpe wrote:
> > > On Mon, Feb 22, 2016 at 04:50:23PM +0200, Jarkko Sakkinen wrote:
> > > 
> > > > I already pushed a fix to my master for this issue:
> > > > 
> > > > https://github.com/jsakkine/linux-tpmdd/commit/6386544ad7bceb3d0248b85da29d4d99eebe9161
> > > 
> > > The goal is to reduce the number of #ifdef'd code segments so we have
> > > fewer problems in future with a large .config test matrix.
> > > 
> > > I'd rather see a __maybe_unused annotation instead.
> > 
> > Agreed that it's a better form but at this point it's probably revert
> > the breaking change and move to that later on. Otherwise, I don't see
> > reason not to include the patch that you authored to the release. I've
> > used it in my test kernels for quite some time now and it has worked
> > without issues.
> > 
> > I sent my fix for review now.
> 
> A warning with some kconfigs is very minor, we can take the time to
> fix it properly for 4.6.

I don't see any problem adding those too ifdefs back for 4.5 release.

> I am surprised the 0day -next builds/etc didn't notice this - Jarkko is
> your tree included in that process somehow? (sorry, I don't remember
> the details)

It has been and I've received notifications from there from time to
time about my master branch and fixed issues accordingly.

> 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
Jerry Snitselaar Feb. 25, 2016, 5:30 p.m. UTC | #7
On Mon Feb 22 16, Jarkko Sakkinen wrote:
>On Mon, Feb 22, 2016 at 12:56:53PM +1100, James Morris wrote:
>> On Sat, 20 Feb 2016, Jarkko Sakkinen wrote:
>>
>> > Hi James,
>> >
>> > I'm sorry for the late pull request for 4.5. The reason for this was
>> > the latency in my previous one. I picked with care the absolutely
>> > critical fixes so that we can make a sound tpmdd release.
>> >
>> > I really hope you can still pick these as one of them is absolutely
>> > critical to get authorization policy sealing API right (kernel keeps
>> > it finger out of user space created objects).
>>
>> Pushed to next for more testing and review.
>>
>> This really is getting too late in the development cycle for so many
>> fixes.  It means the code was not ready to be merged in the first place.
>
>I fully agree what you're saying. I'll learn the lesson here and take
>factors more conservative attitude from now on. No excuses. I'm sorry
>about this.
>
>Partly the reason for recent increase in regressions has been
>increased real-world use of TPM2 and thus issues have started to pop
>up that's a lame excuse anyway.
>

Would it be worthwhile to have a tpm branch that gets pulled by -next
directly so changes will have already been going through the paces in
-next prior to the pull reuqest to James?

snits

------------------------------------------------------------------------------
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
James Morris Feb. 26, 2016, 3:38 a.m. UTC | #8
On Thu, 25 Feb 2016, Jerry Snitselaar wrote:

> On Mon Feb 22 16, Jarkko Sakkinen wrote:
> >On Mon, Feb 22, 2016 at 12:56:53PM +1100, James Morris wrote:
> > > On Sat, 20 Feb 2016, Jarkko Sakkinen wrote:
> > >
> > > > Hi James,
> > > >
> > > > I'm sorry for the late pull request for 4.5. The reason for this was
> > > > the latency in my previous one. I picked with care the absolutely
> > > > critical fixes so that we can make a sound tpmdd release.
> > > >
> > > > I really hope you can still pick these as one of them is absolutely
> > > > critical to get authorization policy sealing API right (kernel keeps
> > > > it finger out of user space created objects).
> > >
> > > Pushed to next for more testing and review.
> > >
> > > This really is getting too late in the development cycle for so many
> > > fixes.  It means the code was not ready to be merged in the first place.
> >
> >I fully agree what you're saying. I'll learn the lesson here and take
> >factors more conservative attitude from now on. No excuses. I'm sorry
> >about this.
> >
> >Partly the reason for recent increase in regressions has been
> >increased real-world use of TPM2 and thus issues have started to pop
> >up that's a lame excuse anyway.
> >
> 
> Would it be worthwhile to have a tpm branch that gets pulled by -next
> directly so changes will have already been going through the paces in
> -next prior to the pull reuqest to James?

That would be useful, some other subsystems do that.
James Morris Feb. 26, 2016, 7:57 a.m. UTC | #9
On Mon, 22 Feb 2016, Jarkko Sakkinen wrote:

> Do you want me to send a pull request containing a fix for the build
> warning or reverting the whole commit? My call would be to apply the
> fix because this commit has been tested both TPM 1.2 by Martin and
> with TPM 2.0 by me and things have worked well.
> 

Send me a pull request just for the fix.

I won't be pushing these changes to Linus for 4.5, they'll have to wait 
until the 4.6.
Jarkko Sakkinen Feb. 26, 2016, 9 a.m. UTC | #10
On Fri, Feb 26, 2016 at 06:57:49PM +1100, James Morris wrote:
> On Mon, 22 Feb 2016, Jarkko Sakkinen wrote:
> 
> > Do you want me to send a pull request containing a fix for the build
> > warning or reverting the whole commit? My call would be to apply the
> > fix because this commit has been tested both TPM 1.2 by Martin and
> > with TPM 2.0 by me and things have worked well.
> > 
> 
> Send me a pull request just for the fix.
> 
> I won't be pushing these changes to Linus for 4.5, they'll have to wait 
> until the 4.6.

OK

> -- 
> James Morris
> <jmorris@namei.org>

/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
Jarkko Sakkinen Feb. 26, 2016, 9:48 a.m. UTC | #11
On Fri, Feb 26, 2016 at 02:38:52PM +1100, James Morris wrote:
> On Thu, 25 Feb 2016, Jerry Snitselaar wrote:
> 
> > On Mon Feb 22 16, Jarkko Sakkinen wrote:
> > >On Mon, Feb 22, 2016 at 12:56:53PM +1100, James Morris wrote:
> > > > On Sat, 20 Feb 2016, Jarkko Sakkinen wrote:
> > > >
> > > > > Hi James,
> > > > >
> > > > > I'm sorry for the late pull request for 4.5. The reason for this was
> > > > > the latency in my previous one. I picked with care the absolutely
> > > > > critical fixes so that we can make a sound tpmdd release.
> > > > >
> > > > > I really hope you can still pick these as one of them is absolutely
> > > > > critical to get authorization policy sealing API right (kernel keeps
> > > > > it finger out of user space created objects).
> > > >
> > > > Pushed to next for more testing and review.
> > > >
> > > > This really is getting too late in the development cycle for so many
> > > > fixes.  It means the code was not ready to be merged in the first place.
> > >
> > >I fully agree what you're saying. I'll learn the lesson here and take
> > >factors more conservative attitude from now on. No excuses. I'm sorry
> > >about this.
> > >
> > >Partly the reason for recent increase in regressions has been
> > >increased real-world use of TPM2 and thus issues have started to pop
> > >up that's a lame excuse anyway.
> > >
> > 
> > Would it be worthwhile to have a tpm branch that gets pulled by -next
> > directly so changes will have already been going through the paces in
> > -next prior to the pull reuqest to James?
> 
> That would be useful, some other subsystems do that.

I'll start using it.

> -- 
> James Morris
> <jmorris@namei.org>

/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