Messages in this thread | | | From | Thomas Garnier <> | Date | Mon, 20 Jun 2016 09:17:05 -0700 | Subject | Re: [PATCH v6 1/3] x86/mm: PUD VA support for physical mapping (x86_64) |
| |
On Fri, Jun 17, 2016 at 2:02 AM, Ingo Molnar <mingo@kernel.org> wrote: > > * Kees Cook <keescook@chromium.org> wrote: > >> From: Thomas Garnier <thgarnie@google.com> >> >> Minor change that allows early boot physical mapping of PUD level virtual >> addresses. The current implementation expects the virtual address to be >> PUD aligned. For KASLR memory randomization, we need to be able to >> randomize the offset used on the PUD table. >> >> It has no impact on current usage. >> >> Signed-off-by: Thomas Garnier <thgarnie@google.com> >> Signed-off-by: Kees Cook <keescook@chromium.org> >> --- >> arch/x86/mm/init_64.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c >> index bce2e5d9edd4..f205f39bd808 100644 >> --- a/arch/x86/mm/init_64.c >> +++ b/arch/x86/mm/init_64.c >> @@ -454,10 +454,10 @@ phys_pud_init(pud_t *pud_page, unsigned long addr, unsigned long end, >> { >> unsigned long pages = 0, next; >> unsigned long last_map_addr = end; >> - int i = pud_index(addr); >> + int i = pud_index((unsigned long)__va(addr)); >> >> >> for (; i < PTRS_PER_PUD; i++, addr = next) { >> - pud_t *pud = pud_page + pud_index(addr); >> + pud_t *pud = pud_page + pud_index((unsigned long)__va(addr)); >> pmd_t *pmd; >> pgprot_t prot = PAGE_KERNEL; > > So I really dislike two things about this code. > > Firstly a pre-existing problem is that the parameter names to phys_pud_init() > suck: > > static unsigned long __meminit > phys_pud_init(pud_t *pud_page, unsigned long addr, unsigned long end, > unsigned long page_size_mask) > > so 'unsigned long addr' is usually the signature of a virtual address - but that's > no true here: it's a physical address. > > Same goes for 'unsigned long end'. Plus it's unclear what the connection between > 'addr' and 'end' - it's not at all obvious 'at a glance' that they are the start > and end addresses of a physical memory range. > > All of these problems can be solved by renaming them to 'paddr_start' and > 'paddr_end'. > > Btw., I believe this misnomer and confusing code resulted in the buggy > 'pud_index(addr)' not being noticed to begin with ... >
I will add a new commit that rename variables as described.
> Secondly, and that's a new problem introduced by this patch: > >> + int i = pud_index((unsigned long)__va(addr)); >> + pud_t *pud = pud_page + pud_index((unsigned long)__va(addr)); > > ... beyond the repetition, using type casts is fragile. Type casts should be a red > flag to anyone involved in low level, security relevant code! So I'm pretty > unhappy about seeing such a problem in such a patch. > > This code should be doing something like: > > unsigned long vaddr_start = __va(paddr_start); > > ... which gets rid of the type cast, the repetition and documents the code much > better as well.
Unfortunately, we can't do that because __va return a void*. We will get this warning on compile:
arch/x86/mm/init_64.c:537:8: warning: assignment makes integer from pointer without a cast [enabled by default] vaddr = __va(paddr_start);
If we used void*, we would need to type cast even more places. What do you think?
> Also see how easily the connection between the variables is > self-documented just by picking names carefully: > > paddr_start > paddr_end > vaddr_start > vaddr_end >
Will do on kernel_physical_mapping_init down.
> Also, _please_ add a comment to phys_pud_init() that explains what the function > does. >
Will do.
> Thanks, > > Ingo
| |