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

Groups > linux.kernel > #1558754

Re: [tpmdd-devel] [PATCH RFC v2 5/5] tpm2: expose resource manager via a device link /dev/tpms<n>

Show all headers | View raw

On Fri, Jan 13, 2017 at 12:02:36PM -0800, James Bottomley wrote:
> > > Actually, no, the devrm is a completely lifetime managed device as
> > > part
> > > of the chip structure.  once you've done a device_del on it, it can
> > > be
> > > kfreed because it's no longer visible to anything else.
> > 
> > No, that isn't enough. Anything else could have obtained a kref on
> > devrm outside of the sphere the device_del manages.
> > 
> > For instance, the cdev does exactly that, via this:
> > 
> > >  	chip->cdev.kobj.parent = &chip->dev.kobj;
> > > +	chip->cdevrm.kobj.parent = &chip->devrm.kobj;
> > 
> > In the worst case the kref the cdev grabs is not released until after
> > tpm_chip_unregister() returns.
> 
> chip_unregister doesn't tear down either device. It's the final
> release of the chip->dev that does that.

I don't think you are seeing the problem.

I think you are assuming cdev_del waits for userspace to close any
open fds. It does not.

Instead cdev independently holds on to a module lock and a kref on the
chip, once userspace has done close() then the kref is dropped and the
module lock let go. This can happen after chip_unregister, after devm
has done the final put_device, and after the driver core has completed
driver detach.

This is why it is necessary for this:

 +	chip->cdevrm.kobj.parent = &chip->devrm.kobj;

To point to a working kref, such as chip->dev.kobj.

My point is this patch has two very subtle bugs caused by devrm having
a broken kref. Yes we can fix those two cases, but this entire class
of bugs is prevented if devrm has a release function that does
put_device(chip->dev).

We have no idea what will happen down the road; most poeple assume
krefs *work*, having a struct with 4 krefs where only 3 work is pretty
wild, IMHO.

> Now there is a related problem that the owner is actually the *wrong*
> module: it holds the tpm module in place not the actual driver module,
> so I can happily attach tcsd to the TPM device then rmmod tpm_tis,
> which causes some interesting issues.  I can fix this, but it's not a
> problem of the current patch.

No, it is correct as is. The cdev fops rely only on the tpm module.
When tpm_chip_unregister returns to the driver the chips->ops is set
to NULL with proper locking - the driver code becomes uncallable at
that point.

Jason

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH RFC v2 0/5] RFC: in-kernel resource manager Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-12 18:50 +0100
  [PATCH RFC v2 1/5] tpm: validate TPM 2.0 commands Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-12 18:50 +0100
    Re: [PATCH RFC v2 1/5] tpm: validate TPM 2.0 commands Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-12 21:40 +0100
  [PATCH RFC v2 3/5] tpm: infrastructure for TPM spaces Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-12 18:50 +0100
    Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM  spaces James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-12 19:50 +0100
      Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM  spaces Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-12 21:40 +0100
    Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM  spaces James Bottomley <jejb@linux.vnet.ibm.com> - 2017-01-12 21:40 +0100
      Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM  spaces Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-13 17:30 +0100
        Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM  spaces Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-16 11:00 +0100
    Re: [PATCH RFC v2 3/5] tpm: infrastructure for TPM spaces Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-12 22:00 +0100
    Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM  spaces James Bottomley <jejb@linux.vnet.ibm.com> - 2017-01-13 02:20 +0100
      Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM  spaces Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-13 17:40 +0100
      Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM  spaces Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-16 10:10 +0100
        Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM  spaces James Bottomley <jejb@linux.vnet.ibm.com> - 2017-01-16 15:30 +0100
          Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM  spaces Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-16 15:50 +0100
            Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM  spaces James Bottomley <jejb@linux.vnet.ibm.com> - 2017-01-16 16:00 +0100
              Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM  spaces Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-16 18:00 +0100
  [PATCH RFC v2 2/5] tpm: export tpm2_flush_context_cmd Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-12 18:50 +0100
  [PATCH RFC v2 5/5] tpm2: expose resource manager via a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-12 18:50 +0100
    Re: [PATCH RFC v2 5/5] tpm2: expose resource manager via a device  link /dev/tpms<n> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-12 19:50 +0100
      Re: [tpmdd-devel] [PATCH RFC v2 5/5] tpm2: expose resource manager  via a device link /dev/tpms<n> James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-13 20:30 +0100
        Re: [tpmdd-devel] [PATCH RFC v2 5/5] tpm2: expose resource manager  via a device link /dev/tpms<n> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-13 20:50 +0100
          Re: [tpmdd-devel] [PATCH RFC v2 5/5] tpm2: expose resource manager  via a device link /dev/tpms<n> James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-13 21:10 +0100
            Re: [tpmdd-devel] [PATCH RFC v2 5/5] tpm2: expose resource manager  via a device link /dev/tpms<n> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-13 22:30 +0100
              Re: [tpmdd-devel] [PATCH RFC v2 5/5] tpm2: expose resource manager  via a device link /dev/tpms<n> James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-14 02:20 +0100
                Re: [tpmdd-devel] [PATCH RFC v2 5/5] tpm2: expose resource manager  via a device link /dev/tpms<n> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-16 18:00 +0100
    Re: [tpmdd-devel] [PATCH RFC v2 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-12 21:00 +0100
    Re: [PATCH RFC v2 5/5] tpm2: expose resource manager via a device  link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-12 22:00 +0100
      Re: [PATCH RFC v2 5/5] tpm2: expose resource manager via a device  link /dev/tpms<n> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-13 18:30 +0100
        Re: [tpmdd-devel] [PATCH RFC v2 5/5] tpm2: expose resource manager  via a device link /dev/tpms<n> James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-13 18:50 +0100
          Re: [tpmdd-devel] [PATCH RFC v2 5/5] tpm2: expose resource manager  via a device link /dev/tpms<n> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-13 19:10 +0100
            Re: [tpmdd-devel] [PATCH RFC v2 5/5] tpm2: expose resource manager  via a device link /dev/tpms<n> James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-13 19:20 +0100
          Re: [tpmdd-devel] [PATCH RFC v2 5/5] tpm2: expose resource manager  via a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-16 10:50 +0100
  [PATCH RFC v2 4/5] tpm: split out tpm-dev.c into tpm-dev.c and tpm-common-dev.c Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-12 18:50 +0100
    Re: [tpmdd-devel] [PATCH RFC v2 4/5] tpm: split out tpm-dev.c into  tpm-dev.c and tpm-common-dev.c James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-13 20:20 +0100

csiph-web