Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Returning an error from xe_bo_move() after the move has been successfully committed causes a Use-After-Free of the new resource in TTM core. - [High] Live migration during an asynchronous move causes silent loss of VF CCS metadata. -- --- Patch [18]: [PATCH v2 18/33] drm/xe: Handle defrag moves in xe_bo_move() --- commit dad45636f401d271e91aa77826c4290b4048e3c7 Author: Matthew Brost <[email protected]> drm/xe: Handle defrag moves in xe_bo_move() This commit adds support for defrag moves in xe_bo_move(). It allows TTM to reallocate a buffer object's backing at a beneficial order and handles the relocation entirely on the GPU via xe_migrate_copy_defrag(). > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c > index 21d087170ef36..8e9fc8b3dffaf 100644 > --- a/drivers/gpu/drm/xe/xe_bo.c > +++ b/drivers/gpu/drm/xe/xe_bo.c [ ... ] > @@ -1309,13 +1325,13 @@ static int xe_bo_move(struct ttm_buffer_object > *ttm_bo, bool evict, > * BBs from BO as it is no longer needed. > */ > if (IS_VF_CCS_READY(xe) && old_mem_type == XE_PL_TT && > - new_mem->mem_type == XE_PL_SYSTEM) > + (new_mem->mem_type == XE_PL_SYSTEM || ctx->defrag)) > xe_sriov_vf_ccs_detach_bo(bo); [Severity: High] This isn't a bug introduced by this patch, but if a live migration is triggered concurrently with a BO defragmentation or eviction, does this synchronous CPU detachment cause a loss of CCS metadata? Because xe_sriov_vf_ccs_detach_bo() and xe_sriov_vf_ccs_attach_bo() are called synchronously on the CPU before the GPU has finished copying the data, they immediately point the host to the new memory for CCS save and restore operations. If a live migration occurs before the GPU copy finishes, the host might save uninitialized CCS data from the new memory, and fail to save the valid CCS data from the old memory. Upon VM restore, the GPU migrate job resumes and copies the corrupted old CCS to the new memory. > > if (IS_VF_CCS_READY(xe) && > ((move_lacks_source && new_mem->mem_type == XE_PL_TT) || > - (old_mem_type == XE_PL_SYSTEM && new_mem->mem_type == XE_PL_TT)) && > - handle_system_ccs) > + ((old_mem_type == XE_PL_SYSTEM || ctx->defrag) && > + new_mem->mem_type == XE_PL_TT)) && handle_system_ccs) > ret = xe_sriov_vf_ccs_attach_bo(bo); [Severity: High] This is a pre-existing issue, but can returning an error code here trigger a use-after-free in the TTM core? If xe_bo_move() commits the memory move earlier in the function by calling ttm_bo_move_accel_cleanup() or ttm_bo_move_null(), new_mem is assigned to bo->resource and the old memory is handed to a ghost object. If xe_sriov_vf_ccs_attach_bo(bo) fails with an error like -ENOMEM, or if any subsequent timeout wait fails, the error code is returned to the TTM core. The TTM core assumes a failed move means the new resource was not adopted: ttm_bo_validate() ttm_bo_handle_move_mem() ret = ttm_bo_move() if (ret) { ... ttm_resource_free(bo, &res); } Since bo->resource already points to the new resource, would this leave bo->resource pointing to freed memory? > > out: -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=18
