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

From	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Newsgroups	linux.kernel
Subject	Re: [PATCH] tpm: don't destroy chip device prematurely
Date	2016-10-05 12:10 +0200
Message-ID	<soRjb-7Pg-1@gated-at.bofh.it> (permalink)
References	(3 earlier) <snWuB-3G8-11@gated-at.bofh.it> <so5xT-Vq-1@gated-at.bofh.it> <soaQV-4YM-9@gated-at.bofh.it> <soqsF-6wU-1@gated-at.bofh.it> <soB4J-5is-11@gated-at.bofh.it>
Organization	Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

Show all headers | View raw

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

Thread

[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

csiph-web