Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] Preallocated pages can be configured with Write-Combined (WC) caching but assigned to a Write-Back (WB) TTM buffer object if the platform rejects USWC, causing PAT aliasing and TTM pool corruption. - [High] Unbounded memory preallocation allows unprivileged users to cause massive memory pressure and kernel stalls before BO size limits are checked. --
--- Patch [33]: [PATCH v2 33/33] drm/amdgpu: Preallocate system BO pages outside the reservation lock --- commit 4ff1d390d0b654be970d2510b2e5cd29bc266121 Author: Matthew Brost <[email protected]> drm/amdgpu: Preallocate system BO pages outside the reservation lock This commit mirrors Xe's out-of-lock preallocation for pure system BOs in amdgpu. When a gem_create request targets only AMDGPU_GEM_DOMAIN_GTT, it fills the full page backing up front before taking the reservation lock, to prevent stalls in reclaim and compaction while holding the lock. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > index 6a0699746fbcd..bef8b5bc8b892 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c [ ... ] > @@ -443,10 +448,29 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, > void *data, > flags |= AMDGPU_GEM_CREATE_NO_CPU_ACCESS; > } > > + /* > + * For system-only (pure GTT) BOs, preallocate the whole page backing > + * up front, outside the reservation lock. Populate under the lock then > + * just installs these pages instead of reclaiming/compacting in the > + * critical section. Best-effort: a short fill falls back to the normal > + * in-lock allocation for the missing pages. > + */ > + if (args->in.domains == AMDGPU_GEM_DOMAIN_GTT) { > + int32_t xcp_id = adev->gmc.mem_partitions ? fpriv->xcp_id : 0; > + int32_t pool_id = amdgpu_ttm_tt_pool_id(adev, xcp_id); > + enum ttm_caching caching = > + (flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC) ? > + ttm_write_combined : ttm_cached; [Severity: Critical] Is it possible for the preallocated pages to be configured with write-combined caching here, but later assigned to a write-back TTM buffer object? If an unprivileged user requests AMDGPU_GEM_CREATE_CPU_GTT_USWC on a platform where amdgpu_bo_support_uswc() returns false, the code preallocates the pages as ttm_write_combined. Later, amdgpu_bo_create() in drivers/gpu/drm/amd/amdgpu/amdgpu_object.c strips the unsupported flag: amdgpu_bo_create() { ... if (!amdgpu_bo_support_uswc(bo->flags)) bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC; ... } This creates a ttm_tt initialized as ttm_cached (write-back). When the object is eventually destroyed, ttm_pool_free() will use the write-back caching state, bypassing the required PAT restoration. Does this leak the write-combined physical pages back into the kernel allocator and cause PAT aliasing or machine check exceptions? > + > + prealloc_pool = amdgpu_ttm_pool(adev, pool_id); > + ttm_pool_prealloc_fill_full(prealloc_pool, caching, &prealloc, > + PFN_UP(size), false); [Severity: High] Could this allow unprivileged users to trigger massive memory pressure before buffer object size limits are checked? The user-provided size is passed directly to ttm_pool_prealloc_fill_full() here, but the size validation against domain limits does not happen until later in the call chain: drivers/gpu/drm/amd/amdgpu/amdgpu_object.c:amdgpu_bo_create() { ... if (!amdgpu_bo_validate_size(adev, size, bp->domain)) return -ENOMEM; ... } If an unprivileged user makes an ioctl request with an excessively large size (e.g., 100 GB), ttm_pool_prealloc_fill_full() will attempt to satisfy the request, triggering intensive reclaim and compaction before the size is ever validated. Would it be safer to validate the size against domain limits before attempting the preallocation? > + } > + > if (flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID) { > r = amdgpu_bo_reserve(vm->root.bo, false); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=33
