| Message ID | 20230707043211.3682710-2-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] mm: lock a vma before stack expansion | expand |
* Suren Baghdasaryan <surenb@google.com> [230707 00:32]: > mmap_region adds a newly created VMA into VMA tree and might modify it > afterwards before dropping the mmap_lock. This poses a problem for page > faults handled under per-VMA locks because they don't take the mmap_lock > and can stumble on this VMA while it's still being modified. Currently > this does not pose a problem since post-addition modifications are done > only for file-backed VMAs, which are not handled under per-VMA lock. > However, once support for handling file-backed page faults with per-VMA > locks is added, this will become a race. > Fix this by write-locking the VMA before inserting it into the VMA tree. > Other places where a new VMA is added into VMA tree do not modify it > after the insertion, so do not need the same locking. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > --- > mm/mmap.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/mm/mmap.c b/mm/mmap.c > index c66e4622a557..84c71431a527 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -2812,6 +2812,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > if (vma->vm_file) > i_mmap_lock_write(vma->vm_file->f_mapping); > > + /* Lock the VMA since it is modified after insertion into VMA tree */ So it is modified, but that i_mmap_lock_write() directly above this comment is potentially moving below the insert and that is why this lock is needed. > + vma_start_write(vma); > vma_iter_store(&vmi, vma); > mm->map_count++; > if (vma->vm_file) { > -- > 2.41.0.255.g8b1d071c50-goog >
On Fri, Jul 7, 2023 at 7:48 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > * Suren Baghdasaryan <surenb@google.com> [230707 00:32]: > > mmap_region adds a newly created VMA into VMA tree and might modify it > > afterwards before dropping the mmap_lock. This poses a problem for page > > faults handled under per-VMA locks because they don't take the mmap_lock > > and can stumble on this VMA while it's still being modified. Currently > > this does not pose a problem since post-addition modifications are done > > only for file-backed VMAs, which are not handled under per-VMA lock. > > However, once support for handling file-backed page faults with per-VMA > > locks is added, this will become a race. > > Fix this by write-locking the VMA before inserting it into the VMA tree. > > Other places where a new VMA is added into VMA tree do not modify it > > after the insertion, so do not need the same locking. > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > --- > > mm/mmap.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > index c66e4622a557..84c71431a527 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -2812,6 +2812,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > > if (vma->vm_file) > > i_mmap_lock_write(vma->vm_file->f_mapping); > > > > + /* Lock the VMA since it is modified after insertion into VMA tree */ > > So it is modified, but that i_mmap_lock_write() directly above this > comment is potentially moving below the insert and that is why this lock > is needed. Correct, we should not rely on i_mmap_lock_write() which can be moved (and is suggested to be moved in https://lore.kernel.org/all/20230606124939.93561-1-yu.ma@intel.com/). > > > + vma_start_write(vma); > > vma_iter_store(&vmi, vma); > > mm->map_count++; > > if (vma->vm_file) { > > -- > > 2.41.0.255.g8b1d071c50-goog > > > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
diff --git a/mm/mmap.c b/mm/mmap.c index c66e4622a557..84c71431a527 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -2812,6 +2812,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr, if (vma->vm_file) i_mmap_lock_write(vma->vm_file->f_mapping); + /* Lock the VMA since it is modified after insertion into VMA tree */ + vma_start_write(vma); vma_iter_store(&vmi, vma); mm->map_count++; if (vma->vm_file) {
mmap_region adds a newly created VMA into VMA tree and might modify it afterwards before dropping the mmap_lock. This poses a problem for page faults handled under per-VMA locks because they don't take the mmap_lock and can stumble on this VMA while it's still being modified. Currently this does not pose a problem since post-addition modifications are done only for file-backed VMAs, which are not handled under per-VMA lock. However, once support for handling file-backed page faults with per-VMA locks is added, this will become a race. Fix this by write-locking the VMA before inserting it into the VMA tree. Other places where a new VMA is added into VMA tree do not modify it after the insertion, so do not need the same locking. Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- mm/mmap.c | 2 ++ 1 file changed, 2 insertions(+)