Re: [PATCH] tpm: vtpm_proxy: Do not run tpm2_shutdown

[Stefan Berger]

On 05/25/2017 06:33 PM, Jarkko Sakkinen wrote:
> On Thu, May 25, 2017 at 04:32:50PM -0400, Stefan Berger wrote:
> > On 05/25/2017 04:09 PM, Jason Gunthorpe wrote:
> > > On Thu, May 25, 2017 at 04:04:24PM -0400, Stefan Berger wrote:
> > > > On 05/25/2017 11:50 AM, Jason Gunthorpe wrote:
> > > > > On Thu, May 25, 2017 at 09:12:36AM -0400, Stefan Berger wrote:
> > > > > > The tpm2_shutdown does not work with the VTPM proxy driver since the
> > > > > > function only gets called when the backend file descriptor is already
> > > > > > closed and at this point no data can be sent anymore. A proper shutdown
> > > > > > would have to be initated by a user space application, such as a container
> > > > > > management stack, that sends the command via the character device before
> > > > > > terminating the TPM emulator.
> > > > > >
> > > > > > To avoid the tpm2_shutdown we introduce a TPM_CHIP_FLAG_NO_SHUTDOWN flag
> > > > > > that only the VTPM proxy driver sets. This also avoids misleading kernel
> > > > > > log messages.
> > > > > This seems strange to me..
> > > > >
> > > > > Why isn't ops null if the fd has gone away?
> > > > >
> > > > > What is the call flow that hits this?
> > > > In this function here.
> > > >
> > > > static void tpm_del_char_device(struct tpm_chip *chip)
> > > > {
> > > >       cdev_device_del(&chip->cdev, &chip->dev);
> > > >
> > > >       /* Make the chip unavailable. */
> > > >       mutex_lock(&idr_lock);
> > > >       idr_replace(&dev_nums_idr, NULL, chip->dev_num);
> > > >       mutex_unlock(&idr_lock);
> > > >
> > > >       /* Make the driver uncallable. */
> > > >       down_write(&chip->ops_sem);
> > > >       if (chip->flags & TPM_CHIP_FLAG_TPM2)
> > > >           tpm2_shutdown(chip, TPM2_SU_CLEAR);
> > > >       chip->ops = NULL;
> > > >       up_write(&chip->ops_sem);
> > > > }
> > > >
> > > > The request cannot be deliver because the anonymous fd has been closed
> > > > already.
> > > The driver must always be able to process requests until
> > > tpm_del_char_device completes, so this is triggering an existing bug
> > > in vtpm. This change in core behvior is not going to fix the bug.
> > >
> > > eg a request from sysfs/etc could come in between vtpm fd closure and
> > > tpm_del_char_device, and it still must be handled properly.
> > >
> > > I guess you need to have transmit command fail fast once the fd is
> > > closed.
> > It doesn't hang. Everything is torn down immediately. What is primarily
> > annoying are these two log messages:
> > tpm tpm0: tpm_transmit: tpm_send: error -32
> > tpm tpm0: transmit returned -32 while stopping the TPM
> >
> >
> >      Stefan
> It's been a while since I've looked into vtpm code. Why was fd closed
> before the above? I can go this through myself once I'm back in Finland

The tear-down only starts when the last file descriptor on /dev/tpm%d 
and the (anonymous) file descriptor on the backend was closed. The 
backend (TPM emulator) can also close before the frontend closes. So it 
can happen that there is no backend anymore that would process commands 
and that would cause error messages being logged for commands sent to 
it. For sure there is no more listener for the TPM2 Shutdown command.


> next week. Just have forgotten this detail and do not have time to study
> this right now.
>
> /Jarkko