linux-kernel - Re: [PATCH v2 4/8] vdso: Introduce vdso/page.h

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <f8256ade-c17f-46d1-bd4a-4d01235be5a0@app.fastmail.com>
Date: Mon, 23 Sep 2024 16:54:22 +0000
From: "Arnd Bergmann" <arnd@...db.de>
To: "Vincenzo Frascino" <vincenzo.frascino@....com>,
 linux-kernel@...r.kernel.org, Linux-Arch <linux-arch@...r.kernel.org>,
 linux-mm@...ck.org
Cc: "Andy Lutomirski" <luto@...nel.org>,
 "Thomas Gleixner" <tglx@...utronix.de>,
 "Jason A . Donenfeld" <Jason@...c4.com>,
 "Christophe Leroy" <christophe.leroy@...roup.eu>,
 "Michael Ellerman" <mpe@...erman.id.au>,
 "Nicholas Piggin" <npiggin@...il.com>, "Naveen N Rao" <naveen@...nel.org>,
 "Ingo Molnar" <mingo@...hat.com>, "Borislav Petkov" <bp@...en8.de>,
 "Dave Hansen" <dave.hansen@...ux.intel.com>,
 "H. Peter Anvin" <hpa@...or.com>, "Theodore Ts'o" <tytso@....edu>,
 "Andrew Morton" <akpm@...ux-foundation.org>,
 "Steven Rostedt" <rostedt@...dmis.org>,
 "Masami Hiramatsu" <mhiramat@...nel.org>,
 "Mathieu Desnoyers" <mathieu.desnoyers@...icios.com>
Subject: Re: [PATCH v2 4/8] vdso: Introduce vdso/page.h

On Mon, Sep 23, 2024, at 14:19, Vincenzo Frascino wrote:
> The VDSO implementation includes headers from outside of the
> vdso/ namespace.
>
> Introduce vdso/page.h to make sure that the generic library
> uses only the allowed namespace.
>
> Cc: Andy Lutomirski <luto@...nel.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Jason A. Donenfeld <Jason@...c4.com>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@....com>

Thanks for the new version. This looks all good, just some
very minor ideas for how to possibly improve the new version:

> +/* PAGE_SHIFT determines the page size */
> +#define PAGE_SHIFT      CONFIG_PAGE_SHIFT
> +
> +#define PAGE_SIZE	(_AC(1,UL) << PAGE_SHIFT)
> +
> +#if defined(CONFIG_PHYS_ADDR_T_64BIT) && !defined(CONFIG_X86_64)
> +#define PAGE_MASK	(~((1 << PAGE_SHIFT) - 1))
> +#else
> +#define PAGE_MASK	(~(PAGE_SIZE-1))
> +#endif

I would open-code the CONFIG_PAGE_SHIFT in PAGE_SIZE
and PAGE_MASK, just to avoid the extra indirection in the
preprocessor. This mainly has the benefit of slightly
shorter compiler warnings when all the macros get
traced back but can also slightly improve compile speed
in case this is used in deeply nested macros.

Without a comment, the special case for CONFIG_X86_64
not very clear, and probably not needed. If you are
worried about introducing an architecture specific
regression, I would suggest instead explaining the
possible issue in the patch description but using the
more generic and simpler #ifdef check.

      Arnd