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

Groups > linux.kernel > #1494539 > unrolled thread

[PATCH] tpm: don't destroy chip device prematurely

Back to article view | Back to linux.kernel

Contents

  [PATCH] tpm: don't destroy chip device prematurely Tomas Winkler <tomas.winkler@intel.com> - 2016-10-02 09:50 +0200
    Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-02 12:20 +0200
      Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-02 12:30 +0200
        Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-02 23:30 +0200
          RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-03 09:10 +0200
            RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-03 09:40 +0200
              Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-03 14:50 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-03 18:10 +0200
                  RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-03 19:40 +0200
            Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-03 14:50 +0200
              Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-04 07:30 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-04 18:50 +0200
                  RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-05 00:00 +0200
                    Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-05 01:20 +0200
                      RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-05 09:50 +0200
                        Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-05 17:20 +0200
                          Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-05 18:40 +0200
                        Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-05 19:20 +0200
                          RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-05 22:10 +0200
                            Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-05 23:20 +0200
                              RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-06 02:50 +0200
                                Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-06 04:10 +0200
                                  RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-07 16:30 +0200
                                    Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-07 21:20 +0200
                                      RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-07 22:20 +0200
                                    Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-08 12:50 +0200
                  Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-05 12:10 +0200
                    Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-05 18:30 +0200
                      Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-06 13:30 +0200
                        Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-06 18:30 +0200
                          Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-06 18:50 +0200
                  Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-05 12:10 +0200
            Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-03 18:10 +0200
              RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-03 19:20 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-03 19:40 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1496128

> On Wed, Oct 05, 2016 at 08:09:17PM +0000, Winkler, Tomas wrote:
> 
> > It could, but that patch was not merged yet, and I believe even if the
> > issue is exposed only with runtime_pm currently, we have a bug in
> > design even w/o runtime pm.
> 
> Please don't make changes without any justification :(

> 
> > > These are all fine, obviously. Todays kernel retains those values
> > > across device_del and we set those values in tpmm_chip_alloc/etc. So
> > > correct values are present as long as the chip memory exists. tpm
> > > continues to hold a kref on the chip so the memory will be around.
> >
> > I'm not saying they are not, but calling deep into the tpm stack and
> > even to the parent device with unutilized device is not sane.
> 
> You keep asserting that, but it just isn't true at all.

Okay, let's rephrase, that calling device_del before tpm_transmit is not sane when using runtime_pm.

> For a long time the tpm subsystem didn't even have a 'struct device'. That is
> something Jarkko and I added.


> The *ONLY* thing it does is act as the anchor for user space - eg it holds the
> sysfs, contains the 'dev' file for the cdev, etc, etc. This was an important clean
> up.
> 
> Internally to the tpm core, and the drivers, the chip->dev does
> *NOTHING* except hold the few variables you pointed out. That is it.
> 
> We could go back to the old code, without the 'dev' and things could still work
> correctly.

You can write the code in one big loop in assembly and it would work as well, this is not the point.

> This is why your assertion the struct device needs to be registered makes no
> sense.

But you did change the code to get more benefits but this also comes with more dependencies and new rules.
I also can go the simple way and go_idle, cmd_ready callbacks and nothing will breaks, but we wanted all those goodies that runtime_pm has (autosuspend and sysfs), but the feature  has longer roots. 


> If the runtime_pm patches change this, then we have a very serious problem.
> Removing this assumption is much harder than a one line patch moving
> device_del.

I'm just saying that the  runtime patches only exposed issue in the design.
 
> I actually have no idea how you'd do it, since we call all sorts of tpm ops
> between device_init and device_add - again device_del is the least of the
> problems if runtime pm insists the chip->dev be registered when running
> transmit_cmd.

This has to be revisited as well. 

> 
> So, I again, strongly advise you to give up on this idea, it is too hard for TPM,
> and does not seem technically needed at this time. Even it it does seem to
> make some kind of intuitive sense.

I'm the last one who want to do extra jobs, we need to make our hw working asap, but we need to do it right.

> > > Is there some other PM path where dev->parent becomes invovled?
> >
> > Of course, the power management utilize the device hierarch, it
> > assumes there is power dependencies between parents and child devices,
> > such as bus controllers and the devices on that bus.
> 
> Sure, but that relationship only maters if something does a PM call on the
> chip->dev, and AFAIK, nothing does that.
> 
> Do you know differently?

You've pasted that code in in the previous mail, parent is involved on device remove.

> 
> You pointed at something that isn't even run and said it is the source of the
> problem.. You really need to set up here and explain exactly what is
> happening.

Sorry, lost you here.
 
> > > Are you just guessing this solves a problem, or were you able to
> > > reproduce Jarkko's report?
> >
> > No, guesses are not my style :), this solves the issue, as you see
> > this was also validated by Jarrko on his setup.
> 
> In the thread you pointed to Jarkko said he could not reproduce the original
> issue. Jarkko can you clarify??

No, he rolled back the runtime_pm patch and the issue disappeared and that's how we found the root cause. 
 
> > > Even if that pm_runtime_put is happening, why doesn't the
> > >
> > > +    pm_runtime_get_sync(chip->dev.parent);
> > >
> > > The runtime_pm patch adds to tpm_transmit take care of bringing the
> > > device back?
> >
> > Unfortunately not because, because it gets out of sync and what is
> > actually happening is that idle callback is called and device is put
> > to idle between send and receive.
> 
> What? As far as I understand this PM stuff, I would call that a very serious bug.

Maybe, but then you have to find what a bug,  currently it looks like wrong usage of the device.
> 

> If a PM transition during transmit_cmd causes the TPM to abort/fail command
> execution then it *MUST* be prevented. Period.

Or, we can call device_del after tpm2_shutdown. 
 
> pm_runtime_get_sync appears to be the correct thing to get the guarentee, so
> I'm very confused by your statement.
> 
> > Now we can find a trick to fix this, but this would be rather w/o
> > while we know what the real issue is.
> 
> don't understand this.. Are you saying that going into idle during tpm_transmit
> is not a bug?

> Sounds like there is some sort of race condition with the pm stuff that needs
> fixing.

> I still don't see what your actual bug is, other than what I already knew -
> somehow PM causes transmit_cmd to fail.

I will send you the actual trace, anyhow I've respin the original version with go_idle and cmd_ready handlers, this is contra productive, the time is just not right. 

Thanks
Tomas


 
Tomas 

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

#1496138

On Thu, Oct 06, 2016 at 12:43:13AM +0000, Winkler, Tomas wrote:

> > You keep asserting that, but it just isn't true at all.
> 
> Okay, let's rephrase, that calling device_del before tpm_transmit is not sane when using runtime_pm.

Maybe, but I haven't heard an explanation from you why that is the case,
and I haven't found one on my own..

> > Sure, but that relationship only maters if something does a PM call on the
> > chip->dev, and AFAIK, nothing does that.
> > 
> > Do you know differently?
> 
> You've pasted that code in in the previous mail, parent is involved on device remove.

I pasted the code to show it didn't seem possible to hit it because
irq_safe should not be true for chip->dev. You never explained how
this code can run.

> > You pointed at something that isn't even run and said it is the source of the
> > problem.. You really need to set up here and explain exactly what is
> > happening.
> 
> Sorry, lost you here.

You haven't done a good job explaining the problem in detail beyond some general
blame toward PM.

> > > > Even if that pm_runtime_put is happening, why doesn't the
> > > >
> > > > +    pm_runtime_get_sync(chip->dev.parent);
> > > >
> > > > The runtime_pm patch adds to tpm_transmit take care of bringing the
> > > > device back?
> > >
> > > Unfortunately not because, because it gets out of sync and what is
> > > actually happening is that idle callback is called and device is put
> > > to idle between send and receive.
> > 
> > What? As far as I understand this PM stuff, I would call that a very serious bug.
> 
> Maybe, but then you have to find what a bug,  currently it looks
> like wrong usage of the device.

Yes, you actually have to find and explain the bug to fix it.

You still haven't explained at all why device_del on the child causes
pm_runtime_get_sync() on the parent to malfunction. There is certainly
seems to be nothing intrinsic about the PM core that would cause that.

*That* is the really critical bit of explanation that is missing.

Until you can provide it there is no reason to continue discussing.

> > If a PM transition during transmit_cmd causes the TPM to abort/fail command
> > execution then it *MUST* be prevented. Period.
> 
> Or, we can call device_del after tpm2_shutdown. 

What? You haven't even established root cause, how do you know this
bug won't manifest in other cases? It could very well be some kind of
generic race-bug triggered by the proximity of device_del and
pm_runtime_get_sync. Or a HW bug of some kind..

> I will send you the actual trace, anyhow I've respin the original
> version with go_idle and cmd_ready handlers, this is contra
> productive, the time is just not right.

I'm deeply skeptical about all your patches if you can't root-cause
identify why the existing implementation isn't working.

But, you can sort out with Jarkko what to do with crb.

As long at the rest of the drivers and the core subsystem are not
broken by the device_del change. Jarkko - can you confirm you will
drop that patch?

Jason

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

#1497182

> On Thu, Oct 06, 2016 at 12:43:13AM +0000, Winkler, Tomas wrote:
> 
> > > You keep asserting that, but it just isn't true at all.
> >
> > Okay, let's rephrase, that calling device_del before tpm_transmit is not sane
> when using runtime_pm.
> 
> Maybe, but I haven't heard an explanation from you why that is the case, and I
> haven't found one on my own..
> 
> > > Sure, but that relationship only maters if something does a PM call
> > > on the
> > > chip->dev, and AFAIK, nothing does that.
> > >
> > > Do you know differently?
> >
> > You've pasted that code in in the previous mail, parent is involved on device
> remove.
> 
> I pasted the code to show it didn't seem possible to hit it because irq_safe
> should not be true for chip->dev. You never explained how this code can run.
> 
> > > You pointed at something that isn't even run and said it is the
> > > source of the problem.. You really need to set up here and explain
> > > exactly what is happening.
> >
> > Sorry, lost you here.
> 
> You haven't done a good job explaining the problem in detail beyond some
> general blame toward PM.
> 
> > > > > Even if that pm_runtime_put is happening, why doesn't the
> > > > >
> > > > > +    pm_runtime_get_sync(chip->dev.parent);
> > > > >
> > > > > The runtime_pm patch adds to tpm_transmit take care of bringing
> > > > > the device back?
> > > >
> > > > Unfortunately not because, because it gets out of sync and what is
> > > > actually happening is that idle callback is called and device is
> > > > put to idle between send and receive.
> > >
> > > What? As far as I understand this PM stuff, I would call that a very serious
> bug.
> >
> > Maybe, but then you have to find what a bug,  currently it looks like
> > wrong usage of the device.
> 
> Yes, you actually have to find and explain the bug to fix it.
> 
> You still haven't explained at all why device_del on the child causes
> pm_runtime_get_sync() on the parent to malfunction. There is certainly seems
> to be nothing intrinsic about the PM core that would cause that.
> 
> *That* is the really critical bit of explanation that is missing.
> 
> Until you can provide it there is no reason to continue discussing.
> 
> > > If a PM transition during transmit_cmd causes the TPM to abort/fail
> > > command execution then it *MUST* be prevented. Period.
> >
> > Or, we can call device_del after tpm2_shutdown.
> 
> What? You haven't even established root cause, how do you know this bug
> won't manifest in other cases? It could very well be some kind of generic race-
> bug triggered by the proximity of device_del and pm_runtime_get_sync. Or a
> HW bug of some kind..
> 
> > I will send you the actual trace, anyhow I've respin the original
> > version with go_idle and cmd_ready handlers, this is contra
> > productive, the time is just not right.
> 
> I'm deeply skeptical about all your patches if you can't root-cause identify why
> the existing implementation isn't working.
> 
> But, you can sort out with Jarkko what to do with crb.
> 
> As long at the rest of the drivers and the core subsystem are not broken by the
> device_del change. Jarkko - can you confirm you will drop that patch?

So  here I'm to say I'm sorry for misleading this, after all the doubts I got back to debugging and traces. 
One thing for a reason moving the device_del, had really made the problem go away,  but the real  problem was unbalance runtime_pm PUT/GET from the tpm_crb probe function. 
I will post the fixed patch, of course, this one should be dropped. 
In any case,  and this is not just to keep my ego up, that calling to the tom stack with unutilized dev is not healthy and we should look for that. 
Thanks
Tomas

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

#1497479

On Fri, Oct 07, 2016 at 02:24:59PM +0000, Winkler, Tomas wrote:

> So here I'm to say I'm sorry for misleading this, after all the
> doubts I got back to debugging and traces.  One thing for a reason
> moving the device_del, had really made the problem go away, but the
> real problem was unbalance runtime_pm PUT/GET from the tpm_crb probe
> function.

Oh this is very good news, I'm glad this was resolved in crb!

Presumably the unbalanced put made the ref count go negative and the
balanced get caused it to go to zero, so pm locking was basically totally
broken? That would explain how an idle callback could run
concurrently with transmit_cmd.

Though a bit of a mystery why device_del had any impact? I'm still
very unclear exactly how the child device effects the parent - and
that seems like pretty important information going forward..

Thanks,
Jason

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

#1497497

> Subject: Re: [PATCH] tpm: don't destroy chip device prematurely
> 
> On Fri, Oct 07, 2016 at 02:24:59PM +0000, Winkler, Tomas wrote:
> 
> > So here I'm to say I'm sorry for misleading this, after all the doubts
> > I got back to debugging and traces.  One thing for a reason moving the
> > device_del, had really made the problem go away, but the real problem
> > was unbalance runtime_pm PUT/GET from the tpm_crb probe function.
> 
> Oh this is very good news, I'm glad this was resolved in crb!
> 
> Presumably the unbalanced put made the ref count go negative and the
> balanced get caused it to go to zero, so pm locking was basically totally
> broken? That would explain how an idle callback could run concurrently with
> transmit_cmd.

This is not due to locking and refcount, but similar. The usage_count went negative  and the idle callback kicked in from the pm work queue, and suspended the device.

> 
> Though a bit of a mystery why device_del had any impact? I'm still very
> unclear exactly how the child device effects the parent - and that seems like
> pretty important information going forward..

Yes, there is some dependency as if device_del is not called the idle callback doesn't kick in between send and receive and that was misleading.  I'm not sure but this could be due to scheduling of the pm worker, but I'm not sure.   In any case we hit the issue even w/o device_del if the device is exercise enough.  I will dig into that later.

Thanks
Tomas

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

#1497704

On Fri, Oct 07, 2016 at 02:24:59PM +0000, Winkler, Tomas wrote:
> So  here I'm to say I'm sorry for misleading this, after all the
> doubts I got back to debugging and traces.  One thing for a reason
> moving the device_del, had really made the problem go away,  but the
> real  problem was unbalance runtime_pm PUT/GET from the tpm_crb probe
> function.  I will post the fixed patch, of course, this one should be
> dropped.  In any case,  and this is not just to keep my ego up, that
> calling to the tom stack with unutilized dev is not healthy and we
> should look for that. 

Great. I'll test the fix once it's available.

/Jarkko

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

#1495790

On Tue, Oct 04, 2016 at 10:47:38AM -0600, Jason Gunthorpe wrote:
> On Tue, Oct 04, 2016 at 08:19:46AM +0300, Jarkko Sakkinen wrote:
> 
> > > Make the driver uncallable first. The worst race that can happen is that
> > > open("/dev/tpm0", ...) returns -EPIPE. I do not consider this fatal at
> > > all.
> > 
> > No responses for this reasonable proposal so I'll show what I mean:
> 
> How is this any better than what Thomas proposed? It seems much worse
> to me since now we have even more stuff in the wrong order.

It moves a logical block to the front instead of moving one thing
from one logical block to another place.

I'll repeat my question: what worse can happen than returning -EPIPE?  I
though the whole rw lock scheme was introduced just for this purpose.

Why there's even that branch in tpm-dev.c if it's so bad to let it
happen?

/Jarkko

> There are three purposes to the ordering as it stands today
>  1) To guarantee that tpm2_shutdown is the last command delivered to
>     the TPM. When it is issued all other ways to access the device
>     are hard fenced off.
>  2) To hard fence the tpm subsystem for the 'platform' driver. Once
>     tpm_del_char_device completes no callback into the driver
>     is possible *at all*. The driver can destroy everything
>     (iounmap, dereg irq, etc) and the driver module can be unloaded.
>  3) To prevent oopsing with the sysfs code. Recall this comment
> 
>         /* The sysfs routines rely on an implicit tpm_try_get_ops, device_del
>          * is called before ops is null'd and the sysfs core synchronizes this
>          * removal so that no callbacks are running or can run again
>          */
> 
>     device_del is what eliminates the sysfs access path, so
>     ordering device_del after ops = null is just unconditionally
>     wrong.
> 
> I still haven't heard an explanation why Thomas's other patches need
> this, or why trying to change this ordering makes any sense at
> all considering how the subsystem is constructed.
> 
> Further, if tpm_crb now needs a registered device, how on earth do all
> the chip ops we call work *before* registration? Or is that another
> bug?
> 
> Why can't tpm_crb return to the pre-registration operating state
> in the driver remove function before calling unregister?
> 
> None of this makes any sense to me.
> 
> This whole thing was very carefully constructed to work *correctly*
> during unregister. Many other subsystems have races and bugs during
> remove (eg see the securityfs discussion). TPM has a hard requirement
> to support safe unregister due to the vtpm stuff, so we don't get to
> screw it up just to support one driver.
> 
> Jason

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

#1495955

On Wed, Oct 05, 2016 at 01:02:34PM +0300, Jarkko Sakkinen wrote:

> I'll repeat my question: what worse can happen than returning -EPIPE?  I
> though the whole rw lock scheme was introduced just for this purpose.

I thought I explained this, if device_del is moved after ops = null
then if sysfs looses the race it will oops the kernel. device_del hard
fences sysfs.

> Why there's even that branch in tpm-dev.c if it's so bad to let it
> happen?

Because cdev_del and device_del do not guarentee that the cdev is
fenced. They just prevent new calls into open(). So the branch in
tpm-dev.c is necessary to avoid a kernel oops if user space holds the
fd open across unregister.

It is the same sitatuion you identified in the securityfs discussion -
user space holding the fd open across a driver unregister.

Jason

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

#1496603

On Wed, Oct 05, 2016 at 10:27:41AM -0600, Jason Gunthorpe wrote:
> On Wed, Oct 05, 2016 at 01:02:34PM +0300, Jarkko Sakkinen wrote:
> 
> > I'll repeat my question: what worse can happen than returning -EPIPE?  I
> > though the whole rw lock scheme was introduced just for this purpose.
> 
> I thought I explained this, if device_del is moved after ops = null
> then if sysfs looses the race it will oops the kernel. device_del hard
> fences sysfs.

Sorry, I missed that comment somehow. Looking at the code it is like
that.

I think that they should be fenced then for the sake of consistency.
I do not see why sysfs code is privileged not to do fencing while other
peers have to do it.

/Jarkko

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

#1496749

On Thu, Oct 06, 2016 at 02:23:57PM +0300, Jarkko Sakkinen wrote:

> I think that they should be fenced then for the sake of consistency.
> I do not see why sysfs code is privileged not to do fencing while other
> peers have to do it.

Certainly the locking could be changed, but it would be nice to have a
reason other than aesthetics.

sysfs is not unique, we also do not grab the rwlock lock during any
commands executed as part of probe. There are basically two locking
regimes - stuff that is proven to by synchronous with probe/remove
(sysfs, probe cmds) and everything else (kapi, cdev)

Further, the current sysfs implementation is nice and sane: the file
accesses cannot fail with ENODEV. That is a useful concrete property
and I don't think we should change it without a good reason.

Jason

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

#1496765

On Thu, Oct 06, 2016 at 10:22:45AM -0600, Jason Gunthorpe wrote:
> On Thu, Oct 06, 2016 at 02:23:57PM +0300, Jarkko Sakkinen wrote:
> 
> > I think that they should be fenced then for the sake of consistency.
> > I do not see why sysfs code is privileged not to do fencing while other
> > peers have to do it.
> 
> Certainly the locking could be changed, but it would be nice to have a
> reason other than aesthetics.
> 
> sysfs is not unique, we also do not grab the rwlock lock during any
> commands executed as part of probe. There are basically two locking
> regimes - stuff that is proven to by synchronous with probe/remove
> (sysfs, probe cmds) and everything else (kapi, cdev)
> 
> Further, the current sysfs implementation is nice and sane: the file
> accesses cannot fail with ENODEV. That is a useful concrete property
> and I don't think we should change it without a good reason.

The last point is certainly legit. I think it even might deserve a
comment of its own in tpm_del_char_device.

I think I have a good idea now what to do. Hold on for RFC patch.

> Jason

/Jarkko

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

#1495792

On Tue, Oct 04, 2016 at 10:47:38AM -0600, Jason Gunthorpe wrote:
> On Tue, Oct 04, 2016 at 08:19:46AM +0300, Jarkko Sakkinen wrote:
> 
> > > Make the driver uncallable first. The worst race that can happen is that
> > > open("/dev/tpm0", ...) returns -EPIPE. I do not consider this fatal at
> > > all.
> > 
> > No responses for this reasonable proposal so I'll show what I mean:
> 
> How is this any better than what Thomas proposed? It seems much worse
> to me since now we have even more stuff in the wrong order.
> 
> There are three purposes to the ordering as it stands today
>  1) To guarantee that tpm2_shutdown is the last command delivered to
>     the TPM. When it is issued all other ways to access the device
>     are hard fenced off.
>  2) To hard fence the tpm subsystem for the 'platform' driver. Once
>     tpm_del_char_device completes no callback into the driver
>     is possible *at all*. The driver can destroy everything
>     (iounmap, dereg irq, etc) and the driver module can be unloaded.
>  3) To prevent oopsing with the sysfs code. Recall this comment
> 
>         /* The sysfs routines rely on an implicit tpm_try_get_ops, device_del
>          * is called before ops is null'd and the sysfs core synchronizes this
>          * removal so that no callbacks are running or can run again
>          */
> 
>     device_del is what eliminates the sysfs access path, so
>     ordering device_del after ops = null is just unconditionally
>     wrong.
> 
> I still haven't heard an explanation why Thomas's other patches need
> this, or why trying to change this ordering makes any sense at
> all considering how the subsystem is constructed.
> 
> Further, if tpm_crb now needs a registered device, how on earth do all
> the chip ops we call work *before* registration? Or is that another
> bug?
> 
> Why can't tpm_crb return to the pre-registration operating state
> in the driver remove function before calling unregister?
> 
> None of this makes any sense to me.
> 
> This whole thing was very carefully constructed to work *correctly*
> during unregister. Many other subsystems have races and bugs during
> remove (eg see the securityfs discussion). TPM has a hard requirement
> to support safe unregister due to the vtpm stuff, so we don't get to
> screw it up just to support one driver.

Obviously a device is needed because it's required by the PM runtime
FW. I'm not following what you're saying about tpm2_shutdown(). With
the change I proposed it's the *very last* command delivered to the
device (because it's fenced by write lock).

> Jason

/Jarkko

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

#1494943

On Mon, Oct 03, 2016 at 07:05:48AM +0000, Winkler, Tomas wrote:

> > This patch is wrong, I though the comments were clear. All entry points to find
> > the device must be deleted before we commit to shutting down the device.
> > 
> > You need to figure out some other way to solve your problem.
> 
> Please be more specific regarding flows you think will be wrong with
> this patch, you must agree that the current code is broken even w/o
> runtime pm.

No, I don't agree. Accessing dev->name is OK after the device_del.

What devicde_del does is fence off all sorts of ways to access the
device, eg sysfs files, bus registrations, etc, etc. That absolutely
must be done before calling tpm_suspend.

Jason

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

#1494976

 
> On Mon, Oct 03, 2016 at 07:05:48AM +0000, Winkler, Tomas wrote:
> 
> > > This patch is wrong, I though the comments were clear. All entry
> > > points to find the device must be deleted before we commit to shutting
> down the device.
> > >
> > > You need to figure out some other way to solve your problem.
> >
> > Please be more specific regarding flows you think will be wrong with
> > this patch, you must agree that the current code is broken even w/o
> > runtime pm.
> 
> No, I don't agree. Accessing dev->name is OK after the device_del.

But you cannot assume that just dev->name is accessed and and runtime_pm breaks this assumption.. 
>
> 
> What devicde_del does is fence off all sorts of ways to access the device, eg
> sysfs files, bus registrations, etc, etc. That absolutely must be done before
> calling tpm_suspend.


But tpm2_shutdown is acceded via tpm_chip , we cannot call device_del before, this is just wrong from the Linux device mode perspective, we have to use other means to close the access to the device.

Thanks
Tomas

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

#1494981

On Mon, Oct 03, 2016 at 05:16:18PM +0000, Winkler, Tomas wrote:

> > > Please be more specific regarding flows you think will be wrong with
> > > this patch, you must agree that the current code is broken even w/o
> > > runtime pm.
> > 
> > No, I don't agree. Accessing dev->name is OK after the device_del.
> 
> But you cannot assume that just dev->name is accessed and and
> runtime_pm breaks this assumption..

It was built around that assumption.

Our chip methods have been built to not require the device to be
registered. We call many of them before we even do device
registration, for instance.

The pm patches can't break that. What is the actual problem anyhow?

> > What devicde_del does is fence off all sorts of ways to access the device, eg
> > sysfs files, bus registrations, etc, etc. That absolutely must be done before
> > calling tpm_suspend.
> 
> But tpm2_shutdown is acceded via tpm_chip , we cannot call
> device_del before, this is just wrong from the Linux device mode
> perspective, we have to use other means to close the access to the
> device.

device_del is the means to close access - that it what it does -
unregister the device from the system. The tpm_chip must be
operational independent of a *registered* device. chip methods can
only assume that the device is *initialized*

This is a basic pattern followed by other subsystems.

This is why it is OK to look at dev->name, but not okay to muck with
other stuff under a chip method.

Jason

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web