linux-kernel - Re: [PATCH 2/3] GHES: Move memory initialisation to ghes

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: <20170814192256.56zoo5hgbyake25b@pd.tnic>
Date:   Mon, 14 Aug 2017 21:22:56 +0200
From:   Borislav Petkov <bp@...e.de>
To:     Punit Agrawal <punit.agrawal@....com>
Cc:     linux-acpi@...r.kernel.org, lorenzo.pieralisi@....com,
        sudeep.holla@....com, linux-kernel@...r.kernel.org,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Punit Agrawal <punit.agrwal@....com>,
        James Morse <james.morse@....com>
Subject: Re: [PATCH 2/3] GHES: Move memory initialisation to ghes_probe()

On Tue, Aug 01, 2017 at 02:36:07PM +0100, Punit Agrawal wrote:
> During the driver init, the ghes driver initialises some data structures

Let's stick to "GHES" in capital letters everywhere.

> which are only used if a GHES platform device is probed. Similarly, the
> init function also checks for support for firmware first mode.
> 
> Create a function, ghes_common_init(), that performs the initialisations
> and checks that are currently performed on driver init. The function is
> called when the GHES device is probed.
> 
> Delaying initialisation and checks until probe has the added benefit of
> reducing driver prints on systems that do not support ACPI APEI.
> 
> Signed-off-by: Punit Agrawal <punit.agrwal@....com>
> Cc: Borislav Petkov <bp@...e.de>
> Cc: James Morse <james.morse@....com>
> ---
>  drivers/acpi/apei/ghes.c | 77 +++++++++++++++++++++++++++---------------------
>  1 file changed, 43 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 007b38abcb34..befb18338acb 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -1088,12 +1088,53 @@ static inline void ghes_nmi_init_cxt(void)
>  }
>  #endif /* CONFIG_HAVE_ACPI_APEI_NMI */
>  
> +static int ghes_common_init(void)
> +{
> +	int rc;
> +	static bool initialised;
> +
> +	if (initialised)
> +		return 0;

Can't say that I like it. Especially for this "initialised" thing, which
practically says that this code doesn't conceptually belong here because
we have to explicitly force it to execute it only once. Even though we
have execute-once path in ghes_init().

What would be much better IMHO is if ghes_init() checked for some APEI
table which is missing on your system and exited early. That would save
us all the trouble and solve the situation properly ...

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--