diff mbox series

[V6,01/20] platform/x86/intel/vsec: Fix xa_alloc memory leak

Message ID 20231129222132.2331261-2-david.e.box@linux.intel.com
State New
Headers
Series intel_pmc: Add telemetry API to read counters |

Commit Message

David E. Box Nov. 29, 2023, 10:21 p.m. UTC 
  Commit 936874b77dd0 ("platform/x86/intel/vsec: Add PCI error recovery
support to Intel PMT") added an xarray to track the list of vsec devices to
be recovered after a PCI error. But it did not provide cleanup for the list
leading to a memory leak that was caught by kmemleak.  Do xa_alloc() before
devm_add_action_or_reset() so that the list may be cleaned up with
xa_erase() in the release function.

Fixes: 936874b77dd0 ("platform/x86/intel/vsec: Add PCI error recovery support to Intel PMT")
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---

V6 - Move xa_alloc() before ida_alloc() to reduce mutex use during error
     recovery.
   - Fix return value after id_alloc() fail
   - Add Fixes tag
   - Add more detail to changelog

V5 - New patch

 drivers/platform/x86/intel/vsec.c | 24 ++++++++++++++----------
 drivers/platform/x86/intel/vsec.h |  1 +
 2 files changed, 15 insertions(+), 10 deletions(-)

Comments

Ilpo Järvinen Nov. 30, 2023, 11:02 a.m. UTC | #1 
On Wed, 29 Nov 2023, David E. Box wrote:

> Commit 936874b77dd0 ("platform/x86/intel/vsec: Add PCI error recovery
> support to Intel PMT") added an xarray to track the list of vsec devices to
> be recovered after a PCI error. But it did not provide cleanup for the list
> leading to a memory leak that was caught by kmemleak.  Do xa_alloc() before
> devm_add_action_or_reset() so that the list may be cleaned up with
> xa_erase() in the release function.
> 
> Fixes: 936874b77dd0 ("platform/x86/intel/vsec: Add PCI error recovery support to Intel PMT")
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
> 
> V6 - Move xa_alloc() before ida_alloc() to reduce mutex use during error
>      recovery.
>    - Fix return value after id_alloc() fail
>    - Add Fixes tag
>    - Add more detail to changelog
> 
> V5 - New patch
> 
>  drivers/platform/x86/intel/vsec.c | 24 ++++++++++++++----------
>  drivers/platform/x86/intel/vsec.h |  1 +
>  2 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c
> index c1f9e4471b28..2d568466b4e2 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);
> @@ -135,19 +137,27 @@ int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent,
>  	struct auxiliary_device *auxdev = &intel_vsec_dev->auxdev;
>  	int ret, id;
>  
> -	mutex_lock(&vsec_ida_lock);
> -	ret = ida_alloc(intel_vsec_dev->ida, GFP_KERNEL);
> -	mutex_unlock(&vsec_ida_lock);
> +	ret = xa_alloc(&auxdev_array, &intel_vsec_dev->id, intel_vsec_dev,
> +		       PMT_XA_LIMIT, GFP_KERNEL);
>  	if (ret < 0) {
>  		kfree(intel_vsec_dev->resource);
>  		kfree(intel_vsec_dev);
>  		return ret;
>  	}
>  
> +	mutex_lock(&vsec_ida_lock);
> +	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 id;

Thanks, this looks much better this way around but it is missing 
xa_alloc() rollback now that the order is reversed.

Once that is fixed,

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Hans de Goede Dec. 4, 2023, 1:51 p.m. UTC | #2 
Hi,

On 11/30/23 12:02, Ilpo Järvinen wrote:
> On Wed, 29 Nov 2023, David E. Box wrote:
> 
>> Commit 936874b77dd0 ("platform/x86/intel/vsec: Add PCI error recovery
>> support to Intel PMT") added an xarray to track the list of vsec devices to
>> be recovered after a PCI error. But it did not provide cleanup for the list
>> leading to a memory leak that was caught by kmemleak.  Do xa_alloc() before
>> devm_add_action_or_reset() so that the list may be cleaned up with
>> xa_erase() in the release function.
>>
>> Fixes: 936874b77dd0 ("platform/x86/intel/vsec: Add PCI error recovery support to Intel PMT")
>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
>> ---
>>
>> V6 - Move xa_alloc() before ida_alloc() to reduce mutex use during error
>>      recovery.
>>    - Fix return value after id_alloc() fail
>>    - Add Fixes tag
>>    - Add more detail to changelog
>>
>> V5 - New patch
>>
>>  drivers/platform/x86/intel/vsec.c | 24 ++++++++++++++----------
>>  drivers/platform/x86/intel/vsec.h |  1 +
>>  2 files changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c
>> index c1f9e4471b28..2d568466b4e2 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);
>> @@ -135,19 +137,27 @@ int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent,
>>  	struct auxiliary_device *auxdev = &intel_vsec_dev->auxdev;
>>  	int ret, id;
>>  
>> -	mutex_lock(&vsec_ida_lock);
>> -	ret = ida_alloc(intel_vsec_dev->ida, GFP_KERNEL);
>> -	mutex_unlock(&vsec_ida_lock);
>> +	ret = xa_alloc(&auxdev_array, &intel_vsec_dev->id, intel_vsec_dev,
>> +		       PMT_XA_LIMIT, GFP_KERNEL);
>>  	if (ret < 0) {
>>  		kfree(intel_vsec_dev->resource);
>>  		kfree(intel_vsec_dev);
>>  		return ret;
>>  	}
>>  
>> +	mutex_lock(&vsec_ida_lock);
>> +	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 id;
> 
> Thanks, this looks much better this way around but it is missing 
> xa_alloc() rollback now that the order is reversed.
> 
> Once that is fixed,
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

I have fixed this up, adding the missing:

	xa_erase(&auxdev_array, intel_vsec_dev->id);

to this error-exit path while merging this.

While looking into this I did find one other thing which
worries me (different issue, needs a separate fix):

intel_vsec_pci_slot_reset() uses

                devm_release_action(&pdev->dev, intel_vsec_remove_aux,
                                    &intel_vsec_dev->auxdev);

and seems to assume that after this intel_vsec_remove_aux()
has run for the auxdev-s. *But this is not the case*

devm_release_action() only removes the action from the list
of devres resources tied to the parent PCI device.

It does *NOT* call the registered action function,
so intel_vsec_remove_aux() is NOT called here.

And then on re-probing the device as is done in
intel_vsec_pci_slot_reset() a second set of aux
devices with the same parent will be created AFAICT.

So it seems that this also needs an explicit
intel_vsec_remove_aux() call for each auxdev!

###

This makes me wonder if the PCI error handling here
and specifically the reset code was ever tested ?

###

Note that simply forcing a reprobe using device_reprobe()
will cause all the aux-devices to also get removed through
the action on driver-unbind without ever needing
the auxdev_array at all!

I guess that you want the removal to happen before
the pci_restore_state(pdev) state though, so that
simply relying on the removal on driver unbind
is not an option ?

Regards,

Hans
David E. Box Dec. 4, 2023, 7:57 p.m. UTC | #3 
Hi Hans,

On Mon, 2023-12-04 at 14:51 +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/30/23 12:02, Ilpo Järvinen wrote:
> > On Wed, 29 Nov 2023, David E. Box wrote:
> > 
> > > Commit 936874b77dd0 ("platform/x86/intel/vsec: Add PCI error recovery
> > > support to Intel PMT") added an xarray to track the list of vsec devices
> > > to
> > > be recovered after a PCI error. But it did not provide cleanup for the
> > > list
> > > leading to a memory leak that was caught by kmemleak.  Do xa_alloc()
> > > before
> > > devm_add_action_or_reset() so that the list may be cleaned up with
> > > xa_erase() in the release function.
> > > 
> > > Fixes: 936874b77dd0 ("platform/x86/intel/vsec: Add PCI error recovery
> > > support to Intel PMT")
> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > ---
> > > 
> > > V6 - Move xa_alloc() before ida_alloc() to reduce mutex use during error
> > >      recovery.
> > >    - Fix return value after id_alloc() fail
> > >    - Add Fixes tag
> > >    - Add more detail to changelog
> > > 
> > > V5 - New patch
> > > 
> > >  drivers/platform/x86/intel/vsec.c | 24 ++++++++++++++----------
> > >  drivers/platform/x86/intel/vsec.h |  1 +
> > >  2 files changed, 15 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/intel/vsec.c
> > > b/drivers/platform/x86/intel/vsec.c
> > > index c1f9e4471b28..2d568466b4e2 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);
> > > @@ -135,19 +137,27 @@ int intel_vsec_add_aux(struct pci_dev *pdev, struct
> > > device *parent,
> > >         struct auxiliary_device *auxdev = &intel_vsec_dev->auxdev;
> > >         int ret, id;
> > >  
> > > -       mutex_lock(&vsec_ida_lock);
> > > -       ret = ida_alloc(intel_vsec_dev->ida, GFP_KERNEL);
> > > -       mutex_unlock(&vsec_ida_lock);
> > > +       ret = xa_alloc(&auxdev_array, &intel_vsec_dev->id, intel_vsec_dev,
> > > +                      PMT_XA_LIMIT, GFP_KERNEL);
> > >         if (ret < 0) {
> > >                 kfree(intel_vsec_dev->resource);
> > >                 kfree(intel_vsec_dev);
> > >                 return ret;
> > >         }
> > >  
> > > +       mutex_lock(&vsec_ida_lock);
> > > +       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 id;
> > 
> > Thanks, this looks much better this way around but it is missing 
> > xa_alloc() rollback now that the order is reversed.
> > 
> > Once that is fixed,
> > 
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> I have fixed this up, adding the missing:
> 
>         xa_erase(&auxdev_array, intel_vsec_dev->id);
> 
> to this error-exit path while merging this.

Thanks. Does that include the rest of the series which was all reviewed by Ilpo?

> 
> While looking into this I did find one other thing which
> worries me (different issue, needs a separate fix):
> 
> intel_vsec_pci_slot_reset() uses
> 
>                 devm_release_action(&pdev->dev, intel_vsec_remove_aux,
>                                     &intel_vsec_dev->auxdev);
> 
> and seems to assume that after this intel_vsec_remove_aux()
> has run for the auxdev-s. *But this is not the case*
> 
> devm_release_action() only removes the action from the list
> of devres resources tied to the parent PCI device.
> 
> It does *NOT* call the registered action function,
> so intel_vsec_remove_aux() is NOT called here.
> 
> And then on re-probing the device as is done in
> intel_vsec_pci_slot_reset() a second set of aux
> devices with the same parent will be created AFAICT.
> 
> So it seems that this also needs an explicit
> intel_vsec_remove_aux() call for each auxdev!
> 
> ###
> 
> This makes me wonder if the PCI error handling here
> and specifically the reset code was ever tested ?

I recall the code was tested using error injection to cause the slot reset to
occur and reprobe was confirmed. I'll have to find out the specific test so that
we can check it again with the proposed fix and ensure no leaks.

> 
> ###
> 
> Note that simply forcing a reprobe using device_reprobe()
> will cause all the aux-devices to also get removed through
> the action on driver-unbind without ever needing
> the auxdev_array at all!

Okay. That would be a lot simpler.

> 
> I guess that you want the removal to happen before
> the pci_restore_state(pdev) state though, so that
> simply relying on the removal on driver unbind
> is not an option ?

I'm not sure this is needed given the simplicity of the device. The array was
added only to track the list of devices and reprobe the one that was reset.
We'll look at changing this to do driver_reprobe() instead. Thanks.

David
Hans de Goede Dec. 4, 2023, 8:13 p.m. UTC | #4 
Hi,

On 12/4/23 20:57, David E. Box wrote:
> Hi Hans,
> 
> On Mon, 2023-12-04 at 14:51 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11/30/23 12:02, Ilpo Järvinen wrote:
>>> On Wed, 29 Nov 2023, David E. Box wrote:
>>>
>>>> Commit 936874b77dd0 ("platform/x86/intel/vsec: Add PCI error recovery
>>>> support to Intel PMT") added an xarray to track the list of vsec devices
>>>> to
>>>> be recovered after a PCI error. But it did not provide cleanup for the
>>>> list
>>>> leading to a memory leak that was caught by kmemleak.  Do xa_alloc()
>>>> before
>>>> devm_add_action_or_reset() so that the list may be cleaned up with
>>>> xa_erase() in the release function.
>>>>
>>>> Fixes: 936874b77dd0 ("platform/x86/intel/vsec: Add PCI error recovery
>>>> support to Intel PMT")
>>>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
>>>> ---
>>>>
>>>> V6 - Move xa_alloc() before ida_alloc() to reduce mutex use during error
>>>>      recovery.
>>>>    - Fix return value after id_alloc() fail
>>>>    - Add Fixes tag
>>>>    - Add more detail to changelog
>>>>
>>>> V5 - New patch
>>>>
>>>>  drivers/platform/x86/intel/vsec.c | 24 ++++++++++++++----------
>>>>  drivers/platform/x86/intel/vsec.h |  1 +
>>>>  2 files changed, 15 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/intel/vsec.c
>>>> b/drivers/platform/x86/intel/vsec.c
>>>> index c1f9e4471b28..2d568466b4e2 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);
>>>> @@ -135,19 +137,27 @@ int intel_vsec_add_aux(struct pci_dev *pdev, struct
>>>> device *parent,
>>>>         struct auxiliary_device *auxdev = &intel_vsec_dev->auxdev;
>>>>         int ret, id;
>>>>  
>>>> -       mutex_lock(&vsec_ida_lock);
>>>> -       ret = ida_alloc(intel_vsec_dev->ida, GFP_KERNEL);
>>>> -       mutex_unlock(&vsec_ida_lock);
>>>> +       ret = xa_alloc(&auxdev_array, &intel_vsec_dev->id, intel_vsec_dev,
>>>> +                      PMT_XA_LIMIT, GFP_KERNEL);
>>>>         if (ret < 0) {
>>>>                 kfree(intel_vsec_dev->resource);
>>>>                 kfree(intel_vsec_dev);
>>>>                 return ret;
>>>>         }
>>>>  
>>>> +       mutex_lock(&vsec_ida_lock);
>>>> +       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 id;
>>>
>>> Thanks, this looks much better this way around but it is missing 
>>> xa_alloc() rollback now that the order is reversed.
>>>
>>> Once that is fixed,
>>>
>>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>
>> I have fixed this up, adding the missing:
>>
>>         xa_erase(&auxdev_array, intel_vsec_dev->id);
>>
>> to this error-exit path while merging this.
> 
> Thanks. Does that include the rest of the series which was all reviewed by Ilpo?

Yes the entire series has been merged into the pdx86/review-hans
branch now.

Regards,

Hans
diff mbox series

Patch

diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c
index c1f9e4471b28..2d568466b4e2 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);
@@ -135,19 +137,27 @@  int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent,
 	struct auxiliary_device *auxdev = &intel_vsec_dev->auxdev;
 	int ret, id;
 
-	mutex_lock(&vsec_ida_lock);
-	ret = ida_alloc(intel_vsec_dev->ida, GFP_KERNEL);
-	mutex_unlock(&vsec_ida_lock);
+	ret = xa_alloc(&auxdev_array, &intel_vsec_dev->id, intel_vsec_dev,
+		       PMT_XA_LIMIT, GFP_KERNEL);
 	if (ret < 0) {
 		kfree(intel_vsec_dev->resource);
 		kfree(intel_vsec_dev);
 		return ret;
 	}
 
+	mutex_lock(&vsec_ida_lock);
+	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 id;
+	}
+
 	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 +179,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;
 };