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