Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Integer truncation in xe_device_ccs_bytes() returning u32 causes buffer overflows and physical memory corruption/panics for BOs >= 1TB. -- --- Patch [25]: [PATCH v2 25/33] drm/xe: Preallocate system BO backing outside the dma-resv lock --- commit aa051bafd6b83b01e59b139bb60870dfa1044d22 Author: Matthew Brost <[email protected]> Subject: drm/xe: Preallocate system BO backing outside the dma-resv lock xe_gem_create_ioctl() populates a buffer object's backing store inside xe_validation_guard(), i.e. under the dma-resv lock. This patch uses the TTM full-preallocation mechanism to move that work out of the lock. > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c > index a010042653495..d9226a3995fe4 100644 > --- a/drivers/gpu/drm/xe/xe_bo.c > +++ b/drivers/gpu/drm/xe/xe_bo.c [ ... ] > @@ -3994,6 +4009,45 @@ int xe_gem_create_ioctl(struct drm_device *dev, void > *data, > return -ENOENT; > } > > + /* > + * For system-only BOs, preallocate the whole backing outside the > + * dma-resv lock so the populate under the validation guard does not > + * stall in reclaim/compaction. The caching mirrors the system-BO path > + * in xe_ttm_tt_create(); the pool falls back to in-line allocation for > + * any shortfall, so a failed fill is harmless. > + */ > + if ((bo_flags & XE_BO_FLAG_SYSTEM) && > + !(bo_flags & (XE_BO_FLAG_DEFER_BACKING | XE_BO_FLAG_VRAM_MASK | > + XE_BO_FLAG_STOLEN))) { > + enum ttm_caching caching = ttm_cached; > + unsigned long num_pages = args->size >> PAGE_SHIFT; > + > + if (!IS_DGFX(xe) && > + args->cpu_caching == DRM_XE_GEM_CPU_CACHING_WC) > + caching = ttm_write_combined; > + > + /* > + * Cover the CCS backup pages appended to the tt (see > + * xe_ttm_tt_create()) so the whole tt is preallocated. > + */ > + if (__xe_bo_needs_ccs_pages(xe, ttm_bo_type_device, bo_flags, > + args->cpu_caching)) > + num_pages += DIV_ROUND_UP(xe_device_ccs_bytes(xe, > + > args->size), > + PAGE_SIZE); [Severity: High] This is a pre-existing issue, but can this lead to an integer truncation for very large buffer sizes? The function xe_device_ccs_bytes() in drivers/gpu/drm/xe/xe_device.c returns a u32, but takes a u64 size parameter. If a user requests a BO allocation of >= 1TB via DRM_IOCTL_XE_GEM_CREATE with CCS enabled, the required CCS size would be 1TB / 256 = 4GB, which silently truncates to 0 in the u32 return type. Because this patch propagates the pattern by calling xe_device_ccs_bytes() on the user-controlled args->size, the extra_pages for the CCS backup could be calculated as 0, under-allocating the BO's SG table. Later, when xe_migrate_copy() processes the BO in chunks, it could advance the CCS cursor past the end of the under-allocated SG table, resulting in a NULL pointer dereference and kernel panic. Should xe_device_ccs_bytes() be updated to return a u64, or should there be size limits enforced earlier in the ioctl? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=25
