Re: [PATCH 1/8] arm64: memory: Fix virt_addr_valid() using __is_lm_address()

From: Will Deacon <will@kernel.org>
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Steve Capper <steve.capper@arm.com>,
	Andrey Konovalov <andreyknvl@google.com>,
	Geert Uytterhoeven <geert@linux-m68k.org>, Qian Cai <cai@lca.pw>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 1/8] arm64: memory: Fix virt_addr_valid() using __is_lm_address()
Date: Wed, 14 Aug 2019 13:02:21 +0100	[thread overview]
Message-ID: <20190814120221.zuguyh6tpku3nvtt@willie-the-truck> (raw)
In-Reply-To: <20190814104022.GI50688@arrakis.emea.arm.com>

On Wed, Aug 14, 2019 at 11:40:23AM +0100, Catalin Marinas wrote:
> On Wed, Aug 14, 2019 at 10:48:20AM +0100, Will Deacon wrote:
> > On Wed, Aug 14, 2019 at 10:19:42AM +0100, Catalin Marinas wrote:
> > > On Tue, Aug 13, 2019 at 06:01:42PM +0100, Will Deacon wrote:
> > > > -#define _virt_addr_is_linear(kaddr)	\
> > > > -	(__tag_reset((u64)(kaddr)) >= PAGE_OFFSET)
> > > > +#define virt_addr_valid(addr)	({					\
> > > > +	__typeof__(addr) __addr = addr;					\
> > > > +	__is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr));	\
> > > > +})
> > > 
> > > There is a slight change of semantics here but I don't think it's an
> > > issue currently. __is_lm_address() is true even for a user address, so
> > > at least the first part of virt_addr_valid() now accepts such addresses.
> > > The pfn would be wrong eventually because of the virt-to-phys offsetting
> > > and pfn_valid() false but we rely on this rather than checking it's a
> > > kernel address. Slight concern as this macro is called from drivers.
> > > 
> > > Should we keep the PAGE_OFFSET check as well?
> > 
> > In virt_addr_valid() or __is_lm_address()?
> > 
> > To be honest with you, I'm not even sure what virt_addr_valid() is supposed
> > to do with non-linear kernel addresses: look at powerpc and riscv, who
> > appear to convert the address straight to a pfn. Many callers check against
> > is_vmalloc_addr() first, but not all of them.
> 
> Even if they call is_vmalloc_addr(), it would return false for user
> address. Anyway, at a quick look, I couldn't find any virt_addr_valid()
> where it would be an issue.

Sure, but my point was more that it would be crazy to have a macro that
accepted user and linear addresses, but not vmalloc addresses!

> > I think passing in a *user* address would be a huge bug in the caller,
> > just like it would be if you called virt_to_phys() on a user address.
> > If we care about that, then I think __is_lm_address() should be the one
> > doing the check against PAGE_OFFSET.
> > 
> > Thoughts? I'd be inclined to leave this patch as it is.
> 
> Leave it as it is. The way pfn_valid() is written it wouldn't return
> true for a user address due to the fact that virt_to_phys() cannot
> return the same physical address for a user and linear map one.
> 
> For this patch:
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Ta,

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel