linux-kernel - RE: [PATCH v2 5/7] EDAC/ghes: Prevent chipset-specific edac from loading after 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]

Date:   Fri, 19 Aug 2022 10:34:43 +0000
From:   Justin He <Justin.He@....com>
To:     "Kani, Toshi" <toshi.kani@....com>
CC:     "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
        "devel@...ica.org" <devel@...ica.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Shuai Xue <xueshuai@...ux.alibaba.com>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        "linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
        nd <nd@....com>, Ard Biesheuvel <ardb@...nel.org>,
        Len Brown <lenb@...nel.org>, James Morse <James.Morse@....com>,
        Tony Luck <tony.luck@...el.com>,
        Borislav Petkov <bp@...en8.de>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Robert Richter <rric@...nel.org>,
        Robert Moore <robert.moore@...el.com>,
        Qiuxu Zhuo <qiuxu.zhuo@...el.com>,
        Yazen Ghannam <yazen.ghannam@....com>
Subject: RE: [PATCH v2 5/7] EDAC/ghes: Prevent chipset-specific edac from
 loading after ghes_edac is registered

Hi Toshi,

> -----Original Message-----
> From: Kani, Toshi <toshi.kani@....com>
> Sent: Friday, August 19, 2022 9:30 AM
> To: Justin He <Justin.He@....com>; Ard Biesheuvel <ardb@...nel.org>; Len
> Brown <lenb@...nel.org>; James Morse <James.Morse@....com>; Tony Luck
> <tony.luck@...el.com>; Borislav Petkov <bp@...en8.de>; Mauro Carvalho
> Chehab <mchehab@...nel.org>; Robert Richter <rric@...nel.org>; Robert
> Moore <robert.moore@...el.com>; Qiuxu Zhuo <qiuxu.zhuo@...el.com>;
> Yazen Ghannam <yazen.ghannam@....com>
> Cc: linux-acpi@...r.kernel.org; linux-kernel@...r.kernel.org;
> linux-edac@...r.kernel.org; devel@...ica.org; Rafael J . Wysocki
> <rafael@...nel.org>; Shuai Xue <xueshuai@...ux.alibaba.com>; Jarkko
> Sakkinen <jarkko@...nel.org>; linux-efi@...r.kernel.org; nd <nd@....com>
> Subject: RE: [PATCH v2 5/7] EDAC/ghes: Prevent chipset-specific edac from
> loading after ghes_edac is registered
> 
> On Wednesday, August 17, 2022 8:35 AM, Jia He wrote:
> > Previous, there is only one edac can be registering in the EDAC core.
> > After ghes_edac is modularized, the registering of ghes devices may be
> > deferred when ghes_edac is loaded. Prevent the chipset-specific edac
> > drivers from loading after ghes_edac is registered, which allow the
> > edac drivers to get selected in e.g. HPE platform.
> ...
> > @@ -1382,6 +1395,7 @@ static int ghes_probe(struct platform_device
> > *ghes_dev)
> >  	platform_set_drvdata(ghes_dev, ghes);
> >
> >  	ghes->dev = &ghes_dev->dev;
> > +	set_ghes_devs_registered(false);
> 
> This does not look right to me.
> 
> The condition of using ghes_edac is (ghes-present) && (ghes-preferred),
> where:
>  - ghes-present is latched on ghes_probe()
>  - ghes-preferred is true if the platform-check passes.
> 
> ghes_get_device() introduced in the previous patch works as the
> ghes-preferred check.
> 
> We cannot use ghes_edac registration as the ghes-present check in this
> scheme since it is deferred to module_init().

What is the logic for ghes-present check? In this patch, I assumed it is equal to 
"ghes_edac devices have been registered". Seems it is not correct.
But should we consider the case as follows:
What if sbridge_init () is invoked before ghes_edac_init()? i.e. Should we get
sb_edac driver selected when ghes_edac is not loaded yet (e.g. on HPE)?

> 
> I'd suggest the following changes:
> - Update ghes_get_device() to check a flag latched on ghes_probe().

Still need your elaborating about the details of the flag. i.e. When is this flag
latched? When is it unlatched?

> - Remove 'force' argument from ghes_get_device().  ghes_edac_init() is too
> late to set this flag.  Also, other edac drivers should not be able to control this
> flag.  I think this force_load kernel option needs to belong to ghes with this
> scheme.
Agree, I will remove force_load in ghes_get_device(), and put all check/update of
force_load in ghes

> - ghes_get_device() exposes 'ghes_devs' to chipset-specific edac drivers, which
> should be internal to ghes.  ghes_get_device() may be renamed to something
> like ghes_edac_preferred() which returns true / false.
> 

Okay, agree

--
Cheers,
Justin (Jia He)