| Message ID | 20231123040355.82139-2-david.e.box@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | intel_pmc: Add telemetry API to read counters | expand |
On Wed, 22 Nov 2023, David E. Box wrote: > Fixes memory leak, caught be kmemleak, due to failure to erase auxiliary > device entries from an xarray on removal. > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> Changelog is quite terse, please improve ;-). Missing Fixes tag. > --- > V5 - New patch > > drivers/platform/x86/intel/vsec.c | 24 ++++++++++++++++-------- > drivers/platform/x86/intel/vsec.h | 1 + > 2 files changed, 17 insertions(+), 8 deletions(-) > > diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c > index c1f9e4471b28..ae811bb65910 100644 > --- a/drivers/platform/x86/intel/vsec.c > +++ b/drivers/platform/x86/intel/vsec.c > @@ -120,6 +120,8 @@ static void intel_vsec_dev_release(struct device *dev) > { > struct intel_vsec_device *intel_vsec_dev = dev_to_ivdev(dev); > > + xa_erase(&auxdev_array, intel_vsec_dev->id); > + > mutex_lock(&vsec_ida_lock); > ida_free(intel_vsec_dev->ida, intel_vsec_dev->auxdev.id); > mutex_unlock(&vsec_ida_lock); > @@ -136,9 +138,21 @@ int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent, > int ret, id; > > mutex_lock(&vsec_ida_lock); > - ret = ida_alloc(intel_vsec_dev->ida, GFP_KERNEL); > + id = ida_alloc(intel_vsec_dev->ida, GFP_KERNEL); > mutex_unlock(&vsec_ida_lock); > + if (id < 0) { > + kfree(intel_vsec_dev->resource); > + kfree(intel_vsec_dev); > + return ret; > + } > + > + ret = xa_alloc(&auxdev_array, &intel_vsec_dev->id, intel_vsec_dev, > + PMT_XA_LIMIT, GFP_KERNEL); > if (ret < 0) { > + mutex_lock(&vsec_ida_lock); > + ida_free(intel_vsec_dev->ida, id); > + mutex_unlock(&vsec_ida_lock); Can't order of xa_alloc() and ida_alloc() be reversed such that you don't need to do this double mutex dance?
On Wed, 22 Nov 2023, David E. Box wrote: > Fixes memory leak, caught be kmemleak, due to failure to erase auxiliary > device entries from an xarray on removal. > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > --- > V5 - New patch > > drivers/platform/x86/intel/vsec.c | 24 ++++++++++++++++-------- > drivers/platform/x86/intel/vsec.h | 1 + > 2 files changed, 17 insertions(+), 8 deletions(-) > > diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c > index c1f9e4471b28..ae811bb65910 100644 > --- a/drivers/platform/x86/intel/vsec.c > +++ b/drivers/platform/x86/intel/vsec.c > @@ -120,6 +120,8 @@ static void intel_vsec_dev_release(struct device *dev) > { > struct intel_vsec_device *intel_vsec_dev = dev_to_ivdev(dev); > > + xa_erase(&auxdev_array, intel_vsec_dev->id); > + > mutex_lock(&vsec_ida_lock); > ida_free(intel_vsec_dev->ida, intel_vsec_dev->auxdev.id); > mutex_unlock(&vsec_ida_lock); > @@ -136,9 +138,21 @@ int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent, > int ret, id; > > mutex_lock(&vsec_ida_lock); > - ret = ida_alloc(intel_vsec_dev->ida, GFP_KERNEL); > + id = ida_alloc(intel_vsec_dev->ida, GFP_KERNEL); > mutex_unlock(&vsec_ida_lock); > + if (id < 0) { > + kfree(intel_vsec_dev->resource); > + kfree(intel_vsec_dev); > + return ret; > + } > + > + ret = xa_alloc(&auxdev_array, &intel_vsec_dev->id, intel_vsec_dev, > + PMT_XA_LIMIT, GFP_KERNEL); > if (ret < 0) { > + mutex_lock(&vsec_ida_lock); > + ida_free(intel_vsec_dev->ida, id); > + mutex_unlock(&vsec_ida_lock); > + > kfree(intel_vsec_dev->resource); > kfree(intel_vsec_dev); > return ret; > @@ -147,7 +161,7 @@ int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent, > if (!parent) > parent = &pdev->dev; > > - auxdev->id = ret; > + auxdev->id = id; > auxdev->name = name; > auxdev->dev.parent = parent; > auxdev->dev.release = intel_vsec_dev_release; > @@ -169,12 +183,6 @@ int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent, > if (ret < 0) > return ret; > > - /* Add auxdev to list */ > - ret = xa_alloc(&auxdev_array, &id, intel_vsec_dev, PMT_XA_LIMIT, > - GFP_KERNEL); > - if (ret) > - return ret; > - > return 0; BTW, there's also another unnecessary return construct around this which remains after this removal. The devm_add_Action_or_reset() value can be returned directly (maybe make another patch from that cleanup though since this is a Fixes patch).
diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c index c1f9e4471b28..ae811bb65910 100644 --- a/drivers/platform/x86/intel/vsec.c +++ b/drivers/platform/x86/intel/vsec.c @@ -120,6 +120,8 @@ static void intel_vsec_dev_release(struct device *dev) { struct intel_vsec_device *intel_vsec_dev = dev_to_ivdev(dev); + xa_erase(&auxdev_array, intel_vsec_dev->id); + mutex_lock(&vsec_ida_lock); ida_free(intel_vsec_dev->ida, intel_vsec_dev->auxdev.id); mutex_unlock(&vsec_ida_lock); @@ -136,9 +138,21 @@ int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent, int ret, id; mutex_lock(&vsec_ida_lock); - ret = ida_alloc(intel_vsec_dev->ida, GFP_KERNEL); + id = ida_alloc(intel_vsec_dev->ida, GFP_KERNEL); mutex_unlock(&vsec_ida_lock); + if (id < 0) { + kfree(intel_vsec_dev->resource); + kfree(intel_vsec_dev); + return ret; + } + + ret = xa_alloc(&auxdev_array, &intel_vsec_dev->id, intel_vsec_dev, + PMT_XA_LIMIT, GFP_KERNEL); if (ret < 0) { + mutex_lock(&vsec_ida_lock); + ida_free(intel_vsec_dev->ida, id); + mutex_unlock(&vsec_ida_lock); + kfree(intel_vsec_dev->resource); kfree(intel_vsec_dev); return ret; @@ -147,7 +161,7 @@ int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent, if (!parent) parent = &pdev->dev; - auxdev->id = ret; + auxdev->id = id; auxdev->name = name; auxdev->dev.parent = parent; auxdev->dev.release = intel_vsec_dev_release; @@ -169,12 +183,6 @@ int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent, if (ret < 0) return ret; - /* Add auxdev to list */ - ret = xa_alloc(&auxdev_array, &id, intel_vsec_dev, PMT_XA_LIMIT, - GFP_KERNEL); - if (ret) - return ret; - return 0; } EXPORT_SYMBOL_NS_GPL(intel_vsec_add_aux, INTEL_VSEC); diff --git a/drivers/platform/x86/intel/vsec.h b/drivers/platform/x86/intel/vsec.h index 0fd042c171ba..0a6201b4a0e9 100644 --- a/drivers/platform/x86/intel/vsec.h +++ b/drivers/platform/x86/intel/vsec.h @@ -45,6 +45,7 @@ struct intel_vsec_device { struct ida *ida; struct intel_vsec_platform_info *info; int num_resources; + int id; /* xa */ void *priv_data; size_t priv_data_size; };
Fixes memory leak, caught be kmemleak, due to failure to erase auxiliary device entries from an xarray on removal. Signed-off-by: David E. Box <david.e.box@linux.intel.com> --- V5 - New patch drivers/platform/x86/intel/vsec.c | 24 ++++++++++++++++-------- drivers/platform/x86/intel/vsec.h | 1 + 2 files changed, 17 insertions(+), 8 deletions(-)