[v4 3/5] drm/amdgpu: fix use after free bug related to amdgpu_driver_release_kms()
Mario Limonciello
mario.limonciello at amd.com
Fri Jan 10 19:03:33 UTC 2025
On 1/9/2025 20:08, 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:
> [16002.085540] BUG: kernel NULL pointer dereference, address: 0000000000000000
> [16002.093792] #PF: supervisor read access in kernel mode
> [16002.099993] #PF: error_code(0x0000) - not-present page
> [16002.106188] PGD 0 P4D 0
> [16002.109464] Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
> [16002.115372] CPU: 2 PID: 14375 Comm: rmmod Kdump: loaded Tainted: G W E 6.10.0+ #2
> [16002.125577] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.0.ES.AL.P.087.05 04/07/2024
> [16002.136858] RIP: 0010:drm_sched_fini+0x3f/0xe0 [gpu_sched]
> [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
> 8b 2c e8 48 89 ef e8 f5 0e 59 d9 48 8b 45 10 48 8d 55 10 48 39
> [16002.164992] RSP: 0018:ffffb570dbbb7da8 EFLAGS: 00010246
> [16002.171316] RAX: 0000000000000000 RBX: ffff96b0fdadc878 RCX: 0000000000000000
> [16002.179784] RDX: 000fffffffe00000 RSI: 0000000000000000 RDI: ffff96b0fdadc8f8
> [16002.188252] RBP: ffff96b0fdadc800 R08: ffff97abbd035040 R09: ffffffff9ac52540
> [16002.196722] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> [16002.205179] R13: 0000000000000000 R14: ffff96b0fdadfc00 R15: 0000000000000000
> [16002.213648] FS: 00007f2737000740(0000) GS:ffff97abbd100000(0000) knlGS:0000000000000000
> [16002.223189] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [16002.230103] CR2: 0000000000000000 CR3: 000000011be3a005 CR4: 0000000000f70ef0
> [16002.238581] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [16002.247053] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
> dxcp]
> [16002.337645] __do_sys_delete_module.constprop.0+0x176/0x310
> [16002.344324] do_syscall_64+0x5d/0x170
> [16002.348858] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [16002.354956] RIP: 0033:0x7f2736a620cb-12-26
>
> Fix it by removing xcp drm devices when failed to probe GPU devices.
>
> Signed-off-by: Jiang Liu <gerry at linux.alibaba.com>
> Tested-by: Shuo Liu <shuox.liu at linux.alibaba.com>
> Reviewed-by: Lijo Lazar <lijo.lazar at amd.com>
I have one nit below, but otherwise LGTM.
Reviewed-by: Mario Limonciello <mario.limonciello at amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c | 70 ++++++++++++++++++----
> drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.h | 4 +-
> 4 files changed, 64 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 5ff53a3b9851..f29a4ad3c6d0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4777,6 +4777,8 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev)
> amdgpu_reset_put_reset_domain(adev->reset_domain);
> adev->reset_domain = NULL;
>
> + amdgpu_xcp_mgr_fini(adev);
> +
> kfree(adev->pci_state);
>
> }
> @@ -6682,7 +6684,7 @@ void amdgpu_device_halt(struct amdgpu_device *adev)
> struct pci_dev *pdev = adev->pdev;
> struct drm_device *ddev = adev_to_drm(adev);
>
> - amdgpu_xcp_dev_unplug(adev);
> + amdgpu_xcp_dev_deregister(adev);
> drm_dev_unplug(ddev);
>
> amdgpu_irq_disable_all(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 62de668e9ff8..41d1b06be600 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2435,7 +2435,7 @@ amdgpu_pci_remove(struct pci_dev *pdev)
> struct drm_device *dev = pci_get_drvdata(pdev);
> struct amdgpu_device *adev = drm_to_adev(dev);
>
> - amdgpu_xcp_dev_unplug(adev);
> + amdgpu_xcp_dev_deregister(adev);
> amdgpu_gmc_prepare_nps_mode_change(adev);
> drm_dev_unplug(dev);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> index e209b5e101df..21993aff0dbc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> @@ -268,21 +268,34 @@ static int amdgpu_xcp_dev_alloc(struct amdgpu_device *adev)
> return ret;
> }
>
> - /* Redirect all IOCTLs to the primary device */
> - adev->xcp_mgr->xcp[i].rdev = p_ddev->render->dev;
> - adev->xcp_mgr->xcp[i].pdev = p_ddev->primary->dev;
> - adev->xcp_mgr->xcp[i].driver = (struct drm_driver *)p_ddev->driver;
> - adev->xcp_mgr->xcp[i].vma_offset_manager = p_ddev->vma_offset_manager;
> - p_ddev->render->dev = ddev;
> - p_ddev->primary->dev = ddev;
> - p_ddev->vma_offset_manager = ddev->vma_offset_manager;
> - p_ddev->driver = &amdgpu_partition_driver;
> adev->xcp_mgr->xcp[i].ddev = p_ddev;
> }
>
> return 0;
> }
>
> +static void amdgpu_xcp_dev_free(struct amdgpu_device *adev)
> +{
> + struct drm_device *p_ddev;
> + int i;
> +
> + if (!adev->xcp_mgr)
> + return;
> +
> + for (i = 1; i < MAX_XCP; i++) {
> + if (!adev->xcp_mgr->xcp[i].ddev)
> + break;
> +
> + p_ddev = adev->xcp_mgr->xcp[i].ddev;
> + adev->xcp_mgr->xcp[i].ddev = NULL;
> +
> + amdgpu_xcp_drm_dev_free(p_ddev);
> + }
> +
> + adev->xcp_mgr->xcp->ddev = NULL;
> +}
> +
> +
You only need one newline here.
> int amdgpu_xcp_mgr_init(struct amdgpu_device *adev, int init_mode,
> int init_num_xcps,
> struct amdgpu_xcp_mgr_funcs *xcp_funcs)
> @@ -310,6 +323,13 @@ int amdgpu_xcp_mgr_init(struct amdgpu_device *adev, int init_mode,
> return amdgpu_xcp_dev_alloc(adev);
> }
>
> +void amdgpu_xcp_mgr_fini(struct amdgpu_device *adev)
> +{
> + amdgpu_xcp_dev_free(adev);
> + kfree(adev->xcp_mgr);
> + adev->xcp_mgr = NULL;
> +}
> +
> int amdgpu_xcp_get_partition(struct amdgpu_xcp_mgr *xcp_mgr,
> enum AMDGPU_XCP_IP_BLOCK ip, int instance)
> {
> @@ -348,23 +368,44 @@ int amdgpu_xcp_dev_register(struct amdgpu_device *adev,
> const struct pci_device_id *ent)
> {
> int i, ret;
> + struct drm_device *p_ddev;
> + struct drm_device *ddev;
>
> if (!adev->xcp_mgr)
> return 0;
>
> + ddev = adev_to_drm(adev);
> +
> for (i = 1; i < MAX_XCP; i++) {
> - if (!adev->xcp_mgr->xcp[i].ddev)
> + if (!adev->xcp_mgr->xcp[i].ddev || adev->xcp_mgr->xcp[i].driver)
> break;
>
> + /* Redirect all IOCTLs to the primary device */
> + p_ddev = adev->xcp_mgr->xcp[i].ddev;
> + adev->xcp_mgr->xcp[i].rdev = p_ddev->render->dev;
> + adev->xcp_mgr->xcp[i].pdev = p_ddev->primary->dev;
> + adev->xcp_mgr->xcp[i].driver = (struct drm_driver *)p_ddev->driver;
> + adev->xcp_mgr->xcp[i].vma_offset_manager = p_ddev->vma_offset_manager;
> + p_ddev->render->dev = ddev;
> + p_ddev->primary->dev = ddev;
> + p_ddev->driver = &amdgpu_partition_driver;
> + p_ddev->vma_offset_manager = ddev->vma_offset_manager;
> +
> ret = drm_dev_register(adev->xcp_mgr->xcp[i].ddev, ent->driver_data);
> - if (ret)
> + if (ret) {
> + p_ddev->render->dev = adev->xcp_mgr->xcp[i].rdev;
> + 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].driver = NULL;
> return ret;
> + }
> }
>
> return 0;
> }
>
> -void amdgpu_xcp_dev_unplug(struct amdgpu_device *adev)
> +void amdgpu_xcp_dev_deregister(struct amdgpu_device *adev)
> {
> struct drm_device *p_ddev;
> int i;
> @@ -373,15 +414,18 @@ void amdgpu_xcp_dev_unplug(struct amdgpu_device *adev)
> return;
>
> for (i = 1; i < MAX_XCP; i++) {
> - if (!adev->xcp_mgr->xcp[i].ddev)
> + if (!adev->xcp_mgr->xcp[i].ddev || !adev->xcp_mgr->xcp[i].driver)
> break;
>
> + // Restore and free the original drm_device.
> p_ddev = adev->xcp_mgr->xcp[i].ddev;
> drm_dev_unplug(p_ddev);
> +
> p_ddev->render->dev = adev->xcp_mgr->xcp[i].rdev;
> 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].driver = NULL;
> }
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.h
> index b63f53242c57..be22d4398463 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.h
> @@ -101,6 +101,7 @@ struct amdgpu_xcp {
> uint8_t id;
> uint8_t mem_id;
> bool valid;
> + bool registered;
> atomic_t ref_cnt;
> struct drm_device *ddev;
> struct drm_device *rdev;
> @@ -155,6 +156,7 @@ int amdgpu_xcp_resume(struct amdgpu_xcp_mgr *xcp_mgr, int xcp_id);
>
> int amdgpu_xcp_mgr_init(struct amdgpu_device *adev, int init_mode,
> int init_xcps, struct amdgpu_xcp_mgr_funcs *xcp_funcs);
> +void amdgpu_xcp_mgr_fini(struct amdgpu_device *adev);
> int amdgpu_xcp_init(struct amdgpu_xcp_mgr *xcp_mgr, int num_xcps, int mode);
> int amdgpu_xcp_query_partition_mode(struct amdgpu_xcp_mgr *xcp_mgr, u32 flags);
> int amdgpu_xcp_switch_partition_mode(struct amdgpu_xcp_mgr *xcp_mgr, int mode);
> @@ -168,7 +170,7 @@ int amdgpu_xcp_get_inst_details(struct amdgpu_xcp *xcp,
>
> int amdgpu_xcp_dev_register(struct amdgpu_device *adev,
> const struct pci_device_id *ent);
> -void amdgpu_xcp_dev_unplug(struct amdgpu_device *adev);
> +void amdgpu_xcp_dev_deregister(struct amdgpu_device *adev);
> int amdgpu_xcp_open_device(struct amdgpu_device *adev,
> struct amdgpu_fpriv *fpriv,
> struct drm_file *file_priv);
More information about the amd-gfx
mailing list