Hi,
On 1/15/24 14:05, Kevin Brodsky wrote:
On 08/01/2024 11:23, Amit Daniel Kachhap wrote:
[...]
+#ifdef CONFIG_CHERI_PURECAP_UABI +#define CHERI_REPRESENTABLE_ALIGNMENT(len) \
- (~cheri_representable_alignment_mask(len) + 1)
+#define CHERI_REPRESENTABLE_BASE(base, len) \
- ((base) & cheri_representable_alignment_mask(len))
To be generic, we could rename those to reserv_representable_* and add one for the length too, with trivial !PCuABI implementations.
Yes makes sense.
[...]
+/**
- reserv_vmi_valid_capability() - Search and matches the reservation interface
- for the capability bound values falling within the reserved virtual address
- range. This function internally uses mmap_lock to synchronize the vma
- updates if mmap_lock not already held.
- @vmi: The vma iterator pointing at the vma.
- @cap: Reservation capability value.
- @locked: Flag to indicate if mmap_lock is already held.
- Return: True if the input capability bound values within the reserved virtual
address range or false otherwise.- */
+bool reserv_vmi_valid_capability(struct vma_iterator *vmi, user_uintptr_t cap,
bool locked);+/**
- reserv_vma_valid_capability() - Search and matches the input vma for the
- capability bound values falling within the reserved virtual address range.
- This function should be called with mmap_lock held.
- @vma: The vma pointer.
- @cap: Reservation capability value.
- Return: True if the input capability bound values within the reserved virtual
address range or false otherwise.- */
+bool reserv_vma_valid_capability(struct vm_area_struct *vma, user_uintptr_t cap);
+/**
- reserv_vma_valid_address() - Search and matches the input vma for the input
- address range falling within the reserved virtual address range. This
- function should be called with mmap_lock held.
- @vma: The vma pointer.
- @start: Virtual address start value.
- @len: Virtual address length.
- Return: True if the input address range within the reserved virtual address
range or false otherwise.- */
+bool reserv_vma_valid_address(struct vm_area_struct *vma, ptraddr_t start, size_t len);
It's pretty unclear what "valid" means in these three functions. We could maybe say "reserv_*_cap_within_reserv" and "reserv_vma_range_within_reserv", since they only check that the capability bounds / range are within some reservation.
yes *_within_reserv() sounds better.
[...]
@@ -571,6 +571,11 @@ struct vm_area_struct { struct vma_numab_state *numab_state; /* NUMA Balancing state */ #endif struct vm_userfaultfd_ctx vm_userfaultfd_ctx; +#ifdef CONFIG_CHERI_PURECAP_UABI
- unsigned long reserv_start;
Should be ptraddr_t too.
- unsigned long reserv_len;
size_t is a bit nicer for lengths, though it's not used very consistently in the mm code.
- unsigned long reserv_perm;
cheri_perms_t I suppose? Shouldn't be a problem as this is #ifdef'd.
+#endif } __randomize_layout;
ok. I will update all the 3 reserv members.
[...]
+bool reserv_vmi_valid_capability(struct vma_iterator *vmi, user_uintptr_t cap, bool locked) +{
- struct vm_area_struct *vma;
- struct mm_struct *mm = current->mm;
- ptraddr_t cap_start = cheri_base_get(cap);
- ptraddr_t cap_end = cap_start + cheri_length_get(cap);
- bool ret = false;
- if (!test_bit(MMF_PCUABI_RESERVE, &mm->flags))
return true;- if (!locked && mmap_read_lock_killable(mm))
return false;- /* Check if there is match with the existing reservations */
- vma = mas_find(&vmi->mas, cap_end);
- if (!vma) {
ret = true;Why do we return true if there is no reservation in this range?
Well this is somewhat inspired from munmap() behaviour. If munmap() is called for non-existing vma then it does not return error. So I tried to have the same behavior and not return the -ERESERVATION error.
Besides I need some clarification regarding this interface. Currently this interface only checks reservation for the first vma and not all vma in the given range. As reservation fields remains constant so I left it like this for performance benefit. Let me know your opinion on this.
goto out;- }
- if (vma->reserv_start <= cap_start &&
vma->reserv_len >= cheri_length_get(cap))ret = true;+out:
- if (!locked)
mmap_read_unlock(mm);- return ret;
+}
+bool reserv_vma_valid_capability(struct vm_area_struct *vma, user_uintptr_t cap) +{
- if (!reserv_vma_is_supported(vma))
return true;- /* Check if there is match with the existing reservations */
- if (vma->reserv_start <= cheri_base_get(cap) &&
vma->reserv_len >= cheri_length_get(cap))return true;- return false;
+}
+bool reserv_vma_valid_address(struct vm_area_struct *vma, ptraddr_t start, size_t len) +{
- if (!reserv_vma_is_supported(vma))
return true;- /* Check if there is match with the existing reservations */
- if (vma->reserv_start <= start && vma->reserv_len >= len)
return true;- return false;
+}
+bool reserv_vma_same_reserv(struct vm_area_struct *vma1, struct vm_area_struct *vma2) +{
- if (!vma1 || !vma2 || !reserv_vma_is_supported(vma1) || !reserv_vma_is_supported(vma1))
return true;- /* Match reservation properties betwen 2 vma's */
- if (reserv_vma_is_supported(vma1) && reserv_vma_is_supported(vma1)
&& vma1->reserv_start == vma2->reserv_start&& vma1->reserv_len == vma2->reserv_len)return true;- return false;
+}
+bool reserv_vma_is_supported(struct vm_area_struct *vma)
I don't think this should be part of the API, as reservations are not enabled per vma. In fact, considering that half the functions (e.g. reserv_range_set_reserv) simply check current->mm->flags, we might as well just do that for all of them, in which case we'd just replace this function with one that takes a struct mm_struct * as argument, as I suggested in patch 6.
I agree with your suggestion that this helper should take mm as input.
A wider question that applies to the whole series is how we want to approach checking mm->flags. Right now we're often testing it both in the helper and in the caller, which is unnecessary. I can see pros and cons for both approaches, and I think we could do it either way, but we should choose one of them. Doing the checking in the helpers probably implies adding more helpers to avoid conditional code in syscall handlers, but that could be quite elegant.
Yes there are some repeated mm->flags checks. It can be moved to helpers only. However get_unmapped_area{*} is the place where still mm->flags checks will be required. I will check more into it.
Thanks, Amit
Kevin
+{
- if (vma && vma->vm_mm && test_bit(MMF_PCUABI_RESERVE,
&vma->vm_mm->flags))return true;- return false;
+}
+#endif /* CONFIG_CHERI_PURECAP_UABI */