[PATCH 0/6] Fix several bugs in error handling during device

[PATCH 0/6] Fix several bugs in error handling during device

[Jiang Liu]

This patchset tries to fix several memory leakages/invalid memory
accesses on error handling path during GPU driver loading/unloading.
They applies to:
https://github.com/ROCm/ROCK-Kernel-Driver/tree/master/drivers

Jiang Liu (6):
  amdgpu: add flags to track sysfs initialization status
  amdgpu: fix invalid memory access in kfd_cleanup_nodes()
  amdgpu: clear adev->in_suspend flag when fails to suspend
  amdgpu: fix use after free bug related to amdgpu_driver_release_kms()
  amdgpu: fix invalid memory access in amdgpu_fence_driver_sw_fini()
  amdgpu: get rid of false warnings caused by amdgpu_irq_put()

 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  3 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 47 +++++++++++++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  |  9 ++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    |  4 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h   |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c    |  1 +
 drivers/gpu/drm/amd/amdkfd/kfd_device.c    |  7 +++-
 drivers/gpu/drm/scheduler/sched_main.c     |  3 ++
 8 files changed, 60 insertions(+), 15 deletions(-)

-- 
2.43.5

[PATCH 1/6] amdgpu: add flags to track sysfs initialization status

[Jiang Liu]

Add flags to track sysfs initialization status, so we can correctly
clean them up on error recover paths.

Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  3 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 34 +++++++++++++++++-----
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 22c7e9669162..e4b13e729770 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1157,6 +1157,9 @@ struct amdgpu_device {
 	bool                            in_runpm;
 	bool                            has_pr3;
 
+	bool				sysfs_en;
+	bool				fru_sysfs_en;
+	bool				reg_state_sysfs_en;
 	bool                            ucode_sysfs_en;
 
 	struct amdgpu_fru_info		*fru_info;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index d1bb9e85b6d7..3244966b0c39 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4533,8 +4533,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 		adev->ucode_sysfs_en = true;
 
 	r = sysfs_create_files(&adev->dev->kobj, amdgpu_dev_attributes);
-	if (r)
+	if (r) {
 		dev_err(adev->dev, "Could not create amdgpu device attr\n");
+		adev->sysfs_en = false;
+	} else {
+		adev->sysfs_en = true;
+	}
+
 #ifdef HAVE_PCI_DRIVER_DEV_GROUPS
 	r = devm_device_add_group(adev->dev, &amdgpu_board_attrs_group);
 	if (r)
@@ -4542,8 +4547,21 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 			"Could not create amdgpu board attributes\n");
 #endif
 
-	amdgpu_fru_sysfs_init(adev);
-	amdgpu_reg_state_sysfs_init(adev);
+	r = amdgpu_fru_sysfs_init(adev);
+	if (r) {
+		dev_err(adev->dev, "Could not create amdgpu fru attr\n");
+		adev->fru_sysfs_en = false;
+	} else {
+		adev->fru_sysfs_en = true;
+	}
+
+	r = amdgpu_reg_state_sysfs_init(adev);
+	if (r) {
+		dev_err(adev->dev, "Could not create amdgpu reg state attr\n");
+		adev->reg_state_sysfs_en = false;
+	} else {
+		adev->reg_state_sysfs_en = true;
+	}
 
 	if (IS_ENABLED(CONFIG_PERF_EVENTS))
 		r = amdgpu_pmu_init(adev);
@@ -4666,10 +4684,12 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
 		amdgpu_pm_sysfs_fini(adev);
 	if (adev->ucode_sysfs_en)
 		amdgpu_ucode_sysfs_fini(adev);
-	sysfs_remove_files(&adev->dev->kobj, amdgpu_dev_attributes);
-	amdgpu_fru_sysfs_fini(adev);
-
-	amdgpu_reg_state_sysfs_fini(adev);
+	if (adev->sysfs_en)
+		sysfs_remove_files(&adev->dev->kobj, amdgpu_dev_attributes);
+	if (adev->fru_sysfs_en)
+		amdgpu_fru_sysfs_fini(adev);
+	if (adev->reg_state_sysfs_en)
+		amdgpu_reg_state_sysfs_fini(adev);
 
 	/* disable ras feature must before hw fini */
 	amdgpu_ras_pre_fini(adev);
-- 
2.43.5

Re: [PATCH 1/6] amdgpu: add flags to track sysfs initialization status

[Chen, Xiaogang]

On 1/1/2025 11:36 PM, Jiang Liu wrote:
> Add flags to track sysfs initialization status, so we can correctly
> clean them up on error recover paths.
>
> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  3 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 34 +++++++++++++++++-----
>   2 files changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 22c7e9669162..e4b13e729770 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1157,6 +1157,9 @@ struct amdgpu_device {
>   	bool                            in_runpm;
>   	bool                            has_pr3;
>   
> +	bool				sysfs_en;
> +	bool				fru_sysfs_en;
> +	bool				reg_state_sysfs_en;
>   	bool                            ucode_sysfs_en;
>   
>   	struct amdgpu_fru_info		*fru_info;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index d1bb9e85b6d7..3244966b0c39 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4533,8 +4533,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>   		adev->ucode_sysfs_en = true;
>   
>   	r = sysfs_create_files(&adev->dev->kobj, amdgpu_dev_attributes);
> -	if (r)
> +	if (r) {
>   		dev_err(adev->dev, "Could not create amdgpu device attr\n");
> +		adev->sysfs_en = false;
> +	} else {
> +		adev->sysfs_en = true;
> +	}
> +
not need use {}
>   #ifdef HAVE_PCI_DRIVER_DEV_GROUPS
>   	r = devm_device_add_group(adev->dev, &amdgpu_board_attrs_group);
>   	if (r)
> @@ -4542,8 +4547,21 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>   			"Could not create amdgpu board attributes\n");
>   #endif
>   
> -	amdgpu_fru_sysfs_init(adev);
> -	amdgpu_reg_state_sysfs_init(adev);
> +	r = amdgpu_fru_sysfs_init(adev);
> +	if (r) {
> +		dev_err(adev->dev, "Could not create amdgpu fru attr\n");
> +		adev->fru_sysfs_en = false;
> +	} else {
> +		adev->fru_sysfs_en = true;
> +	}
not need use {}
> +
> +	r = amdgpu_reg_state_sysfs_init(adev);
> +	if (r) {
> +		dev_err(adev->dev, "Could not create amdgpu reg state attr\n");
> +		adev->reg_state_sysfs_en = false;
> +	} else {
> +		adev->reg_state_sysfs_en = true;
> +	}
same as above
>   
>   	if (IS_ENABLED(CONFIG_PERF_EVENTS))
>   		r = amdgpu_pmu_init(adev);
> @@ -4666,10 +4684,12 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
>   		amdgpu_pm_sysfs_fini(adev);
>   	if (adev->ucode_sysfs_en)
>   		amdgpu_ucode_sysfs_fini(adev);
> -	sysfs_remove_files(&adev->dev->kobj, amdgpu_dev_attributes);
> -	amdgpu_fru_sysfs_fini(adev);
> -
> -	amdgpu_reg_state_sysfs_fini(adev);
> +	if (adev->sysfs_en)
> +		sysfs_remove_files(&adev->dev->kobj, amdgpu_dev_attributes);
> +	if (adev->fru_sysfs_en)
> +		amdgpu_fru_sysfs_fini(adev);
> +	if (adev->reg_state_sysfs_en)
> +		amdgpu_reg_state_sysfs_fini(adev);

If creations of these sys entries fail does kernel send warnings or just 
ignore that when delete these entries? Seems kernel keeps silence for 
this case.

Regards

Xiaogang


>   
>   	/* disable ras feature must before hw fini */
>   	amdgpu_ras_pre_fini(adev);

RE: [PATCH 1/6] amdgpu: add flags to track sysfs initialization status

[Russell, Kent]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Chen, Xiaogang <Xiaogang.Chen@amd.com>
> Sent: Thursday, January 2, 2025 6:08 PM
> To: Jiang Liu <gerry@linux.alibaba.com>; amd-gfx@lists.freedesktop.org; Russell,
> Kent <Kent.Russell@amd.com>; shuox.liu@linux.alibaba.com
> Subject: Re: [PATCH 1/6] amdgpu: add flags to track sysfs initialization status
>
>
> On 1/1/2025 11:36 PM, Jiang Liu wrote:
> > Add flags to track sysfs initialization status, so we can correctly
> > clean them up on error recover paths.
> >
> > Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  3 ++
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 34 +++++++++++++++++-----
> >   2 files changed, 30 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index 22c7e9669162..e4b13e729770 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -1157,6 +1157,9 @@ struct amdgpu_device {
> >     bool                            in_runpm;
> >     bool                            has_pr3;
> >
> > +   bool                            sysfs_en;
> > +   bool                            fru_sysfs_en;
> > +   bool                            reg_state_sysfs_en;
> >     bool                            ucode_sysfs_en;
> >
> >     struct amdgpu_fru_info          *fru_info;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index d1bb9e85b6d7..3244966b0c39 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -4533,8 +4533,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> >             adev->ucode_sysfs_en = true;
> >
> >     r = sysfs_create_files(&adev->dev->kobj, amdgpu_dev_attributes);
> > -   if (r)
> > +   if (r) {
> >             dev_err(adev->dev, "Could not create amdgpu device attr\n");
> > +           adev->sysfs_en = false;
> > +   } else {
> > +           adev->sysfs_en = true;
> > +   }
> > +
> not need use {}

The kernel style guide says that if one of the if blocks requires {}, that all other blocks require it too, even if the else is a single line. It's the last block before 3.1 (Spaces) at https://www.kernel.org/doc/html/v4.10/process/coding-style.html

Though with Lijo's feedback, it may get dropped altogether In v2 anyways.

 Kent

> >   #ifdef HAVE_PCI_DRIVER_DEV_GROUPS
> >     r = devm_device_add_group(adev->dev, &amdgpu_board_attrs_group);
> >     if (r)
> > @@ -4542,8 +4547,21 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> >                     "Could not create amdgpu board attributes\n");
> >   #endif
> >
> > -   amdgpu_fru_sysfs_init(adev);
> > -   amdgpu_reg_state_sysfs_init(adev);
> > +   r = amdgpu_fru_sysfs_init(adev);
> > +   if (r) {
> > +           dev_err(adev->dev, "Could not create amdgpu fru attr\n");
> > +           adev->fru_sysfs_en = false;
> > +   } else {
> > +           adev->fru_sysfs_en = true;
> > +   }
> not need use {}
> > +
> > +   r = amdgpu_reg_state_sysfs_init(adev);
> > +   if (r) {
> > +           dev_err(adev->dev, "Could not create amdgpu reg state attr\n");
> > +           adev->reg_state_sysfs_en = false;
> > +   } else {
> > +           adev->reg_state_sysfs_en = true;
> > +   }
> same as above
> >
> >     if (IS_ENABLED(CONFIG_PERF_EVENTS))
> >             r = amdgpu_pmu_init(adev);
> > @@ -4666,10 +4684,12 @@ void amdgpu_device_fini_hw(struct amdgpu_device
> *adev)
> >             amdgpu_pm_sysfs_fini(adev);
> >     if (adev->ucode_sysfs_en)
> >             amdgpu_ucode_sysfs_fini(adev);
> > -   sysfs_remove_files(&adev->dev->kobj, amdgpu_dev_attributes);
> > -   amdgpu_fru_sysfs_fini(adev);
> > -
> > -   amdgpu_reg_state_sysfs_fini(adev);
> > +   if (adev->sysfs_en)
> > +           sysfs_remove_files(&adev->dev->kobj, amdgpu_dev_attributes);
> > +   if (adev->fru_sysfs_en)
> > +           amdgpu_fru_sysfs_fini(adev);
> > +   if (adev->reg_state_sysfs_en)
> > +           amdgpu_reg_state_sysfs_fini(adev);
>
> If creations of these sys entries fail does kernel send warnings or just
> ignore that when delete these entries? Seems kernel keeps silence for
> this case.
>
> Regards
>
> Xiaogang
>
>
> >
> >     /* disable ras feature must before hw fini */
> >     amdgpu_ras_pre_fini(adev);

Re: [PATCH 1/6] amdgpu: add flags to track sysfs initialization status

[Lazar, Lijo]

On 1/2/2025 11:06 AM, Jiang Liu wrote:
> Add flags to track sysfs initialization status, so we can correctly
> clean them up on error recover paths.
> 
> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  3 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 34 +++++++++++++++++-----
>  2 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 22c7e9669162..e4b13e729770 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1157,6 +1157,9 @@ struct amdgpu_device {
>  	bool                            in_runpm;
>  	bool                            has_pr3;
>  
> +	bool				sysfs_en;
> +	bool				fru_sysfs_en;
> +	bool				reg_state_sysfs_en;

These are not required. If the files are not found, they are ignored and
not considered as errors. sysfs_remove_files() returns void.

Thanks,
Lijo

>  	bool                            ucode_sysfs_en;
>  
>  	struct amdgpu_fru_info		*fru_info;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index d1bb9e85b6d7..3244966b0c39 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4533,8 +4533,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>  		adev->ucode_sysfs_en = true;
>  
>  	r = sysfs_create_files(&adev->dev->kobj, amdgpu_dev_attributes);
> -	if (r)
> +	if (r) {
>  		dev_err(adev->dev, "Could not create amdgpu device attr\n");
> +		adev->sysfs_en = false;
> +	} else {
> +		adev->sysfs_en = true;
> +	}
> +
>  #ifdef HAVE_PCI_DRIVER_DEV_GROUPS
>  	r = devm_device_add_group(adev->dev, &amdgpu_board_attrs_group);
>  	if (r)
> @@ -4542,8 +4547,21 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>  			"Could not create amdgpu board attributes\n");
>  #endif
>  
> -	amdgpu_fru_sysfs_init(adev);
> -	amdgpu_reg_state_sysfs_init(adev);
> +	r = amdgpu_fru_sysfs_init(adev);
> +	if (r) {
> +		dev_err(adev->dev, "Could not create amdgpu fru attr\n");
> +		adev->fru_sysfs_en = false;
> +	} else {
> +		adev->fru_sysfs_en = true;
> +	}
> +
> +	r = amdgpu_reg_state_sysfs_init(adev);
> +	if (r) {
> +		dev_err(adev->dev, "Could not create amdgpu reg state attr\n");
> +		adev->reg_state_sysfs_en = false;
> +	} else {
> +		adev->reg_state_sysfs_en = true;
> +	}
>  
>  	if (IS_ENABLED(CONFIG_PERF_EVENTS))
>  		r = amdgpu_pmu_init(adev);
> @@ -4666,10 +4684,12 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
>  		amdgpu_pm_sysfs_fini(adev);
>  	if (adev->ucode_sysfs_en)
>  		amdgpu_ucode_sysfs_fini(adev);
> -	sysfs_remove_files(&adev->dev->kobj, amdgpu_dev_attributes);
> -	amdgpu_fru_sysfs_fini(adev);
> -
> -	amdgpu_reg_state_sysfs_fini(adev);
> +	if (adev->sysfs_en)
> +		sysfs_remove_files(&adev->dev->kobj, amdgpu_dev_attributes);
> +	if (adev->fru_sysfs_en)
> +		amdgpu_fru_sysfs_fini(adev);
> +	if (adev->reg_state_sysfs_en)
> +		amdgpu_reg_state_sysfs_fini(adev);
>  
>  	/* disable ras feature must before hw fini */
>  	amdgpu_ras_pre_fini(adev);

[PATCH 2/6] amdgpu: fix invalid memory access in kfd_cleanup_nodes()

[Jiang Liu]

On error recover path during device probe, it may trigger invalid
memory access as below:
024-12-25 12:00:53 [ 2703.773040] general protection fault, probably for non-canonical address 0x52445f4749464e4f: 0000 [#1] SMP NOPTI
2024-12-25 12:00:53 [ 2703.785199] CPU: 157 PID: 151951 Comm: rmmod Kdump: loaded Tainted: G        W  OE     5.10.134-17.2.al8.x86_64 #1
2024-12-25 12:00:53 [ 2703.797515] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
2024-12-25 12:00:53 [ 2703.809188] RIP: 0010:kgd2kfd_device_exit+0x6/0x60 [amdgpu]
2024-12-25 12:00:53 [ 2703.816136] Code: ff 48 c7 83 38 01 00 00 80 45 e4 c0 c7 83 40 01 00 00 08 0f 00 00 e9 cd fa ff ff 66 0f 1f 84 00 00 00 00 00 0f
1f 44 00 00 55 <80> bf 28 01 00 00 00 48 89 fd 75 09 48 89 ef 5d e9 65 df 9d f4 8b
2024-12-25 12:00:54 [ 2703.838622] RSP: 0018:ffffb5313df07e10 EFLAGS: 00010202
2024-12-25 12:00:54 [ 2703.845216] RAX: 0000000000000000 RBX: ffff97ad689a3ff0 RCX: 0000000080400014
2024-12-25 12:00:54 [ 2703.853935] RDX: 0000000080400015 RSI: ffff97ad627e93d8 RDI: 52445f4749464e4f
2024-12-25 12:00:54 [ 2703.862652] RBP: ffff97ad689a3ff0 R08: 0000000000000000 R09: ffffffffb5814c00
2024-12-25 12:00:54 [ 2703.871368] R10: ffff97ad627e9280 R11: 0000000000000001 R12: ffffb5313df07e98
2024-12-25 12:00:54 [ 2703.880068] R13: ffff97ad689a1810 R14: 0000000000000001 R15: 0000000000000000
2024-12-25 12:00:54 [ 2703.888768] FS:  00007fa4db81e740(0000) GS:ffff98a93ec80000(0000) knlGS:0000000000000000
2024-12-25 12:00:54 [ 2703.898547] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
2024-12-25 12:00:54 [ 2703.905684] CR2: 00007f4502deca00 CR3: 000001008fc50001 CR4: 0000000002770ee0
2024-12-25 12:00:54 [ 2703.914382] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
2024-12-25 12:00:54 [ 2703.923066] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
2024-12-25 12:00:54 [ 2703.931746] PKRU: 55555554
2024-12-25 12:00:54 [ 2703.935444] Call Trace:
2024-12-25 12:00:54 [ 2703.938962]  amdgpu_amdkfd_device_fini_sw+0x1a/0x40 [amdgpu]
2024-12-25 12:00:54 [ 2703.946080]  amdgpu_device_ip_fini.isra.0+0x3d/0x1b0 [amdgpu]
2024-12-25 12:00:54 [ 2703.953278]  amdgpu_device_fini_sw+0x2a/0x240 [amdgpu]
2024-12-25 12:00:54 [ 2703.959789]  amdgpu_driver_release_kms+0x12/0x30 [amdgpu]
2024-12-25 12:00:54 [ 2703.966501]  devm_drm_dev_init_release+0x42/0x70 [drm]
2024-12-25 12:00:54 [ 2703.972891]  release_nodes+0x6e/0xb0
2024-12-25 12:00:54 [ 2703.977522]  amdgpu_xcp_drv_release+0x38/0x80 [amdxcp]
2024-12-25 12:00:54 [ 2703.983906]  __do_sys_delete_module.constprop.0+0x138/0x2a0
2024-12-25 12:00:54 [ 2703.990775]  ? exit_to_user_mode_loop+0xd6/0x120
2024-12-25 12:00:54 [ 2703.996563]  do_syscall_64+0x2e/0x50
2024-12-25 12:00:54 [ 2704.001166]  entry_SYSCALL_64_after_hwframe+0x62/0xc7
2024-12-25 12:00:54 [ 2704.007432] RIP: 0033:0x7fa4db2620cb
2024-12-25 12:00:54 [ 2704.012025] Code: 73 01 c3 48 8b 0d a5 6d 19 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 b0
00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 75 6d 19 00 f7 d8 64 89 01 48

Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_device.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
index b6c5ffd4630b..58c1b5fcf785 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
@@ -663,6 +663,8 @@ static void kfd_cleanup_nodes(struct kfd_dev *kfd, unsigned int num_nodes)
 
 	for (i = 0; i < num_nodes; i++) {
 		knode = kfd->nodes[i];
+		if (!knode)
+			continue;
 		device_queue_manager_uninit(knode->dqm);
 		kfd_interrupt_exit(knode);
 		kfd_topology_remove_device(knode);
@@ -954,7 +956,10 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
 		kfd_doorbell_fini(kfd);
 		ida_destroy(&kfd->doorbell_ida);
 		kfd_gtt_sa_fini(kfd);
-		amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
+		if (kfd->gtt_mem) {
+			amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
+			kfd->gtt_mem = NULL;
+		}
 	}
 
 	kfree(kfd);
-- 
2.43.5

Re: [PATCH 2/6] amdgpu: fix invalid memory access in kfd_cleanup_nodes()

[Chen, Xiaogang]

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


On 1/1/2025 11:36 PM, Jiang Liu wrote:
> On error recover path during device probe, it may trigger invalid
> memory access as below:
> 024-12-25 12:00:53 [ 2703.773040] general protection fault, probably for non-canonical address 0x52445f4749464e4f: 0000 [#1] SMP NOPTI
> 2024-12-25 12:00:53 [ 2703.785199] CPU: 157 PID: 151951 Comm: rmmod Kdump: loaded Tainted: G        W  OE     5.10.134-17.2.al8.x86_64 #1
> 2024-12-25 12:00:53 [ 2703.797515] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
> 2024-12-25 12:00:53 [ 2703.809188] RIP: 0010:kgd2kfd_device_exit+0x6/0x60 [amdgpu]
> 2024-12-25 12:00:53 [ 2703.816136] Code: ff 48 c7 83 38 01 00 00 80 45 e4 c0 c7 83 40 01 00 00 08 0f 00 00 e9 cd fa ff ff 66 0f 1f 84 00 00 00 00 00 0f
> 1f 44 00 00 55 <80> bf 28 01 00 00 00 48 89 fd 75 09 48 89 ef 5d e9 65 df 9d f4 8b
> 2024-12-25 12:00:54 [ 2703.838622] RSP: 0018:ffffb5313df07e10 EFLAGS: 00010202
> 2024-12-25 12:00:54 [ 2703.845216] RAX: 0000000000000000 RBX: ffff97ad689a3ff0 RCX: 0000000080400014
> 2024-12-25 12:00:54 [ 2703.853935] RDX: 0000000080400015 RSI: ffff97ad627e93d8 RDI: 52445f4749464e4f
> 2024-12-25 12:00:54 [ 2703.862652] RBP: ffff97ad689a3ff0 R08: 0000000000000000 R09: ffffffffb5814c00
> 2024-12-25 12:00:54 [ 2703.871368] R10: ffff97ad627e9280 R11: 0000000000000001 R12: ffffb5313df07e98
> 2024-12-25 12:00:54 [ 2703.880068] R13: ffff97ad689a1810 R14: 0000000000000001 R15: 0000000000000000
> 2024-12-25 12:00:54 [ 2703.888768] FS:  00007fa4db81e740(0000) GS:ffff98a93ec80000(0000) knlGS:0000000000000000
> 2024-12-25 12:00:54 [ 2703.898547] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> 2024-12-25 12:00:54 [ 2703.905684] CR2: 00007f4502deca00 CR3: 000001008fc50001 CR4: 0000000002770ee0
> 2024-12-25 12:00:54 [ 2703.914382] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> 2024-12-25 12:00:54 [ 2703.923066] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
> 2024-12-25 12:00:54 [ 2703.931746] PKRU: 55555554
> 2024-12-25 12:00:54 [ 2703.935444] Call Trace:
> 2024-12-25 12:00:54 [ 2703.938962]  amdgpu_amdkfd_device_fini_sw+0x1a/0x40 [amdgpu]
> 2024-12-25 12:00:54 [ 2703.946080]  amdgpu_device_ip_fini.isra.0+0x3d/0x1b0 [amdgpu]
> 2024-12-25 12:00:54 [ 2703.953278]  amdgpu_device_fini_sw+0x2a/0x240 [amdgpu]
> 2024-12-25 12:00:54 [ 2703.959789]  amdgpu_driver_release_kms+0x12/0x30 [amdgpu]
> 2024-12-25 12:00:54 [ 2703.966501]  devm_drm_dev_init_release+0x42/0x70 [drm]
> 2024-12-25 12:00:54 [ 2703.972891]  release_nodes+0x6e/0xb0
> 2024-12-25 12:00:54 [ 2703.977522]  amdgpu_xcp_drv_release+0x38/0x80 [amdxcp]
> 2024-12-25 12:00:54 [ 2703.983906]  __do_sys_delete_module.constprop.0+0x138/0x2a0
> 2024-12-25 12:00:54 [ 2703.990775]  ? exit_to_user_mode_loop+0xd6/0x120
> 2024-12-25 12:00:54 [ 2703.996563]  do_syscall_64+0x2e/0x50
> 2024-12-25 12:00:54 [ 2704.001166]  entry_SYSCALL_64_after_hwframe+0x62/0xc7
> 2024-12-25 12:00:54 [ 2704.007432] RIP: 0033:0x7fa4db2620cb
> 2024-12-25 12:00:54 [ 2704.012025] Code: 73 01 c3 48 8b 0d a5 6d 19 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 b0
> 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 75 6d 19 00 f7 d8 64 89 01 48
>
> Signed-off-by: Jiang Liu<gerry@linux.alibaba.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_device.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> index b6c5ffd4630b..58c1b5fcf785 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> @@ -663,6 +663,8 @@ static void kfd_cleanup_nodes(struct kfd_dev *kfd, unsigned int num_nodes)
>   
>   	for (i = 0; i < num_nodes; i++) {
>   		knode = kfd->nodes[i];
> +		if (!knode)
> +			continue;

I wonder how knode can be null here? kfd_cleanup_nodes is called by 
kgd2kfd_device_exit under condition if (kfd->init_complete). 
kfd->init_complete is true only after all kfd node got initialized at 
kgd2kfd_device_init. If kfd driver init fail kfd->init_complete would be 
false, then kfd_cleanup_node would not get called.

Regards

Xiaogang

>   		device_queue_manager_uninit(knode->dqm);
>   		kfd_interrupt_exit(knode);
>   		kfd_topology_remove_device(knode);
> @@ -954,7 +956,10 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
>   		kfd_doorbell_fini(kfd);
>   		ida_destroy(&kfd->doorbell_ida);
>   		kfd_gtt_sa_fini(kfd);
> -		amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
> +		if (kfd->gtt_mem) {
> +			amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
> +			kfd->gtt_mem = NULL;
> +		}
>   	}
>   
>   	kfree(kfd);

[-- Attachment #2: Type: text/html, Size: 5540 bytes --]

Re: [PATCH 2/6] amdgpu: fix invalid memory access in kfd_cleanup_nodes()

[Gerry Liu]

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



> 2025年1月3日 07:08，Chen, Xiaogang <xiaogang.chen@amd.com> 写道：
> 
> 
> 
> On 1/1/2025 11:36 PM, Jiang Liu wrote:
>> On error recover path during device probe, it may trigger invalid
>> memory access as below:
>> 024-12-25 12:00:53 [ 2703.773040] general protection fault, probably for non-canonical address 0x52445f4749464e4f: 0000 [#1] SMP NOPTI
>> 2024-12-25 12:00:53 [ 2703.785199] CPU: 157 PID: 151951 Comm: rmmod Kdump: loaded Tainted: G        W  OE     5.10.134-17.2.al8.x86_64 #1
>> 2024-12-25 12:00:53 [ 2703.797515] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
>> 2024-12-25 12:00:53 [ 2703.809188] RIP: 0010:kgd2kfd_device_exit+0x6/0x60 [amdgpu]
>> 2024-12-25 12:00:53 [ 2703.816136] Code: ff 48 c7 83 38 01 00 00 80 45 e4 c0 c7 83 40 01 00 00 08 0f 00 00 e9 cd fa ff ff 66 0f 1f 84 00 00 00 00 00 0f
>> 1f 44 00 00 55 <80> bf 28 01 00 00 00 48 89 fd 75 09 48 89 ef 5d e9 65 df 9d f4 8b
>> 2024-12-25 12:00:54 [ 2703.838622] RSP: 0018:ffffb5313df07e10 EFLAGS: 00010202
>> 2024-12-25 12:00:54 [ 2703.845216] RAX: 0000000000000000 RBX: ffff97ad689a3ff0 RCX: 0000000080400014
>> 2024-12-25 12:00:54 [ 2703.853935] RDX: 0000000080400015 RSI: ffff97ad627e93d8 RDI: 52445f4749464e4f
>> 2024-12-25 12:00:54 [ 2703.862652] RBP: ffff97ad689a3ff0 R08: 0000000000000000 R09: ffffffffb5814c00
>> 2024-12-25 12:00:54 [ 2703.871368] R10: ffff97ad627e9280 R11: 0000000000000001 R12: ffffb5313df07e98
>> 2024-12-25 12:00:54 [ 2703.880068] R13: ffff97ad689a1810 R14: 0000000000000001 R15: 0000000000000000
>> 2024-12-25 12:00:54 [ 2703.888768] FS:  00007fa4db81e740(0000) GS:ffff98a93ec80000(0000) knlGS:0000000000000000
>> 2024-12-25 12:00:54 [ 2703.898547] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> 2024-12-25 12:00:54 [ 2703.905684] CR2: 00007f4502deca00 CR3: 000001008fc50001 CR4: 0000000002770ee0
>> 2024-12-25 12:00:54 [ 2703.914382] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> 2024-12-25 12:00:54 [ 2703.923066] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
>> 2024-12-25 12:00:54 [ 2703.931746] PKRU: 55555554
>> 2024-12-25 12:00:54 [ 2703.935444] Call Trace:
>> 2024-12-25 12:00:54 [ 2703.938962]  amdgpu_amdkfd_device_fini_sw+0x1a/0x40 [amdgpu]
>> 2024-12-25 12:00:54 [ 2703.946080]  amdgpu_device_ip_fini.isra.0+0x3d/0x1b0 [amdgpu]
>> 2024-12-25 12:00:54 [ 2703.953278]  amdgpu_device_fini_sw+0x2a/0x240 [amdgpu]
>> 2024-12-25 12:00:54 [ 2703.959789]  amdgpu_driver_release_kms+0x12/0x30 [amdgpu]
>> 2024-12-25 12:00:54 [ 2703.966501]  devm_drm_dev_init_release+0x42/0x70 [drm]
>> 2024-12-25 12:00:54 [ 2703.972891]  release_nodes+0x6e/0xb0
>> 2024-12-25 12:00:54 [ 2703.977522]  amdgpu_xcp_drv_release+0x38/0x80 [amdxcp]
>> 2024-12-25 12:00:54 [ 2703.983906]  __do_sys_delete_module.constprop.0+0x138/0x2a0
>> 2024-12-25 12:00:54 [ 2703.990775]  ? exit_to_user_mode_loop+0xd6/0x120
>> 2024-12-25 12:00:54 [ 2703.996563]  do_syscall_64+0x2e/0x50
>> 2024-12-25 12:00:54 [ 2704.001166]  entry_SYSCALL_64_after_hwframe+0x62/0xc7
>> 2024-12-25 12:00:54 [ 2704.007432] RIP: 0033:0x7fa4db2620cb
>> 2024-12-25 12:00:54 [ 2704.012025] Code: 73 01 c3 48 8b 0d a5 6d 19 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 b0
>> 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 75 6d 19 00 f7 d8 64 89 01 48
>> 
>> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com> <mailto:gerry@linux.alibaba.com>
>> ---
>>  drivers/gpu/drm/amd/amdkfd/kfd_device.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>> index b6c5ffd4630b..58c1b5fcf785 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>> @@ -663,6 +663,8 @@ static void kfd_cleanup_nodes(struct kfd_dev *kfd, unsigned int num_nodes)
>>  
>>  	for (i = 0; i < num_nodes; i++) {
>>  		knode = kfd->nodes[i];
>> +		if (!knode)
>> +			continue;
> I wonder how knode can be null here? kfd_cleanup_nodes is called by kgd2kfd_device_exit under condition if (kfd->init_complete). kfd->init_complete is true only after all kfd node got initialized at kgd2kfd_device_init. If kfd driver init fail  kfd->init_complete would be false, then kfd_cleanup_node would not get called.
> 
Hi Xiaogang,
	It may get triggered on error handling path of ‘kid2kfd_device_init()`, when it jump to label `node_alloc_error` and 
then call `kfd_cleanup_nodes()`.

Thanks,
Gerry

> 
> Regards
> 
> Xiaogang
> 
>>  		device_queue_manager_uninit(knode->dqm);
>>  		kfd_interrupt_exit(knode);
>>  		kfd_topology_remove_device(knode);
>> @@ -954,7 +956,10 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
>>  		kfd_doorbell_fini(kfd);
>>  		ida_destroy(&kfd->doorbell_ida);
>>  		kfd_gtt_sa_fini(kfd);
>> -		amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>> +		if (kfd->gtt_mem) {
>> +			amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>> +			kfd->gtt_mem = NULL;
>> +		}
>>  	}
>>  
>>  	kfree(kfd);


[-- Attachment #2: Type: text/html, Size: 6614 bytes --]

Re: [PATCH 2/6] amdgpu: fix invalid memory access in kfd_cleanup_nodes()

[Chen, Xiaogang]

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


On 1/2/2025 8:22 PM, Gerry Liu wrote:
>
>
>> 2025年1月3日 07:08，Chen, Xiaogang <xiaogang.chen@amd.com> 写道：
>>
>>
>> On 1/1/2025 11:36 PM, Jiang Liu wrote:
>>> On error recover path during device probe, it may trigger invalid
>>> memory access as below:
>>> 024-12-25 12:00:53 [ 2703.773040] general protection fault, probably for non-canonical address 0x52445f4749464e4f: 0000 [#1] SMP NOPTI
>>> 2024-12-25 12:00:53 [ 2703.785199] CPU: 157 PID: 151951 Comm: rmmod Kdump: loaded Tainted: G        W  OE     5.10.134-17.2.al8.x86_64 #1
>>> 2024-12-25 12:00:53 [ 2703.797515] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
>>> 2024-12-25 12:00:53 [ 2703.809188] RIP: 0010:kgd2kfd_device_exit+0x6/0x60 [amdgpu]
>>> 2024-12-25 12:00:53 [ 2703.816136] Code: ff 48 c7 83 38 01 00 00 80 45 e4 c0 c7 83 40 01 00 00 08 0f 00 00 e9 cd fa ff ff 66 0f 1f 84 00 00 00 00 00 0f
>>> 1f 44 00 00 55 <80> bf 28 01 00 00 00 48 89 fd 75 09 48 89 ef 5d e9 65 df 9d f4 8b
>>> 2024-12-25 12:00:54 [ 2703.838622] RSP: 0018:ffffb5313df07e10 EFLAGS: 00010202
>>> 2024-12-25 12:00:54 [ 2703.845216] RAX: 0000000000000000 RBX: ffff97ad689a3ff0 RCX: 0000000080400014
>>> 2024-12-25 12:00:54 [ 2703.853935] RDX: 0000000080400015 RSI: ffff97ad627e93d8 RDI: 52445f4749464e4f
>>> 2024-12-25 12:00:54 [ 2703.862652] RBP: ffff97ad689a3ff0 R08: 0000000000000000 R09: ffffffffb5814c00
>>> 2024-12-25 12:00:54 [ 2703.871368] R10: ffff97ad627e9280 R11: 0000000000000001 R12: ffffb5313df07e98
>>> 2024-12-25 12:00:54 [ 2703.880068] R13: ffff97ad689a1810 R14: 0000000000000001 R15: 0000000000000000
>>> 2024-12-25 12:00:54 [ 2703.888768] FS:  00007fa4db81e740(0000) GS:ffff98a93ec80000(0000) knlGS:0000000000000000
>>> 2024-12-25 12:00:54 [ 2703.898547] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> 2024-12-25 12:00:54 [ 2703.905684] CR2: 00007f4502deca00 CR3: 000001008fc50001 CR4: 0000000002770ee0
>>> 2024-12-25 12:00:54 [ 2703.914382] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>> 2024-12-25 12:00:54 [ 2703.923066] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
>>> 2024-12-25 12:00:54 [ 2703.931746] PKRU: 55555554
>>> 2024-12-25 12:00:54 [ 2703.935444] Call Trace:
>>> 2024-12-25 12:00:54 [ 2703.938962]  amdgpu_amdkfd_device_fini_sw+0x1a/0x40 [amdgpu]
>>> 2024-12-25 12:00:54 [ 2703.946080]  amdgpu_device_ip_fini.isra.0+0x3d/0x1b0 [amdgpu]
>>> 2024-12-25 12:00:54 [ 2703.953278]  amdgpu_device_fini_sw+0x2a/0x240 [amdgpu]
>>> 2024-12-25 12:00:54 [ 2703.959789]  amdgpu_driver_release_kms+0x12/0x30 [amdgpu]
>>> 2024-12-25 12:00:54 [ 2703.966501]  devm_drm_dev_init_release+0x42/0x70 [drm]
>>> 2024-12-25 12:00:54 [ 2703.972891]  release_nodes+0x6e/0xb0
>>> 2024-12-25 12:00:54 [ 2703.977522]  amdgpu_xcp_drv_release+0x38/0x80 [amdxcp]
>>> 2024-12-25 12:00:54 [ 2703.983906]  __do_sys_delete_module.constprop.0+0x138/0x2a0
>>> 2024-12-25 12:00:54 [ 2703.990775]  ? exit_to_user_mode_loop+0xd6/0x120
>>> 2024-12-25 12:00:54 [ 2703.996563]  do_syscall_64+0x2e/0x50
>>> 2024-12-25 12:00:54 [ 2704.001166]  entry_SYSCALL_64_after_hwframe+0x62/0xc7
>>> 2024-12-25 12:00:54 [ 2704.007432] RIP: 0033:0x7fa4db2620cb
>>> 2024-12-25 12:00:54 [ 2704.012025] Code: 73 01 c3 48 8b 0d a5 6d 19 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 b0
>>> 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 75 6d 19 00 f7 d8 64 89 01 48
>>>
>>> Signed-off-by: Jiang Liu<gerry@linux.alibaba.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdkfd/kfd_device.c | 7 ++++++-
>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>> index b6c5ffd4630b..58c1b5fcf785 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>> @@ -663,6 +663,8 @@ static void kfd_cleanup_nodes(struct kfd_dev *kfd, unsigned int num_nodes)
>>>   
>>>   	for (i = 0; i < num_nodes; i++) {
>>>   		knode = kfd->nodes[i];
>>> +		if (!knode)
>>> +			continue;
>>
>> I wonder how knode can be null here? kfd_cleanup_nodes is called by 
>> kgd2kfd_device_exit under condition if (kfd->init_complete). 
>> kfd->init_complete is true only after all kfd node got initialized at 
>> kgd2kfd_device_init. If kfd driver init fail kfd->init_complete would 
>> be false, then kfd_cleanup_node would not get called.
>>
> Hi Xiaogang,
> It may get triggered on error handling path of 
> ‘kid2kfd_device_init()`, when it jump to label `node_alloc_error` and
> then call `kfd_cleanup_nodes()`.
>
If it was the case kzalloc failed. That means there is no memory 
available even to allocate struct kfd_node. From the backtrace the 
general protection fault happened at

RIP: 0010:kgd2kfd_device_exit+0x6/0x60 [amdgpu]

It happened during driver module got released, not init time. I do not see how the patch is related to the issue you talked.

Regards
Xiaogang


> Thanks,
> Gerry
>
>>
>> Regards
>>
>> Xiaogang
>>
>>>   		device_queue_manager_uninit(knode->dqm);
>>>   		kfd_interrupt_exit(knode);
>>>   		kfd_topology_remove_device(knode);
>>> @@ -954,7 +956,10 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
>>>   		kfd_doorbell_fini(kfd);
>>>   		ida_destroy(&kfd->doorbell_ida);
>>>   		kfd_gtt_sa_fini(kfd);
>>> -		amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>>> +		if (kfd->gtt_mem) {
>>> +			amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>>> +			kfd->gtt_mem = NULL;
>>> +		}
>>>   	}
>>>   
>>>   	kfree(kfd);
>

[-- Attachment #2: Type: text/html, Size: 7839 bytes --]

Re: [PATCH 2/6] amdgpu: fix invalid memory access in kfd_cleanup_nodes()

[Gerry Liu]

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



> 2025年1月3日 13:44，Chen, Xiaogang <xiaogang.chen@amd.com> 写道：
> 
> 
> 
> On 1/2/2025 8:22 PM, Gerry Liu wrote:
>> 
>> 
>>> 2025年1月3日 07:08，Chen, Xiaogang <xiaogang.chen@amd.com <mailto:xiaogang.chen@amd.com>> 写道：
>>> 
>>> 
>>> 
>>> On 1/1/2025 11:36 PM, Jiang Liu wrote:
>>>> On error recover path during device probe, it may trigger invalid
>>>> memory access as below:
>>>> 024-12-25 12:00:53 [ 2703.773040] general protection fault, probably for non-canonical address 0x52445f4749464e4f: 0000 [#1] SMP NOPTI
>>>> 2024-12-25 12:00:53 [ 2703.785199] CPU: 157 PID: 151951 Comm: rmmod Kdump: loaded Tainted: G        W  OE     5.10.134-17.2.al8.x86_64 #1
>>>> 2024-12-25 12:00:53 [ 2703.797515] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
>>>> 2024-12-25 12:00:53 [ 2703.809188] RIP: 0010:kgd2kfd_device_exit+0x6/0x60 [amdgpu]
>>>> 2024-12-25 12:00:53 [ 2703.816136] Code: ff 48 c7 83 38 01 00 00 80 45 e4 c0 c7 83 40 01 00 00 08 0f 00 00 e9 cd fa ff ff 66 0f 1f 84 00 00 00 00 00 0f
>>>> 1f 44 00 00 55 <80> bf 28 01 00 00 00 48 89 fd 75 09 48 89 ef 5d e9 65 df 9d f4 8b
>>>> 2024-12-25 12:00:54 [ 2703.838622] RSP: 0018:ffffb5313df07e10 EFLAGS: 00010202
>>>> 2024-12-25 12:00:54 [ 2703.845216] RAX: 0000000000000000 RBX: ffff97ad689a3ff0 RCX: 0000000080400014
>>>> 2024-12-25 12:00:54 [ 2703.853935] RDX: 0000000080400015 RSI: ffff97ad627e93d8 RDI: 52445f4749464e4f
>>>> 2024-12-25 12:00:54 [ 2703.862652] RBP: ffff97ad689a3ff0 R08: 0000000000000000 R09: ffffffffb5814c00
>>>> 2024-12-25 12:00:54 [ 2703.871368] R10: ffff97ad627e9280 R11: 0000000000000001 R12: ffffb5313df07e98
>>>> 2024-12-25 12:00:54 [ 2703.880068] R13: ffff97ad689a1810 R14: 0000000000000001 R15: 0000000000000000
>>>> 2024-12-25 12:00:54 [ 2703.888768] FS:  00007fa4db81e740(0000) GS:ffff98a93ec80000(0000) knlGS:0000000000000000
>>>> 2024-12-25 12:00:54 [ 2703.898547] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> 2024-12-25 12:00:54 [ 2703.905684] CR2: 00007f4502deca00 CR3: 000001008fc50001 CR4: 0000000002770ee0
>>>> 2024-12-25 12:00:54 [ 2703.914382] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>>> 2024-12-25 12:00:54 [ 2703.923066] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
>>>> 2024-12-25 12:00:54 [ 2703.931746] PKRU: 55555554
>>>> 2024-12-25 12:00:54 [ 2703.935444] Call Trace:
>>>> 2024-12-25 12:00:54 [ 2703.938962]  amdgpu_amdkfd_device_fini_sw+0x1a/0x40 [amdgpu]
>>>> 2024-12-25 12:00:54 [ 2703.946080]  amdgpu_device_ip_fini.isra.0+0x3d/0x1b0 [amdgpu]
>>>> 2024-12-25 12:00:54 [ 2703.953278]  amdgpu_device_fini_sw+0x2a/0x240 [amdgpu]
>>>> 2024-12-25 12:00:54 [ 2703.959789]  amdgpu_driver_release_kms+0x12/0x30 [amdgpu]
>>>> 2024-12-25 12:00:54 [ 2703.966501]  devm_drm_dev_init_release+0x42/0x70 [drm]
>>>> 2024-12-25 12:00:54 [ 2703.972891]  release_nodes+0x6e/0xb0
>>>> 2024-12-25 12:00:54 [ 2703.977522]  amdgpu_xcp_drv_release+0x38/0x80 [amdxcp]
>>>> 2024-12-25 12:00:54 [ 2703.983906]  __do_sys_delete_module.constprop.0+0x138/0x2a0
>>>> 2024-12-25 12:00:54 [ 2703.990775]  ? exit_to_user_mode_loop+0xd6/0x120
>>>> 2024-12-25 12:00:54 [ 2703.996563]  do_syscall_64+0x2e/0x50
>>>> 2024-12-25 12:00:54 [ 2704.001166]  entry_SYSCALL_64_after_hwframe+0x62/0xc7
>>>> 2024-12-25 12:00:54 [ 2704.007432] RIP: 0033:0x7fa4db2620cb
>>>> 2024-12-25 12:00:54 [ 2704.012025] Code: 73 01 c3 48 8b 0d a5 6d 19 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 b0
>>>> 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 75 6d 19 00 f7 d8 64 89 01 48
>>>> 
>>>> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com> <mailto:gerry@linux.alibaba.com>
>>>> ---
>>>>  drivers/gpu/drm/amd/amdkfd/kfd_device.c | 7 ++++++-
>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>>> index b6c5ffd4630b..58c1b5fcf785 100644
>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>>> @@ -663,6 +663,8 @@ static void kfd_cleanup_nodes(struct kfd_dev *kfd, unsigned int num_nodes)
>>>>  
>>>>  	for (i = 0; i < num_nodes; i++) {
>>>>  		knode = kfd->nodes[i];
>>>> +		if (!knode)
>>>> +			continue;
>>> I wonder how knode can be null here? kfd_cleanup_nodes is called by kgd2kfd_device_exit under condition if (kfd->init_complete). kfd->init_complete is true only after all kfd node got initialized at kgd2kfd_device_init. If kfd driver init fail  kfd->init_complete would be false, then kfd_cleanup_node would not get called.
>>> 
>> Hi Xiaogang,
>> 	It may get triggered on error handling path of ‘kid2kfd_device_init()`, when it jump to label `node_alloc_error` and 
>> then call `kfd_cleanup_nodes()`.
>> 
> If it was the case kzalloc failed. That means there is no memory available even to allocate struct kfd_node. From the backtrace the  general protection fault happened at 
> 
> RIP: 0010:kgd2kfd_device_exit+0x6/0x60 [amdgpu]
> 
> It happened during driver module got released, not init time. I do not see how the patch is related to the issue you talked.
Aha, it’s caused by another bug which got fixed by "[PATCH 4/6] amdgpu: fix use after free bug related to amdgpu_driver_release_kms()”.
Without the fix in "[PATCH 4/6] amdgpu: fix use after free bug related to amdgpu_driver_release_kms()”, kgd2kfd_device_exit() will got called
twice in case of device probe failure. I caused me some time to figure out the double free issue.
Actually we should reset kfd->init_completed to false in function kgd2kfd_device_exit().

> 
> Regards
> Xiaogang
> 
> 
> 
>> Thanks,
>> Gerry
>> 
>>> 
>>> Regards
>>> 
>>> Xiaogang
>>> 
>>>>  		device_queue_manager_uninit(knode->dqm);
>>>>  		kfd_interrupt_exit(knode);
>>>>  		kfd_topology_remove_device(knode);
>>>> @@ -954,7 +956,10 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
>>>>  		kfd_doorbell_fini(kfd);
>>>>  		ida_destroy(&kfd->doorbell_ida);
>>>>  		kfd_gtt_sa_fini(kfd);
>>>> -		amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>>>> +		if (kfd->gtt_mem) {
>>>> +			amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>>>> +			kfd->gtt_mem = NULL;
>>>> +		}
>>>>  	}
>>>>  
>>>>  	kfree(kfd);
>> 


[-- Attachment #2: Type: text/html, Size: 10105 bytes --]

Re: [PATCH 2/6] amdgpu: fix invalid memory access in kfd_cleanup_nodes()

[Chen, Xiaogang]

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


On 1/2/2025 11:55 PM, Gerry Liu wrote:
>
>
>> 2025年1月3日 13:44，Chen, Xiaogang <xiaogang.chen@amd.com> 写道：
>>
>>
>> On 1/2/2025 8:22 PM, Gerry Liu wrote:
>>>
>>>
>>>> 2025年1月3日 07:08，Chen, Xiaogang <xiaogang.chen@amd.com> 写道：
>>>>
>>>>
>>>> On 1/1/2025 11:36 PM, Jiang Liu wrote:
>>>>> On error recover path during device probe, it may trigger invalid
>>>>> memory access as below:
>>>>> 024-12-25 12:00:53 [ 2703.773040] general protection fault, probably for non-canonical address 0x52445f4749464e4f: 0000 [#1] SMP NOPTI
>>>>> 2024-12-25 12:00:53 [ 2703.785199] CPU: 157 PID: 151951 Comm: rmmod Kdump: loaded Tainted: G        W  OE     5.10.134-17.2.al8.x86_64 #1
>>>>> 2024-12-25 12:00:53 [ 2703.797515] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
>>>>> 2024-12-25 12:00:53 [ 2703.809188] RIP: 0010:kgd2kfd_device_exit+0x6/0x60 [amdgpu]
>>>>> 2024-12-25 12:00:53 [ 2703.816136] Code: ff 48 c7 83 38 01 00 00 80 45 e4 c0 c7 83 40 01 00 00 08 0f 00 00 e9 cd fa ff ff 66 0f 1f 84 00 00 00 00 00 0f
>>>>> 1f 44 00 00 55 <80> bf 28 01 00 00 00 48 89 fd 75 09 48 89 ef 5d e9 65 df 9d f4 8b
>>>>> 2024-12-25 12:00:54 [ 2703.838622] RSP: 0018:ffffb5313df07e10 EFLAGS: 00010202
>>>>> 2024-12-25 12:00:54 [ 2703.845216] RAX: 0000000000000000 RBX: ffff97ad689a3ff0 RCX: 0000000080400014
>>>>> 2024-12-25 12:00:54 [ 2703.853935] RDX: 0000000080400015 RSI: ffff97ad627e93d8 RDI: 52445f4749464e4f
>>>>> 2024-12-25 12:00:54 [ 2703.862652] RBP: ffff97ad689a3ff0 R08: 0000000000000000 R09: ffffffffb5814c00
>>>>> 2024-12-25 12:00:54 [ 2703.871368] R10: ffff97ad627e9280 R11: 0000000000000001 R12: ffffb5313df07e98
>>>>> 2024-12-25 12:00:54 [ 2703.880068] R13: ffff97ad689a1810 R14: 0000000000000001 R15: 0000000000000000
>>>>> 2024-12-25 12:00:54 [ 2703.888768] FS:  00007fa4db81e740(0000) GS:ffff98a93ec80000(0000) knlGS:0000000000000000
>>>>> 2024-12-25 12:00:54 [ 2703.898547] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>> 2024-12-25 12:00:54 [ 2703.905684] CR2: 00007f4502deca00 CR3: 000001008fc50001 CR4: 0000000002770ee0
>>>>> 2024-12-25 12:00:54 [ 2703.914382] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>>>> 2024-12-25 12:00:54 [ 2703.923066] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
>>>>> 2024-12-25 12:00:54 [ 2703.931746] PKRU: 55555554
>>>>> 2024-12-25 12:00:54 [ 2703.935444] Call Trace:
>>>>> 2024-12-25 12:00:54 [ 2703.938962]  amdgpu_amdkfd_device_fini_sw+0x1a/0x40 [amdgpu]
>>>>> 2024-12-25 12:00:54 [ 2703.946080]  amdgpu_device_ip_fini.isra.0+0x3d/0x1b0 [amdgpu]
>>>>> 2024-12-25 12:00:54 [ 2703.953278]  amdgpu_device_fini_sw+0x2a/0x240 [amdgpu]
>>>>> 2024-12-25 12:00:54 [ 2703.959789]  amdgpu_driver_release_kms+0x12/0x30 [amdgpu]
>>>>> 2024-12-25 12:00:54 [ 2703.966501]  devm_drm_dev_init_release+0x42/0x70 [drm]
>>>>> 2024-12-25 12:00:54 [ 2703.972891]  release_nodes+0x6e/0xb0
>>>>> 2024-12-25 12:00:54 [ 2703.977522]  amdgpu_xcp_drv_release+0x38/0x80 [amdxcp]
>>>>> 2024-12-25 12:00:54 [ 2703.983906]  __do_sys_delete_module.constprop.0+0x138/0x2a0
>>>>> 2024-12-25 12:00:54 [ 2703.990775]  ? exit_to_user_mode_loop+0xd6/0x120
>>>>> 2024-12-25 12:00:54 [ 2703.996563]  do_syscall_64+0x2e/0x50
>>>>> 2024-12-25 12:00:54 [ 2704.001166]  entry_SYSCALL_64_after_hwframe+0x62/0xc7
>>>>> 2024-12-25 12:00:54 [ 2704.007432] RIP: 0033:0x7fa4db2620cb
>>>>> 2024-12-25 12:00:54 [ 2704.012025] Code: 73 01 c3 48 8b 0d a5 6d 19 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 b0
>>>>> 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 75 6d 19 00 f7 d8 64 89 01 48
>>>>>
>>>>> Signed-off-by: Jiang Liu<gerry@linux.alibaba.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdkfd/kfd_device.c | 7 ++++++-
>>>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>>>> index b6c5ffd4630b..58c1b5fcf785 100644
>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>>>> @@ -663,6 +663,8 @@ static void kfd_cleanup_nodes(struct kfd_dev *kfd, unsigned int num_nodes)
>>>>>   
>>>>>   	for (i = 0; i < num_nodes; i++) {
>>>>>   		knode = kfd->nodes[i];
>>>>> +		if (!knode)
>>>>> +			continue;
>>>>
>>>> I wonder how knode can be null here? kfd_cleanup_nodes is called by 
>>>> kgd2kfd_device_exit under condition if (kfd->init_complete). 
>>>> kfd->init_complete is true only after all kfd node got initialized 
>>>> at kgd2kfd_device_init. If kfd driver init fail kfd->init_complete 
>>>> would be false, then kfd_cleanup_node would not get called.
>>>>
>>> Hi Xiaogang,
>>> It may get triggered on error handling path of 
>>> ‘kid2kfd_device_init()`, when it jump to label `node_alloc_error` and
>>> then call `kfd_cleanup_nodes()`.
>>>
>> If it was the case kzalloc failed. That means there is no memory 
>> available even to allocate struct kfd_node. From the backtrace the 
>> general protection fault happened at
>>
>> RIP: 0010:kgd2kfd_device_exit+0x6/0x60 [amdgpu]
>>
>> It happened during driver module got released, not init time. I do not see how the patch is related to the issue you talked.
> Aha, it’s caused by another bug which got fixed by "[PATCH 4/6] 
> amdgpu: fix use after free bug related to amdgpu_driver_release_kms()”.
> Without the fix in "[PATCH 4/6] amdgpu: fix use after free bug related 
> to amdgpu_driver_release_kms()”, kgd2kfd_device_exit() will got called
> twice in case of device probe failure. I caused me some time to figure 
> out the double free issue.
> Actually we should reset kfd->init_completed to false in function 
> kgd2kfd_device_exit().

We can set kfd->init_completed = false at end of kgd2kfd_device_exit, 
but how kgd2kfd_device_exit was called two times? is there another bug 
caused that?

Regards

Xiaogang

>
>> Regards
>> Xiaogang
>>
>>
>>
>>> Thanks,
>>> Gerry
>>>
>>>>
>>>> Regards
>>>>
>>>> Xiaogang
>>>>
>>>>>   		device_queue_manager_uninit(knode->dqm);
>>>>>   		kfd_interrupt_exit(knode);
>>>>>   		kfd_topology_remove_device(knode);
>>>>> @@ -954,7 +956,10 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
>>>>>   		kfd_doorbell_fini(kfd);
>>>>>   		ida_destroy(&kfd->doorbell_ida);
>>>>>   		kfd_gtt_sa_fini(kfd);
>>>>> -		amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>>>>> +		if (kfd->gtt_mem) {
>>>>> +			amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>>>>> +			kfd->gtt_mem = NULL;
>>>>> +		}
>>>>>   	}
>>>>>   
>>>>>   	kfree(kfd);
>>>
>

[-- Attachment #2: Type: text/html, Size: 12212 bytes --]

Re: [PATCH 2/6] amdgpu: fix invalid memory access in kfd_cleanup_nodes()

[Gerry Liu]

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



> 2025年1月3日 14:19，Chen, Xiaogang <xiaogang.chen@amd.com> 写道：
> 
> 
> 
> On 1/2/2025 11:55 PM, Gerry Liu wrote:
>> 
>> 
>>> 2025年1月3日 13:44，Chen, Xiaogang <xiaogang.chen@amd.com <mailto:xiaogang.chen@amd.com>> 写道：
>>> 
>>> 
>>> 
>>> On 1/2/2025 8:22 PM, Gerry Liu wrote:
>>>> 
>>>> 
>>>>> 2025年1月3日 07:08，Chen, Xiaogang <xiaogang.chen@amd.com <mailto:xiaogang.chen@amd.com>> 写道：
>>>>> 
>>>>> 
>>>>> 
>>>>> On 1/1/2025 11:36 PM, Jiang Liu wrote:
>>>>>> On error recover path during device probe, it may trigger invalid
>>>>>> memory access as below:
>>>>>> 024-12-25 12:00:53 [ 2703.773040] general protection fault, probably for non-canonical address 0x52445f4749464e4f: 0000 [#1] SMP NOPTI
>>>>>> 2024-12-25 12:00:53 [ 2703.785199] CPU: 157 PID: 151951 Comm: rmmod Kdump: loaded Tainted: G        W  OE     5.10.134-17.2.al8.x86_64 #1
>>>>>> 2024-12-25 12:00:53 [ 2703.797515] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
>>>>>> 2024-12-25 12:00:53 [ 2703.809188] RIP: 0010:kgd2kfd_device_exit+0x6/0x60 [amdgpu]
>>>>>> 2024-12-25 12:00:53 [ 2703.816136] Code: ff 48 c7 83 38 01 00 00 80 45 e4 c0 c7 83 40 01 00 00 08 0f 00 00 e9 cd fa ff ff 66 0f 1f 84 00 00 00 00 00 0f
>>>>>> 1f 44 00 00 55 <80> bf 28 01 00 00 00 48 89 fd 75 09 48 89 ef 5d e9 65 df 9d f4 8b
>>>>>> 2024-12-25 12:00:54 [ 2703.838622] RSP: 0018:ffffb5313df07e10 EFLAGS: 00010202
>>>>>> 2024-12-25 12:00:54 [ 2703.845216] RAX: 0000000000000000 RBX: ffff97ad689a3ff0 RCX: 0000000080400014
>>>>>> 2024-12-25 12:00:54 [ 2703.853935] RDX: 0000000080400015 RSI: ffff97ad627e93d8 RDI: 52445f4749464e4f
>>>>>> 2024-12-25 12:00:54 [ 2703.862652] RBP: ffff97ad689a3ff0 R08: 0000000000000000 R09: ffffffffb5814c00
>>>>>> 2024-12-25 12:00:54 [ 2703.871368] R10: ffff97ad627e9280 R11: 0000000000000001 R12: ffffb5313df07e98
>>>>>> 2024-12-25 12:00:54 [ 2703.880068] R13: ffff97ad689a1810 R14: 0000000000000001 R15: 0000000000000000
>>>>>> 2024-12-25 12:00:54 [ 2703.888768] FS:  00007fa4db81e740(0000) GS:ffff98a93ec80000(0000) knlGS:0000000000000000
>>>>>> 2024-12-25 12:00:54 [ 2703.898547] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>>> 2024-12-25 12:00:54 [ 2703.905684] CR2: 00007f4502deca00 CR3: 000001008fc50001 CR4: 0000000002770ee0
>>>>>> 2024-12-25 12:00:54 [ 2703.914382] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>>>>> 2024-12-25 12:00:54 [ 2703.923066] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
>>>>>> 2024-12-25 12:00:54 [ 2703.931746] PKRU: 55555554
>>>>>> 2024-12-25 12:00:54 [ 2703.935444] Call Trace:
>>>>>> 2024-12-25 12:00:54 [ 2703.938962]  amdgpu_amdkfd_device_fini_sw+0x1a/0x40 [amdgpu]
>>>>>> 2024-12-25 12:00:54 [ 2703.946080]  amdgpu_device_ip_fini.isra.0+0x3d/0x1b0 [amdgpu]
>>>>>> 2024-12-25 12:00:54 [ 2703.953278]  amdgpu_device_fini_sw+0x2a/0x240 [amdgpu]
>>>>>> 2024-12-25 12:00:54 [ 2703.959789]  amdgpu_driver_release_kms+0x12/0x30 [amdgpu]
>>>>>> 2024-12-25 12:00:54 [ 2703.966501]  devm_drm_dev_init_release+0x42/0x70 [drm]
>>>>>> 2024-12-25 12:00:54 [ 2703.972891]  release_nodes+0x6e/0xb0
>>>>>> 2024-12-25 12:00:54 [ 2703.977522]  amdgpu_xcp_drv_release+0x38/0x80 [amdxcp]
>>>>>> 2024-12-25 12:00:54 [ 2703.983906]  __do_sys_delete_module.constprop.0+0x138/0x2a0
>>>>>> 2024-12-25 12:00:54 [ 2703.990775]  ? exit_to_user_mode_loop+0xd6/0x120
>>>>>> 2024-12-25 12:00:54 [ 2703.996563]  do_syscall_64+0x2e/0x50
>>>>>> 2024-12-25 12:00:54 [ 2704.001166]  entry_SYSCALL_64_after_hwframe+0x62/0xc7
>>>>>> 2024-12-25 12:00:54 [ 2704.007432] RIP: 0033:0x7fa4db2620cb
>>>>>> 2024-12-25 12:00:54 [ 2704.012025] Code: 73 01 c3 48 8b 0d a5 6d 19 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 b0
>>>>>> 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 75 6d 19 00 f7 d8 64 89 01 48
>>>>>> 
>>>>>> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com> <mailto:gerry@linux.alibaba.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/amd/amdkfd/kfd_device.c | 7 ++++++-
>>>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>> 
>>>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>>>>> index b6c5ffd4630b..58c1b5fcf785 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>>>>> @@ -663,6 +663,8 @@ static void kfd_cleanup_nodes(struct kfd_dev *kfd, unsigned int num_nodes)
>>>>>>  
>>>>>>  	for (i = 0; i < num_nodes; i++) {
>>>>>>  		knode = kfd->nodes[i];
>>>>>> +		if (!knode)
>>>>>> +			continue;
>>>>> I wonder how knode can be null here?  kfd_cleanup_nodes is called by kgd2kfd_device_exit under condition if (kfd->init_complete). kfd->init_complete is true only after all kfd node got initialized at kgd2kfd_device_init. If kfd driver init fail  kfd->init_complete would be false, then kfd_cleanup_node would not get called.
>>>>> 
>>>> Hi Xiaogang,
>>>> 	It may get triggered on error handling path of ‘kid2kfd_device_init()`, when it jump to label `node_alloc_error` and 
>>>> then call `kfd_cleanup_nodes()`.
>>>> 
>>> If it was the case kzalloc failed. That means there is no memory available even to allocate struct kfd_node. From the backtrace the general protection fault happened at 
>>> 
>>> RIP: 0010:kgd2kfd_device_exit+0x6/0x60 [amdgpu]
>>> 
>>> It happened during driver module got released, not init time. I do not see how the patch is related to the issue you talked.
>> Aha, it’s caused by another bug which got fixed by "[PATCH 4/6] amdgpu: fix use after free bug related to amdgpu_driver_release_kms()”.
>> Without the fix in "[PATCH 4/6] amdgpu: fix use after free bug related to amdgpu_driver_release_kms()”, kgd2kfd_device_exit() will got called
>> twice in case of device probe failure. I caused me some time to figure out the double free issue.
>> Actually we should reset kfd->init_completed to false in function kgd2kfd_device_exit().
> We can set kfd->init_completed = false at end of kgd2kfd_device_exit, but how kgd2kfd_device_exit was called two times? is there another bug caused that?
> 
I guess it caused by another bug related to the way amdgpu cooperates with the amdgpu_xcp driver. It would be better to enhance amdgpu_xcp driver either.

> Regards
> 
> Xiaogang
> 
> 
>> 
>>> Regards
>>> Xiaogang
>>> 
>>> 
>>> 
>>>> Thanks,
>>>> Gerry
>>>> 
>>>>> 
>>>>> Regards
>>>>> 
>>>>> Xiaogang
>>>>> 
>>>>>>  		device_queue_manager_uninit(knode->dqm);
>>>>>>  		kfd_interrupt_exit(knode);
>>>>>>  		kfd_topology_remove_device(knode);
>>>>>> @@ -954,7 +956,10 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
>>>>>>  		kfd_doorbell_fini(kfd);
>>>>>>  		ida_destroy(&kfd->doorbell_ida);
>>>>>>  		kfd_gtt_sa_fini(kfd);
>>>>>> -		amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>>>>>> +		if (kfd->gtt_mem) {
>>>>>> +			amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>>>>>> +			kfd->gtt_mem = NULL;
>>>>>> +		}
>>>>>>  	}
>>>>>>  
>>>>>>  	kfree(kfd);
>>>> 
>> 


[-- Attachment #2: Type: text/html, Size: 12774 bytes --]

Re: [PATCH 2/6] amdgpu: fix invalid memory access in kfd_cleanup_nodes()

[Chen, Xiaogang]

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


On 1/3/2025 1:05 AM, Gerry Liu wrote:
>
>
>> 2025年1月3日 14:19，Chen, Xiaogang <xiaogang.chen@amd.com> 写道：
>>
>>
>> On 1/2/2025 11:55 PM, Gerry Liu wrote:
>>>
>>>
>>>> 2025年1月3日 13:44，Chen, Xiaogang <xiaogang.chen@amd.com> 写道：
>>>>
>>>>
>>>> On 1/2/2025 8:22 PM, Gerry Liu wrote:
>>>>>
>>>>>
>>>>>> 2025年1月3日 07:08，Chen, Xiaogang <xiaogang.chen@amd.com> 写道：
>>>>>>
>>>>>>
>>>>>> On 1/1/2025 11:36 PM, Jiang Liu wrote:
>>>>>>> On error recover path during device probe, it may trigger invalid
>>>>>>> memory access as below:
>>>>>>> 024-12-25 12:00:53 [ 2703.773040] general protection fault, probably for non-canonical address 0x52445f4749464e4f: 0000 [#1] SMP NOPTI
>>>>>>> 2024-12-25 12:00:53 [ 2703.785199] CPU: 157 PID: 151951 Comm: rmmod Kdump: loaded Tainted: G        W  OE     5.10.134-17.2.al8.x86_64 #1
>>>>>>> 2024-12-25 12:00:53 [ 2703.797515] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
>>>>>>> 2024-12-25 12:00:53 [ 2703.809188] RIP: 0010:kgd2kfd_device_exit+0x6/0x60 [amdgpu]
>>>>>>> 2024-12-25 12:00:53 [ 2703.816136] Code: ff 48 c7 83 38 01 00 00 80 45 e4 c0 c7 83 40 01 00 00 08 0f 00 00 e9 cd fa ff ff 66 0f 1f 84 00 00 00 00 00 0f
>>>>>>> 1f 44 00 00 55 <80> bf 28 01 00 00 00 48 89 fd 75 09 48 89 ef 5d e9 65 df 9d f4 8b
>>>>>>> 2024-12-25 12:00:54 [ 2703.838622] RSP: 0018:ffffb5313df07e10 EFLAGS: 00010202
>>>>>>> 2024-12-25 12:00:54 [ 2703.845216] RAX: 0000000000000000 RBX: ffff97ad689a3ff0 RCX: 0000000080400014
>>>>>>> 2024-12-25 12:00:54 [ 2703.853935] RDX: 0000000080400015 RSI: ffff97ad627e93d8 RDI: 52445f4749464e4f
>>>>>>> 2024-12-25 12:00:54 [ 2703.862652] RBP: ffff97ad689a3ff0 R08: 0000000000000000 R09: ffffffffb5814c00
>>>>>>> 2024-12-25 12:00:54 [ 2703.871368] R10: ffff97ad627e9280 R11: 0000000000000001 R12: ffffb5313df07e98
>>>>>>> 2024-12-25 12:00:54 [ 2703.880068] R13: ffff97ad689a1810 R14: 0000000000000001 R15: 0000000000000000
>>>>>>> 2024-12-25 12:00:54 [ 2703.888768] FS:  00007fa4db81e740(0000) GS:ffff98a93ec80000(0000) knlGS:0000000000000000
>>>>>>> 2024-12-25 12:00:54 [ 2703.898547] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>>>> 2024-12-25 12:00:54 [ 2703.905684] CR2: 00007f4502deca00 CR3: 000001008fc50001 CR4: 0000000002770ee0
>>>>>>> 2024-12-25 12:00:54 [ 2703.914382] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>>>>>> 2024-12-25 12:00:54 [ 2703.923066] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
>>>>>>> 2024-12-25 12:00:54 [ 2703.931746] PKRU: 55555554
>>>>>>> 2024-12-25 12:00:54 [ 2703.935444] Call Trace:
>>>>>>> 2024-12-25 12:00:54 [ 2703.938962]  amdgpu_amdkfd_device_fini_sw+0x1a/0x40 [amdgpu]
>>>>>>> 2024-12-25 12:00:54 [ 2703.946080]  amdgpu_device_ip_fini.isra.0+0x3d/0x1b0 [amdgpu]
>>>>>>> 2024-12-25 12:00:54 [ 2703.953278]  amdgpu_device_fini_sw+0x2a/0x240 [amdgpu]
>>>>>>> 2024-12-25 12:00:54 [ 2703.959789]  amdgpu_driver_release_kms+0x12/0x30 [amdgpu]
>>>>>>> 2024-12-25 12:00:54 [ 2703.966501]  devm_drm_dev_init_release+0x42/0x70 [drm]
>>>>>>> 2024-12-25 12:00:54 [ 2703.972891]  release_nodes+0x6e/0xb0
>>>>>>> 2024-12-25 12:00:54 [ 2703.977522]  amdgpu_xcp_drv_release+0x38/0x80 [amdxcp]
>>>>>>> 2024-12-25 12:00:54 [ 2703.983906]  __do_sys_delete_module.constprop.0+0x138/0x2a0
>>>>>>> 2024-12-25 12:00:54 [ 2703.990775]  ? exit_to_user_mode_loop+0xd6/0x120
>>>>>>> 2024-12-25 12:00:54 [ 2703.996563]  do_syscall_64+0x2e/0x50
>>>>>>> 2024-12-25 12:00:54 [ 2704.001166]  entry_SYSCALL_64_after_hwframe+0x62/0xc7
>>>>>>> 2024-12-25 12:00:54 [ 2704.007432] RIP: 0033:0x7fa4db2620cb
>>>>>>> 2024-12-25 12:00:54 [ 2704.012025] Code: 73 01 c3 48 8b 0d a5 6d 19 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 b0
>>>>>>> 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 75 6d 19 00 f7 d8 64 89 01 48
>>>>>>>
>>>>>>> Signed-off-by: Jiang Liu<gerry@linux.alibaba.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/amd/amdkfd/kfd_device.c | 7 ++++++-
>>>>>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>>>>>> index b6c5ffd4630b..58c1b5fcf785 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>>>>>> @@ -663,6 +663,8 @@ static void kfd_cleanup_nodes(struct kfd_dev *kfd, unsigned int num_nodes)
>>>>>>>   
>>>>>>>   	for (i = 0; i < num_nodes; i++) {
>>>>>>>   		knode = kfd->nodes[i];
>>>>>>> +		if (!knode)
>>>>>>> +			continue;
>>>>>>
>>>>>> I wonder how knode can be null here? kfd_cleanup_nodes is called 
>>>>>> by kgd2kfd_device_exit under condition if (kfd->init_complete). 
>>>>>> kfd->init_complete is true only after all kfd node got 
>>>>>> initialized at kgd2kfd_device_init. If kfd driver init fail 
>>>>>> kfd->init_complete would be false, then kfd_cleanup_node would 
>>>>>> not get called.
>>>>>>
>>>>> Hi Xiaogang,
>>>>> It may get triggered on error handling path of 
>>>>> ‘kid2kfd_device_init()`, when it jump to label `node_alloc_error` and
>>>>> then call `kfd_cleanup_nodes()`.
>>>>>
>>>> If it was the case kzalloc failed. That means there is no memory 
>>>> available even to allocate struct kfd_node. From the backtrace the 
>>>> general protection fault happened at
>>>>
>>>> RIP: 0010:kgd2kfd_device_exit+0x6/0x60 [amdgpu]
>>>>
>>>> It happened during driver module got released, not init time. I do not see how the patch is related to the issue you talked.
>>> Aha, it’s caused by another bug which got fixed by "[PATCH 4/6] 
>>> amdgpu: fix use after free bug related to amdgpu_driver_release_kms()”.
>>> Without the fix in "[PATCH 4/6] amdgpu: fix use after free bug 
>>> related to amdgpu_driver_release_kms()”, kgd2kfd_device_exit() will 
>>> got called
>>> twice in case of device probe failure. I caused me some time to 
>>> figure out the double free issue.
>>> Actually we should reset kfd->init_completed to false in function 
>>> kgd2kfd_device_exit().
>>
>> We can set kfd->init_completed = false at end of kgd2kfd_device_exit, 
>> but how kgd2kfd_device_exit was called two times? is there another 
>> bug caused that?
>>
> I guess it caused by another bug related to the way amdgpu cooperates 
> with the amdgpu_xcp driver. It would be better to enhance amdgpu_xcp 
> driver either.

kfd driver has considered which kfd nodes got initialized and release 
them accordingly. From what saw here seems you may mix different issues 
or not target the real issue. Let's have backtrace match the changes.

Regards

Xiaogang

>
>> Regards
>>
>> Xiaogang
>>
>>
>>>
>>>> Regards
>>>> Xiaogang
>>>>
>>>>
>>>>
>>>>> Thanks,
>>>>> Gerry
>>>>>
>>>>>>
>>>>>> Regards
>>>>>>
>>>>>> Xiaogang
>>>>>>
>>>>>>>   		device_queue_manager_uninit(knode->dqm);
>>>>>>>   		kfd_interrupt_exit(knode);
>>>>>>>   		kfd_topology_remove_device(knode);
>>>>>>> @@ -954,7 +956,10 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
>>>>>>>   		kfd_doorbell_fini(kfd);
>>>>>>>   		ida_destroy(&kfd->doorbell_ida);
>>>>>>>   		kfd_gtt_sa_fini(kfd);
>>>>>>> -		amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>>>>>>> +		if (kfd->gtt_mem) {
>>>>>>> +			amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>>>>>>> +			kfd->gtt_mem = NULL;
>>>>>>> +		}
>>>>>>>   	}
>>>>>>>   
>>>>>>>   	kfree(kfd);
>>>>>
>>>
>

[-- Attachment #2: Type: text/html, Size: 15000 bytes --]

Re: [PATCH 2/6] amdgpu: fix invalid memory access in kfd_cleanup_nodes()

[Gerry Liu]

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



> 2025年1月4日 01:33，Chen, Xiaogang <xiaogang.chen@amd.com> 写道：
> 
> 
> 
> On 1/3/2025 1:05 AM, Gerry Liu wrote:
>> 
>> 
>>> 2025年1月3日 14:19，Chen, Xiaogang <xiaogang.chen@amd.com <mailto:xiaogang.chen@amd.com>> 写道：
>>> 
>>> 
>>> 
>>> On 1/2/2025 11:55 PM, Gerry Liu wrote:
>>>> 
>>>> 
>>>>> 2025年1月3日 13:44，Chen, Xiaogang <xiaogang.chen@amd.com <mailto:xiaogang.chen@amd.com>> 写道：
>>>>> 
>>>>> 
>>>>> 
>>>>> On 1/2/2025 8:22 PM, Gerry Liu wrote:
>>>>>> 
>>>>>> 
>>>>>>> 2025年1月3日 07:08，Chen, Xiaogang <xiaogang.chen@amd.com <mailto:xiaogang.chen@amd.com>> 写道：
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> On 1/1/2025 11:36 PM, Jiang Liu wrote:
>>>>>>>> On error recover path during device probe, it may trigger invalid
>>>>>>>> memory access as below:
>>>>>>>> 024-12-25 12:00:53 [ 2703.773040] general protection fault, probably for non-canonical address 0x52445f4749464e4f: 0000 [#1] SMP NOPTI
>>>>>>>> 2024-12-25 12:00:53 [ 2703.785199] CPU: 157 PID: 151951 Comm: rmmod Kdump: loaded Tainted: G        W  OE     5.10.134-17.2.al8.x86_64 #1
>>>>>>>> 2024-12-25 12:00:53 [ 2703.797515] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
>>>>>>>> 2024-12-25 12:00:53 [ 2703.809188] RIP: 0010:kgd2kfd_device_exit+0x6/0x60 [amdgpu]
>>>>>>>> 2024-12-25 12:00:53 [ 2703.816136] Code: ff 48 c7 83 38 01 00 00 80 45 e4 c0 c7 83 40 01 00 00 08 0f 00 00 e9 cd fa ff ff 66 0f 1f 84 00 00 00 00 00 0f
>>>>>>>> 1f 44 00 00 55 <80> bf 28 01 00 00 00 48 89 fd 75 09 48 89 ef 5d e9 65 df 9d f4 8b
>>>>>>>> 2024-12-25 12:00:54 [ 2703.838622] RSP: 0018:ffffb5313df07e10 EFLAGS: 00010202
>>>>>>>> 2024-12-25 12:00:54 [ 2703.845216] RAX: 0000000000000000 RBX: ffff97ad689a3ff0 RCX: 0000000080400014
>>>>>>>> 2024-12-25 12:00:54 [ 2703.853935] RDX: 0000000080400015 RSI: ffff97ad627e93d8 RDI: 52445f4749464e4f
>>>>>>>> 2024-12-25 12:00:54 [ 2703.862652] RBP: ffff97ad689a3ff0 R08: 0000000000000000 R09: ffffffffb5814c00
>>>>>>>> 2024-12-25 12:00:54 [ 2703.871368] R10: ffff97ad627e9280 R11: 0000000000000001 R12: ffffb5313df07e98
>>>>>>>> 2024-12-25 12:00:54 [ 2703.880068] R13: ffff97ad689a1810 R14: 0000000000000001 R15: 0000000000000000
>>>>>>>> 2024-12-25 12:00:54 [ 2703.888768] FS:  00007fa4db81e740(0000) GS:ffff98a93ec80000(0000) knlGS:0000000000000000
>>>>>>>> 2024-12-25 12:00:54 [ 2703.898547] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>>>>> 2024-12-25 12:00:54 [ 2703.905684] CR2: 00007f4502deca00 CR3: 000001008fc50001 CR4: 0000000002770ee0
>>>>>>>> 2024-12-25 12:00:54 [ 2703.914382] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>>>>>>> 2024-12-25 12:00:54 [ 2703.923066] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
>>>>>>>> 2024-12-25 12:00:54 [ 2703.931746] PKRU: 55555554
>>>>>>>> 2024-12-25 12:00:54 [ 2703.935444] Call Trace:
>>>>>>>> 2024-12-25 12:00:54 [ 2703.938962]  amdgpu_amdkfd_device_fini_sw+0x1a/0x40 [amdgpu]
>>>>>>>> 2024-12-25 12:00:54 [ 2703.946080]  amdgpu_device_ip_fini.isra.0+0x3d/0x1b0 [amdgpu]
>>>>>>>> 2024-12-25 12:00:54 [ 2703.953278]  amdgpu_device_fini_sw+0x2a/0x240 [amdgpu]
>>>>>>>> 2024-12-25 12:00:54 [ 2703.959789]  amdgpu_driver_release_kms+0x12/0x30 [amdgpu]
>>>>>>>> 2024-12-25 12:00:54 [ 2703.966501]  devm_drm_dev_init_release+0x42/0x70 [drm]
>>>>>>>> 2024-12-25 12:00:54 [ 2703.972891]  release_nodes+0x6e/0xb0
>>>>>>>> 2024-12-25 12:00:54 [ 2703.977522]  amdgpu_xcp_drv_release+0x38/0x80 [amdxcp]
>>>>>>>> 2024-12-25 12:00:54 [ 2703.983906]  __do_sys_delete_module.constprop.0+0x138/0x2a0
>>>>>>>> 2024-12-25 12:00:54 [ 2703.990775]  ? exit_to_user_mode_loop+0xd6/0x120
>>>>>>>> 2024-12-25 12:00:54 [ 2703.996563]  do_syscall_64+0x2e/0x50
>>>>>>>> 2024-12-25 12:00:54 [ 2704.001166]  entry_SYSCALL_64_after_hwframe+0x62/0xc7
>>>>>>>> 2024-12-25 12:00:54 [ 2704.007432] RIP: 0033:0x7fa4db2620cb
>>>>>>>> 2024-12-25 12:00:54 [ 2704.012025] Code: 73 01 c3 48 8b 0d a5 6d 19 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 b0
>>>>>>>> 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 75 6d 19 00 f7 d8 64 89 01 48
>>>>>>>> 
>>>>>>>> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com> <mailto:gerry@linux.alibaba.com>
>>>>>>>> ---
>>>>>>>>  drivers/gpu/drm/amd/amdkfd/kfd_device.c | 7 ++++++-
>>>>>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>>> 
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>>>>>>> index b6c5ffd4630b..58c1b5fcf785 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>>>>>>> @@ -663,6 +663,8 @@ static void kfd_cleanup_nodes(struct kfd_dev *kfd, unsigned int num_nodes)
>>>>>>>>  
>>>>>>>>  	for (i = 0; i < num_nodes; i++) {
>>>>>>>>  		knode = kfd->nodes[i];
>>>>>>>> +		if (!knode)
>>>>>>>> +			continue;
>>>>>>> I wonder how knode can be null here? kfd_cleanup_nodes is called by kgd2kfd_device_exit under condition if (kfd->init_complete). kfd->init_complete is true only after all kfd node got initialized at kgd2kfd_device_init. If kfd driver init fail  kfd->init_complete would be false, then kfd_cleanup_node would not get called.
>>>>>>> 
>>>>>> Hi Xiaogang,
>>>>>> 	It may get triggered on error handling path of ‘kid2kfd_device_init()`, when it jump to label `node_alloc_error` and 
>>>>>> then call `kfd_cleanup_nodes()`.
>>>>>> 
>>>>> If it was the case kzalloc failed. That means there is no memory available even to allocate struct kfd_node. From the backtrace the general protection fault happened at 
>>>>> 
>>>>> RIP: 0010:kgd2kfd_device_exit+0x6/0x60 [amdgpu]
>>>>> 
>>>>> It happened during driver module got released, not init time. I do not see how the patch is related to the issue you talked.
>>>> Aha, it’s caused by another bug which got fixed by "[PATCH 4/6] amdgpu: fix use after free bug related to amdgpu_driver_release_kms()”.
>>>> Without the fix in "[PATCH 4/6] amdgpu: fix use after free bug related to amdgpu_driver_release_kms()”, kgd2kfd_device_exit() will got called
>>>> twice in case of device probe failure. I caused me some time to figure out the double free issue.
>>>> Actually we should reset kfd->init_completed to false in function kgd2kfd_device_exit().
>>> We can set kfd->init_completed = false at end of kgd2kfd_device_exit, but how kgd2kfd_device_exit was called two times? is there another bug caused that?
>>> 
>> I guess it caused by another bug related to the way amdgpu cooperates with the amdgpu_xcp driver. It would be better to enhance amdgpu_xcp driver either.
> kfd driver has considered which kfd nodes got initialized and release them accordingly. From what saw here seems you may mix different issues or not target the real issue. Let's have backtrace match the changes.
> 
Sure, I will rework this patch with log message to address the possible resource leakage.
> Regards
> 
> Xiaogang
> 
>> 
>>> Regards
>>> 
>>> Xiaogang
>>> 
>>> 
>>>> 
>>>>> Regards
>>>>> Xiaogang
>>>>> 
>>>>> 
>>>>> 
>>>>>> Thanks,
>>>>>> Gerry
>>>>>> 
>>>>>>> 
>>>>>>> Regards
>>>>>>> 
>>>>>>> Xiaogang
>>>>>>> 
>>>>>>>>  		device_queue_manager_uninit(knode->dqm);
>>>>>>>>  		kfd_interrupt_exit(knode);
>>>>>>>>  		kfd_topology_remove_device(knode);
>>>>>>>> @@ -954,7 +956,10 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
>>>>>>>>  		kfd_doorbell_fini(kfd);
>>>>>>>>  		ida_destroy(&kfd->doorbell_ida);
>>>>>>>>  		kfd_gtt_sa_fini(kfd);
>>>>>>>> -		amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>>>>>>>> +		if (kfd->gtt_mem) {
>>>>>>>> +			amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>>>>>>>> +			kfd->gtt_mem = NULL;
>>>>>>>> +		}
>>>>>>>>  	}
>>>>>>>>  
>>>>>>>>  	kfree(kfd);
>>>>>> 
>>>> 
>> 


[-- Attachment #2: Type: text/html, Size: 15733 bytes --]

[PATCH 3/6] amdgpu: clear adev->in_suspend flag when fails to suspend

[Jiang Liu]

Clear adev->in_suspend flag when fails to suspend, otherwise it will
cause too much warnings like:
[ 1802.212027] ------------[ cut here ]------------
[ 1802.212028] WARNING: CPU: 97 PID: 11282 at drivers/gpu/drm/amd/amdgpu/amdgpu_object.c:452 amdgpu_bo_free_kernel+0xf9/0x120 [amdgpu]
[ 1802.212198] Modules linked in: amdgpu(E-) tcp_diag(E) inet_diag(E) rfkill(E) intel_rapl_msr(E) intel_rapl_common(E) intel_uncore_frequency(E) intel_uncore_frequency_common(E) i10nm_edac(E) nfit(E) x86_pkg_temp_thermal(E) intel_powerclamp(E) coretemp(E) kvm_intel(E) kvm(E) crct10dif_pclmul(E) crc32_pclmul(E) ghash_clmulni_intel(E) rapl(E) iTCO_wdt(E) pmt_telemetry(E) iTCO_vendor_support(E) pmt_class(E) intel_cstate(E) snd_hda_intel(E) ipmi_ssif(E) snd_intel_dspcfg(E) snd_hda_codec(E) snd_hda_core(E) snd_hwdep(E) snd_pcm(E) snd_timer(E) intel_uncore(E) snd(E) cdc_ether(E) pcspkr(E) i2c_i801(E) idxd(E) mei_me(E) usbnet(E) ses(E) isst_if_mmio(E) isst_if_mbox_pci(E) mii(E) joydev(E) soundcore(E) isst_if_common(E) idxd_bus(E) mei(E) enclosure(E) intel_vsec(E) i2c_smbus(E) i2c_ismt(E) sunrpc(E) acpi_power_meter(E) ipmi_si(E) acpi_ipmi(E) ipmi_devintf(E) acpi_pad(E) ipmi_msghandler(E) vfat(E) fat(E) sg(E) video(E) amdxcp(E) drm_ttm_helper(E) ttm(E) drm_exec(E) gpu_sched(E) drm_suballoc_helper(E) crc32c_intel(E) drm_buddy(E)
[ 1802.212223]  ast(E) drm_shmem_helper(E) drm_display_helper(E) i2c_algo_bit(E) drm_kms_helper(E) virtio_net(E) mpt3sas(E) drm(E) net_failover(E) ahci(E) raid_class(E) failover(E) libahci(E) scsi_transport_sas(E) dimlib(E) libata(E) i2c_core(E) wmi(E) pinctrl_emmitsburg(E) [last unloaded: amdgpu(E)]
[ 1802.212231] CPU: 97 PID: 11282 Comm: rmmod Kdump: loaded Tainted: G        W   E      6.10.0+ #2
[ 1802.212232] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
[ 1802.212232] RIP: 0010:amdgpu_bo_free_kernel+0xf9/0x120 [amdgpu]
[ 1802.212401] Code: 00 00 00 4d 85 e4 74 08 49 c7 04 24 00 00 00 00 48 85 ed 74 08 48 c7 45 00 00 00 00 00 5b 5d 41 5c 41 5d 41 5e c3 cc cc cc cc <0f> 0b e9 3b ff ff ff 3d 00 fe ff ff 74 b3 49 8b be b0 10 ff ff 4c
[ 1802.212402] RSP: 0018:ffffbe0e92087cb0 EFLAGS: 00010202
[ 1802.212403] RAX: 0000000000000206 RBX: ffff9cbc06c37f10 RCX: 000000000020000d
[ 1802.212404] RDX: ffff9cbc06c37f18 RSI: ffff9cbc06c37f58 RDI: ffff9cbc06c37f10
[ 1802.212407] RBP: ffff9cbc06c37f18 R08: ffff9cba42388800 R09: 000000000020000d
[ 1802.212409] R10: 0000000000040000 R11: 0000000000000006 R12: ffff9cbc06c37f58
[ 1802.212410] R13: ffff9cba4238dc00 R14: ffff9cbc06c0ef50 R15: ffff9cbc06c00000
[ 1802.212411] FS:  00007f927dc4e740(0000) GS:ffff9db47e880000(0000) knlGS:0000000000000000
[ 1802.212412] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1802.212413] CR2: 000056342b2be1a8 CR3: 00000003f1546005 CR4: 0000000000f70ef0
[ 1802.212413] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1802.212414] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
[ 1802.212415] PKRU: 55555554
[ 1802.212415] Call Trace:
[ 1802.212416]  <TASK>
[ 1802.212417]  ? __warn+0x83/0x130
[ 1802.212419]  ? amdgpu_bo_free_kernel+0xf9/0x120 [amdgpu]
[ 1802.212586]  ? __report_bug+0xea/0x100
[ 1802.212588]  ? report_bug+0x24/0x70
[ 1802.212589]  ? handle_bug+0x3c/0x70
[ 1802.212590]  ? exc_invalid_op+0x18/0x70
[ 1802.212591]  ? asm_exc_invalid_op+0x1a/0x20
[ 1802.212594]  ? amdgpu_bo_free_kernel+0xf9/0x120 [amdgpu]
[ 1802.212746]  amdgpu_ring_fini+0x91/0x120 [amdgpu]
[ 1802.212901]  amdgpu_jpeg_sw_fini+0xb2/0xe0 [amdgpu]
[ 1802.213106]  amdgpu_device_ip_fini.isra.0+0xb1/0x1c0 [amdgpu]
[ 1802.213247]  amdgpu_device_fini_sw+0x49/0x290 [amdgpu]
[ 1802.213413]  amdgpu_driver_release_kms+0x16/0x30 [amdgpu]
[ 1802.213576]  devm_drm_dev_init_release+0x4e/0x70 [drm]
[ 1802.213602]  release_nodes+0x35/0xb0
[ 1802.213605]  devres_release_all+0x8f/0xd0
[ 1802.213606]  device_unbind_cleanup+0xe/0x70
[ 1802.213609]  device_release_driver_internal+0x1bc/0x200
[ 1802.213611]  driver_detach+0x48/0x90
[ 1802.213613]  bus_remove_driver+0x6d/0xf0
[ 1802.213615]  pci_unregister_driver+0x2e/0xb0
[ 1802.213618]  amdgpu_exit+0x15/0x1c4 [amdgpu]
[ 1802.213861]  __do_sys_delete_module.constprop.0+0x176/0x310
[ 1802.213863]  do_syscall_64+0x5d/0x170
[ 1802.213866]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 1802.213869] RIP: 0033:0x7f927d6620cb
[ 1802.213870] Code: 73 01 c3 48 8b 0d a5 6d 19 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 b0 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 75 6d 19 00 f7 d8 64 89 01 48
[ 1802.213871] RSP: 002b:00007fff031d9d78 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
[ 1802.213873] RAX: ffffffffffffffda RBX: 000055d8d6df69e0 RCX: 00007f927d6620cb
[ 1802.213873] RDX: 000000000000000a RSI: 0000000000000800 RDI: 000055d8d6df6a48
[ 1802.213874] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
[ 1802.213875] R10: 00007f927d7aaac0 R11: 0000000000000206 R12: 00007fff031d9fa0
[ 1802.213876] R13: 00007fff031da5f1 R14: 000055d8d6df62a0 R15: 000055d8d6df69e0
[ 1802.213878]  </TASK>
[ 1802.213879] ---[ end trace 0000000000000000 ]---

Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 3244966b0c39..d9717e1c6b57 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4866,7 +4866,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
 		amdgpu_virt_fini_data_exchange(adev);
 		r = amdgpu_virt_request_full_gpu(adev, false);
 		if (r)
-			return r;
+			goto error_out;
 	}
 
 	if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D3))
@@ -4886,7 +4886,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
 
 	r = amdgpu_device_evict_resources(adev);
 	if (r)
-		return r;
+		goto error_out;
 
 	amdgpu_ttm_set_buffer_funcs_status(adev, false);
 
@@ -4899,9 +4899,12 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
 
 	r = amdgpu_dpm_notify_rlc_state(adev, false);
 	if (r)
-		return r;
+		goto error_out;
 
 	return 0;
+error_out:
+	adev->in_suspend = false;
+	return r;
 }
 
 /**
@@ -4963,8 +4966,10 @@ int amdgpu_device_resume(struct drm_device *dev, bool fbcon)
 		amdgpu_virt_release_full_gpu(adev, true);
 	}
 
-	if (r)
+	if (r) {
+		adev->in_suspend = false;
 		return r;
+	}
 
 	/* Make sure IB tests flushed */
 	flush_delayed_work(&adev->delayed_init_work);
-- 
2.43.5

[PATCH 4/6] amdgpu: fix use after free bug related to amdgpu_driver_release_kms()

[Jiang Liu]

If some GPU device failed to probe, `rmmod amdgpu` will trigger a use
after free bug related to amdgpu_driver_release_kms() as:
2024-12-26 16:17:45 [16002.085540] BUG: kernel NULL pointer dereference, address: 0000000000000000
2024-12-26 16:17:45 [16002.093792] #PF: supervisor read access in kernel mode
2024-12-26 16:17:45 [16002.099993] #PF: error_code(0x0000) - not-present page
2024-12-26 16:17:45 [16002.106188] PGD 0 P4D 0
2024-12-26 16:17:45 [16002.109464] Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
2024-12-26 16:17:45 [16002.115372] CPU: 2 PID: 14375 Comm: rmmod Kdump: loaded Tainted: G        W   E      6.10.0+ #2
2024-12-26 16:17:45 [16002.125577] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
2024-12-26 16:17:45 [16002.136858] RIP: 0010:drm_sched_fini+0x3f/0xe0 [gpu_sched]
2024-12-26 16:17:45 [16002.143463] Code: 60 c6 87 be 00 00 00 01 e8 ce e0 90 d8 48 8d bb 80 00 00 00 e8 c2 e0 90 d8 8b 43 20 85 c0 74 51 45 31 e4 48 8b
43 28 4d 63 ec <4a> 8b 2c e8 48 89 ef e8 f5 0e 59 d9 48 8b 45 10 48 8d 55 10 48 39
2024-12-26 16:17:45 [16002.164992] RSP: 0018:ffffb570dbbb7da8 EFLAGS: 00010246
2024-12-26 16:17:45 [16002.171316] RAX: 0000000000000000 RBX: ffff96b0fdadc878 RCX: 0000000000000000
2024-12-26 16:17:46 [16002.179784] RDX: 000fffffffe00000 RSI: 0000000000000000 RDI: ffff96b0fdadc8f8
2024-12-26 16:17:46 [16002.188252] RBP: ffff96b0fdadc800 R08: ffff97abbd035040 R09: ffffffff9ac52540
2024-12-26 16:17:46 [16002.196722] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
2024-12-26 16:17:46 [16002.205179] R13: 0000000000000000 R14: ffff96b0fdadfc00 R15: 0000000000000000
2024-12-26 16:17:46 [16002.213648] FS:  00007f2737000740(0000) GS:ffff97abbd100000(0000) knlGS:0000000000000000
2024-12-26 16:17:46 [16002.223189] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
2024-12-26 16:17:46 [16002.230103] CR2: 0000000000000000 CR3: 000000011be3a005 CR4: 0000000000f70ef0
2024-12-26 16:17:46 [16002.238581] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
2024-12-26 16:17:46 [16002.247053] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
e024se+0x3c/0x90 [amdxcp]
2024-12-26 16:17:46 [16002.337645]  __do_sys_delete_module.constprop.0+0x176/0x310
2024-12-26 16:17:46 [16002.344324]  do_syscall_64+0x5d/0x170
2024-12-26 16:17:46 [16002.348858]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
2024-12-26 16:17:46 [16002.354956] RIP: 0033:0x7f2736a620cb-12-26

Fix it by unplugging xcp drm devices when failed to probe GPU devices.

Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
Tested-by: Shuo Liu <shuox.liu@linux.alibaba.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 4 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 5ffe1dad9622..e7f35e3a6d2d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -164,8 +164,10 @@ int amdgpu_driver_load_kms(struct amdgpu_device *adev, unsigned long flags)
 		DRM_WARN("smart shift update failed\n");
 
 out:
-	if (r)
+	if (r) {
+		amdgpu_xcp_dev_unplug(adev);
 		amdgpu_driver_unload_kms(dev);
+	}
 
 	return r;
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
index a6d456ec6aeb..ef4eaacf67f6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
@@ -382,6 +382,7 @@ void amdgpu_xcp_dev_unplug(struct amdgpu_device *adev)
 		p_ddev->primary->dev = adev->xcp_mgr->xcp[i].pdev;
 		p_ddev->driver =  adev->xcp_mgr->xcp[i].driver;
 		p_ddev->vma_offset_manager = adev->xcp_mgr->xcp[i].vma_offset_manager;
+		adev->xcp_mgr->xcp[i].ddev = NULL;
 	}
 }
 
-- 
2.43.5

Re: [PATCH 4/6] amdgpu: fix use after free bug related to amdgpu_driver_release_kms()

[Lazar, Lijo]

On 1/2/2025 11:06 AM, Jiang Liu wrote:
> If some GPU device failed to probe, `rmmod amdgpu` will trigger a use
> after free bug related to amdgpu_driver_release_kms() as:
> 2024-12-26 16:17:45 [16002.085540] BUG: kernel NULL pointer dereference, address: 0000000000000000
> 2024-12-26 16:17:45 [16002.093792] #PF: supervisor read access in kernel mode
> 2024-12-26 16:17:45 [16002.099993] #PF: error_code(0x0000) - not-present page
> 2024-12-26 16:17:45 [16002.106188] PGD 0 P4D 0
> 2024-12-26 16:17:45 [16002.109464] Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
> 2024-12-26 16:17:45 [16002.115372] CPU: 2 PID: 14375 Comm: rmmod Kdump: loaded Tainted: G        W   E      6.10.0+ #2
> 2024-12-26 16:17:45 [16002.125577] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
> 2024-12-26 16:17:45 [16002.136858] RIP: 0010:drm_sched_fini+0x3f/0xe0 [gpu_sched]
> 2024-12-26 16:17:45 [16002.143463] Code: 60 c6 87 be 00 00 00 01 e8 ce e0 90 d8 48 8d bb 80 00 00 00 e8 c2 e0 90 d8 8b 43 20 85 c0 74 51 45 31 e4 48 8b
> 43 28 4d 63 ec <4a> 8b 2c e8 48 89 ef e8 f5 0e 59 d9 48 8b 45 10 48 8d 55 10 48 39
> 2024-12-26 16:17:45 [16002.164992] RSP: 0018:ffffb570dbbb7da8 EFLAGS: 00010246
> 2024-12-26 16:17:45 [16002.171316] RAX: 0000000000000000 RBX: ffff96b0fdadc878 RCX: 0000000000000000
> 2024-12-26 16:17:46 [16002.179784] RDX: 000fffffffe00000 RSI: 0000000000000000 RDI: ffff96b0fdadc8f8
> 2024-12-26 16:17:46 [16002.188252] RBP: ffff96b0fdadc800 R08: ffff97abbd035040 R09: ffffffff9ac52540
> 2024-12-26 16:17:46 [16002.196722] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> 2024-12-26 16:17:46 [16002.205179] R13: 0000000000000000 R14: ffff96b0fdadfc00 R15: 0000000000000000
> 2024-12-26 16:17:46 [16002.213648] FS:  00007f2737000740(0000) GS:ffff97abbd100000(0000) knlGS:0000000000000000
> 2024-12-26 16:17:46 [16002.223189] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> 2024-12-26 16:17:46 [16002.230103] CR2: 0000000000000000 CR3: 000000011be3a005 CR4: 0000000000f70ef0
> 2024-12-26 16:17:46 [16002.238581] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> 2024-12-26 16:17:46 [16002.247053] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
> e024se+0x3c/0x90 [amdxcp]
> 2024-12-26 16:17:46 [16002.337645]  __do_sys_delete_module.constprop.0+0x176/0x310
> 2024-12-26 16:17:46 [16002.344324]  do_syscall_64+0x5d/0x170
> 2024-12-26 16:17:46 [16002.348858]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 2024-12-26 16:17:46 [16002.354956] RIP: 0033:0x7f2736a620cb-12-26
> 
> Fix it by unplugging xcp drm devices when failed to probe GPU devices.
> 
> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
> Tested-by: Shuo Liu <shuox.liu@linux.alibaba.com>

Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>

Thanks,
Lijo

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 4 +++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 5ffe1dad9622..e7f35e3a6d2d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -164,8 +164,10 @@ int amdgpu_driver_load_kms(struct amdgpu_device *adev, unsigned long flags)
>  		DRM_WARN("smart shift update failed\n");
>  
>  out:
> -	if (r)
> +	if (r) {
> +		amdgpu_xcp_dev_unplug(adev);
>  		amdgpu_driver_unload_kms(dev);
> +	}
>  
>  	return r;
>  }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> index a6d456ec6aeb..ef4eaacf67f6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> @@ -382,6 +382,7 @@ void amdgpu_xcp_dev_unplug(struct amdgpu_device *adev)
>  		p_ddev->primary->dev = adev->xcp_mgr->xcp[i].pdev;
>  		p_ddev->driver =  adev->xcp_mgr->xcp[i].driver;
>  		p_ddev->vma_offset_manager = adev->xcp_mgr->xcp[i].vma_offset_manager;
> +		adev->xcp_mgr->xcp[i].ddev = NULL;
>  	}
>  }
>  

Re: [PATCH 4/6] amdgpu: fix use after free bug related to amdgpu_driver_release_kms()

[Chen, Xiaogang]

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


On 1/1/2025 11:36 PM, Jiang Liu wrote:
> If some GPU device failed to probe, `rmmod amdgpu` will trigger a use
> after free bug related to amdgpu_driver_release_kms() as:
> 2024-12-26 16:17:45 [16002.085540] BUG: kernel NULL pointer dereference, address: 0000000000000000
> 2024-12-26 16:17:45 [16002.093792] #PF: supervisor read access in kernel mode
> 2024-12-26 16:17:45 [16002.099993] #PF: error_code(0x0000) - not-present page
> 2024-12-26 16:17:45 [16002.106188] PGD 0 P4D 0
> 2024-12-26 16:17:45 [16002.109464] Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
> 2024-12-26 16:17:45 [16002.115372] CPU: 2 PID: 14375 Comm: rmmod Kdump: loaded Tainted: G        W   E      6.10.0+ #2
> 2024-12-26 16:17:45 [16002.125577] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
> 2024-12-26 16:17:45 [16002.136858] RIP: 0010:drm_sched_fini+0x3f/0xe0 [gpu_sched]
> 2024-12-26 16:17:45 [16002.143463] Code: 60 c6 87 be 00 00 00 01 e8 ce e0 90 d8 48 8d bb 80 00 00 00 e8 c2 e0 90 d8 8b 43 20 85 c0 74 51 45 31 e4 48 8b
> 43 28 4d 63 ec <4a> 8b 2c e8 48 89 ef e8 f5 0e 59 d9 48 8b 45 10 48 8d 55 10 48 39
> 2024-12-26 16:17:45 [16002.164992] RSP: 0018:ffffb570dbbb7da8 EFLAGS: 00010246
> 2024-12-26 16:17:45 [16002.171316] RAX: 0000000000000000 RBX: ffff96b0fdadc878 RCX: 0000000000000000
> 2024-12-26 16:17:46 [16002.179784] RDX: 000fffffffe00000 RSI: 0000000000000000 RDI: ffff96b0fdadc8f8
> 2024-12-26 16:17:46 [16002.188252] RBP: ffff96b0fdadc800 R08: ffff97abbd035040 R09: ffffffff9ac52540
> 2024-12-26 16:17:46 [16002.196722] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> 2024-12-26 16:17:46 [16002.205179] R13: 0000000000000000 R14: ffff96b0fdadfc00 R15: 0000000000000000
> 2024-12-26 16:17:46 [16002.213648] FS:  00007f2737000740(0000) GS:ffff97abbd100000(0000) knlGS:0000000000000000
> 2024-12-26 16:17:46 [16002.223189] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> 2024-12-26 16:17:46 [16002.230103] CR2: 0000000000000000 CR3: 000000011be3a005 CR4: 0000000000f70ef0
> 2024-12-26 16:17:46 [16002.238581] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> 2024-12-26 16:17:46 [16002.247053] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
> e024se+0x3c/0x90 [amdxcp]
> 2024-12-26 16:17:46 [16002.337645]  __do_sys_delete_module.constprop.0+0x176/0x310
> 2024-12-26 16:17:46 [16002.344324]  do_syscall_64+0x5d/0x170
> 2024-12-26 16:17:46 [16002.348858]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 2024-12-26 16:17:46 [16002.354956] RIP: 0033:0x7f2736a620cb-12-26
>
> Fix it by unplugging xcp drm devices when failed to probe GPU devices.
>
> Signed-off-by: Jiang Liu<gerry@linux.alibaba.com>
> Tested-by: Shuo Liu<shuox.liu@linux.alibaba.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 4 +++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c | 1 +
>   2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 5ffe1dad9622..e7f35e3a6d2d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -164,8 +164,10 @@ int amdgpu_driver_load_kms(struct amdgpu_device *adev, unsigned long flags)
>   		DRM_WARN("smart shift update failed\n");
>   
>   out:
> -	if (r)
> +	if (r) {
> +		amdgpu_xcp_dev_unplug(adev);
>   		amdgpu_driver_unload_kms(dev);
> +	}
>   

I wonder if you can call amdgpu_xcp_dev_unplug here. It will call 
drm_dev_unplug that unplugs a hotpluggable DRM device. In you case 
amdgpu_driver_load_kms failed during probe time. We need unwind 
amdgpu_driver_load_kms. Why need do unplug a DRM device?

The backtrace show:

2024-12-26 16:17:45 [16002.136858] RIP: 0010:drm_sched_fini+0x3f/0xe0 [gpu_sched]

has:

2024-12-26 16:17:45 [16002.085540] BUG: kernel NULL pointer dereference, address: 0000000000000000

How this change is related to the invalid access above? Can you explain 
more?

Regards

Xiaogang

>   	return r;
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> index a6d456ec6aeb..ef4eaacf67f6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> @@ -382,6 +382,7 @@ void amdgpu_xcp_dev_unplug(struct amdgpu_device *adev)
>   		p_ddev->primary->dev = adev->xcp_mgr->xcp[i].pdev;
>   		p_ddev->driver =  adev->xcp_mgr->xcp[i].driver;
>   		p_ddev->vma_offset_manager = adev->xcp_mgr->xcp[i].vma_offset_manager;
> +		adev->xcp_mgr->xcp[i].ddev = NULL;
>   	}
>   }
>   

[-- Attachment #2: Type: text/html, Size: 5631 bytes --]

Re: [PATCH 4/6] amdgpu: fix use after free bug related to amdgpu_driver_release_kms()

[Gerry Liu]

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



> 2025年1月3日 13:58，Chen, Xiaogang <xiaogang.chen@amd.com> 写道：
> 
> 
> 
> On 1/1/2025 11:36 PM, Jiang Liu wrote:
>> If some GPU device failed to probe, `rmmod amdgpu` will trigger a use
>> after free bug related to amdgpu_driver_release_kms() as:
>> 2024-12-26 16:17:45 [16002.085540] BUG: kernel NULL pointer dereference, address: 0000000000000000
>> 2024-12-26 16:17:45 [16002.093792] #PF: supervisor read access in kernel mode
>> 2024-12-26 16:17:45 [16002.099993] #PF: error_code(0x0000) - not-present page
>> 2024-12-26 16:17:45 [16002.106188] PGD 0 P4D 0
>> 2024-12-26 16:17:45 [16002.109464] Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
>> 2024-12-26 16:17:45 [16002.115372] CPU: 2 PID: 14375 Comm: rmmod Kdump: loaded Tainted: G        W   E      6.10.0+ #2
>> 2024-12-26 16:17:45 [16002.125577] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
>> 2024-12-26 16:17:45 [16002.136858] RIP: 0010:drm_sched_fini+0x3f/0xe0 [gpu_sched]
>> 2024-12-26 16:17:45 [16002.143463] Code: 60 c6 87 be 00 00 00 01 e8 ce e0 90 d8 48 8d bb 80 00 00 00 e8 c2 e0 90 d8 8b 43 20 85 c0 74 51 45 31 e4 48 8b
>> 43 28 4d 63 ec <4a> 8b 2c e8 48 89 ef e8 f5 0e 59 d9 48 8b 45 10 48 8d 55 10 48 39
>> 2024-12-26 16:17:45 [16002.164992] RSP: 0018:ffffb570dbbb7da8 EFLAGS: 00010246
>> 2024-12-26 16:17:45 [16002.171316] RAX: 0000000000000000 RBX: ffff96b0fdadc878 RCX: 0000000000000000
>> 2024-12-26 16:17:46 [16002.179784] RDX: 000fffffffe00000 RSI: 0000000000000000 RDI: ffff96b0fdadc8f8
>> 2024-12-26 16:17:46 [16002.188252] RBP: ffff96b0fdadc800 R08: ffff97abbd035040 R09: ffffffff9ac52540
>> 2024-12-26 16:17:46 [16002.196722] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
>> 2024-12-26 16:17:46 [16002.205179] R13: 0000000000000000 R14: ffff96b0fdadfc00 R15: 0000000000000000
>> 2024-12-26 16:17:46 [16002.213648] FS:  00007f2737000740(0000) GS:ffff97abbd100000(0000) knlGS:0000000000000000
>> 2024-12-26 16:17:46 [16002.223189] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> 2024-12-26 16:17:46 [16002.230103] CR2: 0000000000000000 CR3: 000000011be3a005 CR4: 0000000000f70ef0
>> 2024-12-26 16:17:46 [16002.238581] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> 2024-12-26 16:17:46 [16002.247053] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
>> e024se+0x3c/0x90 [amdxcp]
>> 2024-12-26 16:17:46 [16002.337645]  __do_sys_delete_module.constprop.0+0x176/0x310
>> 2024-12-26 16:17:46 [16002.344324]  do_syscall_64+0x5d/0x170
>> 2024-12-26 16:17:46 [16002.348858]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
>> 2024-12-26 16:17:46 [16002.354956] RIP: 0033:0x7f2736a620cb-12-26
>> 
>> Fix it by unplugging xcp drm devices when failed to probe GPU devices.
>> 
>> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com> <mailto:gerry@linux.alibaba.com>
>> Tested-by: Shuo Liu <shuox.liu@linux.alibaba.com> <mailto:shuox.liu@linux.alibaba.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 4 +++-
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c | 1 +
>>  2 files changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index 5ffe1dad9622..e7f35e3a6d2d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -164,8 +164,10 @@ int amdgpu_driver_load_kms(struct amdgpu_device *adev, unsigned long flags)
>>  		DRM_WARN("smart shift update failed\n");
>>  
>>  out:
>> -	if (r)
>> +	if (r) {
>> +		amdgpu_xcp_dev_unplug(adev);
>>  		amdgpu_driver_unload_kms(dev);
>> +	}
>>  
> I wonder if you can call amdgpu_xcp_dev_unplug here. It will call drm_dev_unplug that unplugs a hotpluggable DRM device. In you case amdgpu_driver_load_kms failed during probe time. We need unwind amdgpu_driver_load_kms. Why need do unplug a DRM device?
> 
> The backtrace show:
> 
> 2024-12-26 16:17:45 [16002.136858] RIP: 0010:drm_sched_fini+0x3f/0xe0 [gpu_sched]
> has:
> 
> 2024-12-26 16:17:45 [16002.085540] BUG: kernel NULL pointer dereference, address: 0000000000000000
> How this change is related to the invalid access above? Can you explain more?
> 
Per my understanding, `amdgpu_xcp_dev_unplug(adev)` is a workaround for design flaw of the amdgpu_xcp driver.
Currently the amdgpu_xcp driver assumes everything will go as expected and there’s no failure or GPU hot-lug operations.
So it only provides an interface `amdgpu_xcp_drm_dev_alloc()` to create xcp devices for a GPU instance, and another interface
`amdgpu_xcp_drv_release()` to destroy xcp devices for GPU instances. There’s no interface to undo what done by `amdgpu_xcp_drm_dev_alloc()`.
And I found `amdgpu_xcp_dev_unplug(adev)` can undo work done by `amdgpu_xcp_drm_dev_alloc()`. 

So it’s a workaround, the fundamental solution should be to enhance amdgpu_xcp driver to provide interface to unroll work
done by `amdgpu_xcp_drm_dev_alloc()`.

The code paths to trigger the use after free are:
1) in function amdgpu_xcp_dev_alloc(), we allocate a drm_device by calling amdgpu_xcp_drm_dev_alloc(), and then change the device’s driver to amdgpu_partition_driver.
2) in function amdgpu_xcp_dev_unplug(), it restores drm_device’s driver to the original device driver.

In function amdgpu_driver_load_kms(), if we don’t call amdgpu_xcp_dev_unplug() on error recover path, the `xcp_dev[idx].driver` will still points to amdgpu_partition_driver.
Later when amdgpu_xcp_drv_release() gets called, it will call platform_device_unregister() -> amdgpu_partition_driver.release -> amdgpu_driver_release_kms().
Here when amdgpu_driver_release_kms() gets called, the corresponding amdgpu_device object has already been released on error recovery path in function amdgpu_pci_probe().

So just like amdgpu_pci_remove(), we call amdgpu_xcp_gpu_dev_unplug() before calling amdkcl_drm_dev_release().
> Regards
> 
> Xiaogang
> 
> 
> 
> 
> 
>>  	return r;
>>  }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>> index a6d456ec6aeb..ef4eaacf67f6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>> @@ -382,6 +382,7 @@ void amdgpu_xcp_dev_unplug(struct amdgpu_device *adev)
>>  		p_ddev->primary->dev = adev->xcp_mgr->xcp[i].pdev;
>>  		p_ddev->driver =  adev->xcp_mgr->xcp[i].driver;
>>  		p_ddev->vma_offset_manager = adev->xcp_mgr->xcp[i].vma_offset_manager;
>> +		adev->xcp_mgr->xcp[i].ddev = NULL;
>>  	}
>>  }
>>  


[-- Attachment #2: Type: text/html, Size: 8475 bytes --]

Re: [PATCH 4/6] amdgpu: fix use after free bug related to amdgpu_driver_release_kms()

[Shuo Liu]

On Fri  3.Jan'25 at 15:02:38 +0800, Gerry Liu wrote:
>
>
>> 2025年1月3日 13:58，Chen, Xiaogang <xiaogang.chen@amd.com> 写道：
>>
>>
>>
>> On 1/1/2025 11:36 PM, Jiang Liu wrote:
>>> If some GPU device failed to probe, `rmmod amdgpu` will trigger a use
>>> after free bug related to amdgpu_driver_release_kms() as:
>>> 2024-12-26 16:17:45 [16002.085540] BUG: kernel NULL pointer dereference, address: 0000000000000000
>>> 2024-12-26 16:17:45 [16002.093792] #PF: supervisor read access in kernel mode
>>> 2024-12-26 16:17:45 [16002.099993] #PF: error_code(0x0000) - not-present page
>>> 2024-12-26 16:17:45 [16002.106188] PGD 0 P4D 0
>>> 2024-12-26 16:17:45 [16002.109464] Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
>>> 2024-12-26 16:17:45 [16002.115372] CPU: 2 PID: 14375 Comm: rmmod Kdump: loaded Tainted: G        W   E      6.10.0+ #2
>>> 2024-12-26 16:17:45 [16002.125577] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
>>> 2024-12-26 16:17:45 [16002.136858] RIP: 0010:drm_sched_fini+0x3f/0xe0 [gpu_sched]
>>> 2024-12-26 16:17:45 [16002.143463] Code: 60 c6 87 be 00 00 00 01 e8 ce e0 90 d8 48 8d bb 80 00 00 00 e8 c2 e0 90 d8 8b 43 20 85 c0 74 51 45 31 e4 48 8b
>>> 43 28 4d 63 ec <4a> 8b 2c e8 48 89 ef e8 f5 0e 59 d9 48 8b 45 10 48 8d 55 10 48 39
>>> 2024-12-26 16:17:45 [16002.164992] RSP: 0018:ffffb570dbbb7da8 EFLAGS: 00010246
>>> 2024-12-26 16:17:45 [16002.171316] RAX: 0000000000000000 RBX: ffff96b0fdadc878 RCX: 0000000000000000
>>> 2024-12-26 16:17:46 [16002.179784] RDX: 000fffffffe00000 RSI: 0000000000000000 RDI: ffff96b0fdadc8f8
>>> 2024-12-26 16:17:46 [16002.188252] RBP: ffff96b0fdadc800 R08: ffff97abbd035040 R09: ffffffff9ac52540
>>> 2024-12-26 16:17:46 [16002.196722] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
>>> 2024-12-26 16:17:46 [16002.205179] R13: 0000000000000000 R14: ffff96b0fdadfc00 R15: 0000000000000000
>>> 2024-12-26 16:17:46 [16002.213648] FS:  00007f2737000740(0000) GS:ffff97abbd100000(0000) knlGS:0000000000000000
>>> 2024-12-26 16:17:46 [16002.223189] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> 2024-12-26 16:17:46 [16002.230103] CR2: 0000000000000000 CR3: 000000011be3a005 CR4: 0000000000f70ef0
>>> 2024-12-26 16:17:46 [16002.238581] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>> 2024-12-26 16:17:46 [16002.247053] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
>>> e024se+0x3c/0x90 [amdxcp]
>>> 2024-12-26 16:17:46 [16002.337645]  __do_sys_delete_module.constprop.0+0x176/0x310
>>> 2024-12-26 16:17:46 [16002.344324]  do_syscall_64+0x5d/0x170
>>> 2024-12-26 16:17:46 [16002.348858]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>> 2024-12-26 16:17:46 [16002.354956] RIP: 0033:0x7f2736a620cb-12-26
>>>
>>> Fix it by unplugging xcp drm devices when failed to probe GPU devices.
>>>
>>> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com> <mailto:gerry@linux.alibaba.com>
>>> Tested-by: Shuo Liu <shuox.liu@linux.alibaba.com> <mailto:shuox.liu@linux.alibaba.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 4 +++-
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c | 1 +
>>>  2 files changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> index 5ffe1dad9622..e7f35e3a6d2d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> @@ -164,8 +164,10 @@ int amdgpu_driver_load_kms(struct amdgpu_device *adev, unsigned long flags)
>>>  		DRM_WARN("smart shift update failed\n");
>>>
>>>  out:
>>> -	if (r)
>>> +	if (r) {
>>> +		amdgpu_xcp_dev_unplug(adev);
>>>  		amdgpu_driver_unload_kms(dev);
>>> +	}
>>>
>> I wonder if you can call amdgpu_xcp_dev_unplug here. It will call drm_dev_unplug that unplugs a hotpluggable DRM device. In you case amdgpu_driver_load_kms failed during probe time. We need unwind amdgpu_driver_load_kms. Why need do unplug a DRM device?
>>
>> The backtrace show:
>>
>> 2024-12-26 16:17:45 [16002.136858] RIP: 0010:drm_sched_fini+0x3f/0xe0 [gpu_sched]
>> has:
>>
>> 2024-12-26 16:17:45 [16002.085540] BUG: kernel NULL pointer dereference, address: 0000000000000000
>> How this change is related to the invalid access above? Can you explain more?
>>
>Per my understanding, `amdgpu_xcp_dev_unplug(adev)` is a workaround for design flaw of the amdgpu_xcp driver.
>Currently the amdgpu_xcp driver assumes everything will go as expected and there’s no failure or GPU hot-lug operations.
>So it only provides an interface `amdgpu_xcp_drm_dev_alloc()` to create xcp devices for a GPU instance, and another interface
>`amdgpu_xcp_drv_release()` to destroy xcp devices for GPU instances. There’s no interface to undo what done by `amdgpu_xcp_drm_dev_alloc()`.
>And I found `amdgpu_xcp_dev_unplug(adev)` can undo work done by `amdgpu_xcp_drm_dev_alloc()`.
>
>So it’s a workaround, the fundamental solution should be to enhance amdgpu_xcp driver to provide interface to unroll work
>done by `amdgpu_xcp_drm_dev_alloc()`.
Agree. Actually, some ops of amdgpu_partition_driver cannot be reused
directly by amd_xcp drm device. DRM doesn't use its minor->dev as 
param of such callbacks, just like .release(). When
amdgpu_driver_release_kms() use a amd_xcp drm dev to find the `struct
amdgpu_device *adev`, unexcepted memory accesses.

shuo
>
>The code paths to trigger the use after free are:
>1) in function amdgpu_xcp_dev_alloc(), we allocate a drm_device by calling amdgpu_xcp_drm_dev_alloc(), and then change the device’s driver to amdgpu_partition_driver.
>2) in function amdgpu_xcp_dev_unplug(), it restores drm_device’s driver to the original device driver.
>
>In function amdgpu_driver_load_kms(), if we don’t call amdgpu_xcp_dev_unplug() on error recover path, the `xcp_dev[idx].driver` will still points to amdgpu_partition_driver.
>Later when amdgpu_xcp_drv_release() gets called, it will call platform_device_unregister() -> amdgpu_partition_driver.release -> amdgpu_driver_release_kms().
>Here when amdgpu_driver_release_kms() gets called, the corresponding amdgpu_device object has already been released on error recovery path in function amdgpu_pci_probe().
>
>So just like amdgpu_pci_remove(), we call amdgpu_xcp_gpu_dev_unplug() before calling amdkcl_drm_dev_release().
>> Regards
>>
>> Xiaogang
>>
>>
>>
>>
>>
>>>  	return r;
>>>  }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>>> index a6d456ec6aeb..ef4eaacf67f6 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>>> @@ -382,6 +382,7 @@ void amdgpu_xcp_dev_unplug(struct amdgpu_device *adev)
>>>  		p_ddev->primary->dev = adev->xcp_mgr->xcp[i].pdev;
>>>  		p_ddev->driver =  adev->xcp_mgr->xcp[i].driver;
>>>  		p_ddev->vma_offset_manager = adev->xcp_mgr->xcp[i].vma_offset_manager;
>>> +		adev->xcp_mgr->xcp[i].ddev = NULL;
>>>  	}
>>>  }
>>>
>

Re: [PATCH 4/6] amdgpu: fix use after free bug related to amdgpu_driver_release_kms()

[Chen, Xiaogang]

On 1/3/2025 1:43 AM, Shuo Liu wrote:
> On Fri  3.Jan'25 at 15:02:38 +0800, Gerry Liu wrote:
>>
>>
>>> 2025年1月3日 13:58，Chen, Xiaogang <xiaogang.chen@amd.com> 写道：
>>>
>>>
>>>
>>> On 1/1/2025 11:36 PM, Jiang Liu wrote:
>>>> If some GPU device failed to probe, `rmmod amdgpu` will trigger a use
>>>> after free bug related to amdgpu_driver_release_kms() as:
>>>> 2024-12-26 16:17:45 [16002.085540] BUG: kernel NULL pointer 
>>>> dereference, address: 0000000000000000
>>>> 2024-12-26 16:17:45 [16002.093792] #PF: supervisor read access in 
>>>> kernel mode
>>>> 2024-12-26 16:17:45 [16002.099993] #PF: error_code(0x0000) - 
>>>> not-present page
>>>> 2024-12-26 16:17:45 [16002.106188] PGD 0 P4D 0
>>>> 2024-12-26 16:17:45 [16002.109464] Oops: Oops: 0000 [#1] PREEMPT 
>>>> SMP NOPTI
>>>> 2024-12-26 16:17:45 [16002.115372] CPU: 2 PID: 14375 Comm: rmmod 
>>>> Kdump: loaded Tainted: G        W   E      6.10.0+ #2
>>>> 2024-12-26 16:17:45 [16002.125577] Hardware name: Alibaba Alibaba 
>>>> Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
>>>> 2024-12-26 16:17:45 [16002.136858] RIP: 
>>>> 0010:drm_sched_fini+0x3f/0xe0 [gpu_sched]
>>>> 2024-12-26 16:17:45 [16002.143463] Code: 60 c6 87 be 00 00 00 01 e8 
>>>> ce e0 90 d8 48 8d bb 80 00 00 00 e8 c2 e0 90 d8 8b 43 20 85 c0 74 
>>>> 51 45 31 e4 48 8b
>>>> 43 28 4d 63 ec <4a> 8b 2c e8 48 89 ef e8 f5 0e 59 d9 48 8b 45 10 48 
>>>> 8d 55 10 48 39
>>>> 2024-12-26 16:17:45 [16002.164992] RSP: 0018:ffffb570dbbb7da8 
>>>> EFLAGS: 00010246
>>>> 2024-12-26 16:17:45 [16002.171316] RAX: 0000000000000000 RBX: 
>>>> ffff96b0fdadc878 RCX: 0000000000000000
>>>> 2024-12-26 16:17:46 [16002.179784] RDX: 000fffffffe00000 RSI: 
>>>> 0000000000000000 RDI: ffff96b0fdadc8f8
>>>> 2024-12-26 16:17:46 [16002.188252] RBP: ffff96b0fdadc800 R08: 
>>>> ffff97abbd035040 R09: ffffffff9ac52540
>>>> 2024-12-26 16:17:46 [16002.196722] R10: 0000000000000000 R11: 
>>>> 0000000000000000 R12: 0000000000000000
>>>> 2024-12-26 16:17:46 [16002.205179] R13: 0000000000000000 R14: 
>>>> ffff96b0fdadfc00 R15: 0000000000000000
>>>> 2024-12-26 16:17:46 [16002.213648] FS: 00007f2737000740(0000) 
>>>> GS:ffff97abbd100000(0000) knlGS:0000000000000000
>>>> 2024-12-26 16:17:46 [16002.223189] CS:  0010 DS: 0000 ES: 0000 CR0: 
>>>> 0000000080050033
>>>> 2024-12-26 16:17:46 [16002.230103] CR2: 0000000000000000 CR3: 
>>>> 000000011be3a005 CR4: 0000000000f70ef0
>>>> 2024-12-26 16:17:46 [16002.238581] DR0: 0000000000000000 DR1: 
>>>> 0000000000000000 DR2: 0000000000000000
>>>> 2024-12-26 16:17:46 [16002.247053] DR3: 0000000000000000 DR6: 
>>>> 00000000fffe07f0 DR7: 0000000000000400
>>>> e024se+0x3c/0x90 [amdxcp]
>>>> 2024-12-26 16:17:46 [16002.337645] 
>>>> __do_sys_delete_module.constprop.0+0x176/0x310
>>>> 2024-12-26 16:17:46 [16002.344324]  do_syscall_64+0x5d/0x170
>>>> 2024-12-26 16:17:46 [16002.348858] 
>>>> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>>> 2024-12-26 16:17:46 [16002.354956] RIP: 0033:0x7f2736a620cb-12-26
>>>>
>>>> Fix it by unplugging xcp drm devices when failed to probe GPU devices.
>>>>
>>>> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com> 
>>>> <mailto:gerry@linux.alibaba.com>
>>>> Tested-by: Shuo Liu <shuox.liu@linux.alibaba.com> 
>>>> <mailto:shuox.liu@linux.alibaba.com>
>>>> ---
>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 4 +++-
>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c | 1 +
>>>>  2 files changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> index 5ffe1dad9622..e7f35e3a6d2d 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> @@ -164,8 +164,10 @@ int amdgpu_driver_load_kms(struct 
>>>> amdgpu_device *adev, unsigned long flags)
>>>>          DRM_WARN("smart shift update failed\n");
>>>>
>>>>  out:
>>>> -    if (r)
>>>> +    if (r) {
>>>> +        amdgpu_xcp_dev_unplug(adev);
>>>>          amdgpu_driver_unload_kms(dev);
>>>> +    }
>>>>
>>> I wonder if you can call amdgpu_xcp_dev_unplug here. It will call 
>>> drm_dev_unplug that unplugs a hotpluggable DRM device. In you case 
>>> amdgpu_driver_load_kms failed during probe time. We need unwind 
>>> amdgpu_driver_load_kms. Why need do unplug a DRM device?
>>>
>>> The backtrace show:
>>>
>>> 2024-12-26 16:17:45 [16002.136858] RIP: 
>>> 0010:drm_sched_fini+0x3f/0xe0 [gpu_sched]
>>> has:
>>>
>>> 2024-12-26 16:17:45 [16002.085540] BUG: kernel NULL pointer 
>>> dereference, address: 0000000000000000
>>> How this change is related to the invalid access above? Can you 
>>> explain more?
>>>
>> Per my understanding, `amdgpu_xcp_dev_unplug(adev)` is a workaround 
>> for design flaw of the amdgpu_xcp driver.
>> Currently the amdgpu_xcp driver assumes everything will go as 
>> expected and there’s no failure or GPU hot-lug operations.
>> So it only provides an interface `amdgpu_xcp_drm_dev_alloc()` to 
>> create xcp devices for a GPU instance, and another interface
>> `amdgpu_xcp_drv_release()` to destroy xcp devices for GPU instances. 
>> There’s no interface to undo what done by `amdgpu_xcp_drm_dev_alloc()`.
>> And I found `amdgpu_xcp_dev_unplug(adev)` can undo work done by 
>> `amdgpu_xcp_drm_dev_alloc()`.
>>
>> So it’s a workaround, the fundamental solution should be to enhance 
>> amdgpu_xcp driver to provide interface to unroll work
>> done by `amdgpu_xcp_drm_dev_alloc()`.
> Agree. Actually, some ops of amdgpu_partition_driver cannot be reused
> directly by amd_xcp drm device. DRM doesn't use its minor->dev as 
> param of such callbacks, just like .release(). When
> amdgpu_driver_release_kms() use a amd_xcp drm dev to find the `struct
> amdgpu_device *adev`, unexcepted memory accesses.

I understand the issue is from xcp driver, but I do not see it is a 
right workaround. The solution should be unwinding 
amdgpu_xcp_drm_dev_alloc(), not unplug drm device though part of 
amdgpu_xcp_dev_unplug(adev) may meet what you want.

still, the bracktrace has

2024-12-26 16:17:45 [16002.136858] RIP: 0010:drm_sched_fini+0x3f/0xe0 
[gpu_sched]

How the root cause of that is from xcp driver?

Regards

Xiaogang

>
> shuo
>>
>> The code paths to trigger the use after free are:
>> 1) in function amdgpu_xcp_dev_alloc(), we allocate a drm_device by 
>> calling amdgpu_xcp_drm_dev_alloc(), and then change the device’s 
>> driver to amdgpu_partition_driver.
>> 2) in function amdgpu_xcp_dev_unplug(), it restores drm_device’s 
>> driver to the original device driver.
>>
>> In function amdgpu_driver_load_kms(), if we don’t call 
>> amdgpu_xcp_dev_unplug() on error recover path, the 
>> `xcp_dev[idx].driver` will still points to amdgpu_partition_driver.
>> Later when amdgpu_xcp_drv_release() gets called, it will call 
>> platform_device_unregister() -> amdgpu_partition_driver.release -> 
>> amdgpu_driver_release_kms().
>> Here when amdgpu_driver_release_kms() gets called, the corresponding 
>> amdgpu_device object has already been released on error recovery path 
>> in function amdgpu_pci_probe().
>>
>> So just like amdgpu_pci_remove(), we call amdgpu_xcp_gpu_dev_unplug() 
>> before calling amdkcl_drm_dev_release().
>>> Regards
>>>
>>> Xiaogang
>>>
>>>
>>>
>>>
>>>
>>>>      return r;
>>>>  }
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>>>> index a6d456ec6aeb..ef4eaacf67f6 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>>>> @@ -382,6 +382,7 @@ void amdgpu_xcp_dev_unplug(struct amdgpu_device 
>>>> *adev)
>>>>          p_ddev->primary->dev = adev->xcp_mgr->xcp[i].pdev;
>>>>          p_ddev->driver = adev->xcp_mgr->xcp[i].driver;
>>>>          p_ddev->vma_offset_manager = 
>>>> adev->xcp_mgr->xcp[i].vma_offset_manager;
>>>> +        adev->xcp_mgr->xcp[i].ddev = NULL;
>>>>      }
>>>>  }
>>>>
>>

Re: [PATCH 4/6] amdgpu: fix use after free bug related to amdgpu_driver_release_kms()

[Gerry Liu]

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



> 2025年1月4日 01:34，Chen, Xiaogang <xiaogang.chen@amd.com> 写道：
> 
> 
> On 1/3/2025 1:43 AM, Shuo Liu wrote:
>> On Fri  3.Jan'25 at 15:02:38 +0800, Gerry Liu wrote:
>>> 
>>> 
>>>> 2025年1月3日 13:58，Chen, Xiaogang <xiaogang.chen@amd.com> 写道：
>>>> 
>>>> 
>>>> 
>>>> On 1/1/2025 11:36 PM, Jiang Liu wrote:
>>>>> If some GPU device failed to probe, `rmmod amdgpu` will trigger a use
>>>>> after free bug related to amdgpu_driver_release_kms() as:
>>>>> 2024-12-26 16:17:45 [16002.085540] BUG: kernel NULL pointer dereference, address: 0000000000000000
>>>>> 2024-12-26 16:17:45 [16002.093792] #PF: supervisor read access in kernel mode
>>>>> 2024-12-26 16:17:45 [16002.099993] #PF: error_code(0x0000) - not-present page
>>>>> 2024-12-26 16:17:45 [16002.106188] PGD 0 P4D 0
>>>>> 2024-12-26 16:17:45 [16002.109464] Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
>>>>> 2024-12-26 16:17:45 [16002.115372] CPU: 2 PID: 14375 Comm: rmmod Kdump: loaded Tainted: G        W   E      6.10.0+ #2
>>>>> 2024-12-26 16:17:45 [16002.125577] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
>>>>> 2024-12-26 16:17:45 [16002.136858] RIP: 0010:drm_sched_fini+0x3f/0xe0 [gpu_sched]
>>>>> 2024-12-26 16:17:45 [16002.143463] Code: 60 c6 87 be 00 00 00 01 e8 ce e0 90 d8 48 8d bb 80 00 00 00 e8 c2 e0 90 d8 8b 43 20 85 c0 74 51 45 31 e4 48 8b
>>>>> 43 28 4d 63 ec <4a> 8b 2c e8 48 89 ef e8 f5 0e 59 d9 48 8b 45 10 48 8d 55 10 48 39
>>>>> 2024-12-26 16:17:45 [16002.164992] RSP: 0018:ffffb570dbbb7da8 EFLAGS: 00010246
>>>>> 2024-12-26 16:17:45 [16002.171316] RAX: 0000000000000000 RBX: ffff96b0fdadc878 RCX: 0000000000000000
>>>>> 2024-12-26 16:17:46 [16002.179784] RDX: 000fffffffe00000 RSI: 0000000000000000 RDI: ffff96b0fdadc8f8
>>>>> 2024-12-26 16:17:46 [16002.188252] RBP: ffff96b0fdadc800 R08: ffff97abbd035040 R09: ffffffff9ac52540
>>>>> 2024-12-26 16:17:46 [16002.196722] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
>>>>> 2024-12-26 16:17:46 [16002.205179] R13: 0000000000000000 R14: ffff96b0fdadfc00 R15: 0000000000000000
>>>>> 2024-12-26 16:17:46 [16002.213648] FS: 00007f2737000740(0000) GS:ffff97abbd100000(0000) knlGS:0000000000000000
>>>>> 2024-12-26 16:17:46 [16002.223189] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>> 2024-12-26 16:17:46 [16002.230103] CR2: 0000000000000000 CR3: 000000011be3a005 CR4: 0000000000f70ef0
>>>>> 2024-12-26 16:17:46 [16002.238581] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>>>> 2024-12-26 16:17:46 [16002.247053] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
>>>>> e024se+0x3c/0x90 [amdxcp]
>>>>> 2024-12-26 16:17:46 [16002.337645] __do_sys_delete_module.constprop.0+0x176/0x310
>>>>> 2024-12-26 16:17:46 [16002.344324]  do_syscall_64+0x5d/0x170
>>>>> 2024-12-26 16:17:46 [16002.348858] entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>>>> 2024-12-26 16:17:46 [16002.354956] RIP: 0033:0x7f2736a620cb-12-26
>>>>> 
>>>>> Fix it by unplugging xcp drm devices when failed to probe GPU devices.
>>>>> 
>>>>> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com> <mailto:gerry@linux.alibaba.com>
>>>>> Tested-by: Shuo Liu <shuox.liu@linux.alibaba.com> <mailto:shuox.liu@linux.alibaba.com>
>>>>> ---
>>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 4 +++-
>>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c | 1 +
>>>>>  2 files changed, 4 insertions(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>>> index 5ffe1dad9622..e7f35e3a6d2d 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>>> @@ -164,8 +164,10 @@ int amdgpu_driver_load_kms(struct amdgpu_device *adev, unsigned long flags)
>>>>>          DRM_WARN("smart shift update failed\n");
>>>>> 
>>>>>  out:
>>>>> -    if (r)
>>>>> +    if (r) {
>>>>> +        amdgpu_xcp_dev_unplug(adev);
>>>>>          amdgpu_driver_unload_kms(dev);
>>>>> +    }
>>>>> 
>>>> I wonder if you can call amdgpu_xcp_dev_unplug here. It will call drm_dev_unplug that unplugs a hotpluggable DRM device. In you case amdgpu_driver_load_kms failed during probe time. We need unwind amdgpu_driver_load_kms. Why need do unplug a DRM device?
>>>> 
>>>> The backtrace show:
>>>> 
>>>> 2024-12-26 16:17:45 [16002.136858] RIP: 0010:drm_sched_fini+0x3f/0xe0 [gpu_sched]
>>>> has:
>>>> 
>>>> 2024-12-26 16:17:45 [16002.085540] BUG: kernel NULL pointer dereference, address: 0000000000000000
>>>> How this change is related to the invalid access above? Can you explain more?
>>>> 
>>> Per my understanding, `amdgpu_xcp_dev_unplug(adev)` is a workaround for design flaw of the amdgpu_xcp driver.
>>> Currently the amdgpu_xcp driver assumes everything will go as expected and there’s no failure or GPU hot-lug operations.
>>> So it only provides an interface `amdgpu_xcp_drm_dev_alloc()` to create xcp devices for a GPU instance, and another interface
>>> `amdgpu_xcp_drv_release()` to destroy xcp devices for GPU instances. There’s no interface to undo what done by `amdgpu_xcp_drm_dev_alloc()`.
>>> And I found `amdgpu_xcp_dev_unplug(adev)` can undo work done by `amdgpu_xcp_drm_dev_alloc()`.
>>> 
>>> So it’s a workaround, the fundamental solution should be to enhance amdgpu_xcp driver to provide interface to unroll work
>>> done by `amdgpu_xcp_drm_dev_alloc()`.
>> Agree. Actually, some ops of amdgpu_partition_driver cannot be reused
>> directly by amd_xcp drm device. DRM doesn't use its minor->dev as param of such callbacks, just like .release(). When
>> amdgpu_driver_release_kms() use a amd_xcp drm dev to find the `struct
>> amdgpu_device *adev`, unexcepted memory accesses.
> 
> I understand the issue is from xcp driver, but I do not see it is a right workaround. The solution should be unwinding amdgpu_xcp_drm_dev_alloc(), not unplug drm device though part of amdgpu_xcp_dev_unplug(adev) may meet what you want.
> 
> still, the bracktrace has
> 
> 2024-12-26 16:17:45 [16002.136858] RIP: 0010:drm_sched_fini+0x3f/0xe0 [gpu_sched]
> 
> How the root cause of that is from xcp driver?
> 
After a second thought, I will try to add a new patch to fix the root cause.
With the new patch applied, current workaround becomes the right fix then:)

> Regards
> 
> Xiaogang
> 
>> 
>> shuo
>>> 
>>> The code paths to trigger the use after free are:
>>> 1) in function amdgpu_xcp_dev_alloc(), we allocate a drm_device by calling amdgpu_xcp_drm_dev_alloc(), and then change the device’s driver to amdgpu_partition_driver.
>>> 2) in function amdgpu_xcp_dev_unplug(), it restores drm_device’s driver to the original device driver.
>>> 
>>> In function amdgpu_driver_load_kms(), if we don’t call amdgpu_xcp_dev_unplug() on error recover path, the `xcp_dev[idx].driver` will still points to amdgpu_partition_driver.
>>> Later when amdgpu_xcp_drv_release() gets called, it will call platform_device_unregister() -> amdgpu_partition_driver.release -> amdgpu_driver_release_kms().
>>> Here when amdgpu_driver_release_kms() gets called, the corresponding amdgpu_device object has already been released on error recovery path in function amdgpu_pci_probe().
>>> 
>>> So just like amdgpu_pci_remove(), we call amdgpu_xcp_gpu_dev_unplug() before calling amdkcl_drm_dev_release().
>>>> Regards
>>>> 
>>>> Xiaogang
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>>>      return r;
>>>>>  }
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>>>>> index a6d456ec6aeb..ef4eaacf67f6 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>>>>> @@ -382,6 +382,7 @@ void amdgpu_xcp_dev_unplug(struct amdgpu_device *adev)
>>>>>          p_ddev->primary->dev = adev->xcp_mgr->xcp[i].pdev;
>>>>>          p_ddev->driver = adev->xcp_mgr->xcp[i].driver;
>>>>>          p_ddev->vma_offset_manager = adev->xcp_mgr->xcp[i].vma_offset_manager;
>>>>> +        adev->xcp_mgr->xcp[i].ddev = NULL;
>>>>>      }
>>>>>  }


[-- Attachment #2: Type: text/html, Size: 19160 bytes --]

[PATCH 5/6] amdgpu: fix invalid memory access in amdgpu_fence_driver_sw_fini()

[Jiang Liu]

Function detects initialization status by checking sched->ops, so set
sched->ops to non-NULL just before return in function drm_sched_init()
to avoid possible invalid memory access on error recover path.

Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 5adab4b3386c..ce2afdfc0158 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1331,6 +1331,8 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 	if (sched->own_submit_wq)
 		destroy_workqueue(sched->submit_wq);
 	drm_err(sched, "%s: Failed to setup GPU scheduler--out of memory\n", __func__);
+	// amdgpu_fence_driver_sw_fini() checks sched->ops for status.
+	sched->ops = NULL;
 	return -ENOMEM;
 }
 EXPORT_SYMBOL(drm_sched_init);
@@ -1375,6 +1377,7 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched)
 	sched->ready = false;
 	kfree(sched->sched_rq);
 	sched->sched_rq = NULL;
+	sched->ops = NULL;
 }
 EXPORT_SYMBOL(drm_sched_fini);
 
-- 
2.43.5

Re: [PATCH 5/6] amdgpu: fix invalid memory access in amdgpu_fence_driver_sw_fini()

[Chen, Xiaogang]

On 1/1/2025 11:36 PM, Jiang Liu wrote:
> Function detects initialization status by checking sched->ops, so set
> sched->ops to non-NULL just before return in function drm_sched_init()
> to avoid possible invalid memory access on error recover path.
>
> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
> ---
>   drivers/gpu/drm/scheduler/sched_main.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 5adab4b3386c..ce2afdfc0158 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -1331,6 +1331,8 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
>   	if (sched->own_submit_wq)
>   		destroy_workqueue(sched->submit_wq);
>   	drm_err(sched, "%s: Failed to setup GPU scheduler--out of memory\n", __func__);
> +	// amdgpu_fence_driver_sw_fini() checks sched->ops for status.
> +	sched->ops = NULL;

Instead change drm/scheduler how about change amdgpu driver? At 
amdgpu_device_init_schedulers if drm_sched_init fail for a ring 
scheduler set

ring->sched->op = NULL;

It is not drm issue. If drm's scheduler init fail its operation has no 
meaning, we can set it to null at amdgpu driver.

Regards

Xiaogang

>   	return -ENOMEM;
>   }
>   EXPORT_SYMBOL(drm_sched_init);
> @@ -1375,6 +1377,7 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched)
>   	sched->ready = false;
>   	kfree(sched->sched_rq);
>   	sched->sched_rq = NULL;
> +	sched->ops = NULL;
>   }
>   EXPORT_SYMBOL(drm_sched_fini);
>   

Re: [PATCH 5/6] amdgpu: fix invalid memory access in amdgpu_fence_driver_sw_fini()

[Gerry Liu]

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



> 2025年1月3日 07:09，Chen, Xiaogang <xiaogang.chen@amd.com> 写道：
> 
> 
> On 1/1/2025 11:36 PM, Jiang Liu wrote:
>> Function detects initialization status by checking sched->ops, so set
>> sched->ops to non-NULL just before return in function drm_sched_init()
>> to avoid possible invalid memory access on error recover path.
>> 
>> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
>> ---
>>  drivers/gpu/drm/scheduler/sched_main.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>> index 5adab4b3386c..ce2afdfc0158 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -1331,6 +1331,8 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
>>  	if (sched->own_submit_wq)
>>  		destroy_workqueue(sched->submit_wq);
>>  	drm_err(sched, "%s: Failed to setup GPU scheduler--out of memory\n", __func__);
>> +	// amdgpu_fence_driver_sw_fini() checks sched->ops for status.
>> +	sched->ops = NULL;
> 
> Instead change drm/scheduler how about change amdgpu driver? At amdgpu_device_init_schedulers if drm_sched_init fail for a ring scheduler set
> 
> ring->sched->op = NULL;
> 
> It is not drm issue. If drm's scheduler init fail its operation has no meaning, we can set it to null at amdgpu driver.
Good suggestion, I will fix it in this way.

> 
> Regards
> 
> Xiaogang
> 
>>  	return -ENOMEM;
>>  }
>>  EXPORT_SYMBOL(drm_sched_init);
>> @@ -1375,6 +1377,7 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched)
>>  	sched->ready = false;
>>  	kfree(sched->sched_rq);
>>  	sched->sched_rq = NULL;
>> +	sched->ops = NULL;
>>  }
>>  EXPORT_SYMBOL(drm_sched_fini);


[-- Attachment #2: Type: text/html, Size: 10726 bytes --]

[PATCH 6/6] amdgpu: get rid of false warnings caused by amdgpu_irq_put()

[Jiang Liu]

If error happens before amdgpu_fence_driver_hw_init() gets called during
device probe, it will trigger a false warning in amdgpu_irq_put() as
below:
[ 1209.300996] ------------[ cut here ]------------
[ 1209.301061] WARNING: CPU: 48 PID: 293 at /tmp/amd.Rc9jFrl7/amd/amdgpu/amdgpu_irq.c:633 amdgpu_irq_put+0x45/0x70 [amdgpu]
[ 1209.301062] Modules linked in: ...
[ 1209.301093] CPU: 48 PID: 293 Comm: kworker/48:1 Kdump: loaded Tainted: G        W  OE     5.10.134-17.2.al8.x86_64 #1
[ 1209.301094] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
[ 1209.301095] Workqueue: events work_for_cpu_fn
[ 1209.301159] RIP: 0010:amdgpu_irq_put+0x45/0x70 [amdgpu]
[ 1209.301160] Code: 48 8b 4e 10 48 83 39 00 74 2c 89 d1 48 8d 04 88 8b 08 85 c9 74 14 f0 ff 08 b8 00 00 00 00 74 05 c3 cc cc cc cc e9 8b fd ff ff <0f> 0b b8 ea ff ff ff c3 cc cc cc cc b8 ea ff ff ff c3 cc cc cc cc
[ 1209.301162] RSP: 0018:ffffb08a99c8fd88 EFLAGS: 00010246
[ 1209.301162] RAX: ffff9efe1bcbf500 RBX: ffff9efe1cc3e400 RCX: 0000000000000000
[ 1209.301163] RDX: 0000000000000000 RSI: ffff9efe1cc3b108 RDI: ffff9efe1cc00000
[ 1209.301163] RBP: ffff9efe1cc10818 R08: 0000000000000001 R09: 000000000000000d
[ 1209.301164] R10: ffffb08a99c8fb48 R11: ffffffffa2068018 R12: ffff9efe1cc109d0
[ 1209.301164] R13: ffff9efe1cc00010 R14: ffff9efe1cc00000 R15: ffff9efe1cc3b108
[ 1209.301165] FS:  0000000000000000(0000) GS:ffff9ff9fce00000(0000) knlGS:0000000000000000
[ 1209.301165] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1209.301165] CR2: 00007fd0f6e860d0 CR3: 0000010092baa003 CR4: 0000000002770ee0
[ 1209.301166] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1209.301166] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
[ 1209.301167] PKRU: 55555554
[ 1209.301167] Call Trace:
[ 1209.301225]  amdgpu_fence_driver_hw_fini+0xda/0x110 [amdgpu]
[ 1209.301284]  amdgpu_device_fini_hw+0xaf/0x200 [amdgpu]
[ 1209.301342]  amdgpu_driver_load_kms+0x7f/0xc0 [amdgpu]
[ 1209.301400]  amdgpu_pci_probe+0x1cd/0x4a0 [amdgpu]
[ 1209.301401]  local_pci_probe+0x40/0xa0
[ 1209.301402]  work_for_cpu_fn+0x13/0x20
[ 1209.301403]  process_one_work+0x1ad/0x380
[ 1209.301404]  worker_thread+0x1c8/0x310
[ 1209.301405]  ? process_one_work+0x380/0x380
[ 1209.301406]  kthread+0x118/0x140
[ 1209.301407]  ? __kthread_bind_mask+0x60/0x60
[ 1209.301408]  ret_from_fork+0x1f/0x30
[ 1209.301410] ---[ end trace 733f120fe2ab13e5 ]---
[ 1209.301418] ------------[ cut here ]------------

Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 9 +++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h  | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index d4f3fb3519c8..141ec22b7e7b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -614,9 +614,11 @@ void amdgpu_fence_driver_hw_fini(struct amdgpu_device *adev)
 
 		if (!drm_dev_is_unplugged(adev_to_drm(adev)) &&
 		    ring->fence_drv.irq_src &&
-		    amdgpu_fence_need_ring_interrupt_restore(ring))
+		    ring->fence_drv.irq_enabled) {
 			amdgpu_irq_put(adev, ring->fence_drv.irq_src,
 				       ring->fence_drv.irq_type);
+		        ring->fence_drv.irq_enabled = false;
+		}
 
 		del_timer_sync(&ring->fence_drv.fallback_timer);
 	}
@@ -691,9 +693,12 @@ void amdgpu_fence_driver_hw_init(struct amdgpu_device *adev)
 
 		/* enable the interrupt */
 		if (ring->fence_drv.irq_src &&
-		    amdgpu_fence_need_ring_interrupt_restore(ring))
+		    !ring->fence_drv.irq_enabled &&
+		    amdgpu_fence_need_ring_interrupt_restore(ring)) {
 			amdgpu_irq_get(adev, ring->fence_drv.irq_src,
 				       ring->fence_drv.irq_type);
+		        ring->fence_drv.irq_enabled = true;
+		}
 	}
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index f93f51002201..96a0f9066c69 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -118,6 +118,7 @@ struct amdgpu_fence_driver {
 	uint32_t			sync_seq;
 	atomic_t			last_seq;
 	bool				initialized;
+	bool				irq_enabled;
 	struct amdgpu_irq_src		*irq_src;
 	unsigned			irq_type;
 	struct timer_list		fallback_timer;
-- 
2.43.5