Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <198aadd7a91152393ec56d421fa042d30378af40.camel@linux.intel.com>
Subject: Re: [PATCH V6 01/20] platform/x86/intel/vsec: Fix xa_alloc memory
 leak
From:   "David E. Box" <david.e.box@linux.intel.com>
Reply-To: david.e.box@linux.intel.com
To:     Hans de Goede <hdegoede@redhat.com>,
        Ilpo =?ISO-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        platform-driver-x86@vger.kernel.org, rajvi.jingar@linux.intel.com
Date:   Mon, 04 Dec 2023 11:57:22 -0800
In-Reply-To: <f2a3bab9-296c-43a1-9b7e-944c5044feaf@redhat.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>
Organization: David E. Box
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
MIME-Version: 1.0
Precedence: bulk

Hi Hans,

On Mon, 2023-12-04 at 14:51 +0100, Hans de Goede wrote:
> Hi,
>=20
> On 11/30/23 12:02, Ilpo J=C3=A4rvinen wrote:
> > On Wed, 29 Nov 2023, David E. Box wrote:
> >=20
> > > Commit 936874b77dd0 ("platform/x86/intel/vsec: Add PCI error recovery
> > > support to Intel PMT") added an xarray to track the list of vsec devi=
ces
> > > to
> > > be recovered after a PCI error. But it did not provide cleanup for th=
e
> > > list
> > > leading to a memory leak that was caught by kmemleak.=C2=A0 Do xa_all=
oc()
> > > before
> > > devm_add_action_or_reset() so that the list may be cleaned up with
> > > xa_erase() in the release function.
> > >=20
> > > 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>
> > > ---
> > >=20
> > > V6 - Move xa_alloc() before ida_alloc() to reduce mutex use during er=
ror
> > > =C2=A0=C2=A0=C2=A0=C2=A0 recovery.
> > > =C2=A0=C2=A0 - Fix return value after id_alloc() fail
> > > =C2=A0=C2=A0 - Add Fixes tag
> > > =C2=A0=C2=A0 - Add more detail to changelog
> > >=20
> > > V5 - New patch
> > >=20
> > > =C2=A0drivers/platform/x86/intel/vsec.c | 24 ++++++++++++++----------
> > > =C2=A0drivers/platform/x86/intel/vsec.h |=C2=A0 1 +
> > > =C2=A02 files changed, 15 insertions(+), 10 deletions(-)
> > >=20
> > > 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)
> > > =C2=A0{
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct intel_vsec_dev=
ice *intel_vsec_dev =3D dev_to_ivdev(dev);
> > > =C2=A0
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0xa_erase(&auxdev_array, in=
tel_vsec_dev->id);
> > > +
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_lock(&vsec_ida_=
lock);
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ida_free(intel_vsec_d=
ev->ida, intel_vsec_dev->auxdev.id);
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_unlock(&vsec_id=
a_lock);
> > > @@ -135,19 +137,27 @@ int intel_vsec_add_aux(struct pci_dev *pdev, st=
ruct
> > > device *parent,
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct auxiliary_devi=
ce *auxdev =3D &intel_vsec_dev->auxdev;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int ret, id;
> > > =C2=A0
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_lock(&vsec_ida_lock)=
;
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ret =3D ida_alloc(intel_vs=
ec_dev->ida, GFP_KERNEL);
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_unlock(&vsec_ida_loc=
k);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ret =3D xa_alloc(&auxdev_a=
rray, &intel_vsec_dev->id, intel_vsec_dev,
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 PMT_XA_LIMIT, =
GFP_KERNEL);
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (ret < 0) {
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0kfree(intel_vsec_dev->resource);
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0kfree(intel_vsec_dev);
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0return ret;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> > > =C2=A0
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_lock(&vsec_ida_lock)=
;
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0id =3D ida_alloc(intel_vse=
c_dev->ida, GFP_KERNEL);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_unlock(&vsec_ida_loc=
k);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (id < 0) {
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0kfree(intel_vsec_dev->resource);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0kfree(intel_vsec_dev);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0return id;
> >=20
> > Thanks, this looks much better this way around but it is missing=20
> > xa_alloc() rollback now that the order is reversed.
> >=20
> > Once that is fixed,
> >=20
> > Reviewed-by: Ilpo J=C3=A4rvinen <ilpo.jarvinen@linux.intel.com>
>=20
> I have fixed this up, adding the missing:
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0xa_erase(&auxdev_array, i=
ntel_vsec_dev->id);
>=20
> to this error-exit path while merging this.

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

>=20
> While looking into this I did find one other thing which
> worries me (different issue, needs a separate fix):
>=20
> intel_vsec_pci_slot_reset() uses
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 devm_release_action(&pdev->dev, intel_vsec_remove_aux,
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 &intel_vsec=
_dev->auxdev);
>=20
> and seems to assume that after this intel_vsec_remove_aux()
> has run for the auxdev-s. *But this is not the case*
>=20
> devm_release_action() only removes the action from the list
> of devres resources tied to the parent PCI device.
>=20
> It does *NOT* call the registered action function,
> so intel_vsec_remove_aux() is NOT called here.
>=20
> 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.
>=20
> So it seems that this also needs an explicit
> intel_vsec_remove_aux() call for each auxdev!
>=20
> ###
>=20
> 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.

>=20
> ###
>=20
> 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.

>=20
> 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 w=
as
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