Re: [PATCH 2/2] arm64: Define PAGE_OFFSET using GENMASK_ULL

From: Yury Norov <ynorov@caviumnetworks.com>
To: Matthias Kaehlcke <mka@chromium.org>
Cc: zijun_hu <zijun_hu@htc.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Laura Abbott <labbott@redhat.com>,
	Nick Desaulniers <ndesaulniers@google.com>,
	linux-kernel@vger.kernel.org,
	Greg Hackmann <ghackmann@google.com>,
	Doug Anderson <dianders@chromium.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/2] arm64: Define PAGE_OFFSET using GENMASK_ULL
Date: Thu, 3 Aug 2017 16:20:35 +0300	[thread overview]
Message-ID: <20170803132035.gixd7m7oxtehpquu@yury-thinkpad> (raw)
In-Reply-To: <20170802225159.159536-2-mka@chromium.org>

Hi Matthias,

On Wed, Aug 02, 2017 at 03:51:59PM -0700, Matthias Kaehlcke wrote:
> As is the definition causes an integer overflow, which is expected,
> however clang raises the following warning:
> 
> arch/arm64/kernel/head.S:47:8: warning:
>       integer overflow in preprocessor expression
>   #elif (PAGE_OFFSET & 0x1fffff) != 0
>          ^~~~~~~~~~~
> arch/arm64/include/asm/memory.h:52:46: note:
>       expanded from macro 'PAGE_OFFSET'
>   #define PAGE_OFFSET             (UL(0xffffffffffffffff) << (VA_BITS - 1))
>                                       ~~~~~~~~~~~~~~~~~~  ^  ~~~~~~~~~~~~~
> 
> Use GENMASK_ULL() instead of shifting explicitly, the macro takes care
> of avoiding the overflow.
> 
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  arch/arm64/include/asm/memory.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index 32f82723338a..732d4eed8edd 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -65,7 +65,7 @@
>   */
>  #define VA_BITS			(CONFIG_ARM64_VA_BITS)
>  #define VA_START		(UL(0xffffffffffffffff) << VA_BITS)

IIUC VA_START should also produce warnings and should be also reworked.

> -#define PAGE_OFFSET		(UL(0xffffffffffffffff) << (VA_BITS - 1))
> +#define PAGE_OFFSET		GENMASK_ULL(BITS_PER_LONG_LONG - 1, VA_BITS - 1)

The original type of PAGE_OFFSET is UL, and after your patch becomes ULL.
This is the same for arm64. But it would be less questionable if you
will specify it explicitly, or use GENMASK() instead of GENMASK_ULL().

>  #define KIMAGE_VADDR		(MODULES_END)
>  #define MODULES_END		(MODULES_VADDR + MODULES_VSIZE)
>  #define MODULES_VADDR		(VA_START + KASAN_SHADOW_SIZE)
> -- 
> 2.14.0.rc1.383.gd1ce394fe2-goog
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel