| Message ID | 20240531163217.1584450-2-Liam.Howlett@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Avoid MAP_FIXED gap exposure | expand |
On Fri, May 31, 2024 at 9:33 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > The vma iterator may be left pointing to the newly created vma. This > happens when inserting the new vma at the end of the old vma > (!new_below). > > The incorrect position in the vma iterator is not exposed currently > since the vma iterator is repositioned in the munmap path and is not > reused in any of the other paths. > > This has limited impact in the current code, but is required for future > changes. > > Fixes: b2b3b886738f ("mm: don't use __vma_adjust() in __split_vma()") > Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com> > --- > mm/mmap.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/mm/mmap.c b/mm/mmap.c > index 83b4682ec85c..31d464e6a656 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -2442,6 +2442,9 @@ static int __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, > /* Success. */ > if (new_below) > vma_next(vmi); > + else > + vma_prev(vmi); > + IIUC the goal is to always point vmi to the old (original) vma? If so, then change LGTM. Reviewed-by: Suren Baghdasaryan <surenb@google.com> > return 0; > > out_free_mpol: > -- > 2.43.0 >
* Suren Baghdasaryan <surenb@google.com> [240605 20:51]: > On Fri, May 31, 2024 at 9:33 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > > > The vma iterator may be left pointing to the newly created vma. This > > happens when inserting the new vma at the end of the old vma > > (!new_below). > > > > The incorrect position in the vma iterator is not exposed currently > > since the vma iterator is repositioned in the munmap path and is not > > reused in any of the other paths. > > > > This has limited impact in the current code, but is required for future > > changes. > > > > Fixes: b2b3b886738f ("mm: don't use __vma_adjust() in __split_vma()") > > Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com> > > --- > > mm/mmap.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > index 83b4682ec85c..31d464e6a656 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -2442,6 +2442,9 @@ static int __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, > > /* Success. */ > > if (new_below) > > vma_next(vmi); > > + else > > + vma_prev(vmi); > > + > > IIUC the goal is to always point vmi to the old (original) vma? If so, > then change LGTM. Yes, we need the iterator to keep pointing to the original VMAs, I think this makes sense. I will update the function comment in the next revision to state as much. > > Reviewed-by: Suren Baghdasaryan <surenb@google.com> > > > return 0; > > > > out_free_mpol: > > -- > > 2.43.0 > >
On Fri, May 31, 2024 at 12:32:13PM -0400, Liam R. Howlett wrote: > The vma iterator may be left pointing to the newly created vma. This > happens when inserting the new vma at the end of the old vma > (!new_below). > > The incorrect position in the vma iterator is not exposed currently > since the vma iterator is repositioned in the munmap path and is not > reused in any of the other paths. > > This has limited impact in the current code, but is required for future > changes. > > Fixes: b2b3b886738f ("mm: don't use __vma_adjust() in __split_vma()") > Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com> > --- > mm/mmap.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/mm/mmap.c b/mm/mmap.c > index 83b4682ec85c..31d464e6a656 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -2442,6 +2442,9 @@ static int __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, > /* Success. */ > if (new_below) > vma_next(vmi); > + else > + vma_prev(vmi); > + > return 0; > > out_free_mpol: > -- > 2.43.0 > Looks good to me. As Suren alludes to, I agree that it's important to comment to indicate that you want to move the iterator to point to the VMA that's been shrunk rather than the newly inserted VMA. Reviewed-by: Lorenzo Stoakes <lstoakes@gmail.com>
diff --git a/mm/mmap.c b/mm/mmap.c index 83b4682ec85c..31d464e6a656 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -2442,6 +2442,9 @@ static int __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, /* Success. */ if (new_below) vma_next(vmi); + else + vma_prev(vmi); + return 0; out_free_mpol:
The vma iterator may be left pointing to the newly created vma. This happens when inserting the new vma at the end of the old vma (!new_below). The incorrect position in the vma iterator is not exposed currently since the vma iterator is repositioned in the munmap path and is not reused in any of the other paths. This has limited impact in the current code, but is required for future changes. Fixes: b2b3b886738f ("mm: don't use __vma_adjust() in __split_vma()") Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com> --- mm/mmap.c | 3 +++ 1 file changed, 3 insertions(+)