Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Baoquan He]

On 03/03/17 at 04:23pm, Borislav Petkov wrote:
> Ok,
> 
> TBH, I still don't like adding yet another define and paying attention
> to whether I should use image size or mapping size. After your patch,
> KERNEL_IMAGE_SIZE is used to enforce the actual image size from
> exploding:
> 
> arch/x86/include/asm/page_32_types.h:43:#define KERNEL_IMAGE_SIZE       (512 * 1024 * 1024)
> arch/x86/include/asm/page_32_types.h:49:#define KERNEL_MAPPING_SIZE     KERNEL_IMAGE_SIZE
> arch/x86/include/asm/page_64_types.h:56:#define KERNEL_IMAGE_SIZE       (512 * 1024 * 1024)
> arch/x86/include/asm/pgtable_32.h:83: *     (KERNEL_IMAGE_SIZE/4096) / 1024 pages (worst case, non PAE)
> arch/x86/include/asm/pgtable_32.h:84: *     (KERNEL_IMAGE_SIZE/4096) / 512 + 4 pages (worst case for PAE)
> arch/x86/include/asm/pgtable_32.h:91: * KERNEL_IMAGE_SIZE should be greater than pa(_end)
> arch/x86/kernel/vmlinux.lds.S:356:. = ASSERT((_end - LOAD_OFFSET <= KERNEL_IMAGE_SIZE),
> arch/x86/kernel/vmlinux.lds.S:357:         "kernel image bigger than KERNEL_IMAGE_SIZE");
> arch/x86/kernel/vmlinux.lds.S:370:. = ASSERT((_end - _text <= KERNEL_IMAGE_SIZE),
> arch/x86/kernel/vmlinux.lds.S:371:         "kernel image bigger than KERNEL_IMAGE_SIZE");
> 
> So what I'd do is keep KERNEL_IMAGE_SIZE and make it default 1G and use it
> everywhere.
> 
> Then, define a separate define which is used only in vmlinux.lds.S to
> enforce the size check. Having MAPPING_SIZE and IMAGE_SIZE is just
> needlessly confusing.

Yes, in fact if only look at 64 bit, what I am doing is just as you
suggested. KERNEL_IMAGE_SIZE is only used to limit the image size,
namely _end - _text.

. = ASSERT((_end - _text <= KERNEL_IMAGE_SIZE),
           "kernel image bigger than KERNEL_IMAGE_SIZE");

And I could get what's confusing, should be the name of
KERNEL_MAPPING_SIZE. If only talking about the kernel itself, the
mapping size needed to cover the whole kernel iamge should be the same
as kernel image size. Say kernel image size ( _end - _text) is 86M, then
if kaslr-ed to a postion at 0xffffffffa0000000, which is 512M above the
starting address of virtual address space used for kernel mapping, the
kernel iamge will be mapped at region:
	[0xffffffffa0000000, 0xffffffffa5600000]

So here KERNEL_MAPPING_SIZE is not meaning the size of the mapping
region of kernel image itself, but the whole virtual address space which
is available only for kernel mapping, nobody else can touch this area.
It's 1G big, and cover region [0xffffffff80000000, 0xffffffffbfffffff].
It might be named as SIZE_OF_SPACE_FOR_KERNEL_MAPPING, but that is not a
good name, so I just named as KERNEL_MAPPING_SIZE. It could be
confusing.

> 
> Especially if this is just some inconsistency you're addressing and not
> some real issue.

I think it's not simply fixing inconsistency thing now, according to a
lot of discussion, we all agree that there's no need to change the size
of space for kernel mapping back and forth, 512M to 1G, 1G back to 512M,
risk isn't felt when shrink kernel modules space to 1G constantly.

> 
> BUT(!), don't take my word for it. Rather, do what the maintainers
> propose. Who knows, they might have a much better idea.

Sorry about that. Just think your words are very convincing on removing
people's doubt if it's risky to shrink kernel modules space to 1G. Will
remove the words mentioning you said it since you don't like it. Didn't
realize that, no offence.

Thanks
Baoquan