[PATCH v3 1/5] arm64: mm: BUG on unsupported manipulations of live kernel mappings

[Catalin Marinas]

On Thu, Oct 13, 2016 at 03:48:04PM +0100, Ard Biesheuvel wrote:
> On 13 October 2016 at 15:44, Catalin Marinas <catalin.marinas at arm.com> wrote:
> > On Thu, Oct 13, 2016 at 01:25:53PM +0100, Ard Biesheuvel wrote:
> >
> > (fixing up alignment to make it readable)
> >
> 
> I apologise on Gmail's behalf
> 
> >> """
> >> /*
> >>  * Returns whether updating a live page table entry is safe:
> >>  * - if the old and new values are identical,
> >>  * - if an invalid mapping is turned into a valid one (or vice versa),
> >>  * - if the entry is a block or page mapping, and the old and new values
> >>  *   only differ in the PXN/RDONLY/WRITE bits.
> >>  *
> >>  * NOTE: 'safe' does not imply that no TLB maintenance is required, it only
> >>  *       means that no TLB conflicts should occur as a result of the update.
> >>  */
> >> #define __set_pgattr_is_safe(type, old, new, blocktype) \
> >>       (type ## _val(old) == type ## _val(new) || \
> >>        ((type ## _val(old) ^ type ## _val(new)) & PTE_VALID) != 0 || \
> >>        (((type ## _val(old) & PTE_TYPE_MASK) == blocktype) && \
> >>         (((type ## _val(old) ^ type ## _val(new)) & \
> >>        ~(PTE_PXN | PTE_RDONLY | PTE_WRITE)) == 0)))
> >>
> >> static inline bool set_live_pte_is_safe(pte_t old, pte_t new)
> >> {
> >>       return __set_pgattr_is_safe(pte, old, new, PTE_TYPE_PAGE);
> >> }
> >>
> >> static inline bool set_live_pmd_is_safe(pmd_t old, pmd_t new)
> >> {
> >>       return __set_pgattr_is_safe(pmd, old, new, PMD_TYPE_SECT);
> >> }
> >>
> >> static inline bool set_live_pud_is_safe(pud_t old, pud_t new)
> >> {
> >>       return __set_pgattr_is_safe(pud, old, new, PUD_TYPE_SECT);
> >> }
> >
> > The set_ prefix is slightly confusing as it suggests (to me) having a
> > side effect. Maybe pgattr_set_is_safe()?
> >
> > But it looks like we make it more complicated needed by using pte_t
> > instead of pteval_t as argument. How about just using the pteval_t as
> > argument (and it's fine to call it with pmdval_t, pudval_t as well):
> >
> > #define pgattr_set_is_safe(oldval, newval) \
> >         ...
> 
> Well, the only problem there is that the permission bit check should
> only apply to level 3 page mappings (bit[1] == 1) and level 1/2 block
> mappings (bit[1] == 0), so we would still need two versions

I was suggesting that you only replace the "... & ~modifiable_attr_mask"
check in your patch to avoid writing it three times. The macro that you
proposed above does more but it is also more unreadable.

-- 
Catalin