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 <[email protected]> > --- > 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 <[email protected]>
