| Message ID | 20240817-mseal-depessimize-v3-3-d8d2e037df30@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4a2dd02b09160ee43f96c759fafa7b56dfc33816 |
| Headers | show |
| Series | mm: Optimize mseal checks | expand |
* Pedro Falcato <pedro.falcato@gmail.com> [240816 20:19]: > Avoid taking an extra trip down the mmap tree by checking the vmas > directly. mprotect (per POSIX) tolerates partial failure. > > Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com> Reviewed-by: Liam R. Howlett <Liam.Howlett@Oracle.com> > --- > mm/mprotect.c | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > diff --git a/mm/mprotect.c b/mm/mprotect.c > index 446f8e5f10d9..0c5d6d06107d 100644 > --- a/mm/mprotect.c > +++ b/mm/mprotect.c > @@ -611,6 +611,9 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb, > unsigned long charged = 0; > int error; > > + if (!can_modify_vma(vma)) > + return -EPERM; > + > if (newflags == oldflags) { > *pprev = vma; > return 0; > @@ -769,15 +772,6 @@ static int do_mprotect_pkey(unsigned long start, size_t len, > } > } > > - /* > - * checking if memory is sealed. > - * can_modify_mm assumes we have acquired the lock on MM. > - */ > - if (unlikely(!can_modify_mm(current->mm, start, end))) { > - error = -EPERM; > - goto out; > - } > - > prev = vma_prev(&vmi); > if (start > vma->vm_start) > prev = vma; > > -- > 2.46.0 >
On Sat, Aug 17, 2024 at 01:18:30AM GMT, Pedro Falcato wrote: > Avoid taking an extra trip down the mmap tree by checking the vmas > directly. mprotect (per POSIX) tolerates partial failure. Pretty sure this also applies to any such mXXX() operation, though I haven't read the formalised POSIX spec. But in practice, this is how it is :) > > Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com> > --- > mm/mprotect.c | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > diff --git a/mm/mprotect.c b/mm/mprotect.c > index 446f8e5f10d9..0c5d6d06107d 100644 > --- a/mm/mprotect.c > +++ b/mm/mprotect.c > @@ -611,6 +611,9 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb, > unsigned long charged = 0; > int error; > > + if (!can_modify_vma(vma)) > + return -EPERM; > + I'm glad to get rid of the unlikely() too, imo these should _only_ be added based on actual data to back them up rather than because the programmer instinctively 'feels' that something is unlikely from the compiler's point of view. > if (newflags == oldflags) { > *pprev = vma; > return 0; > @@ -769,15 +772,6 @@ static int do_mprotect_pkey(unsigned long start, size_t len, > } > } > > - /* > - * checking if memory is sealed. > - * can_modify_mm assumes we have acquired the lock on MM. > - */ > - if (unlikely(!can_modify_mm(current->mm, start, end))) { > - error = -EPERM; > - goto out; > - } > - This will allow the vm_ops->mprotect() caller to run on the vma before initiating the mprotect() fixup, a quick survey suggests that sgx uses this to see if mprotect() should be permitted in sgx_vma_mprotect() (so fine), and um uses it to actually do an mprotect() call on host memory (honestly fine too). Looking at the struct vm_operations_struct declaration I see: /* * Called by mprotect() to make driver-specific permission * checks before mprotect() is finalised. The VMA must not * be modified. Returns 0 if mprotect() can proceed. */ int (*mprotect)(struct vm_area_struct *vma, unsigned long start, unsigned long end, unsigned long newflags); Which explicitly says DO NOT MODIFY THE VMA. So we're good. > prev = vma_prev(&vmi); > if (start > vma->vm_start) > prev = vma; > > -- > 2.46.0 > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
diff --git a/mm/mprotect.c b/mm/mprotect.c index 446f8e5f10d9..0c5d6d06107d 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c @@ -611,6 +611,9 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb, unsigned long charged = 0; int error; + if (!can_modify_vma(vma)) + return -EPERM; + if (newflags == oldflags) { *pprev = vma; return 0; @@ -769,15 +772,6 @@ static int do_mprotect_pkey(unsigned long start, size_t len, } } - /* - * checking if memory is sealed. - * can_modify_mm assumes we have acquired the lock on MM. - */ - if (unlikely(!can_modify_mm(current->mm, start, end))) { - error = -EPERM; - goto out; - } - prev = vma_prev(&vmi); if (start > vma->vm_start) prev = vma;
Avoid taking an extra trip down the mmap tree by checking the vmas directly. mprotect (per POSIX) tolerates partial failure. Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com> --- mm/mprotect.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)