linux-kernel - Re: [PATCH 3/6] arm64/vdso: Add time napespace page

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: <20200416104527.GD4987@lakrids.cambridge.arm.com>
Date:   Thu, 16 Apr 2020 11:45:27 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Andrei Vagin <avagin@...il.com>
Cc:     Vincenzo Frascino <vincenzo.frascino@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Dmitry Safonov <dima@...sta.com>
Subject: Re: [PATCH 3/6] arm64/vdso: Add time napespace page

Hi Andrei,

On Wed, Apr 15, 2020 at 10:26:15PM -0700, Andrei Vagin wrote:
> diff --git a/arch/arm64/include/asm/vdso.h b/arch/arm64/include/asm/vdso.h
> index 07468428fd29..351c145d3808 100644
> --- a/arch/arm64/include/asm/vdso.h
> +++ b/arch/arm64/include/asm/vdso.h
> @@ -12,6 +12,12 @@
>   */
>  #define VDSO_LBASE	0x0
>  
> +#ifdef CONFIG_TIME_NS
> +#define __VVAR_PAGES    2
> +#else
> +#define __VVAR_PAGES    1
> +#endif
> +
>  #ifndef __ASSEMBLY__
  
> +#ifdef CONFIG_TIME_NS
> +static __always_inline const struct vdso_data *__arch_get_timens_vdso_data(void)
> +{
> +	const struct vdso_data *ret;
> +
> +	ret = _timens_data;
> +	OPTIMIZER_HIDE_VAR(ret);
> +
> +	return ret;
> +}
> +#endif

Sorry for the confusion here, but please either:

* Add a preparatory patch making __arch_get_vdso_data() use
  OPTIMIZER_HIDE_VAR(), and use OPTIMIZER_HIDE_VAR() here.

* Use the same assembly as __arch_get_vdso_data() currently does.

... and either way add a comment here:

	/* See __arch_get_vdso_data() */

... so taht the rationale is obvious.

[...]

> +enum vvar_pages {
> +	VVAR_DATA_PAGE_OFFSET = 0,
> +#ifdef CONFIG_TIME_NS
> +	VVAR_TIMENS_PAGE_OFFSET = 1,
> +#endif /* CONFIG_TIME_NS */
> +	VVAR_NR_PAGES = __VVAR_PAGES,
> +};

Pet peeve, but we don't need the initializers here, as enums start from
zero. The last element shouldn't have a trailing comma as we don't
expect to add elements after it in future.

Rather than assigning to VVAR_NR_PAGES, it'd be better to use a
BUILD_BUG_ON() to verify that it is the number we expect:

enum vvar_pages {
	VVAR_DATA_PAGE,
#ifdef CONFIG_TIME_NS
	VVAR_TIMENS_PAGE,
#endif
	VVAR_NR_PAGES
};

BUILD_BUG_ON(VVAR_NR_PAGES != __VVAR_PAGES);

Thanks,
Mark.