[linux-morello] Re: [PATCH RFC v2 07/19] mm/mremap: Add the PCuABI reservation interfaces

Hi,

On 10/27/23 19:36, Kevin Brodsky wrote:

On 25/10/2023 07:36, Amit Daniel Kachhap wrote:

...

[...]

@@ -3362,6 +3366,8 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap, new_vma->vm_start = addr; new_vma->vm_end = addr + len; new_vma->vm_pgoff = pgoff;

• if (vma->vm_flags & VM_PCUABI_RESERVE)
  

• 	reserv_vma_insert_entry(new_vma, addr, len, 0);
  

  if (vma_dup_policy(vma, new_vma)) goto out_free_vma; if (anon_vma_clone(new_vma, vma))

I don't think these changes are particularly related to mremap(), mapping can be split or merged for other reasons. It would make sense to have a separate patch for everything concerning mapping / reservation interactions that isn't related to a specific syscall.

mremap is the only user of copy_vma(). Anyways I can try adding vma operations (copy_vma, merge_vma) in a single patch.

...

[...]

@@ -956,6 +978,40 @@ SYSCALL_DEFINE5(__retptr__(mremap), user_uintptr_t, addr, unsigned long, old_len goto out; } +#ifdef CONFIG_CHERI_PURECAP_UABI

• if (!(vma->vm_flags & VM_PCUABI_RESERVE))

• goto skip_pcuabi_checks;
  

• if (!capability_owns_range(user_addr, addr, old_len ? old_len : new_len))

• goto out;
  

• if (!reserv_vma_range_fully_mapped(vma, addr, old_len ? old_len : new_len)) {

I'm not sure why we need this check, we are not creating a new mapping on the old_addr side.

ok.

...

• ret = -ERESERVATION;
  

• goto out;
  

• }
• if (cheri_tag_get(user_new_addr)) {

• if (!capability_owns_range(user_new_addr, new_addr, new_len) ||
  

•     !(flags & MREMAP_FIXED))
  

The spec clarifies that new_addr is only considered if MREMAP_FIXED is specified (because it is an optional argument). We should skip those checks otherwise.

ok.

...

• 	goto out;
  

• vma_new = vma_lookup(mm, new_addr);
  

• if (!reserv_vma_range_fully_mapped(vma_new, new_addr, new_len)) {
  

• 	ret = -ERESERVATION;
  

• 	goto out;
  

• }
  

• } else {

• if (!cheri_is_null_derived(user_new_addr))
  

• 	goto out;
  

• }
• /*

• * If new_len > old_len and flags does not contain MREMAP_MAYMOVE
  

• * then this fails as PCuABI does not allow increasing reservation.
  

• */
  

• if (new_len > old_len && !(flags & MREMAP_MAYMOVE)) {

• ret = -ERESERVATION;
  

Given the rule in the spec, that is:

...

The semantics of MREMAP_MAYMOVE is modified so that it is considered

that a mapping must be moved if the range (old_address.address, new_size) does not fit in the reservation to which the old mapping belongs, in addition to existing conditions (i.e. not clashing with existing mappings).

I don't think we need any explicit check: if the mapping cannot be grown in place, and MREMAP_MAYMOVE is not passed, then we will fail with -ENOMEM as usual, which is exactly what is specified.

ok.

...

• goto out;
  

• }
• old_perm = vma->reserv_perm;

+skip_pcuabi_checks: +#endif

• if (is_vm_hugetlb_page(vma)) { struct hstate *h __maybe_unused = hstate_vma(vma);

@@ -977,7 +1033,7 @@ SYSCALL_DEFINE5(__retptr__(mremap), user_uintptr_t, addr, unsigned long, old_len } if (flags & (MREMAP_FIXED | MREMAP_DONTUNMAP)) {

• ret = mremap_to(addr, old_len, new_addr, new_len,
  

• ret = mremap_to(user_addr, old_len, user_new_addr, new_len,
  	&locked, flags, &uf, &uf_unmap_early,
  	&uf_unmap);
  

  goto out;

@@ -993,7 +1049,7 @@ SYSCALL_DEFINE5(__retptr__(mremap), user_uintptr_t, addr, unsigned long, old_len int retval; VMA_ITERATOR(vmi, mm, addr + new_len);

• retval = do_vmi_munmap(&vmi, mm, addr + new_len,
  

• retval = do_vmi_munmap(&vmi, mm, user_addr + new_len,
  	       old_len - new_len, &uf_unmap, true);
  

  /* Returning 1 indicates mmap_lock is downgraded to read. */ if (retval == 1) {

@@ -1003,7 +1059,7 @@ SYSCALL_DEFINE5(__retptr__(mremap), user_uintptr_t, addr, unsigned long, old_len goto out; }

• ret = addr;
  

• ret = user_addr;
  

  goto out; }

@@ -1019,8 +1075,13 @@ SYSCALL_DEFINE5(__retptr__(mremap), user_uintptr_t, addr, unsigned long, old_len /* old_len exactly to the end of the area.. */ if (old_len == vma->vm_end - addr) {

• /* can we just expand the current mapping? */
  

• if (vma_expandable(vma, new_len - old_len)) {
  

• /*
  

•  * can we just expand the current mapping?
  

•  * PCuABI specification does not allow increasing reservation
  

•  * size so just skip this path.
  

Reservations cannot be expanded, but mappings still can. Sure, in most cases the mapping ends where the reservation does, but we shouldn't assume it. We should rather check whether there is enough free space after the mapping, within the reservation - I suppose this means modifying vma_expandable().

ok

I'm realising there is some ambiguity on this point in the spec, as the Reservations section claims that unmapped space within a reservation cannot be reused. That's true when it comes to creating new mappings (e.g. mmap), but expanding mappings is still allowed. I will amend the spec to clarify this.

Yes this will make it clear.

//Amit

Kevin

...

•  */
  

• if (!(vma->vm_flags & VM_PCUABI_RESERVE) &&
  

• 		vma_expandable(vma, new_len - old_len)) {
  long pages = (new_len - old_len) >> PAGE_SHIFT;
  unsigned long extension_start = addr + old_len;
  unsigned long extension_end = addr + new_len;
  

@@ -1083,8 +1144,14 @@ SYSCALL_DEFINE5(__retptr__(mremap), user_uintptr_t, addr, unsigned long, old_len goto out; }

• ret = move_vma(vma, addr, old_len, new_len, new_addr,
  

• ret = move_vma(vma, user_addr, old_len, new_len, new_addr,
         &locked, flags, &uf, &uf_unmap);
  

• if (!IS_ERR_VALUE(ret)) {
  

• 	if (vma->vm_flags & VM_PCUABI_RESERVE)
  

• 		ret = build_owning_capability(new_addr, new_len, old_perm);
  

• 	else
  

• 		ret = (user_uintptr_t)uaddr_to_user_ptr_safe(new_addr);
  

• }
  

  } out: if (offset_in_page(ret))

@@ -1098,8 +1165,5 @@ SYSCALL_DEFINE5(__retptr__(mremap), user_uintptr_t, addr, unsigned long, old_len userfaultfd_unmap_complete(mm, &uf_unmap_early); mremap_userfaultfd_complete(&uf, addr, ret, old_len); userfaultfd_unmap_complete(mm, &uf_unmap);

• /* TODO [PCuABI] - derive proper capability */
• return IS_ERR_VALUE(ret) ?

• ret :
  

• (user_intptr_t)uaddr_to_user_ptr_safe((ptraddr_t)ret);
  

• return ret; }