Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] `amdgpu_ttm_enable_buffer_funcs` is called asynchronously without locks, racing with device teardown. Pre-existing issues: - [High] Iterating over `hive->device_list` without holding `hive->hive_lock` causes a data race and potential use-after-free. -- --- Patch [2]: [PATCH v3 2/3] drm/amdgpu: delay ttm buffer func enablement on xgmi --- commit 9b07616fd6cac22b50c5fa10e4c312c149b74bbd Author: Pierre-Eric Pelloux-Prayer <[email protected]> drm/amdgpu: delay ttm buffer func enablement on xgmi This commit delays SDMA engine init and ttm buffer func enablement until after the reset lock is dropped for XGMI devices. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c > index 2725230aa5e39..45e31b3daf065 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c > @@ -1380,6 +1380,9 @@ static void amdgpu_xgmi_reset_on_init_work(struct > work_struct *work) > amdgpu_device_unlock_reset_domain(tmp_adev->reset_domain); > > list_for_each_entry(tmp_adev, &hive->device_list, gmc.xgmi.head) { [Severity: High] This is a pre-existing issue, but does iterating over hive->device_list without holding hive->hive_lock cause a data race and potential use-after-free? If a device is hot-unplugged or unbound while this async work is running, couldn't another thread mutate the list (e.g., via amdgpu_xgmi_remove_device), causing list corruption or accessing a freed device? > + /* Enable ttm buffers funcs after the reset lock has been > dropped. */ > + amdgpu_ttm_enable_buffer_funcs(tmp_adev); [Severity: High] Does calling amdgpu_ttm_enable_buffer_funcs() asynchronously without device locks introduce a race condition with device teardown? If a device is unbound while reset_on_init_work is queued, the teardown path calls amdgpu_device_fini_hw() -> amdgpu_ttm_disable_buffer_funcs(). Since buffer_funcs_enabled is still false, it returns early. If this async work runs afterward, it calls amdgpu_ttm_enable_buffer_funcs() which allocates memory and sets buffer_funcs_enabled to true: amdgpu_ttm_enable_buffer_funcs() { ... adev->mman.clear_entities = kcalloc(num_clear_entities, sizeof(struct amdgpu_ttm_buffer_entity), GFP_KERNEL); ... } Will this cause memory allocations to leak because teardown has already completed, or potentially execute on an already-freed tmp_adev? > + > r = amdgpu_ras_init_badpage_info(tmp_adev); > if (r && r != -EHWPOISON) > dev_err(tmp_adev->dev, -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
