Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] drm_gpusvm passes uninitialized pfns arrays to hmm_range_fault, causing unintended page faults and write requests. - [High] Memory leak of `mm_struct` in `drm_gpusvm_range_evict` on allocation failure. -- --- Patch [4]: [PATCH 4/4] fixup! drm/gpusvm: use hmm_range_fault_unlocked_timeout() for range faults --- commit afecb2c70fcbb0789fe0b4b948e05bb9f8070c63 Author: Stanislav Kinsburskii <[email protected]> fixup! drm/gpusvm: use hmm_range_fault_unlocked_timeout() for range faults The patch removes the outer deadline tracking from hmm_range_fault loops and passes msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT) directly to each hmm_range_fault_unlocked_timeout() call. This avoids accounting for outer driver retry logic inside the HMM retry budget. > diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c > index b8f2dd9982f53..76e8a0028c7fc 100644 > --- a/drivers/gpu/drm/drm_gpusvm.c > +++ b/drivers/gpu/drm/drm_gpusvm.c > @@ -867,8 +866,7 @@ enum drm_gpusvm_scan_result drm_gpusvm_scan_mm(struct > drm_gpusvm_range *range, > hmm_range.hmm_pfns = pfns; > > retry: > - err = hmm_range_fault_unlocked_timeout(&hmm_range, > - max(timeout - jiffies, 1L)); > + err = hmm_range_fault_unlocked_timeout(&hmm_range, timeout); [Severity: High] This is a pre-existing issue, but does using kvmalloc_array() without zeroing memory cause unintended page faults? HMM reads hmm_pfns as input to determine per-page fault requests. The uninitialized memory can contain random bits like HMM_PFN_REQ_FAULT or HMM_PFN_REQ_WRITE, potentially triggering unintended faults or violating read-only mapping constraints. Additionally, the retry loop reuses output flags (HMM_PFN_VALID), which aliases with HMM_PFN_REQ_FAULT (both are the MSB), unintentionally forcing faults on previously valid pages during a scan. Should this allocate with kvcalloc() instead? [ ... ] > @@ -1495,8 +1489,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm, > } > > hmm_range.hmm_pfns = pfns; > - err = hmm_range_fault_unlocked_timeout(&hmm_range, > - max_t(long, timeout - jiffies, 1)); > + err = hmm_range_fault_unlocked_timeout(&hmm_range, timeout); [Severity: High] This is also a pre-existing issue, but similar to drm_gpusvm_scan_mm(), pfns is allocated with kvmalloc_array() earlier in drm_gpusvm_get_pages() and passed to hmm_range_fault_unlocked_timeout() here. Could this uninitialized memory cause unintended write requests or page faults? [ ... ] > @@ -1734,8 +1726,7 @@ int drm_gpusvm_range_evict(struct drm_gpusvm *gpusvm, > return -ENOMEM; [Severity: High] This isn't a bug introduced by this patch, but does returning -ENOMEM here leak the mm_struct? Earlier in drm_gpusvm_range_evict(), mmget_not_zero(mm) increments the reference count: if (!mmget_not_zero(mm)) return -EFAULT; pfns = kvmalloc_array(npages, sizeof(*pfns), GFP_KERNEL); if (!pfns) return -ENOMEM; If the kvmalloc_array() allocation for pfns fails, it returns directly without calling mmput(mm). > > hmm_range.hmm_pfns = pfns; > - err = hmm_range_fault_unlocked_timeout(&hmm_range, > - max_t(long, timeout - jiffies, 1)); > + err = hmm_range_fault_unlocked_timeout(&hmm_range, timeout); [Severity: High] This is a pre-existing issue, but again, pfns is allocated with kvmalloc_array() and passed here. Does this need to be kvcalloc() to prevent uninitialized memory from causing unintended page faults? > > kvfree(pfns); > mmput(mm); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/178405975214.1082778.5193079941156341151.stgit@skinsburskii?part=4
