Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] `hmm_range_fault_unlocked()` uses uninterruptible `mmap_read_lock()` inside a retry sequence, which can delay or block fatal signals. - [High] Unconditional dereference of `hmm_vma_walk->locked` in `hmm_do_fault()` can cause a kernel panic if a buggy fault handler returns `VM_FAULT_RETRY` or `VM_FAULT_COMPLETED` without `FAULT_FLAG_ALLOW_RETRY`. --
commit 0808f6917e681bb06bd68894103530cfbabe07b9 Author: Stanislav Kinsburskii <[email protected]> mm/hmm: add hmm_range_fault_unlocked() for mmap lock-drop support This commit adds hmm_range_fault_unlocked() for callers that do not need to hold mmap_lock across any work outside the HMM fault itself. It takes mmap_read_lock() internally and allows handle_mm_fault() to drop it during fault handling. > diff --git a/mm/hmm.c b/mm/hmm.c > index 2129b1ee4c35b..316116994209c 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c [ ... ] > @@ -651,37 +663,33 @@ static int hmm_do_fault(struct mm_struct *mm, > fault_flags |= FAULT_FLAG_WRITE; > } > > - for (; addr < end; addr += PAGE_SIZE) > - if (handle_mm_fault(vma, addr, fault_flags, NULL) & > - VM_FAULT_ERROR) > - return -EFAULT; > + for (; addr < end; addr += PAGE_SIZE) { > + vm_fault_t ret; > + > + ret = handle_mm_fault(vma, addr, fault_flags, NULL); > + > + if (ret & (VM_FAULT_COMPLETED | VM_FAULT_RETRY)) { > + *hmm_vma_walk->locked = 0; [Severity: High] Can this result in a NULL pointer dereference? When the legacy hmm_range_fault() is called, it passes NULL for the locked parameter, meaning hmm_vma_walk->locked will be NULL. If a buggy fault handler incorrectly returns VM_FAULT_RETRY even when FAULT_FLAG_ALLOW_RETRY is not set, it seems this would unconditionally dereference the NULL pointer here. > + return HMM_FAULT_UNLOCKED; > + } [ ... ] > +int hmm_range_fault_unlocked(struct hmm_range *range) > +{ > + struct mm_struct *mm = range->notifier->mm; > + int locked = 1; > + int ret; > + > + mmap_read_lock(mm); [Severity: High] Should this use mmap_read_lock_killable() instead of the uninterruptible mmap_read_lock()? Since this function is designed to be called inside a retry loop, if the mmap lock is highly contended, it appears a task could enter an uninterruptible sleep here and block fatal signals indefinitely. This might defeat the purpose of the FAULT_FLAG_KILLABLE flag that is set during the actual fault. > + ret = hmm_range_fault_locked(range, &locked); > + if (locked) > + mmap_read_unlock(mm); > + > + return ret; > +} > +EXPORT_SYMBOL(hmm_range_fault_unlocked); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/178336023903.504354.7500950448226027718.stgit@skinsburskii?part=2
