| Message ID | 195c3956c70a142b12465e09b4aa5e33a898b789.1740911247.git.lorenzo.stoakes@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | refactor mremap and fix bug | expand |
On Mon, Mar 03, 2025 at 11:08:31AM +0000, Lorenzo Stoakes wrote: > Consider the case of a a partial mremap() (that results in a VMA split) of > an accountable VMA (i.e. which has the VM_ACCOUNT flag set) whose start > address is zero, with the MREMAP_MAYMOVE flag specified and a scenario > where a move does in fact occur: > > addr end > | | > v v > |-------------| > | vma | > |-------------| > 0 > > This move is affected by unmapping the range [addr, end). In order to > prevent an incorrect decrement of accounted memory which has already been > determined, the mremap() code in move_vma() clears VM_ACCOUNT from the VMA > prior to doing so, before reestablishing it in each of the VMAs post-split: > > addr end > | | > v v > |---| |---| > | A | | B | > |---| |---| > > Commit 6b73cff239e5 ("mm: change munmap splitting order and move_vma()") > changed this logic such as to determine whether there is a need to do so by > establishing account_start and account_end and, in the instance where such > an operation is required, assigning them to vma->vm_start and vma->vm_end. > > Later the code checks if the operation is required for 'A' referenced above > thusly: > > if (account_start) { > ... > } > > However, if the VMA described above has vma->vm_start == 0, which is now > assigned to account_start, this branch will not be executed. > > As a result, the VMA 'A' above will remain stripped of its VM_ACCOUNT flag, > incorrectly. > > The fix is to simply convert these variables to booleans and set them as > required. > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > Fixes: 6b73cff239e5 ("mm: change munmap splitting order and move_vma()") > Cc: stable@vger.kernel.org > --- Looks good to me, Reviewed-by: Harry Yoo <harry.yoo@oracle.com>
* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250303 06:08]: > Consider the case of a a partial mremap() (that results in a VMA split) of > an accountable VMA (i.e. which has the VM_ACCOUNT flag set) whose start > address is zero, with the MREMAP_MAYMOVE flag specified and a scenario > where a move does in fact occur: > > addr end > | | > v v > |-------------| > | vma | > |-------------| > 0 > > This move is affected by unmapping the range [addr, end). In order to > prevent an incorrect decrement of accounted memory which has already been > determined, the mremap() code in move_vma() clears VM_ACCOUNT from the VMA > prior to doing so, before reestablishing it in each of the VMAs post-split: > > addr end > | | > v v > |---| |---| > | A | | B | > |---| |---| > > Commit 6b73cff239e5 ("mm: change munmap splitting order and move_vma()") > changed this logic such as to determine whether there is a need to do so by > establishing account_start and account_end and, in the instance where such > an operation is required, assigning them to vma->vm_start and vma->vm_end. > > Later the code checks if the operation is required for 'A' referenced above > thusly: > > if (account_start) { > ... > } > > However, if the VMA described above has vma->vm_start == 0, which is now > assigned to account_start, this branch will not be executed. > > As a result, the VMA 'A' above will remain stripped of its VM_ACCOUNT flag, > incorrectly. > > The fix is to simply convert these variables to booleans and set them as > required. > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > Fixes: 6b73cff239e5 ("mm: change munmap splitting order and move_vma()") > Cc: stable@vger.kernel.org Thanks for taking care of this. Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com> > --- > mm/mremap.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/mm/mremap.c b/mm/mremap.c > index cff7f552f909..c3e4c86d0b8d 100644 > --- a/mm/mremap.c > +++ b/mm/mremap.c > @@ -705,8 +705,8 @@ static unsigned long move_vma(struct vm_area_struct *vma, > unsigned long vm_flags = vma->vm_flags; > unsigned long new_pgoff; > unsigned long moved_len; > - unsigned long account_start = 0; > - unsigned long account_end = 0; > + bool account_start = false; > + bool account_end = false; > unsigned long hiwater_vm; > int err = 0; > bool need_rmap_locks; > @@ -790,9 +790,9 @@ static unsigned long move_vma(struct vm_area_struct *vma, > if (vm_flags & VM_ACCOUNT && !(flags & MREMAP_DONTUNMAP)) { > vm_flags_clear(vma, VM_ACCOUNT); > if (vma->vm_start < old_addr) > - account_start = vma->vm_start; > + account_start = true; > if (vma->vm_end > old_addr + old_len) > - account_end = vma->vm_end; > + account_end = true; > } > > /* > @@ -832,7 +832,7 @@ static unsigned long move_vma(struct vm_area_struct *vma, > /* OOM: unable to split vma, just get accounts right */ > if (vm_flags & VM_ACCOUNT && !(flags & MREMAP_DONTUNMAP)) > vm_acct_memory(old_len >> PAGE_SHIFT); > - account_start = account_end = 0; > + account_start = account_end = false; > } > > if (vm_flags & VM_LOCKED) { > -- > 2.48.1 >
diff --git a/mm/mremap.c b/mm/mremap.c index cff7f552f909..c3e4c86d0b8d 100644 --- a/mm/mremap.c +++ b/mm/mremap.c @@ -705,8 +705,8 @@ static unsigned long move_vma(struct vm_area_struct *vma, unsigned long vm_flags = vma->vm_flags; unsigned long new_pgoff; unsigned long moved_len; - unsigned long account_start = 0; - unsigned long account_end = 0; + bool account_start = false; + bool account_end = false; unsigned long hiwater_vm; int err = 0; bool need_rmap_locks; @@ -790,9 +790,9 @@ static unsigned long move_vma(struct vm_area_struct *vma, if (vm_flags & VM_ACCOUNT && !(flags & MREMAP_DONTUNMAP)) { vm_flags_clear(vma, VM_ACCOUNT); if (vma->vm_start < old_addr) - account_start = vma->vm_start; + account_start = true; if (vma->vm_end > old_addr + old_len) - account_end = vma->vm_end; + account_end = true; } /* @@ -832,7 +832,7 @@ static unsigned long move_vma(struct vm_area_struct *vma, /* OOM: unable to split vma, just get accounts right */ if (vm_flags & VM_ACCOUNT && !(flags & MREMAP_DONTUNMAP)) vm_acct_memory(old_len >> PAGE_SHIFT); - account_start = account_end = 0; + account_start = account_end = false; } if (vm_flags & VM_LOCKED) {
Consider the case of a a partial mremap() (that results in a VMA split) of an accountable VMA (i.e. which has the VM_ACCOUNT flag set) whose start address is zero, with the MREMAP_MAYMOVE flag specified and a scenario where a move does in fact occur: addr end | | v v |-------------| | vma | |-------------| 0 This move is affected by unmapping the range [addr, end). In order to prevent an incorrect decrement of accounted memory which has already been determined, the mremap() code in move_vma() clears VM_ACCOUNT from the VMA prior to doing so, before reestablishing it in each of the VMAs post-split: addr end | | v v |---| |---| | A | | B | |---| |---| Commit 6b73cff239e5 ("mm: change munmap splitting order and move_vma()") changed this logic such as to determine whether there is a need to do so by establishing account_start and account_end and, in the instance where such an operation is required, assigning them to vma->vm_start and vma->vm_end. Later the code checks if the operation is required for 'A' referenced above thusly: if (account_start) { ... } However, if the VMA described above has vma->vm_start == 0, which is now assigned to account_start, this branch will not be executed. As a result, the VMA 'A' above will remain stripped of its VM_ACCOUNT flag, incorrectly. The fix is to simply convert these variables to booleans and set them as required. Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> Fixes: 6b73cff239e5 ("mm: change munmap splitting order and move_vma()") Cc: stable@vger.kernel.org --- mm/mremap.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)