Received-SPF: pass (google.com: domain of linux-kernel+bounces-130221-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 3 Apr 2024 17:51:44 +0100
From: Mark Rutland <mark.rutland@arm.com>
To: Jarkko Sakkinen <jarkko@kernel.org>
Cc: linux-kernel@vger.kernel.org, Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, anil.s.keshavamurthy@intel.com,
	aou@eecs.berkeley.edu, davem@davemloft.net,
	linux-arm-kernel@lists.infradead.org, mhiramat@kernel.org,
	naveen.n.rao@linux.ibm.com, palmer@dabbelt.com,
	paul.walmsley@sifive.com
Subject: Re: [PATCH v2 1/4] arm64: patching: always use fixmap
Message-ID: <Zg2JIFNy8IgwIyUV@FVFF77S0Q05N>
References: <20240403150154.667649-1-mark.rutland@arm.com>
 <20240403150154.667649-2-mark.rutland@arm.com>
 <D0AMI3962WW0.3JKFCSUXVSSVL@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <D0AMI3962WW0.3JKFCSUXVSSVL@kernel.org>

On Wed, Apr 03, 2024 at 07:20:31PM +0300, Jarkko Sakkinen wrote:
> On Wed Apr 3, 2024 at 6:01 PM EEST, Mark Rutland wrote:
> > For historical reasons, patch_map() won't bother to fixmap non-image
> > addresses when CONFIG_STRICT_MODULE_RWX=n, matching the behaviour prior
> > to the introduction of CONFIG_STRICT_MODULE_RWX. However, as arm64
> > doesn't select CONFIG_ARCH_OPTIONAL_KERNEL_RWX, CONFIG_MODULES implies
> > CONFIG_STRICT_MODULE_RWX, so any kernel built with module support will
> > use the fixmap for any non-image address.
> 
> Not familiar with the config flag but I'd guess it is essentially
> w^x enforcement right for the sections?

Essentially, yes.

> > Historically we only used patch_map() for the kernel image and modules,
> > but these days its also used by BPF and KPROBES to write to read-only
> > pages of executable text. Currently these both depend on CONFIG_MODULES,
> > but we'd like to change that in subsequent patches, which will require
> > using the fixmap regardless of CONFIG_STRICT_MODULE_RWX.
> >
> > This patch changes patch_map() to always use the fixmap, and simplifies
> > the logic:
> >
> > * Use is_image_text() directly in the if-else, rather than using a
> >   temporary boolean variable.
> >
> > * Use offset_in_page() to get the offset within the mapping.
> >
> > * Remove uintaddr and cast the address directly when using
> >   is_image_text().
> >
> > For kernels built with CONFIG_MODULES=y, there should be no functional
> > change as a result of this patch.
> >
> > For kernels built with CONFIG_MODULES=n, patch_map() will use the fixmap
> > for non-image addresses, but there are no extant users with non-image
> > addresses when CONFIG_MODULES=n, and hence there should be no functional
> > change as a result of this patch alone.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm64/kernel/patching.c | 10 +++-------
> >  1 file changed, 3 insertions(+), 7 deletions(-)
> >
> > Catalin, Will, this is a prerequisite for the final two patches in the
> > series. Are you happy for this go via the tracing tree?
> >
> > Mark.
> >
> > diff --git a/arch/arm64/kernel/patching.c b/arch/arm64/kernel/patching.c
> > index 2555349303684..f0f3a2a82ca5a 100644
> > --- a/arch/arm64/kernel/patching.c
> > +++ b/arch/arm64/kernel/patching.c
> > @@ -30,20 +30,16 @@ static bool is_image_text(unsigned long addr)
> >  
> >  static void __kprobes *patch_map(void *addr, int fixmap)
> >  {
> > -	unsigned long uintaddr = (uintptr_t) addr;
> > -	bool image = is_image_text(uintaddr);
> >  	struct page *page;
> >  
> > -	if (image)
> > +	if (is_image_text((unsigned long)addr))
> >  		page = phys_to_page(__pa_symbol(addr));
> > -	else if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
> > -		page = vmalloc_to_page(addr);
> >  	else
> > -		return addr;
> > +		page = vmalloc_to_page(addr);
> >  
> >  	BUG_ON(!page);
> >  	return (void *)set_fixmap_offset(fixmap, page_to_phys(page) +
> > -			(uintaddr & ~PAGE_MASK));
> > +					 offset_in_page(addr));
> 
> nit: could be a single line but i guess it is up to the taste (and
> subsystem maintainer). I.e. checkpatch will allow it at least.
> 
> I don't mind it too much just mentioning for completeness.

At that point it goes to 93 chars long, and I stuck with the existing line
wrapping at 80 chars. I'd rather have a temporary 'phys_addr_t phys' variable
and do:

	phys = page_to_phys(page) + offset_in_page(addr);
	return (void *)set_fixmap(fixmap, phys);

.. but I'll leave this as-is for now.

> >  }
> >  
> >  static void __kprobes patch_unmap(int fixmap)
> 
> If my assumption about the config flag holds this makes sense:
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.rg>

Thanks! I assume that should be "kernel.org", with an 'o' ;)

Mark.