From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754849AbcARL5G (ORCPT ); Mon, 18 Jan 2016 06:57:06 -0500 Received: from foss.arm.com ([217.140.101.70]:34132 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754573AbcARL5E (ORCPT ); Mon, 18 Jan 2016 06:57:04 -0500 Date: Mon, 18 Jan 2016 11:56:40 +0000 From: Mark Rutland To: Ard Biesheuvel Cc: Laura Abbott , Catalin Marinas , Will Deacon , Kees Cook , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH] arm64: Allow vmalloc regions to be set with set_memory_* Message-ID: <20160118115640.GK21067@leverpostej> References: <1452635187-8057-1-git-send-email-labbott@fedoraproject.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jan 13, 2016 at 05:10:31PM +0100, Ard Biesheuvel wrote: > On 13 January 2016 at 15:03, Ard Biesheuvel wrote: > > On 12 January 2016 at 22:46, Laura Abbott wrote: > >> > >> The range of set_memory_* is currently restricted to the module address > >> range because of difficulties in breaking down larger block sizes. > >> vmalloc maps PAGE_SIZE pages so it is safe to use as well. Update the > >> function ranges and add a comment explaining why the range is restricted > >> the way it is. > >> > >> Signed-off-by: Laura Abbott > >> --- > >> This should let the protections for the eBPF work as expected, I don't > >> know if there is some sort of self test for thatL. > > > > > > This is going to conflict with my KASLR implementation, since it puts > > the kernel image right in the middle of the vmalloc area, and the > > kernel is obviously mapped with block mappings. In fact, I am > > proposing enabling huge-vmap for arm64 as well, since it seems an > > improvement generally, but also specifically allows me to unmap the > > __init section using the generic vunmap code (remove_vm_area). But in > > general, I think the assumption that the whole vmalloc area is mapped > > using pages is not tenable. > > > > AFAICT, vmalloc still use pages exclusively even with huge-vmap (but > > ioremap does not). So perhaps it would make sense to check for the > > VM_ALLOC bit in the VMA flags (which I will not set for the kernel > > regions either) > > > > Something along these lines, perhaps? > > diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c > index 3571c7309c5e..bda0a776c58e 100644 > --- a/arch/arm64/mm/pageattr.c > +++ b/arch/arm64/mm/pageattr.c > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > #include > > #include > @@ -44,6 +45,7 @@ static int change_memory_common(unsigned long addr > unsigned long end = start + size; > int ret; > struct page_change_data data; > + struct vm_struct *area; > > if (!PAGE_ALIGNED(addr)) { > start &= PAGE_MASK; > @@ -51,10 +53,14 @@ static int change_memory_common(unsigned long addr, > WARN_ON_ONCE(1); > } > > - if (start < MODULES_VADDR || start >= MODULES_END) > - return -EINVAL; > - > - if (end < MODULES_VADDR || end >= MODULES_END) > + /* > + * Check whether the [addr, addr + size) interval is entirely > + * covered by precisely one VM area that has the VM_ALLOC flag set > + */ > + area = find_vm_area((void *)addr); > + if (!area || > + end > (unsigned long)area->addr + area->size || > + !(area->flags & VM_ALLOC)) > return -EINVAL; > > data.set_mask = set_mask; Neat. That fixes the fencepost bug too. Looks good to me, though as Laura suggested we should have a comment as to why we limit changes to such regions. Fancy taking her wording below and spinning this as a patch? > >> + /* > >> + * This check explicitly excludes most kernel memory. Most kernel > >> + * memory is mapped with a larger page size and breaking down the > >> + * larger page size without causing TLB conflicts is very difficult. > >> + * > >> + * If you need to call set_memory_* on a range, the recommendation is > >> + * to use vmalloc since that range is mapped with pages. > >> + */ Thanks, Mark.