Re: [RFC v8 PATCH 3/5] mm: mmap: zap pages with read mmap_sem in munmap
On 08/22/2018 02:56 PM, owner-linux...@kvack.org wrote: > > > On 8/22/18 2:42 PM, Dave Hansen wrote: >> On 08/22/2018 02:10 PM, Kirill A. Shutemov wrote: For x86, mpx_notify_unmap() looks finally zap the VM_MPX vmas in bound table range with zap_page_range() and doesn't update vm flags, so it sounds ok to me since vmas have been detached, nobody can find those vmas. But, I'm not familiar with the details of mpx, maybe Kirill could help to confirm this? >>> I don't see anything obviously dependent on down_write() in >>> mpx_notify_unmap(), but Dave should know better. >> We need mmap_sem for write in mpx_notify_unmap(). >> >> Its job is to clean up bounds tables, but bounds tables are dynamically >> allocated and destroyed by the kernel. When we destroy a table, we also >> destroy the VMA for the bounds table *itself*, separate from the VMA >> being unmapped. ... > Does it depends on unmap_region()? Or IOW, does it has to be called > after unmap_region()? Now the calling sequence is: > > detach vmas > unmap_region() > mpx_notify_unmap() > > I'm wondering if it is safe to move it up before unmap_region() like: > > detach vmas > mpx_notify_unmap() > unmap_region() > > With this change we also can do our optimization to do unmap_region() > with read mmap_sem. Otherwise it does cause problem. I think changing the ordering is fine. The MPX bounds table unmapping is entirely driven by the VMAs being unmapped, so the page table unmapping in unmap_region() should not affect it.
Re: [RFC v8 PATCH 3/5] mm: mmap: zap pages with read mmap_sem in munmap
On 8/22/18 2:42 PM, Dave Hansen wrote: On 08/22/2018 02:10 PM, Kirill A. Shutemov wrote: For x86, mpx_notify_unmap() looks finally zap the VM_MPX vmas in bound table range with zap_page_range() and doesn't update vm flags, so it sounds ok to me since vmas have been detached, nobody can find those vmas. But, I'm not familiar with the details of mpx, maybe Kirill could help to confirm this? I don't see anything obviously dependent on down_write() in mpx_notify_unmap(), but Dave should know better. We need mmap_sem for write in mpx_notify_unmap(). Its job is to clean up bounds tables, but bounds tables are dynamically allocated and destroyed by the kernel. When we destroy a table, we also destroy the VMA for the bounds table *itself*, separate from the VMA being unmapped. Thanks for confirming this. I didn't realize there is VMA for bounds table itself. But, this code is very likely to go away soon. If it's causing a problem for you, let me know and I'll see if I can get to removing it faster. Does it depends on unmap_region()? Or IOW, does it has to be called after unmap_region()? Now the calling sequence is: detach vmas unmap_region() mpx_notify_unmap() I'm wondering if it is safe to move it up before unmap_region() like: detach vmas mpx_notify_unmap() unmap_region() With this change we also can do our optimization to do unmap_region() with read mmap_sem. Otherwise it does cause problem. Thanks, Yang
Re: [RFC v8 PATCH 3/5] mm: mmap: zap pages with read mmap_sem in munmap
On 08/22/2018 02:10 PM, Kirill A. Shutemov wrote: >> For x86, mpx_notify_unmap() looks finally zap the VM_MPX vmas in bound table >> range with zap_page_range() and doesn't update vm flags, so it sounds ok to >> me since vmas have been detached, nobody can find those vmas. But, I'm not >> familiar with the details of mpx, maybe Kirill could help to confirm this? > I don't see anything obviously dependent on down_write() in > mpx_notify_unmap(), but Dave should know better. We need mmap_sem for write in mpx_notify_unmap(). Its job is to clean up bounds tables, but bounds tables are dynamically allocated and destroyed by the kernel. When we destroy a table, we also destroy the VMA for the bounds table *itself*, separate from the VMA being unmapped. But, this code is very likely to go away soon. If it's causing a problem for you, let me know and I'll see if I can get to removing it faster.
Re: [RFC v8 PATCH 3/5] mm: mmap: zap pages with read mmap_sem in munmap
On Wed, Aug 22, 2018 at 01:45:44PM -0700, Yang Shi wrote: > > > On 8/22/18 4:19 AM, Vlastimil Babka wrote: > > On 08/15/2018 08:49 PM, Yang Shi wrote: > > > + downgrade_write(&mm->mmap_sem); > > > + > > > + /* Zap mappings with read mmap_sem */ > > > + unmap_region(mm, start_vma, prev, start, end); > > > + > > > + arch_unmap(mm, start_vma, start, end); > > Hmm, did you check that all architectures' arch_unmap() is safe with > > read mmap_sem instead of write mmap_sem? E.g. x86 does > > mpx_notify_unmap() there where I would be far from sure at first glance... > > Yes, I'm also not quite sure if it is 100% safe or not. I was trying to move > this before downgrade_write, however, I'm not sure if it is ok or not too, > so I keep the calling sequence. > > For architectures, just x86 and ppc really do something. PPC just uses it > for vdso unmap which should just happen during process exit, so it sounds > safe. > > For x86, mpx_notify_unmap() looks finally zap the VM_MPX vmas in bound table > range with zap_page_range() and doesn't update vm flags, so it sounds ok to > me since vmas have been detached, nobody can find those vmas. But, I'm not > familiar with the details of mpx, maybe Kirill could help to confirm this? I don't see anything obviously dependent on down_write() in mpx_notify_unmap(), but Dave should know better. -- Kirill A. Shutemov
Re: [RFC v8 PATCH 3/5] mm: mmap: zap pages with read mmap_sem in munmap
On 8/22/18 4:19 AM, Vlastimil Babka wrote: On 08/15/2018 08:49 PM, Yang Shi wrote: + downgrade_write(&mm->mmap_sem); + + /* Zap mappings with read mmap_sem */ + unmap_region(mm, start_vma, prev, start, end); + + arch_unmap(mm, start_vma, start, end); Hmm, did you check that all architectures' arch_unmap() is safe with read mmap_sem instead of write mmap_sem? E.g. x86 does mpx_notify_unmap() there where I would be far from sure at first glance... Yes, I'm also not quite sure if it is 100% safe or not. I was trying to move this before downgrade_write, however, I'm not sure if it is ok or not too, so I keep the calling sequence. For architectures, just x86 and ppc really do something. PPC just uses it for vdso unmap which should just happen during process exit, so it sounds safe. For x86, mpx_notify_unmap() looks finally zap the VM_MPX vmas in bound table range with zap_page_range() and doesn't update vm flags, so it sounds ok to me since vmas have been detached, nobody can find those vmas. But, I'm not familiar with the details of mpx, maybe Kirill could help to confirm this? Thanks, Yang + remove_vma_list(mm, start_vma); + up_read(&mm->mmap_sem);
Re: [RFC v8 PATCH 3/5] mm: mmap: zap pages with read mmap_sem in munmap
On 8/22/18 4:11 AM, Vlastimil Babka wrote:
On 08/15/2018 08:49 PM, Yang Shi wrote:
+ start_vma = munmap_lookup_vma(mm, start, end);
+ if (!start_vma)
+ goto out;
+ if (IS_ERR(start_vma)) {
+ ret = PTR_ERR(start_vma);
+ goto out;
+ }
+
+ prev = start_vma->vm_prev;
+
+ if (unlikely(uf)) {
+ ret = userfaultfd_unmap_prep(start_vma, start, end, uf);
+ if (ret)
+ goto out;
+ }
+
You sure it's ok to redo this in case of goto regular path? The
preparations have some side-effects... I would rather move this after
the regular path check?
This preparation sets vma->vm_userfaultfd_ctx.ctx for each vmas. But,
before doing this, it calls has_unmap_ctx() to check if the ctx has been
set or not. If it has been set, it just skip the vma. It sounds ok, right?
Re: [RFC v8 PATCH 3/5] mm: mmap: zap pages with read mmap_sem in munmap
On 08/15/2018 08:49 PM, Yang Shi wrote: > + downgrade_write(&mm->mmap_sem); > + > + /* Zap mappings with read mmap_sem */ > + unmap_region(mm, start_vma, prev, start, end); > + > + arch_unmap(mm, start_vma, start, end); Hmm, did you check that all architectures' arch_unmap() is safe with read mmap_sem instead of write mmap_sem? E.g. x86 does mpx_notify_unmap() there where I would be far from sure at first glance... > + remove_vma_list(mm, start_vma); > + up_read(&mm->mmap_sem);
Re: [RFC v8 PATCH 3/5] mm: mmap: zap pages with read mmap_sem in munmap
On 08/15/2018 08:49 PM, Yang Shi wrote:
> + start_vma = munmap_lookup_vma(mm, start, end);
> + if (!start_vma)
> + goto out;
> + if (IS_ERR(start_vma)) {
> + ret = PTR_ERR(start_vma);
> + goto out;
> + }
> +
> + prev = start_vma->vm_prev;
> +
> + if (unlikely(uf)) {
> + ret = userfaultfd_unmap_prep(start_vma, start, end, uf);
> + if (ret)
> + goto out;
> + }
> +
You sure it's ok to redo this in case of goto regular path? The
preparations have some side-effects... I would rather move this after
the regular path check?
Re: [RFC v8 PATCH 3/5] mm: mmap: zap pages with read mmap_sem in munmap
On 8/15/18 7:46 PM, Matthew Wilcox wrote:
On Wed, Aug 15, 2018 at 02:54:13PM -0700, Yang Shi wrote:
On 8/15/18 2:09 PM, Matthew Wilcox wrote:
On Wed, Aug 15, 2018 at 12:16:06PM -0700, Matthew Wilcox wrote:
(not even compiled, and I can see a good opportunity for combining the
VM_LOCKED loop with the has_uprobes loop)
I was rushing to get that sent earlier. Here it is tidied up to
actually compile.
Thanks for the example. Yes, I believe the code still can be compacted to
save some lines. However, the cover letter and the commit log of this patch
has elaborated the discussion in the earlier reviews about why we do it in
this way.
You mean the other callers which need to hold mmap_sem write-locked for
longer? I hadn't really considered those; how about this?
Thanks. Yes, this is the other potential implementation. My rationale
about a separate function for the optimized path is I would prefer
optimize this step by step by starting with some relatively simple way,
then add enhancement on top of it.
And, I would prefer keep the current implementation of do_munmap since
it is called somewhere else and it might be called by the optimized path
for some reason until we are confident enough that the optimization
doesn't have regression.
This sounds like separate function vs an extra parameter. We do save
some lines with extra parameter instead of a separate function.
Thanks,
Yang
mmap.c | 47 +--
1 file changed, 29 insertions(+), 18 deletions(-)
diff --git a/mm/mmap.c b/mm/mmap.c
index de699523c0b7..06dc31d1da8c 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2798,11 +2798,11 @@ int split_vma(struct mm_struct *mm, struct
vm_area_struct *vma,
* work. This now handles partial unmappings.
* Jeremy Fitzhardinge
*/
-int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
- struct list_head *uf)
+static int __do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
+ struct list_head *uf, bool downgrade)
{
unsigned long end;
- struct vm_area_struct *vma, *prev, *last;
+ struct vm_area_struct *vma, *prev, *last, *tmp;
if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE-start)
return -EINVAL;
@@ -2816,7 +2816,7 @@ int do_munmap(struct mm_struct *mm, unsigned long start,
size_t len,
if (!vma)
return 0;
prev = vma->vm_prev;
- /* we have start < vma->vm_end */
+ /* we have start < vma->vm_end */
/* if it doesn't overlap, we have nothing.. */
end = start + len;
@@ -2873,18 +2873,22 @@ int do_munmap(struct mm_struct *mm, unsigned long
start, size_t len,
/*
* unlock any mlock()ed ranges before detaching vmas
+* and check to see if there's any reason we might have to hold
+* the mmap_sem write-locked while unmapping regions.
*/
- if (mm->locked_vm) {
- struct vm_area_struct *tmp = vma;
- while (tmp && tmp->vm_start < end) {
- if (tmp->vm_flags & VM_LOCKED) {
- mm->locked_vm -= vma_pages(tmp);
- munlock_vma_pages_all(tmp);
- }
- tmp = tmp->vm_next;
+ for (tmp = vma; tmp && tmp->vm_start < end; tmp = tmp->vm_next) {
+ if (tmp->vm_flags & VM_LOCKED) {
+ mm->locked_vm -= vma_pages(tmp);
+ munlock_vma_pages_all(tmp);
}
+ if (tmp->vm_file &&
+ has_uprobes(tmp, tmp->vm_start, tmp->vm_end))
+ downgrade = false;
}
+ if (downgrade)
+ downgrade_write(&mm->mmap_sem);
+
/*
* Remove the vma's, and unmap the actual pages
*/
@@ -2896,7 +2900,13 @@ int do_munmap(struct mm_struct *mm, unsigned long start,
size_t len,
/* Fix up all other VM information */
remove_vma_list(mm, vma);
- return 0;
+ return downgrade ? 1 : 0;
+}
+
+int do_unmap(struct mm_struct *mm, unsigned long start, size_t len,
+ struct list_head *uf)
+{
+ return __do_munmap(mm, start, len, uf, false);
}
int vm_munmap(unsigned long start, size_t len)
@@ -2905,11 +2915,12 @@ int vm_munmap(unsigned long start, size_t len)
struct mm_struct *mm = current->mm;
LIST_HEAD(uf);
- if (down_write_killable(&mm->mmap_sem))
- return -EINTR;
-
- ret = do_munmap(mm, start, len, &uf);
- up_write(&mm->mmap_sem);
+ down_write(&mm->mmap_sem);
+ ret = __do_munmap(mm, start, len, &uf, true);
+ if (ret == 1)
+ up_read(&mm->mmap_sem);
+ else
+ up_write(&mm->mmap_sem);
userfaultfd_unmap_complete(mm, &uf);
return ret;
}
Re: [RFC v8 PATCH 3/5] mm: mmap: zap pages with read mmap_sem in munmap
On Wed, Aug 15, 2018 at 02:54:13PM -0700, Yang Shi wrote:
>
>
> On 8/15/18 2:09 PM, Matthew Wilcox wrote:
> > On Wed, Aug 15, 2018 at 12:16:06PM -0700, Matthew Wilcox wrote:
> > > (not even compiled, and I can see a good opportunity for combining the
> > > VM_LOCKED loop with the has_uprobes loop)
> > I was rushing to get that sent earlier. Here it is tidied up to
> > actually compile.
>
> Thanks for the example. Yes, I believe the code still can be compacted to
> save some lines. However, the cover letter and the commit log of this patch
> has elaborated the discussion in the earlier reviews about why we do it in
> this way.
You mean the other callers which need to hold mmap_sem write-locked for
longer? I hadn't really considered those; how about this?
mmap.c | 47 +--
1 file changed, 29 insertions(+), 18 deletions(-)
diff --git a/mm/mmap.c b/mm/mmap.c
index de699523c0b7..06dc31d1da8c 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2798,11 +2798,11 @@ int split_vma(struct mm_struct *mm, struct
vm_area_struct *vma,
* work. This now handles partial unmappings.
* Jeremy Fitzhardinge
*/
-int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
- struct list_head *uf)
+static int __do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
+ struct list_head *uf, bool downgrade)
{
unsigned long end;
- struct vm_area_struct *vma, *prev, *last;
+ struct vm_area_struct *vma, *prev, *last, *tmp;
if ((offset_in_page(start)) || start > TASK_SIZE || len >
TASK_SIZE-start)
return -EINVAL;
@@ -2816,7 +2816,7 @@ int do_munmap(struct mm_struct *mm, unsigned long start,
size_t len,
if (!vma)
return 0;
prev = vma->vm_prev;
- /* we have start < vma->vm_end */
+ /* we have start < vma->vm_end */
/* if it doesn't overlap, we have nothing.. */
end = start + len;
@@ -2873,18 +2873,22 @@ int do_munmap(struct mm_struct *mm, unsigned long
start, size_t len,
/*
* unlock any mlock()ed ranges before detaching vmas
+* and check to see if there's any reason we might have to hold
+* the mmap_sem write-locked while unmapping regions.
*/
- if (mm->locked_vm) {
- struct vm_area_struct *tmp = vma;
- while (tmp && tmp->vm_start < end) {
- if (tmp->vm_flags & VM_LOCKED) {
- mm->locked_vm -= vma_pages(tmp);
- munlock_vma_pages_all(tmp);
- }
- tmp = tmp->vm_next;
+ for (tmp = vma; tmp && tmp->vm_start < end; tmp = tmp->vm_next) {
+ if (tmp->vm_flags & VM_LOCKED) {
+ mm->locked_vm -= vma_pages(tmp);
+ munlock_vma_pages_all(tmp);
}
+ if (tmp->vm_file &&
+ has_uprobes(tmp, tmp->vm_start, tmp->vm_end))
+ downgrade = false;
}
+ if (downgrade)
+ downgrade_write(&mm->mmap_sem);
+
/*
* Remove the vma's, and unmap the actual pages
*/
@@ -2896,7 +2900,13 @@ int do_munmap(struct mm_struct *mm, unsigned long start,
size_t len,
/* Fix up all other VM information */
remove_vma_list(mm, vma);
- return 0;
+ return downgrade ? 1 : 0;
+}
+
+int do_unmap(struct mm_struct *mm, unsigned long start, size_t len,
+ struct list_head *uf)
+{
+ return __do_munmap(mm, start, len, uf, false);
}
int vm_munmap(unsigned long start, size_t len)
@@ -2905,11 +2915,12 @@ int vm_munmap(unsigned long start, size_t len)
struct mm_struct *mm = current->mm;
LIST_HEAD(uf);
- if (down_write_killable(&mm->mmap_sem))
- return -EINTR;
-
- ret = do_munmap(mm, start, len, &uf);
- up_write(&mm->mmap_sem);
+ down_write(&mm->mmap_sem);
+ ret = __do_munmap(mm, start, len, &uf, true);
+ if (ret == 1)
+ up_read(&mm->mmap_sem);
+ else
+ up_write(&mm->mmap_sem);
userfaultfd_unmap_complete(mm, &uf);
return ret;
}
Re: [RFC v8 PATCH 3/5] mm: mmap: zap pages with read mmap_sem in munmap
On 8/15/18 2:09 PM, Matthew Wilcox wrote:
On Wed, Aug 15, 2018 at 12:16:06PM -0700, Matthew Wilcox wrote:
(not even compiled, and I can see a good opportunity for combining the
VM_LOCKED loop with the has_uprobes loop)
I was rushing to get that sent earlier. Here it is tidied up to
actually compile.
Thanks for the example. Yes, I believe the code still can be compacted
to save some lines. However, the cover letter and the commit log of this
patch has elaborated the discussion in the earlier reviews about why we
do it in this way.
Or you just mean I don't have to call do_munmap() for the special
mappings with the "downgrade" flag to save some cycles since do_munmap()
will redo something which have been done?
Thanks,
Yang
Note the diffstat:
mmap.c | 71
++---
1 file changed, 42 insertions(+), 29 deletions(-)
I think that's a pretty small extra price to pay for having this improved
scalability.
diff --git a/mm/mmap.c b/mm/mmap.c
index de699523c0b7..b77bb3908f8c 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2802,7 +2802,9 @@ int do_munmap(struct mm_struct *mm, unsigned long start,
size_t len,
struct list_head *uf)
{
unsigned long end;
- struct vm_area_struct *vma, *prev, *last;
+ struct vm_area_struct *vma, *prev, *last, *tmp;
+ int res = 0;
+ bool downgrade = false;
if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE-start)
return -EINVAL;
@@ -2811,17 +2813,20 @@ int do_munmap(struct mm_struct *mm, unsigned long
start, size_t len,
if (len == 0)
return -EINVAL;
+ if (down_write_killable(&mm->mmap_sem))
+ return -EINTR;
+
/* Find the first overlapping VMA */
vma = find_vma(mm, start);
if (!vma)
- return 0;
+ goto unlock;
prev = vma->vm_prev;
- /* we have start < vma->vm_end */
+ /* we have start < vma->vm_end */
/* if it doesn't overlap, we have nothing.. */
end = start + len;
if (vma->vm_start >= end)
- return 0;
+ goto unlock;
/*
* If we need to split any vma, do it now to save pain later.
@@ -2831,28 +2836,27 @@ int do_munmap(struct mm_struct *mm, unsigned long
start, size_t len,
* places tmp vma above, and higher split_vma places tmp vma below.
*/
if (start > vma->vm_start) {
- int error;
-
/*
* Make sure that map_count on return from munmap() will
* not exceed its limit; but let map_count go just above
* its limit temporarily, to help free resources as expected.
*/
+ res = -ENOMEM;
if (end < vma->vm_end && mm->map_count >= sysctl_max_map_count)
- return -ENOMEM;
+ goto unlock;
- error = __split_vma(mm, vma, start, 0);
- if (error)
- return error;
+ res = __split_vma(mm, vma, start, 0);
+ if (res)
+ goto unlock;
prev = vma;
}
/* Does it split the last one? */
last = find_vma(mm, end);
if (last && end > last->vm_start) {
- int error = __split_vma(mm, last, end, 1);
- if (error)
- return error;
+ res = __split_vma(mm, last, end, 1);
+ if (res)
+ goto unlock;
}
vma = prev ? prev->vm_next : mm->mmap;
@@ -2866,25 +2870,31 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
* split, despite we could. This is unlikely enough
* failure that it's not worth optimizing it for.
*/
- int error = userfaultfd_unmap_prep(vma, start, end, uf);
- if (error)
- return error;
+ res = userfaultfd_unmap_prep(vma, start, end, uf);
+ if (res)
+ goto unlock;
}
/*
* unlock any mlock()ed ranges before detaching vmas
+* and check to see if there's any reason we might have to hold
+* the mmap_sem write-locked while unmapping regions.
*/
- if (mm->locked_vm) {
- struct vm_area_struct *tmp = vma;
- while (tmp && tmp->vm_start < end) {
- if (tmp->vm_flags & VM_LOCKED) {
- mm->locked_vm -= vma_pages(tmp);
- munlock_vma_pages_all(tmp);
- }
- tmp = tmp->vm_next;
+ downgrade = true;
+
+ for (tmp = vma; tmp && tmp->vm_start < end; tmp = tmp->vm_next) {
+ if (tmp->vm_flags & VM_LOCKED) {
+ mm->locked_vm -= vma_pages(tmp);
+
Re: [RFC v8 PATCH 3/5] mm: mmap: zap pages with read mmap_sem in munmap
On Wed, Aug 15, 2018 at 12:16:06PM -0700, Matthew Wilcox wrote:
> (not even compiled, and I can see a good opportunity for combining the
> VM_LOCKED loop with the has_uprobes loop)
I was rushing to get that sent earlier. Here it is tidied up to
actually compile.
Note the diffstat:
mmap.c | 71 ++---
1 file changed, 42 insertions(+), 29 deletions(-)
I think that's a pretty small extra price to pay for having this improved
scalability.
diff --git a/mm/mmap.c b/mm/mmap.c
index de699523c0b7..b77bb3908f8c 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2802,7 +2802,9 @@ int do_munmap(struct mm_struct *mm, unsigned long start,
size_t len,
struct list_head *uf)
{
unsigned long end;
- struct vm_area_struct *vma, *prev, *last;
+ struct vm_area_struct *vma, *prev, *last, *tmp;
+ int res = 0;
+ bool downgrade = false;
if ((offset_in_page(start)) || start > TASK_SIZE || len >
TASK_SIZE-start)
return -EINVAL;
@@ -2811,17 +2813,20 @@ int do_munmap(struct mm_struct *mm, unsigned long
start, size_t len,
if (len == 0)
return -EINVAL;
+ if (down_write_killable(&mm->mmap_sem))
+ return -EINTR;
+
/* Find the first overlapping VMA */
vma = find_vma(mm, start);
if (!vma)
- return 0;
+ goto unlock;
prev = vma->vm_prev;
- /* we have start < vma->vm_end */
+ /* we have start < vma->vm_end */
/* if it doesn't overlap, we have nothing.. */
end = start + len;
if (vma->vm_start >= end)
- return 0;
+ goto unlock;
/*
* If we need to split any vma, do it now to save pain later.
@@ -2831,28 +2836,27 @@ int do_munmap(struct mm_struct *mm, unsigned long
start, size_t len,
* places tmp vma above, and higher split_vma places tmp vma below.
*/
if (start > vma->vm_start) {
- int error;
-
/*
* Make sure that map_count on return from munmap() will
* not exceed its limit; but let map_count go just above
* its limit temporarily, to help free resources as expected.
*/
+ res = -ENOMEM;
if (end < vma->vm_end && mm->map_count >= sysctl_max_map_count)
- return -ENOMEM;
+ goto unlock;
- error = __split_vma(mm, vma, start, 0);
- if (error)
- return error;
+ res = __split_vma(mm, vma, start, 0);
+ if (res)
+ goto unlock;
prev = vma;
}
/* Does it split the last one? */
last = find_vma(mm, end);
if (last && end > last->vm_start) {
- int error = __split_vma(mm, last, end, 1);
- if (error)
- return error;
+ res = __split_vma(mm, last, end, 1);
+ if (res)
+ goto unlock;
}
vma = prev ? prev->vm_next : mm->mmap;
@@ -2866,25 +2870,31 @@ int do_munmap(struct mm_struct *mm, unsigned long
start, size_t len,
* split, despite we could. This is unlikely enough
* failure that it's not worth optimizing it for.
*/
- int error = userfaultfd_unmap_prep(vma, start, end, uf);
- if (error)
- return error;
+ res = userfaultfd_unmap_prep(vma, start, end, uf);
+ if (res)
+ goto unlock;
}
/*
* unlock any mlock()ed ranges before detaching vmas
+* and check to see if there's any reason we might have to hold
+* the mmap_sem write-locked while unmapping regions.
*/
- if (mm->locked_vm) {
- struct vm_area_struct *tmp = vma;
- while (tmp && tmp->vm_start < end) {
- if (tmp->vm_flags & VM_LOCKED) {
- mm->locked_vm -= vma_pages(tmp);
- munlock_vma_pages_all(tmp);
- }
- tmp = tmp->vm_next;
+ downgrade = true;
+
+ for (tmp = vma; tmp && tmp->vm_start < end; tmp = tmp->vm_next) {
+ if (tmp->vm_flags & VM_LOCKED) {
+ mm->locked_vm -= vma_pages(tmp);
+ munlock_vma_pages_all(tmp);
}
+ if (tmp->vm_file &&
+ has_uprobes(tmp, tmp->vm_start, tmp->vm_end))
+ downgrade = false;
}
+ if (downgrade)
+ downgrade_write(&mm->mmap_sem);
+
/*
* Remove the vma's, and unmap the actual pages
*/
@@ -2896,7 +2906,14 @@ int do_munmap(struct mm_struct *mm, unsigned lo
Re: [RFC v8 PATCH 3/5] mm: mmap: zap pages with read mmap_sem in munmap
On Thu, Aug 16, 2018 at 02:49:48AM +0800, Yang Shi wrote:
> +static int do_munmap_zap_rlock(struct mm_struct *mm, unsigned long start,
> +size_t len, struct list_head *uf)
> +{
> + unsigned long end;
> + struct vm_area_struct *start_vma, *prev, *vma;
> + int ret = 0;
> +
> + if (!addr_ok(start, len))
> + return -EINVAL;
> +
> + len = PAGE_ALIGN(len);
> +
> + end = start + len;
> +
> + /*
> + * Need write mmap_sem to split vmas and detach vmas
> + * splitting vma up-front to save PITA to clean if it is failed
> + */
> + if (down_write_killable(&mm->mmap_sem))
> + return -EINTR;
> +
> + start_vma = munmap_lookup_vma(mm, start, end);
> + if (!start_vma)
> + goto out;
> + if (IS_ERR(start_vma)) {
> + ret = PTR_ERR(start_vma);
> + goto out;
> + }
> +
> + prev = start_vma->vm_prev;
> +
> + if (unlikely(uf)) {
> + ret = userfaultfd_unmap_prep(start_vma, start, end, uf);
> + if (ret)
> + goto out;
> + }
> +
> + /*
> + * Unmapping vmas, which have:
> + * VM_HUGETLB or
> + * VM_PFNMAP or
> + * uprobes
> + * need get done with write mmap_sem held since they may update
> + * vm_flags. Deal with such mappings with regular do_munmap() call.
> + */
> + for (vma = start_vma; vma && vma->vm_start < end; vma = vma->vm_next) {
> + if ((vma->vm_file &&
> + has_uprobes(vma, vma->vm_start, vma->vm_end)) ||
> + (vma->vm_flags & (VM_HUGETLB | VM_PFNMAP)))
> + goto regular_path;
but ... that's going to redo all the work you already did! Why not just this:
(not even compiled, and I can see a good opportunity for combining the
VM_LOCKED loop with the has_uprobes loop)
diff --git a/mm/mmap.c b/mm/mmap.c
index de699523c0b7..8d121db36efc 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2803,6 +2803,8 @@ int do_munmap(struct mm_struct *mm, unsigned long start,
size_t len,
{
unsigned long end;
struct vm_area_struct *vma, *prev, *last;
+ int res = 0;
+ bool downgrade = false;
if ((offset_in_page(start)) || start > TASK_SIZE || len >
TASK_SIZE-start)
return -EINVAL;
@@ -2811,17 +2813,20 @@ int do_munmap(struct mm_struct *mm, unsigned long
start, size_t len,
if (len == 0)
return -EINVAL;
+ if (down_write_killable(&mm->mmap_sem))
+ return -EINTR;
+
/* Find the first overlapping VMA */
vma = find_vma(mm, start);
if (!vma)
- return 0;
+ goto unlock;
prev = vma->vm_prev;
- /* we have start < vma->vm_end */
+ /* we have start < vma->vm_end */
/* if it doesn't overlap, we have nothing.. */
end = start + len;
if (vma->vm_start >= end)
- return 0;
+ goto unlock;
/*
* If we need to split any vma, do it now to save pain later.
@@ -2831,28 +2836,27 @@ int do_munmap(struct mm_struct *mm, unsigned long
start, size_t len,
* places tmp vma above, and higher split_vma places tmp vma below.
*/
if (start > vma->vm_start) {
- int error;
-
/*
* Make sure that map_count on return from munmap() will
* not exceed its limit; but let map_count go just above
* its limit temporarily, to help free resources as expected.
*/
+ res = -ENOMEM
if (end < vma->vm_end && mm->map_count >= sysctl_max_map_count)
- return -ENOMEM;
+ goto unlock;
- error = __split_vma(mm, vma, start, 0);
- if (error)
- return error;
+ res = __split_vma(mm, vma, start, 0);
+ if (res)
+ goto unlock;
prev = vma;
}
/* Does it split the last one? */
last = find_vma(mm, end);
if (last && end > last->vm_start) {
- int error = __split_vma(mm, last, end, 1);
- if (error)
- return error;
+ res = __split_vma(mm, last, end, 1);
+ if (res)
+ goto unlock;
}
vma = prev ? prev->vm_next : mm->mmap;
@@ -2866,9 +2870,19 @@ int do_munmap(struct mm_struct *mm, unsigned long start,
size_t len,
* split, despite we could. This is unlikely enough
* failure that it's not worth optimizing it for.
*/
- int error = userfaultfd_unmap_prep(vma, start, end, uf);
- if (error)
- return error;
+ result = userfaultfd_unmap_prep(vma, start, end, uf);
+ if (result)
+
