On Fri, 2023-10-13 at 13:54 +0300, Ilpo Järvinen wrote:
> On Thu, 12 Oct 2023, David E. Box wrote:
>
> > On Thu, 2023-10-12 at 17:46 +0300, Ilpo Järvinen wrote:
> > > On Wed, 11 Oct 2023, David E. Box wrote:
> > >
> > > > Use cleanup.h helpers to handle cleanup of resources in
> > > > intel_vsec_add_dev() after failures.
> > > >
> > > > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> > > > ---
> > > > V3 - New patch.
> > > >
> > > > drivers/platform/x86/intel/vsec.c | 17 ++++++++++-------
> > > > 1 file changed, 10 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/platform/x86/intel/vsec.c
> > > > b/drivers/platform/x86/intel/vsec.c
> > > > index 15866b7d3117..f03ab89ab7c0 100644
> > > > --- a/drivers/platform/x86/intel/vsec.c
> > > > +++ b/drivers/platform/x86/intel/vsec.c
> > > > @@ -15,6 +15,7 @@
> > > >
> > > > #include <linux/auxiliary_bus.h>
> > > > #include <linux/bits.h>
> > > > +#include <linux/cleanup.h>
> > > > #include <linux/delay.h>
> > > > #include <linux/kernel.h>
> > > > #include <linux/idr.h>
> > > > @@ -155,10 +156,10 @@ EXPORT_SYMBOL_NS_GPL(intel_vsec_add_aux,
> > > > INTEL_VSEC);
> > > > static int intel_vsec_add_dev(struct pci_dev *pdev, struct
> > > > intel_vsec_header *header,
> > > > struct intel_vsec_platform_info *info)
> > > > {
> > > > - struct intel_vsec_device *intel_vsec_dev;
> > > > + struct intel_vsec_device __free(kfree) *intel_vsec_dev = NULL;
> > > > struct resource *res, *tmp;
> > > > unsigned long quirks = info->quirks;
> > > > - int i;
> > > > + int ret, i;
> > > >
> > > > if (!intel_vsec_supported(header->id, info->caps))
> > > > return -EINVAL;
> > > > @@ -178,10 +179,8 @@ static int intel_vsec_add_dev(struct pci_dev *pdev,
> > > > struct intel_vsec_header *he
> > > > return -ENOMEM;
> > > >
> > > > res = kcalloc(header->num_entries, sizeof(*res), GFP_KERNEL);
> > > > - if (!res) {
> > > > - kfree(intel_vsec_dev);
> > > > + if (!res)
> > > > return -ENOMEM;
> > > > - }
> > > >
> > > > if (quirks & VSEC_QUIRK_TABLE_SHIFT)
> > > > header->offset >>= TABLE_OFFSET_SHIFT;
> > > > @@ -208,8 +207,12 @@ static int intel_vsec_add_dev(struct pci_dev *pdev,
> > > > struct intel_vsec_header *he
> > > > else
> > > > intel_vsec_dev->ida = &intel_vsec_ida;
> > > >
> > > > - return intel_vsec_add_aux(pdev, NULL, intel_vsec_dev,
> > > > - intel_vsec_name(header->id));
> > > > + ret = intel_vsec_add_aux(pdev, NULL, intel_vsec_dev,
> > > > + intel_vsec_name(header->id));
> > > > +
> > > > + no_free_ptr(intel_vsec_dev);
> > > > +
> > > > + return ret;
> > >
> > > So if intel_vsec_add_aux() returned an error, intel_vsec_dev won't be
> > > freed because of the call call to no_free_ptr() before return. I that's
> > > not what you intended?
> >
> > It will have been freed if intel_vsec_add_aux() fails. It's a little messy.
> > That
> > function creates the auxdev and assigns the release function which will free
> > intel_vsec_dev when the device is removed. But there are failure points that
> > will also invoke the release function. Because of this, for all the failure
> > points in that function we free intel_vsec_dev so that it's state doesn't
> > need
> > to be questioned here. This happens explicitly if the failure is before
> > auxiliary_device_init(), or through the release function invoked by
> > auxiliary_device_uninit() if after.
>
> Oh, that's really convoluted and no wonder I missed it completely. It
> might even be that using cleanup.h here isn't really worth it. I know
> I pushed you into that direction but I didn't realize all the complexity
> at that point.
>
> If you still want to keep using cleanup.h, it would perhaps be less
> dangerous if you change the code such that no_free_ptr() for
> intel_vsec_dev and the resource (in 4/16, that's a similar case, isn't
> it?)
yes
> are before the intel_vsec_add_aux() call (and I'd also add a comment
> to explain that freeing them is now responsability of
> intel_vsec_add_aux()). That way, we don't leave a trap into there where
> somebody happily adds another no_free_ptr() to the same group and it
> causes a mem leak.
Sure. After the comment I'd just do this then still the value is still needed,
ret = intel_vsec_add_aux(pdev, NULL, no_free_ptr(intel_vsec_dev),
intel_vsec_name(header->id));
David
