2020-09-30 20:20:33

by Dirk Gouders

Subject: BUG: amdgpu: NULL pointer dereference introduced in 5.9-rc1

Commit c1cf79ca5ced46 (drm/amdgpu: use IP discovery table for renoir)
introduced a NULL pointer dereference when booting with
amdgpu.discovery=0.

For amdgpu.discovery=0 that commit effectively removed the call of
vega10_reg_base_init(adev), so I tested the correctness of the bisect
session by restoring that function call for amdgpu_discovery == 0 and with
that change, the NULL pointer dereference does not occur:

diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
index 84d811b6e48b..2e93c5e1e7e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/soc15.c
+++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
@@ -699,7 +699,8 @@ static void soc15_reg_base_init(struct amdgpu_device *adev)
"fallback to legacy init method\n");
vega10_reg_base_init(adev);
}
- }
+ } else
+ vega10_reg_base_init(adev);
break;
case CHIP_VEGA20:
vega20_reg_base_init(adev);

Dirk

2020-10-01 14:07:03

by Alex Deucher

Subject: Re: BUG: amdgpu: NULL pointer dereference introduced in 5.9-rc1

On Wed, Sep 30, 2020 at 4:46 PM Dirk Gouders <[email protected]> wrote:
>
> Commit c1cf79ca5ced46 (drm/amdgpu: use IP discovery table for renoir)
> introduced a NULL pointer dereference when booting with
> amdgpu.discovery=0.
>
> For amdgpu.discovery=0 that commit effectively removed the call of
> vega10_reg_base_init(adev), so I tested the correctness of the bisect
> session by restoring that function call for amdgpu_discovery == 0 and with
> that change, the NULL pointer dereference does not occur:
>

Can I add your Signed-off-by?

Thanks,

Alex

> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
> index 84d811b6e48b..2e93c5e1e7e6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
> @@ -699,7 +699,8 @@ static void soc15_reg_base_init(struct amdgpu_device *adev)
> "fallback to legacy init method\n");
> vega10_reg_base_init(adev);
> }
> - }
> + } else
> + vega10_reg_base_init(adev);
> break;
> case CHIP_VEGA20:
> vega20_reg_base_init(adev);
>
> Dirk
> _______________________________________________
> amd-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

2020-10-01 20:03:32

by Dirk Gouders

Subject: [PATCH 0/1] drm/amdgpu: fix NULL pointer dereference for Renoir

Alex Deucher <[email protected]> writes:

> On Wed, Sep 30, 2020 at 4:46 PM Dirk Gouders <[email protected]> wrote:
>>
>> Commit c1cf79ca5ced46 (drm/amdgpu: use IP discovery table for renoir)
>> introduced a NULL pointer dereference when booting with
>> amdgpu.discovery=0.
>>
>> For amdgpu.discovery=0 that commit effectively removed the call of
>> vega10_reg_base_init(adev), so I tested the correctness of the bisect
>> session by restoring that function call for amdgpu_discovery == 0 and with
>> that change, the NULL pointer dereference does not occur:
>>
>
> Can I add your Signed-off-by?

I did not expect the diff to be seen as a proposed patch, not even that it
shows the correct fix.

Anyway, I did my best to create a hopefully acceptable patch with
some modification of the code that avoids "else" and an identical function call
at two places in the code.

I testet that patch with amdgpu.discovery={0,1} and together with the patch for the
first issue you helped me with. The result is no more call traces.

Thank you for your patient assistance with the two issues.

Dirk


> Thanks,
>
> Alex
>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
>> index 84d811b6e48b..2e93c5e1e7e6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
>> @@ -699,7 +699,8 @@ static void soc15_reg_base_init(struct amdgpu_device *adev)
>> "fallback to legacy init method\n");
>> vega10_reg_base_init(adev);
>> }
>> - }
>> + } else
>> + vega10_reg_base_init(adev);
>> break;
>> case CHIP_VEGA20:
>> vega20_reg_base_init(adev);
>>
>> Dirk
>> _______________________________________________
>> amd-gfx mailing list
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Dirk Gouders (1):
drm/amdgpu: fix NULL pointer dereference for Renoir

drivers/gpu/drm/amd/amdgpu/soc15.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

--
2.26.2

2020-10-01 20:03:41

by Dirk Gouders

Subject: [PATCH 1/1] drm/amdgpu: fix NULL pointer dereference for Renoir

Commit c1cf79ca5ced46 (drm/amdgpu: use IP discovery table for renoir)
introduced a NULL pointer dereference when booting with
amdgpu.discovery=0, because it removed the call of vega10_reg_base_init()
for that case.

Fix this by calling that funcion if amdgpu_discovery == 0 in addition to
the case that amdgpu_discovery_reg_base_init() failed.

Fixes: c1cf79ca5ced46 (drm/amdgpu: use IP discovery table for renoir)
Signed-off-by: Dirk Gouders <[email protected]>
Cc: Hawking Zhang <[email protected]>
Cc: Evan Quan <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/soc15.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
index 84d811b6e48b..f8cb62b326d6 100644
--- a/drivers/gpu/drm/amd/amdgpu/soc15.c
+++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
@@ -694,12 +694,12 @@ static void soc15_reg_base_init(struct amdgpu_device *adev)
* it doesn't support SRIOV. */
if (amdgpu_discovery) {
r = amdgpu_discovery_reg_base_init(adev);
- if (r) {
- DRM_WARN("failed to init reg base from ip discovery table, "
- "fallback to legacy init method\n");
- vega10_reg_base_init(adev);
- }
+ if (r == 0)
+ break;
+ DRM_WARN("failed to init reg base from ip discovery table, "
+ "fallback to legacy init method\n");
}
+ vega10_reg_base_init(adev);
break;
case CHIP_VEGA20:
vega20_reg_base_init(adev);
--
2.26.2

2020-10-01 20:39:27

by Dirk Gouders

Subject: Re: [PATCH 1/1] drm/amdgpu: fix NULL pointer dereference for Renoir

Dirk Gouders <[email protected]> writes:

> Commit c1cf79ca5ced46 (drm/amdgpu: use IP discovery table for renoir)
> introduced a NULL pointer dereference when booting with
> amdgpu.discovery=0, because it removed the call of vega10_reg_base_init()
> for that case.
>
> Fix this by calling that funcion if amdgpu_discovery == 0 in addition to
> the case that amdgpu_discovery_reg_base_init() failed.
>
> Fixes: c1cf79ca5ced46 (drm/amdgpu: use IP discovery table for renoir)
> Signed-off-by: Dirk Gouders <[email protected]>
> Cc: Hawking Zhang <[email protected]>
> Cc: Evan Quan <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/soc15.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
> index 84d811b6e48b..f8cb62b326d6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
> @@ -694,12 +694,12 @@ static void soc15_reg_base_init(struct amdgpu_device *adev)
> * it doesn't support SRIOV. */
> if (amdgpu_discovery) {
> r = amdgpu_discovery_reg_base_init(adev);
> - if (r) {
> - DRM_WARN("failed to init reg base from ip discovery table, "
> - "fallback to legacy init method\n");
> - vega10_reg_base_init(adev);
> - }
> + if (r == 0)
> + break;

Grrr, wrong indentation here.
But I will wait for your review before v1.

Dirk


> + DRM_WARN("failed to init reg base from ip discovery table, "
> + "fallback to legacy init method\n");
> }
> + vega10_reg_base_init(adev);
> break;
> case CHIP_VEGA20:
> vega20_reg_base_init(adev);

2020-10-01 20:49:29

by Alex Deucher

Subject: Re: [PATCH 1/1] drm/amdgpu: fix NULL pointer dereference for Renoir

On Thu, Oct 1, 2020 at 4:33 PM Dirk Gouders <[email protected]> wrote:
>
> Dirk Gouders <[email protected]> writes:
>
> > Commit c1cf79ca5ced46 (drm/amdgpu: use IP discovery table for renoir)
> > introduced a NULL pointer dereference when booting with
> > amdgpu.discovery=0, because it removed the call of vega10_reg_base_init()
> > for that case.
> >
> > Fix this by calling that funcion if amdgpu_discovery == 0 in addition to
> > the case that amdgpu_discovery_reg_base_init() failed.
> >
> > Fixes: c1cf79ca5ced46 (drm/amdgpu: use IP discovery table for renoir)
> > Signed-off-by: Dirk Gouders <[email protected]>
> > Cc: Hawking Zhang <[email protected]>
> > Cc: Evan Quan <[email protected]>
> > ---
> > drivers/gpu/drm/amd/amdgpu/soc15.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
> > index 84d811b6e48b..f8cb62b326d6 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
> > @@ -694,12 +694,12 @@ static void soc15_reg_base_init(struct amdgpu_device *adev)
> > * it doesn't support SRIOV. */
> > if (amdgpu_discovery) {
> > r = amdgpu_discovery_reg_base_init(adev);
> > - if (r) {
> > - DRM_WARN("failed to init reg base from ip discovery table, "
> > - "fallback to legacy init method\n");
> > - vega10_reg_base_init(adev);
> > - }
> > + if (r == 0)
> > + break;
>
> Grrr, wrong indentation here.
> But I will wait for your review before v1.

Fixed up locally and applied. Thanks!

Alex


>
> Dirk
>
>
> > + DRM_WARN("failed to init reg base from ip discovery table, "
> > + "fallback to legacy init method\n");
> > }
> > + vega10_reg_base_init(adev);
> > break;
> > case CHIP_VEGA20:
> > vega20_reg_base_init(adev);