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

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

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 ask one question. In earlier response to you stated that one
> should get rid of devm? How'd you fix this problem without devm and why
> you want to get rid of it in this scenario?

To get rid of devm each driver would have to be updated to have the
right goto-error unwind to device_put the chip. It is alot of tricky
work to do this, which is why I suggested the tpmm_ scheme when you
did the original two step alloc patches.

For drivers like Stefan's non-devm might be needed, I'm still not
clear on that to be honest. There are rules when devm can be used and
not, and using it outside a probe() function or similar is something
I'm not sure works..

Jason

------------------------------------------------------------------------------
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