[PATCH 0/2] drm: Fix PCI device refcount leak

[PATCH 0/2] drm: Fix PCI device refcount leak

[Xiongfeng Wang]

As comment of pci_get_class() says, it returns a pci_device with its
refcount increased and decreased the refcount for the input parameter
@from if it is not NULL.

If we use pci_get_class() to iterate all the PCI device of a certain
class, when we find the expected device and break the iteration loop,
the refcount of the found PCI device is increased. When finish using
the PCI device, we need to call pci_dev_put() to decrease the refcount.


Xiongfeng Wang (2):
  drm/radeon: Fix PCI device refcount leak in radeon_atrm_get_bios()
  drm/amdgpu: Fix PCI device refcount leak in amdgpu_atrm_get_bios()

 drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c | 1 +
 drivers/gpu/drm/radeon/radeon_bios.c     | 1 +
 2 files changed, 2 insertions(+)

-- 
2.20.1

[PATCH 1/2] drm/radeon: Fix PCI device refcount leak in radeon_atrm_get_bios()

[Xiongfeng Wang]

As comment of pci_get_class() says, it returns a pci_device with its
refcount increased and decreased the refcount for the input parameter
@from if it is not NULL.

If we break the loop in radeon_atrm_get_bios() with 'pdev' not NULL, we
need to call pci_dev_put() to decrease the refcount. Add the missing
pci_dev_put() to avoid refcount leak.

Fixes: d8ade3526b2a ("drm/radeon: handle non-VGA class pci devices with ATRM")
Fixes: c61e2775873f ("drm/radeon: split ATRM support out from the ATPX handler (v3)")
Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
---
 drivers/gpu/drm/radeon/radeon_bios.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
index 33121655d50b..2df6ce3e32cb 100644
--- a/drivers/gpu/drm/radeon/radeon_bios.c
+++ b/drivers/gpu/drm/radeon/radeon_bios.c
@@ -227,6 +227,7 @@ static bool radeon_atrm_get_bios(struct radeon_device *rdev)
 
 	if (!found)
 		return false;
+	pci_dev_put(pdev);
 
 	rdev->bios = kmalloc(size, GFP_KERNEL);
 	if (!rdev->bios) {
-- 
2.20.1

[PATCH 2/2] drm/amdgpu: Fix PCI device refcount leak in amdgpu_atrm_get_bios()

[Xiongfeng Wang]

As comment of pci_get_class() says, it returns a pci_device with its
refcount increased and decreased the refcount for the input parameter
@from if it is not NULL.

If we break the loop in amdgpu_atrm_get_bios() with 'pdev' not NULL, we
need to call pci_dev_put() to decrease the refcount. Add the missing
pci_dev_put() to avoid refcount leak.

Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
index e363f56c72af..30c28a69e847 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
@@ -317,6 +317,7 @@ static bool amdgpu_atrm_get_bios(struct amdgpu_device *adev)
 
 	if (!found)
 		return false;
+	pci_dev_put(pdev);
 
 	adev->bios = kmalloc(size, GFP_KERNEL);
 	if (!adev->bios) {
-- 
2.20.1

Re: [PATCH 1/2] drm/radeon: Fix PCI device refcount leak in radeon_atrm_get_bios()

[Alex Deucher]

On Tue, Nov 22, 2022 at 6:12 AM Xiongfeng Wang
<wangxiongfeng2@huawei.com> wrote:
>
> As comment of pci_get_class() says, it returns a pci_device with its
> refcount increased and decreased the refcount for the input parameter
> @from if it is not NULL.
>
> If we break the loop in radeon_atrm_get_bios() with 'pdev' not NULL, we
> need to call pci_dev_put() to decrease the refcount. Add the missing
> pci_dev_put() to avoid refcount leak.

For both patches, I think pci_dev_put() needs to go into the loops.
There are 2 or more GPUs on the systems where this is relevant.

Alex

>
> Fixes: d8ade3526b2a ("drm/radeon: handle non-VGA class pci devices with ATRM")
> Fixes: c61e2775873f ("drm/radeon: split ATRM support out from the ATPX handler (v3)")
> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> ---
>  drivers/gpu/drm/radeon/radeon_bios.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
> index 33121655d50b..2df6ce3e32cb 100644
> --- a/drivers/gpu/drm/radeon/radeon_bios.c
> +++ b/drivers/gpu/drm/radeon/radeon_bios.c
> @@ -227,6 +227,7 @@ static bool radeon_atrm_get_bios(struct radeon_device *rdev)
>
>         if (!found)
>                 return false;
> +       pci_dev_put(pdev);
>
>         rdev->bios = kmalloc(size, GFP_KERNEL);
>         if (!rdev->bios) {
> --
> 2.20.1
>

Re: [PATCH 1/2] drm/radeon: Fix PCI device refcount leak in radeon_atrm_get_bios()

[Alex Deucher]

On Tue, Nov 22, 2022 at 6:12 AM Xiongfeng Wang
<wangxiongfeng2@huawei.com> wrote:
>
> As comment of pci_get_class() says, it returns a pci_device with its
> refcount increased and decreased the refcount for the input parameter
> @from if it is not NULL.
>
> If we break the loop in radeon_atrm_get_bios() with 'pdev' not NULL, we
> need to call pci_dev_put() to decrease the refcount. Add the missing
> pci_dev_put() to avoid refcount leak.

For both patches, I think pci_dev_put() needs to go into the loops.
There are 2 or more GPUs on the systems where this is relevant.

Alex

>
> Fixes: d8ade3526b2a ("drm/radeon: handle non-VGA class pci devices with ATRM")
> Fixes: c61e2775873f ("drm/radeon: split ATRM support out from the ATPX handler (v3)")
> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> ---
>  drivers/gpu/drm/radeon/radeon_bios.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
> index 33121655d50b..2df6ce3e32cb 100644
> --- a/drivers/gpu/drm/radeon/radeon_bios.c
> +++ b/drivers/gpu/drm/radeon/radeon_bios.c
> @@ -227,6 +227,7 @@ static bool radeon_atrm_get_bios(struct radeon_device *rdev)
>
>         if (!found)
>                 return false;
> +       pci_dev_put(pdev);
>
>         rdev->bios = kmalloc(size, GFP_KERNEL);
>         if (!rdev->bios) {
> --
> 2.20.1
>

Re: [PATCH 1/2] drm/radeon: Fix PCI device refcount leak in radeon_atrm_get_bios()

[Lazar, Lijo]

On 11/22/2022 8:19 PM, Alex Deucher wrote:
> On Tue, Nov 22, 2022 at 6:12 AM Xiongfeng Wang
> <wangxiongfeng2@huawei.com> wrote:
>>
>> As comment of pci_get_class() says, it returns a pci_device with its
>> refcount increased and decreased the refcount for the input parameter
>> @from if it is not NULL.
>>
>> If we break the loop in radeon_atrm_get_bios() with 'pdev' not NULL, we
>> need to call pci_dev_put() to decrease the refcount. Add the missing
>> pci_dev_put() to avoid refcount leak.
> 
> For both patches, I think pci_dev_put() needs to go into the loops.
> There are 2 or more GPUs on the systems where this is relevant.
> 

As per the logic, the loop breaks when it finds a valid ATRM handle. So 
dev_put is required only for that device.

When inside the loop this happens -  "decreased the refcount for the 
input parameter @from if it is not NULL"

Thanks,
Lijo

> Alex
> 
>>
>> Fixes: d8ade3526b2a ("drm/radeon: handle non-VGA class pci devices with ATRM")
>> Fixes: c61e2775873f ("drm/radeon: split ATRM support out from the ATPX handler (v3)")
>> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
>> ---
>>   drivers/gpu/drm/radeon/radeon_bios.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
>> index 33121655d50b..2df6ce3e32cb 100644
>> --- a/drivers/gpu/drm/radeon/radeon_bios.c
>> +++ b/drivers/gpu/drm/radeon/radeon_bios.c
>> @@ -227,6 +227,7 @@ static bool radeon_atrm_get_bios(struct radeon_device *rdev)
>>
>>          if (!found)
>>                  return false;
>> +       pci_dev_put(pdev);
>>
>>          rdev->bios = kmalloc(size, GFP_KERNEL);
>>          if (!rdev->bios) {
>> --
>> 2.20.1
>>

Re: [PATCH 1/2] drm/radeon: Fix PCI device refcount leak in radeon_atrm_get_bios()

[Lazar, Lijo]

On 11/22/2022 8:19 PM, Alex Deucher wrote:
> On Tue, Nov 22, 2022 at 6:12 AM Xiongfeng Wang
> <wangxiongfeng2@huawei.com> wrote:
>>
>> As comment of pci_get_class() says, it returns a pci_device with its
>> refcount increased and decreased the refcount for the input parameter
>> @from if it is not NULL.
>>
>> If we break the loop in radeon_atrm_get_bios() with 'pdev' not NULL, we
>> need to call pci_dev_put() to decrease the refcount. Add the missing
>> pci_dev_put() to avoid refcount leak.
> 
> For both patches, I think pci_dev_put() needs to go into the loops.
> There are 2 or more GPUs on the systems where this is relevant.
> 

As per the logic, the loop breaks when it finds a valid ATRM handle. So 
dev_put is required only for that device.

When inside the loop this happens -  "decreased the refcount for the 
input parameter @from if it is not NULL"

Thanks,
Lijo

> Alex
> 
>>
>> Fixes: d8ade3526b2a ("drm/radeon: handle non-VGA class pci devices with ATRM")
>> Fixes: c61e2775873f ("drm/radeon: split ATRM support out from the ATPX handler (v3)")
>> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
>> ---
>>   drivers/gpu/drm/radeon/radeon_bios.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
>> index 33121655d50b..2df6ce3e32cb 100644
>> --- a/drivers/gpu/drm/radeon/radeon_bios.c
>> +++ b/drivers/gpu/drm/radeon/radeon_bios.c
>> @@ -227,6 +227,7 @@ static bool radeon_atrm_get_bios(struct radeon_device *rdev)
>>
>>          if (!found)
>>                  return false;
>> +       pci_dev_put(pdev);
>>
>>          rdev->bios = kmalloc(size, GFP_KERNEL);
>>          if (!rdev->bios) {
>> --
>> 2.20.1
>>

Re: [PATCH 1/2] drm/radeon: Fix PCI device refcount leak in radeon_atrm_get_bios()

[Alex Deucher]

On Tue, Nov 22, 2022 at 9:59 AM Lazar, Lijo <lijo.lazar@amd.com> wrote:
>
>
>
> On 11/22/2022 8:19 PM, Alex Deucher wrote:
> > On Tue, Nov 22, 2022 at 6:12 AM Xiongfeng Wang
> > <wangxiongfeng2@huawei.com> wrote:
> >>
> >> As comment of pci_get_class() says, it returns a pci_device with its
> >> refcount increased and decreased the refcount for the input parameter
> >> @from if it is not NULL.
> >>
> >> If we break the loop in radeon_atrm_get_bios() with 'pdev' not NULL, we
> >> need to call pci_dev_put() to decrease the refcount. Add the missing
> >> pci_dev_put() to avoid refcount leak.
> >
> > For both patches, I think pci_dev_put() needs to go into the loops.
> > There are 2 or more GPUs on the systems where this is relevant.
> >
>
> As per the logic, the loop breaks when it finds a valid ATRM handle. So
> dev_put is required only for that device.

Sure, but what if the handle is on the second DISPLAY_VGA or
DISPLAY_OTHER class PCI device on the system?  We've already called
pci_get_class() for the first PCI device that matched.

Alex

>
> When inside the loop this happens -  "decreased the refcount for the
> input parameter @from if it is not NULL"
>
> Thanks,
> Lijo
>
> > Alex
> >
> >>
> >> Fixes: d8ade3526b2a ("drm/radeon: handle non-VGA class pci devices with ATRM")
> >> Fixes: c61e2775873f ("drm/radeon: split ATRM support out from the ATPX handler (v3)")
> >> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> >> ---
> >>   drivers/gpu/drm/radeon/radeon_bios.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
> >> index 33121655d50b..2df6ce3e32cb 100644
> >> --- a/drivers/gpu/drm/radeon/radeon_bios.c
> >> +++ b/drivers/gpu/drm/radeon/radeon_bios.c
> >> @@ -227,6 +227,7 @@ static bool radeon_atrm_get_bios(struct radeon_device *rdev)
> >>
> >>          if (!found)
> >>                  return false;
> >> +       pci_dev_put(pdev);
> >>
> >>          rdev->bios = kmalloc(size, GFP_KERNEL);
> >>          if (!rdev->bios) {
> >> --
> >> 2.20.1
> >>

Re: [PATCH 1/2] drm/radeon: Fix PCI device refcount leak in radeon_atrm_get_bios()

[Alex Deucher]

On Tue, Nov 22, 2022 at 9:59 AM Lazar, Lijo <lijo.lazar@amd.com> wrote:
>
>
>
> On 11/22/2022 8:19 PM, Alex Deucher wrote:
> > On Tue, Nov 22, 2022 at 6:12 AM Xiongfeng Wang
> > <wangxiongfeng2@huawei.com> wrote:
> >>
> >> As comment of pci_get_class() says, it returns a pci_device with its
> >> refcount increased and decreased the refcount for the input parameter
> >> @from if it is not NULL.
> >>
> >> If we break the loop in radeon_atrm_get_bios() with 'pdev' not NULL, we
> >> need to call pci_dev_put() to decrease the refcount. Add the missing
> >> pci_dev_put() to avoid refcount leak.
> >
> > For both patches, I think pci_dev_put() needs to go into the loops.
> > There are 2 or more GPUs on the systems where this is relevant.
> >
>
> As per the logic, the loop breaks when it finds a valid ATRM handle. So
> dev_put is required only for that device.

Sure, but what if the handle is on the second DISPLAY_VGA or
DISPLAY_OTHER class PCI device on the system?  We've already called
pci_get_class() for the first PCI device that matched.

Alex

>
> When inside the loop this happens -  "decreased the refcount for the
> input parameter @from if it is not NULL"
>
> Thanks,
> Lijo
>
> > Alex
> >
> >>
> >> Fixes: d8ade3526b2a ("drm/radeon: handle non-VGA class pci devices with ATRM")
> >> Fixes: c61e2775873f ("drm/radeon: split ATRM support out from the ATPX handler (v3)")
> >> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> >> ---
> >>   drivers/gpu/drm/radeon/radeon_bios.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
> >> index 33121655d50b..2df6ce3e32cb 100644
> >> --- a/drivers/gpu/drm/radeon/radeon_bios.c
> >> +++ b/drivers/gpu/drm/radeon/radeon_bios.c
> >> @@ -227,6 +227,7 @@ static bool radeon_atrm_get_bios(struct radeon_device *rdev)
> >>
> >>          if (!found)
> >>                  return false;
> >> +       pci_dev_put(pdev);
> >>
> >>          rdev->bios = kmalloc(size, GFP_KERNEL);
> >>          if (!rdev->bios) {
> >> --
> >> 2.20.1
> >>

Re: [PATCH 1/2] drm/radeon: Fix PCI device refcount leak in radeon_atrm_get_bios()

[Lazar, Lijo]

[-- Attachment #1: Type: text/plain, Size: 3166 bytes --]

[AMD Official Use Only - General]

When only second GPU has valid ATRM handle -
then it stays inside the loop and in the next call to pci_get_class(), it passes pdev reference to first GPU as the "from" param. That time it drops the reference count of "from" device.

Thanks,
Lijo
________________________________
From: Alex Deucher <alexdeucher@gmail.com>
Sent: Tuesday, November 22, 2022 9:55:33 PM
To: Lazar, Lijo <Lijo.Lazar@amd.com>
Cc: Xiongfeng Wang <wangxiongfeng2@huawei.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; airlied@gmail.com <airlied@gmail.com>; daniel@ffwll.ch <daniel@ffwll.ch>; Zhang, Hawking <Hawking.Zhang@amd.com>; dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; yangyingliang@huawei.com <yangyingliang@huawei.com>
Subject: Re: [PATCH 1/2] drm/radeon: Fix PCI device refcount leak in radeon_atrm_get_bios()

On Tue, Nov 22, 2022 at 9:59 AM Lazar, Lijo <lijo.lazar@amd.com> wrote:
>
>
>
> On 11/22/2022 8:19 PM, Alex Deucher wrote:
> > On Tue, Nov 22, 2022 at 6:12 AM Xiongfeng Wang
> > <wangxiongfeng2@huawei.com> wrote:
> >>
> >> As comment of pci_get_class() says, it returns a pci_device with its
> >> refcount increased and decreased the refcount for the input parameter
> >> @from if it is not NULL.
> >>
> >> If we break the loop in radeon_atrm_get_bios() with 'pdev' not NULL, we
> >> need to call pci_dev_put() to decrease the refcount. Add the missing
> >> pci_dev_put() to avoid refcount leak.
> >
> > For both patches, I think pci_dev_put() needs to go into the loops.
> > There are 2 or more GPUs on the systems where this is relevant.
> >
>
> As per the logic, the loop breaks when it finds a valid ATRM handle. So
> dev_put is required only for that device.

Sure, but what if the handle is on the second DISPLAY_VGA or
DISPLAY_OTHER class PCI device on the system?  We've already called
pci_get_class() for the first PCI device that matched.

Alex

>
> When inside the loop this happens -  "decreased the refcount for the
> input parameter @from if it is not NULL"
>
> Thanks,
> Lijo
>
> > Alex
> >
> >>
> >> Fixes: d8ade3526b2a ("drm/radeon: handle non-VGA class pci devices with ATRM")
> >> Fixes: c61e2775873f ("drm/radeon: split ATRM support out from the ATPX handler (v3)")
> >> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> >> ---
> >>   drivers/gpu/drm/radeon/radeon_bios.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
> >> index 33121655d50b..2df6ce3e32cb 100644
> >> --- a/drivers/gpu/drm/radeon/radeon_bios.c
> >> +++ b/drivers/gpu/drm/radeon/radeon_bios.c
> >> @@ -227,6 +227,7 @@ static bool radeon_atrm_get_bios(struct radeon_device *rdev)
> >>
> >>          if (!found)
> >>                  return false;
> >> +       pci_dev_put(pdev);
> >>
> >>          rdev->bios = kmalloc(size, GFP_KERNEL);
> >>          if (!rdev->bios) {
> >> --
> >> 2.20.1
> >>

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 20423 bytes --]

Re: [PATCH 1/2] drm/radeon: Fix PCI device refcount leak in radeon_atrm_get_bios()

[Lazar, Lijo]

[-- Attachment #1: Type: text/plain, Size: 3166 bytes --]

[AMD Official Use Only - General]

When only second GPU has valid ATRM handle -
then it stays inside the loop and in the next call to pci_get_class(), it passes pdev reference to first GPU as the "from" param. That time it drops the reference count of "from" device.

Thanks,
Lijo
________________________________
From: Alex Deucher <alexdeucher@gmail.com>
Sent: Tuesday, November 22, 2022 9:55:33 PM
To: Lazar, Lijo <Lijo.Lazar@amd.com>
Cc: Xiongfeng Wang <wangxiongfeng2@huawei.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; airlied@gmail.com <airlied@gmail.com>; daniel@ffwll.ch <daniel@ffwll.ch>; Zhang, Hawking <Hawking.Zhang@amd.com>; dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; yangyingliang@huawei.com <yangyingliang@huawei.com>
Subject: Re: [PATCH 1/2] drm/radeon: Fix PCI device refcount leak in radeon_atrm_get_bios()

On Tue, Nov 22, 2022 at 9:59 AM Lazar, Lijo <lijo.lazar@amd.com> wrote:
>
>
>
> On 11/22/2022 8:19 PM, Alex Deucher wrote:
> > On Tue, Nov 22, 2022 at 6:12 AM Xiongfeng Wang
> > <wangxiongfeng2@huawei.com> wrote:
> >>
> >> As comment of pci_get_class() says, it returns a pci_device with its
> >> refcount increased and decreased the refcount for the input parameter
> >> @from if it is not NULL.
> >>
> >> If we break the loop in radeon_atrm_get_bios() with 'pdev' not NULL, we
> >> need to call pci_dev_put() to decrease the refcount. Add the missing
> >> pci_dev_put() to avoid refcount leak.
> >
> > For both patches, I think pci_dev_put() needs to go into the loops.
> > There are 2 or more GPUs on the systems where this is relevant.
> >
>
> As per the logic, the loop breaks when it finds a valid ATRM handle. So
> dev_put is required only for that device.

Sure, but what if the handle is on the second DISPLAY_VGA or
DISPLAY_OTHER class PCI device on the system?  We've already called
pci_get_class() for the first PCI device that matched.

Alex

>
> When inside the loop this happens -  "decreased the refcount for the
> input parameter @from if it is not NULL"
>
> Thanks,
> Lijo
>
> > Alex
> >
> >>
> >> Fixes: d8ade3526b2a ("drm/radeon: handle non-VGA class pci devices with ATRM")
> >> Fixes: c61e2775873f ("drm/radeon: split ATRM support out from the ATPX handler (v3)")
> >> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> >> ---
> >>   drivers/gpu/drm/radeon/radeon_bios.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
> >> index 33121655d50b..2df6ce3e32cb 100644
> >> --- a/drivers/gpu/drm/radeon/radeon_bios.c
> >> +++ b/drivers/gpu/drm/radeon/radeon_bios.c
> >> @@ -227,6 +227,7 @@ static bool radeon_atrm_get_bios(struct radeon_device *rdev)
> >>
> >>          if (!found)
> >>                  return false;
> >> +       pci_dev_put(pdev);
> >>
> >>          rdev->bios = kmalloc(size, GFP_KERNEL);
> >>          if (!rdev->bios) {
> >> --
> >> 2.20.1
> >>

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 20423 bytes --]

Re: [PATCH 1/2] drm/radeon: Fix PCI device refcount leak in radeon_atrm_get_bios()

[Alex Deucher]

On Tue, Nov 22, 2022 at 12:10 PM Lazar, Lijo <Lijo.Lazar@amd.com> wrote:
>
> [AMD Official Use Only - General]
>
> When only second GPU has valid ATRM handle -
> then it stays inside the loop and in the next call to pci_get_class(), it passes pdev reference to first GPU as the "from" param. That time it drops the reference count of "from" device.

ah, right, that was the part I missed.  Thanks.

Alex


>
> Thanks,
> Lijo
> ________________________________
> From: Alex Deucher <alexdeucher@gmail.com>
> Sent: Tuesday, November 22, 2022 9:55:33 PM
> To: Lazar, Lijo <Lijo.Lazar@amd.com>
> Cc: Xiongfeng Wang <wangxiongfeng2@huawei.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; airlied@gmail.com <airlied@gmail.com>; daniel@ffwll.ch <daniel@ffwll.ch>; Zhang, Hawking <Hawking.Zhang@amd.com>; dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; yangyingliang@huawei.com <yangyingliang@huawei.com>
> Subject: Re: [PATCH 1/2] drm/radeon: Fix PCI device refcount leak in radeon_atrm_get_bios()
>
> On Tue, Nov 22, 2022 at 9:59 AM Lazar, Lijo <lijo.lazar@amd.com> wrote:
> >
> >
> >
> > On 11/22/2022 8:19 PM, Alex Deucher wrote:
> > > On Tue, Nov 22, 2022 at 6:12 AM Xiongfeng Wang
> > > <wangxiongfeng2@huawei.com> wrote:
> > >>
> > >> As comment of pci_get_class() says, it returns a pci_device with its
> > >> refcount increased and decreased the refcount for the input parameter
> > >> @from if it is not NULL.
> > >>
> > >> If we break the loop in radeon_atrm_get_bios() with 'pdev' not NULL, we
> > >> need to call pci_dev_put() to decrease the refcount. Add the missing
> > >> pci_dev_put() to avoid refcount leak.
> > >
> > > For both patches, I think pci_dev_put() needs to go into the loops.
> > > There are 2 or more GPUs on the systems where this is relevant.
> > >
> >
> > As per the logic, the loop breaks when it finds a valid ATRM handle. So
> > dev_put is required only for that device.
>
> Sure, but what if the handle is on the second DISPLAY_VGA or
> DISPLAY_OTHER class PCI device on the system?  We've already called
> pci_get_class() for the first PCI device that matched.
>
> Alex
>
> >
> > When inside the loop this happens -  "decreased the refcount for the
> > input parameter @from if it is not NULL"
> >
> > Thanks,
> > Lijo
> >
> > > Alex
> > >
> > >>
> > >> Fixes: d8ade3526b2a ("drm/radeon: handle non-VGA class pci devices with ATRM")
> > >> Fixes: c61e2775873f ("drm/radeon: split ATRM support out from the ATPX handler (v3)")
> > >> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> > >> ---
> > >>   drivers/gpu/drm/radeon/radeon_bios.c | 1 +
> > >>   1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
> > >> index 33121655d50b..2df6ce3e32cb 100644
> > >> --- a/drivers/gpu/drm/radeon/radeon_bios.c
> > >> +++ b/drivers/gpu/drm/radeon/radeon_bios.c
> > >> @@ -227,6 +227,7 @@ static bool radeon_atrm_get_bios(struct radeon_device *rdev)
> > >>
> > >>          if (!found)
> > >>                  return false;
> > >> +       pci_dev_put(pdev);
> > >>
> > >>          rdev->bios = kmalloc(size, GFP_KERNEL);
> > >>          if (!rdev->bios) {
> > >> --
> > >> 2.20.1
> > >>

Re: [PATCH 1/2] drm/radeon: Fix PCI device refcount leak in radeon_atrm_get_bios()

[Alex Deucher]

On Tue, Nov 22, 2022 at 12:10 PM Lazar, Lijo <Lijo.Lazar@amd.com> wrote:
>
> [AMD Official Use Only - General]
>
> When only second GPU has valid ATRM handle -
> then it stays inside the loop and in the next call to pci_get_class(), it passes pdev reference to first GPU as the "from" param. That time it drops the reference count of "from" device.

ah, right, that was the part I missed.  Thanks.

Alex


>
> Thanks,
> Lijo
> ________________________________
> From: Alex Deucher <alexdeucher@gmail.com>
> Sent: Tuesday, November 22, 2022 9:55:33 PM
> To: Lazar, Lijo <Lijo.Lazar@amd.com>
> Cc: Xiongfeng Wang <wangxiongfeng2@huawei.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; airlied@gmail.com <airlied@gmail.com>; daniel@ffwll.ch <daniel@ffwll.ch>; Zhang, Hawking <Hawking.Zhang@amd.com>; dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; yangyingliang@huawei.com <yangyingliang@huawei.com>
> Subject: Re: [PATCH 1/2] drm/radeon: Fix PCI device refcount leak in radeon_atrm_get_bios()
>
> On Tue, Nov 22, 2022 at 9:59 AM Lazar, Lijo <lijo.lazar@amd.com> wrote:
> >
> >
> >
> > On 11/22/2022 8:19 PM, Alex Deucher wrote:
> > > On Tue, Nov 22, 2022 at 6:12 AM Xiongfeng Wang
> > > <wangxiongfeng2@huawei.com> wrote:
> > >>
> > >> As comment of pci_get_class() says, it returns a pci_device with its
> > >> refcount increased and decreased the refcount for the input parameter
> > >> @from if it is not NULL.
> > >>
> > >> If we break the loop in radeon_atrm_get_bios() with 'pdev' not NULL, we
> > >> need to call pci_dev_put() to decrease the refcount. Add the missing
> > >> pci_dev_put() to avoid refcount leak.
> > >
> > > For both patches, I think pci_dev_put() needs to go into the loops.
> > > There are 2 or more GPUs on the systems where this is relevant.
> > >
> >
> > As per the logic, the loop breaks when it finds a valid ATRM handle. So
> > dev_put is required only for that device.
>
> Sure, but what if the handle is on the second DISPLAY_VGA or
> DISPLAY_OTHER class PCI device on the system?  We've already called
> pci_get_class() for the first PCI device that matched.
>
> Alex
>
> >
> > When inside the loop this happens -  "decreased the refcount for the
> > input parameter @from if it is not NULL"
> >
> > Thanks,
> > Lijo
> >
> > > Alex
> > >
> > >>
> > >> Fixes: d8ade3526b2a ("drm/radeon: handle non-VGA class pci devices with ATRM")
> > >> Fixes: c61e2775873f ("drm/radeon: split ATRM support out from the ATPX handler (v3)")
> > >> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> > >> ---
> > >>   drivers/gpu/drm/radeon/radeon_bios.c | 1 +
> > >>   1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
> > >> index 33121655d50b..2df6ce3e32cb 100644
> > >> --- a/drivers/gpu/drm/radeon/radeon_bios.c
> > >> +++ b/drivers/gpu/drm/radeon/radeon_bios.c
> > >> @@ -227,6 +227,7 @@ static bool radeon_atrm_get_bios(struct radeon_device *rdev)
> > >>
> > >>          if (!found)
> > >>                  return false;
> > >> +       pci_dev_put(pdev);
> > >>
> > >>          rdev->bios = kmalloc(size, GFP_KERNEL);
> > >>          if (!rdev->bios) {
> > >> --
> > >> 2.20.1
> > >>

Re: [PATCH 2/2] drm/amdgpu: Fix PCI device refcount leak in amdgpu_atrm_get_bios()

[Alex Deucher]

Applied the series.  Thanks!

Alex

On Tue, Nov 22, 2022 at 6:13 AM Xiongfeng Wang
<wangxiongfeng2@huawei.com> wrote:
>
> As comment of pci_get_class() says, it returns a pci_device with its
> refcount increased and decreased the refcount for the input parameter
> @from if it is not NULL.
>
> If we break the loop in amdgpu_atrm_get_bios() with 'pdev' not NULL, we
> need to call pci_dev_put() to decrease the refcount. Add the missing
> pci_dev_put() to avoid refcount leak.
>
> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> index e363f56c72af..30c28a69e847 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> @@ -317,6 +317,7 @@ static bool amdgpu_atrm_get_bios(struct amdgpu_device *adev)
>
>         if (!found)
>                 return false;
> +       pci_dev_put(pdev);
>
>         adev->bios = kmalloc(size, GFP_KERNEL);
>         if (!adev->bios) {
> --
> 2.20.1
>

Re: [PATCH 2/2] drm/amdgpu: Fix PCI device refcount leak in amdgpu_atrm_get_bios()

[Alex Deucher]

Applied the series.  Thanks!

Alex

On Tue, Nov 22, 2022 at 6:13 AM Xiongfeng Wang
<wangxiongfeng2@huawei.com> wrote:
>
> As comment of pci_get_class() says, it returns a pci_device with its
> refcount increased and decreased the refcount for the input parameter
> @from if it is not NULL.
>
> If we break the loop in amdgpu_atrm_get_bios() with 'pdev' not NULL, we
> need to call pci_dev_put() to decrease the refcount. Add the missing
> pci_dev_put() to avoid refcount leak.
>
> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> index e363f56c72af..30c28a69e847 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> @@ -317,6 +317,7 @@ static bool amdgpu_atrm_get_bios(struct amdgpu_device *adev)
>
>         if (!found)
>                 return false;
> +       pci_dev_put(pdev);
>
>         adev->bios = kmalloc(size, GFP_KERNEL);
>         if (!adev->bios) {
> --
> 2.20.1
>