Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Unchecked return value from folio_alloc() leading to a NULL pointer dereference under memory pressure. - [High] VMA iteration on an external mm_struct is performed without checking if the address space is stable. -- --- Patch [3]: [PATCH 3/4] lib/test_hmm: remove dead NULL checks after GFP_NOFAIL allocations --- commit f8d849c03b7bcb5aa594e8169391ebb8d17dfdcc Author: Gou Hao <[email protected]> lib/test_hmm: remove dead NULL checks after GFP_NOFAIL allocations This commit removes unreachable NULL checks for kvcalloc() allocations that use the __GFP_NOFAIL flag. Since __GFP_NOFAIL guarantees the allocation will succeed, checking the return value is unnecessary. > diff --git a/lib/test_hmm.c b/lib/test_hmm.c > index 9c59d1ceb5b57..d615e4e5fc447 100644 > --- a/lib/test_hmm.c > +++ b/lib/test_hmm.c > @@ -1209,16 +1209,10 @@ static int dmirror_migrate_to_device(struct dmirror > *dmirror, > if (!mmget_not_zero(mm)) > return -EINVAL; > > - ret = -ENOMEM; > src_pfns = kvcalloc(PTRS_PER_PTE, sizeof(*src_pfns), > GFP_KERNEL | __GFP_NOFAIL); > - if (!src_pfns) > - goto free_mem; > - > dst_pfns = kvcalloc(PTRS_PER_PTE, sizeof(*dst_pfns), > GFP_KERNEL | __GFP_NOFAIL); > - if (!dst_pfns) > - goto free_mem; > > ret = 0; > mmap_read_lock(mm); [Severity: High] This is a pre-existing issue, but is it safe to iterate over the VMAs of an external mm_struct here without first verifying the address space is stable? As noted in the MM subsystem guidelines, when iterating over VMAs on an external mm_struct, we need to ensure the tree is stable: lib/test_hmm.c:dmirror_migrate_to_device() { ... mmap_read_lock(mm); for (addr = start; addr < end; addr = next) { vma = vma_lookup(mm, addr); ... } If the target process is OOM-killed or fails dup_mmap(), the maple tree could contain XA_ZERO_ENTRY markers, which might cause vma_lookup() or the traversal to crash. Could this code call check_stable_address_space(mm) right after acquiring the mmap lock? [Severity: High] This is another pre-existing issue in the same file, but could a failed memory allocation cause a NULL pointer dereference under memory pressure? In dmirror_device_evict_chunk(), folio_alloc() is called without the __GFP_NOFAIL flag, but the return value is not checked before use: lib/test_hmm.c:dmirror_device_evict_chunk() { ... dpage = folio_page(folio_alloc(GFP_HIGHUSER_MOVABLE, order), 0); lock_page(dpage); ... } If folio_alloc() returns NULL due to memory pressure, folio_page() evaluates to NULL, which is then unconditionally passed to lock_page(). Would it make sense to add a NULL check here, or perhaps use __GFP_NOFAIL if the allocation is truly required to succeed? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
