Re: [RFC PATCH 3/4] Implement driver for supporting multiple emulated TPMs

[Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

On Tue, Jan 26, 2016 at 06:06:17PM -0800, Jarkko Sakkinen wrote:
> On Tue, Jan 26, 2016 at 10:58:16AM -0700, Jason Gunthorpe wrote:
> > On Tue, Jan 26, 2016 at 05:56:59AM -0800, Jarkko Sakkinen wrote:
> > > On Mon, Jan 25, 2016 at 08:19:19PM -0700, Jason Gunthorpe wrote:
> > > > On Mon, Jan 25, 2016 at 05:46:52PM -0800, Jarkko Sakkinen wrote:
> > > > > > The only issue is error unwind, the tpmm version assumes devm works
> > > > > > for that, but the vtpm_dev will not be destroyed if the tpm attach
> > > > > > fails, do devm is not appropriate.
> > > > > 
> > > > > At the moment tpmm_chip_alloc() does not use devm for anything. Are you
> > > > > speaking about stuff that happens between alloc and register as some of
> > > > > the drivers use devm to associate resources to pdev at that point?
> > > > 
> > > > Ugh, well I missed that when you made those patches.
> > > > 
> > > > You need to put the devm back ASAP, as it is all the drivers now have
> > > > broken error handling. eg
> > > > 
> > > > static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
> > > >                         acpi_handle acpi_dev_handle)
> > > > [..]
> > > >         chip = tpmm_chip_alloc(dev, &tpm_tis);
> > > > [..]
> > > >         if (IS_ERR(chip))
> > > > 	        return PTR_ERR(chip);
> > > > 			
> > > > Obviously leaks chip.
> > > 
> > > Could you describe how it leaks a chip and what do you mean by verb
> > > "leak"?
> > 
> > It looks like you alread figured this out. release is never called, so
> > the kalloc for chip is never freed.
> > 
> > > > The function is called tpmm_ specifically because it is supposed to
> > > > use devm resource unwind, it was a big mistake to remove the devm and
> > > > not rename the function or update the comments.
> > > 
> > > The function does not reserve any resources using devm. Should it?
> > 
> > It originally did, but commit 313d21eeab9282e01fdcecd40e9ca87e0953627f
> > took it out and didn't change the function name.
> > 
> > The original version looked like:
> > 
> > struct tpm_chip *tpmm_chip_alloc(struct device *dev,
> >                                  const struct tpm_class_ops *ops)
> > {
> > [..]
> >         devm_add_action(dev, tpmm_chip_remove, chip);
> >         dev_set_drvdata(dev, chip);
> > 
> >         return chip;
> > }
> > 
> > > > With the change to use a struct device the devm action should have
> > > > been a device_put to pair with the device_initialize.
> > > 
> > > Well, I found a regression that is luckily a rare one.
> > 
> > It isn't rare, any error path will cause this.
> > 
> > > So now I do have regression that does show up. If you don't have it,
> > > it does not make sense to "fix" the code even if it looks obviously
> > > wrong.
> > 
> > That isn't the kernel way, this is very obviosly a bug, I don't need
> > to see 'proof' to know it needs fixing.

I have to defend my position here. Of course you have to proof in a way
or another that there is a regresssion. Otherwise you cannot speak that
there is a bug. It's by definition like that. I need to see the
regression in order to apply a bug fix to the tpmdd tree.

/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=267308311&iu=/4140