[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Thu, 9 Mar 2017 20:40:55 +0100
From: Ard Biesheuvel <ard.biesheuvel@...aro.org>
To: Mark Rutland <mark.rutland@....com>
Cc: "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, Kees Cook <keescook@...omium.org>,
Laura Abbott <labbott@...oraproject.org>, kernel-hardening@...ts.openwall.com,
Will Deacon <will.deacon@....com>, Catalin Marinas <catalin.marinas@....com>,
"kvmarm@...ts.cs.columbia.edu" <kvmarm@...ts.cs.columbia.edu>, Marc Zyngier <marc.zyngier@....com>
Subject: Re: [PATCH v5 10/10] arm64: mm: set the contiguous bit for kernel
mappings where appropriate
On 9 March 2017 at 20:33, Mark Rutland <mark.rutland@....com> wrote:
> On Thu, Mar 09, 2017 at 09:25:12AM +0100, Ard Biesheuvel wrote:
>> +static inline u64 pte_cont_addr_end(u64 addr, u64 end)
>> +{
>> + return min((addr + CONT_PTE_SIZE) & CONT_PTE_MASK, end);
>> +}
>> +
>> +static inline u64 pmd_cont_addr_end(u64 addr, u64 end)
>> +{
>> + return min((addr + CONT_PMD_SIZE) & CONT_PMD_MASK, end);
>> +}
>
> These differ structurally from the usual p??_addr_end() macros defined
> in include/asm-generic/pgtable.h. I agree the asm-generic macros aren't
> pretty, but it would be nice to be consistent.
>
> I don't think the above handle a partial contiguous span at the end of
> the address space (e.g. where end is initial PAGE_SIZE away from 2^64),
> whereas the asm-generic form does, AFAICT.
>
> Can we please use:
>
> #define pte_cont_addr_end(addr, end) \
> ({ unsigned long __boundary = ((addr) + CONT_PTE_SIZE) & CONT_PTE_MASK; \
> (__boundary - 1 < (end) - 1)? __boundary: (end); \
> })
>
> #define pmd_cont_addr_end(addr, end) \
> ({ unsigned long __boundary = ((addr) + CONT_PMD_SIZE) & CONT_PMD_MASK; \
> (__boundary - 1 < (end) - 1)? __boundary: (end); \
> })
>
> ... instead?
>
OK, so that's what the -1 is for. Either version is fine by me.
> [...]
>
>> +static void init_pte(pte_t *pte, unsigned long addr, unsigned long end,
>> + phys_addr_t phys, pgprot_t prot)
>> {
>> + do {
>> + pte_t old_pte = *pte;
>> +
>> + set_pte(pte, pfn_pte(__phys_to_pfn(phys), prot));
>> +
>> + /*
>> + * After the PTE entry has been populated once, we
>> + * only allow updates to the permission attributes.
>> + */
>> + BUG_ON(!pgattr_change_is_safe(pte_val(old_pte), pte_val(*pte)));
>> +
>> + } while (pte++, addr += PAGE_SIZE, phys += PAGE_SIZE, addr != end);
>> +}
>> +
>> +static void alloc_init_cont_pte(pmd_t *pmd, unsigned long addr,
>> + unsigned long end, phys_addr_t phys,
>> + pgprot_t prot,
>> + phys_addr_t (*pgtable_alloc)(void),
>> + int flags)
>> +{
>> + unsigned long next;
>> pte_t *pte;
>>
>> BUG_ON(pmd_sect(*pmd));
>> @@ -136,45 +156,30 @@ static void alloc_init_pte(pmd_t *pmd, unsigned long addr,
>>
>> pte = pte_set_fixmap_offset(pmd, addr);
>> do {
>> - pte_t old_pte = *pte;
>> + pgprot_t __prot = prot;
>>
>> - set_pte(pte, pfn_pte(__phys_to_pfn(phys), prot));
>> - phys += PAGE_SIZE;
>> + next = pte_cont_addr_end(addr, end);
>>
>> - /*
>> - * After the PTE entry has been populated once, we
>> - * only allow updates to the permission attributes.
>> - */
>> - BUG_ON(!pgattr_change_is_safe(pte_val(old_pte), pte_val(*pte)));
>> + /* use a contiguous mapping if the range is suitably aligned */
>> + if ((((addr | next | phys) & ~CONT_PTE_MASK) == 0) &&
>> + (flags & NO_CONT_MAPPINGS) == 0)
>> + __prot = __pgprot(pgprot_val(prot) | PTE_CONT);
>>
>> - } while (pte++, addr += PAGE_SIZE, addr != end);
>> + init_pte(pte, addr, next, phys, __prot);
>> +
>> + phys += next - addr;
>> + pte += (next - addr) / PAGE_SIZE;
>> + } while (addr = next, addr != end);
>>
>> pte_clear_fixmap();
>> }
>
> I think it would be preferable to pass the pmd down into
> alloc_init_pte(), so that we don't have to mess with the pte in both
> alloc_init_cont_pte() and alloc_init_pte().
>
> Likewise for alloc_init_cont_pmd() and alloc_init_pmd(), regarding the
> pmd.
>
> I realise we'll redundantly map/unmap the PTE for each contiguous span,
> but I doubt there's a case it has a noticeable impact.
>
OK
> With lots of memory we'll use blocks at a higher level, and for
> debug_pagealloc we'll pass the whole pte down to init_pte() as we
> currently do.
>
> [...]
>
>> + if (pud_none(*pud)) {
>> + phys_addr_t pmd_phys;
>> + BUG_ON(!pgtable_alloc);
>> + pmd_phys = pgtable_alloc();
>> + pmd = pmd_set_fixmap(pmd_phys);
>> + __pud_populate(pud, pmd_phys, PUD_TYPE_TABLE);
>> + pmd_clear_fixmap();
>> + }
>
> It looks like when the splitting logic was removed, we forgot to remove
> the fixmapping here (and for the pmd_none() case). The __p?d_populate
> functions don't touch the next level table, so there's no reason to
> fixmap it.
>
> Would you mind spinning a patch to rip those out?
>
Ah right, pmd is not even referenced in the __pud_populate invocation.
Yes, I will add a patch before this one to remove that.
> [...]
>
>> void __init create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
>> unsigned long virt, phys_addr_t size,
>> pgprot_t prot, bool page_mappings_only)
>> {
>> - int flags;
>> + int flags = NO_CONT_MAPPINGS;
>>
>> BUG_ON(mm == &init_mm);
>>
>> if (page_mappings_only)
>> - flags = NO_BLOCK_MAPPINGS;
>> + flags |= NO_BLOCK_MAPPINGS;
>
> Why is it never safe to use cont mappings here?
>
> EFI's the only caller of this, and the only case I can see that we need
> to avoid contiguous entries for are the runtime services data/code, due
> to efi_set_mapping_permissions(). We map those with page_mappings_only
> set.
>
> I couldn't spot why we'd need to avoid cont entries otherwise.
>
> What am I missing?
>
Nothing. It is erring on the side of caution, really, since there is
no performance concern here.
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
