Re: [PATCH] arm64: correct modules range of kernel virtual memory layout

[Will Deacon]

On Tue, Aug 08, 2017 at 03:04:55PM +0100, Ard Biesheuvel wrote:
> On 8 August 2017 at 14:19, Will Deacon <will.deacon@xxxxxxx> wrote:
> > On Tue, Aug 08, 2017 at 01:27:25PM +0800, Miles Chen wrote:
> >> On Tue, 2017-08-08 at 12:44 +0800, Miles Chen wrote:
> >> > Agreed.MODULES_VADDR should be phased out. Considering the kernel
> >> > modules live somewhere between [VMALLOC_START, VMALLOC_END) now:
> >> > (arch/arm64/kernel/module.c:module_alloc). I suggest the following
> >> > changes:
> >> >
> >> > 1. is_vmalloc_or_module_addr() should return is_vmalloc_addr() directly
> >> > 2. arch/arm64/mm/dump.c does not need MODULES_VADDR and MODULES_END.
> >> > 3. kasan uses [module_alloc_base, module_alloc_base + MODULES_VSIZE) to
> >> > get the shadow memory? (the kernel modules still live in this range when
> >> > kasan is enabled)
> >> > 4. remove modules line in kernel memory layout
> >> > (optional, thanks for Ard's feedback)
> >> > 5. remove MODULE_VADDR, MODULES_END definition
> >>
> >> I was wrong about this. is_vmalloc_or_module_addr() is defined
> >> in mm/vmalloc and it uses MODULES_VADDR and MODULES_END.
> >> May it is better to give MODULES_VADDR and MODULES_END
> >> proper values, not remove them.
> >
> > I think the only cases where the modules area isn't completely contained
> > within vmalloc is where either randomization is disabled
> > (CONFIG_RANDOMIZE_BASE=n) or we fail in kaslr_early_init. However, in both
> > of these cases, module_alloc_base is set correctly, so perhaps we could
> > defined MODULES_VADDR in terms of that oto get is_vmalloc_or_module_addr
> > working properly.
> 
> is_vmalloc_or_module_addr() already works properly: modules are loaded
> into their own dedicated region, or in the vmalloc space otherwise.
> Note that this even applies when disregarding KASRL: the module PLT
> support uses the vmalloc region as overflow if the module region is
> exhausted.

I'm not sure I'd say it works properly in all cases. If we're placing
modules in the vmalloc area, then the piece of VA space below that shouldn't
be treated as the module area, otherwise we could say that
is_vmalloc_or_module_addr() could return 1 for all areas of unused VA
space, which I don't think is right.

> > We'd need to add some code to the table dumper to avoid
> > printing the module area if it's contained within vmalloc, and that could
> > also be used for the kernel memory layout print.
> >
> 
> I agree it may be misleading if the module region is empty while
> modules have been loaded. But let's not conflate the module *region*
> with the actual locations where modules are loaded: without
> CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, this is a 128 MB region that
> overlaps the kernel .text section.

Sure, but what use is there in communicating the module region if we don't
actually load modules there? It only serves to confuse people IMO.

Will