CONFIG_DEBUG_SET_MODULE_RONX broken (was Re: [PATCHv2] arm64: add support to dump the kernel page tables)

[Rusty Russell]

Laura Abbott <lauraa at codeaurora.org> writes:
> On 11/25/2014 10:05 PM, Rusty Russell wrote:
>>>
>>> Yep, I'm seeing the same thing. We're failing the bounds check:
>>>
>>> if (!is_module_address(start) || !is_module_address(end - 1))
>>>                   return -EINVAL;
>>
>> That's a weird test, but I can agree that it's now broken.  What's it for?
>>
>
> Both arm and arm64 map underlying memory with page table mappings that may
> be greater than PAGE_SIZE. Rather than deal with the hassle of trying to
> tear down the larger mappings and call stop_machine to flush the TLBs, we
> just disallow changing attributes of arbitrary memory. Module memory is
> always mapped with 4K pages so it's safe to change the attributes, hence
> the bounds check above. On arm we just bounds check via
> MODULE_START <= addr < MODULE_END so that wasn't affected.

OK, perhaps as you say, we should do this.

>> Yes, but you only need the first change in your patch: mod->init_size
>> should already be aligned (and unlike mod->core_size, we haven't
>> modified it).
>>
>
> the init size can be modified via get_offset though. In my testing I needed
> to align up both mod->init_size and mod->core_size for is_module_address to
> pass on all set_memory_* calls made by the module layer.

You're right.  It doesn't seem to hurt any other code path, but if you
want this, please send me a proper explanation w/ signed-off-by.

Thanks!
Rusty.

>>> diff --git a/kernel/module.c b/kernel/module.c
>>> index 972151b..3791330 100644
>>> --- a/kernel/module.c
>>> +++ b/kernel/module.c
>>> @@ -2316,10 +2316,14 @@ static void layout_symtab(struct module *mod, struct load_info *info)
>>>           info->stroffs = mod->core_size = info->symoffs + ndst * sizeof(Elf_Sym);
>>>           mod->core_size += strtab_size;
>>>
>>> +       mod->core_size = debug_align(mod->core_size);
>>> +
>>>           /* Put string table section at end of init part of module. */
>>>           strsect->sh_flags |= SHF_ALLOC;
>>>           strsect->sh_entsize = get_offset(mod, &mod->init_size, strsect,
>>>                                            info->index.str) | INIT_OFFSET_MASK;
>>> +
>>> +       mod->init_size = debug_align(mod->init_size);
>>>           pr_debug("\t%s\n", info->secstrings + strsect->sh_name);
>>>    }
>>>
>>> I haven't tried a bisect to see if this is new.
>>>
>>> I'm kind of tempted to switch the bounds check back to
>>> (addr >= MODULES_VADDR && addr < MODULES_END) unless there is a clean way to
>>> fixup module.c
>>
>> Thanks,
>> Rusty.
>>
>
> Thanks,
> Laura
>
> -- 
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project