Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: Arvind Sankar <niveditas98@gmail.com>
From:   Arvind Sankar <nivedita@alum.mit.edu>
Date:   Wed, 28 Oct 2020 16:45:49 -0400
To:     Borislav Petkov <bp@alien8.de>
Cc:     Arvind Sankar <nivedita@alum.mit.edu>, x86@kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] x86/build: Fix vmlinux size check on 64-bit
Message-ID: <20201028204549.GA2231038@rani.riverdale.lan>
References: <20201005151539.2214095-1-nivedita@alum.mit.edu>
 <20201027200803.GL15580@zn.tnic>
 <20201027211422.GC1833548@rani.riverdale.lan>
 <20201028133909.GA27112@zn.tnic>
 <20201028164551.GA1989568@rani.riverdale.lan>
 <20201028194355.GB27112@zn.tnic>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20201028194355.GB27112@zn.tnic>
Precedence: bulk

On Wed, Oct 28, 2020 at 08:43:55PM +0100, Borislav Petkov wrote:
> On Wed, Oct 28, 2020 at 12:45:51PM -0400, Arvind Sankar wrote:
> > You don't want to try to run the kernel from physical address 0 in any
> > case. The default is set to 16MiB to avoid low memory, historically to
> > avoid the 24-bit ISA DMA range.
> 
> Sure, that's why I wrote:
> 
> "... so I guess this should be a range > 0 specification but I guess not
> important."
> 
> So how about a sentence or two alluding to that fact in the help text of
> that option?

It's mentioned in the commit message for ceefccc93932, but yeah, it
would be useful to have in the help text I guess. But that's not really
related to this patch.

> 
> > This doesn't matter for the 64-bit kernel, which can be run from any
> > physical address independent of the RELOCATABLE/PHYSICAL_START settings.
> > It only matters on 32-bit, where VA and PA are tied together by
> > 	VA == __PAGE_OFFSET + PA
> 
> You mean the kernel text mapping I assume because we do
> 
> #define __va(x)                 ((void *)((unsigned long)(x)+PAGE_OFFSET))
> 
> on 64-bit too but that's the direct mapping of all physical memory.

Yes, I meant the virtual addresses of the kernel symbols: the 32-bit
kernel needs relocation processing to be loaded at a different physical
address, but the 64-bit kernel doesn't unless the virtual address is
also being changed.

> 
> > KERNEL_IMAGE_SIZE is _not_ the size of the kernel image, the name is
> > misleading.
> 
> So that needs fixing too, I guess.

It's become ABI I think: looks like it's included by that name in
vmcoreinfo for kexec crash dumps.

> 
> > It is the maximum VA that the kernel can occupy, it is used
> > to prepopulate the PMD-level pagetable for initial boot (level2_kernel_pgt)
> > and is also used to define MODULES_VADDR, so it _is_ talking about
> > mappings. If you have a 30MiB kernel that is placed at a starting VA of
> > 510MiB when KERNEL_IMAGE_SIZE is 512MiB, it won't boot.
> 
> ... because not the whole kernel will be mapped, sure. There's a comment
> above KERNEL_IMAGE_SIZE which could use some of that explanation.

Hm, it also looks like KERNEL_IMAGE_SIZE is entirely unused on 32-bit
except for this linker script check and for KASLR. I'll do a v2 cleaning
up those comments.

> 
> > Increasing vmlinux size can trigger the problem by pushing _end
> > beyond KERNEL_IMAGE_SIZE, but the problem occurs once _end -
> > __START_KERNEL_map exceeds KERNEL_IMAGE_SIZE, not when _end - _text
> > exceeds it, hence this patch.
> 
> Understood - in both cases, once _end goes beyond the 512MiB end of the
> PMD mapping, we've lost. Please add that part to the commit message too
> because we will forget.
> 

That's what this bit in the commit message was trying to explain:
  The check uses (_end - _text), but this is not enough. The initial PMD
  used in startup_64() (level2_kernel_pgt) can only map upto
  KERNEL_IMAGE_SIZE from __START_KERNEL_map, not from _text.