Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <f7c8db7e-0b2a-4afc-96c6-da5baddf696e@redhat.com>
Date:   Mon, 4 Dec 2023 21:13:22 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V6 01/20] platform/x86/intel/vsec: Fix xa_alloc memory
 leak
Content-Language: en-US, nl
To:     david.e.box@linux.intel.com,
        =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        platform-driver-x86@vger.kernel.org, rajvi.jingar@linux.intel.com
References: <20231129222132.2331261-1-david.e.box@linux.intel.com>
 <20231129222132.2331261-2-david.e.box@linux.intel.com>
 <5c21230-1176-4168-f31f-a0c1f1713ca8@linux.intel.com>
 <f2a3bab9-296c-43a1-9b7e-944c5044feaf@redhat.com>
 <198aadd7a91152393ec56d421fa042d30378af40.camel@linux.intel.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <198aadd7a91152393ec56d421fa042d30378af40.camel@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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