Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [High] Permanent leak of `vm_committed_as` committed memory when `do_vmi_munmap()` fails in `unmap_source_vma()`. - [High] Permanent leak of `vm_committed_as` (delta size) in the `move_vma()` error path when page table moves fail. - [High] Permanent leak of `mm->locked_vm` when `MREMAP_DONTUNMAP` is called on a locked VMA. - [High] `mremap_to()` with `MREMAP_SHRINK` permanently inflates committed memory by failing to reset `vrm->delta`. -- --- Patch [13]: [PATCH v2 13/13] mm/mremap: convert mremap code to use vma_flags_t --- commit 2acb878e611c682f5c4e70b5f73085e003532a49 Author: Lorenzo Stoakes <[email protected]> mm/mremap: convert mremap code to use vma_flags_t This commit replaces legacy vm_flags_t with vma_flags_t bitwise values across the mremap codebase. It updates flag modification functions to their corresponding vma_* counterparts. No functional changes are intended. > diff --git a/mm/mremap.c b/mm/mremap.c > index 384ef4cc2195b..b64aa1f6e07ed 100644 > --- a/mm/mremap.c > +++ b/mm/mremap.c [ ... ] > @@ -985,7 +985,7 @@ static bool vrm_calc_charge(struct vma_remap_struct *vrm) > { > unsigned long charged; > > - if (!(vrm->vma->vm_flags & VM_ACCOUNT)) > + if (!vma_test(vrm->vma, VMA_ACCOUNT_BIT)) > return true; [Severity: High] This isn't a bug introduced by this patch, but since we're updating the accounting logic here, could this lead to a committed memory leak when handling a shrink operation? When mremap_to() processes MREMAP_SHRINK, it sets vrm->old_len to vrm->new_len but appears to leave vrm->delta at its original value. When vrm_calc_charge() is subsequently called during move_vma(), it uses vrm->delta to charge new committed memory, effectively re-charging the memory that was just shrunk. Does this permanently inflate vm_committed_as under strict overcommit? [ ... ] > @@ -1195,7 +1195,7 @@ static void unmap_source_vma(struct vma_remap_struct > *vrm) > * portions of the original VMA that remain. > */ > if (accountable_move) { > - vm_flags_clear(vma, VM_ACCOUNT); > + vma_clear_flags(vma, VMA_ACCOUNT_BIT); > /* We are about to split vma, so store the start/end. */ > vm_start = vma->vm_start; > vm_end = vma->vm_end; [Severity: High] This is a pre-existing issue, but I noticed a potential leak of committed memory here during error handling in unmap_source_vma(). If do_vmi_munmap() fails shortly after this block (e.g., due to OOM when splitting an adjacent VMA), the error path calls vm_acct_memory() but returns early without restoring VMA_ACCOUNT_BIT on the surviving VMA: if (err) { /* OOM: unable to split vma, just get accounts right */ vm_acct_memory(len >> PAGE_SHIFT); return; } Because the bit is stripped, will the system permanently leak this charge since do_vmi_munmap() won't unaccount it when the VMA is eventually torn down? [Severity: High] This is also a pre-existing issue on the same path. Can this lead to a similar leak if move_page_tables() fails inside copy_vma_and_data()? In that scenario, copy_vma_and_data() assigns new_vma to vrm->vma and returns an error. Then, move_vma() invokes unmap_source_vma() to clean up new_vma. This block will strip VMA_ACCOUNT_BIT from new_vma before unmapping it, which seems to prevent do_vmi_munmap() from uncharging the delta added earlier by vrm_calc_charge(). [ ... ] > @@ -1330,8 +1332,8 @@ static void dontunmap_complete(struct vma_remap_struct > *vrm, > unsigned long old_start = vrm->vma->vm_start; > unsigned long old_end = vrm->vma->vm_end; > > - /* We always clear VM_LOCKED[ONFAULT] on the old VMA. */ > - vm_flags_clear(vrm->vma, VM_LOCKED_MASK); > + /* We always clear VMA_LOCKED[ONFAULT]_BIT on the old VMA. */ > + vma_clear_flags_mask(vrm->vma, VMA_LOCKED_MASK); [Severity: High] This is a pre-existing issue, but could stripping the locked mask here leak the mm->locked_vm count? By clearing VMA_LOCKED_MASK unconditionally from the old VMA without decrementing mm->locked_vm or unlocking the pages, it seems the old VMA will no longer account for its locked pages when it is eventually unmapped. Since move_vma() already added the new VMA's size to mm->locked_vm, does this permanently inflate the process's locked memory count? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
