| 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
| ||
|
Message-ID: <aAdlefbyU_oqOVIg@gmail.com>
Date: Tue, 22 Apr 2025 11:46:33 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Borislav Petkov <bp@...en8.de>
Cc: David Kaplan <david.kaplan@....com>,
Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
Josh Poimboeuf <jpoimboe@...nel.org>,
Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>,
Ingo Molnar <mingo@...hat.com>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
"H . Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 00/16] Attack vector controls (part 1)
* Borislav Petkov <bp@...en8.de> wrote:
> On Fri, Apr 18, 2025 at 10:03:42PM +0200, Ingo Molnar wrote:
> > /* Select the proper CPU mitigations before patching alternatives: */
> > mitigation_select_spectre_v1();
> > mitigation_select_spectre_v2();
> > mitigation_select_retbleed();
> > mitigation_select_spectre_v2_user();
> > mitigation_select_ssb();
> > mitigation_select_l1tf();
> > mitigation_select_mds();
> > mitigation_update_taa();
> > mitigation_select_taa();
> > mitigation_select_mmio();
> > mitigation_select_rfds();
> > mitigation_select_srbds();
> > mitigation_select_l1d_flush();
> > mitigation_select_srso();
> > mitigation_select_gds();
> > mitigation_select_bhi();
>
> The bad side of that is that you have a whole set of letters
> - "mitigation_select" - before the *actual* name which is the only thing one
> is interested in. With the vectors, one is now interested in the operation too
> - select, update or apply.
I have three counter-arguments:
1)
The above pattern is not a big problem really, as the human brain has
no trouble ignoring well-structured syntactic repetitions on the left
side and will focus on the right side column.
Unlike the current status quo, which your reply didn't quote, so I'll
quote it again:
static void __init spectre_v1_select_mitigation(void);
static void __init spectre_v2_select_mitigation(void);
static void __init retbleed_select_mitigation(void);
static void __init spectre_v2_user_select_mitigation(void);
static void __init ssb_select_mitigation(void);
static void __init l1tf_select_mitigation(void);
static void __init mds_select_mitigation(void);
static void __init md_clear_update_mitigation(void);
static void __init md_clear_select_mitigation(void);
static void __init taa_select_mitigation(void);
static void __init mmio_select_mitigation(void);
static void __init srbds_select_mitigation(void);
static void __init l1d_flush_select_mitigation(void);
static void __init srso_select_mitigation(void);
static void __init gds_select_mitigation(void);
Which is far more visually messy.
2)
As to shortening the function names to reduce (but not eliminate ...)
the mess:
That extra mitigation_ prefix or _mitigation postfix might sound like
repetitious when used in mass-calls like above - but it's really useful
when reading the actual definitions:
> Short, sweet,
... there's such a thing as too short, too sweet, too ambiguous, which
is why we ended up with the current name of gds_select_mitigation() et
al to begin with.
But let's see an example. Consider:
static void __init gds_select(void)
{
u64 mcu_ctrl;
What do I select? Some GDS detail? Or the main mitigation itself?
Nothing really tells me.
While with:
static void __init mitigation_select_gds(void)
{
u64 mcu_ctrl;
It's immediately clear that this is the main function that selects the
GDS mitigation.
3)
A proper namespace also makes it *much* easier to grep for specific
primitives.
With your suggested 'gds_select()' naming, if I want to search for all
gds_ primitives, I get:
starship:~/tip> git grep gds_ arch/x86/kernel/cpu/bugs.c
arch/x86/kernel/cpu/bugs.c:static void __init gds_select_mitigation(void);
arch/x86/kernel/cpu/bugs.c: gds_select_mitigation();
arch/x86/kernel/cpu/bugs.c:enum gds_mitigations {
arch/x86/kernel/cpu/bugs.c:static enum gds_mitigations gds_mitigation __ro_after_init =
arch/x86/kernel/cpu/bugs.c:static const char * const gds_strings[] = {
arch/x86/kernel/cpu/bugs.c:bool gds_ucode_mitigated(void)
arch/x86/kernel/cpu/bugs.c: return (gds_mitigation == GDS_MITIGATION_FULL ||
arch/x86/kernel/cpu/bugs.c: gds_mitigation == GDS_MITIGATION_FULL_LOCKED);
arch/x86/kernel/cpu/bugs.c:EXPORT_SYMBOL_GPL(gds_ucode_mitigated);
arch/x86/kernel/cpu/bugs.c:void update_gds_msr(void)
arch/x86/kernel/cpu/bugs.c: switch (gds_mitigation) {
arch/x86/kernel/cpu/bugs.c:static void __init gds_select_mitigation(void)
arch/x86/kernel/cpu/bugs.c: gds_mitigation = GDS_MITIGATION_HYPERVISOR;
arch/x86/kernel/cpu/bugs.c: gds_mitigation = GDS_MITIGATION_OFF;
arch/x86/kernel/cpu/bugs.c: if (gds_mitigation == GDS_MITIGATION_FORCE) {
arch/x86/kernel/cpu/bugs.c: * here rather than in update_gds_msr()
arch/x86/kernel/cpu/bugs.c: gds_mitigation = GDS_MITIGATION_UCODE_NEEDED;
arch/x86/kernel/cpu/bugs.c: if (gds_mitigation == GDS_MITIGATION_FORCE)
arch/x86/kernel/cpu/bugs.c: gds_mitigation = GDS_MITIGATION_FULL;
arch/x86/kernel/cpu/bugs.c: if (gds_mitigation == GDS_MITIGATION_OFF)
arch/x86/kernel/cpu/bugs.c: * but others are then update_gds_msr() will WARN() of the state
arch/x86/kernel/cpu/bugs.c: * mismatch. If the boot CPU is locked update_gds_msr() will
arch/x86/kernel/cpu/bugs.c: gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
arch/x86/kernel/cpu/bugs.c: update_gds_msr();
arch/x86/kernel/cpu/bugs.c: pr_info("%s\n", gds_strings[gds_mitigation]);
arch/x86/kernel/cpu/bugs.c:static int __init gds_parse_cmdline(char *str)
arch/x86/kernel/cpu/bugs.c: gds_mitigation = GDS_MITIGATION_OFF;
arch/x86/kernel/cpu/bugs.c: gds_mitigation = GDS_MITIGATION_FORCE;
arch/x86/kernel/cpu/bugs.c:early_param("gather_data_sampling", gds_parse_cmdline);
arch/x86/kernel/cpu/bugs.c:static ssize_t gds_show_state(char *buf)
arch/x86/kernel/cpu/bugs.c: return sysfs_emit(buf, "%s\n", gds_strings[gds_mitigation]);
arch/x86/kernel/cpu/bugs.c: return gds_show_state(buf);
Or, if I limit this to function calls only:
arch/x86/kernel/cpu/bugs.c:static void __init gds_select_mitigation(void);
arch/x86/kernel/cpu/bugs.c: gds_select_mitigation();
arch/x86/kernel/cpu/bugs.c:bool gds_ucode_mitigated(void)
arch/x86/kernel/cpu/bugs.c:void update_gds_msr(void)
arch/x86/kernel/cpu/bugs.c:static void __init gds_select_mitigation(void)
arch/x86/kernel/cpu/bugs.c: * here rather than in update_gds_msr()
arch/x86/kernel/cpu/bugs.c: * but others are then update_gds_msr() will WARN() of the state
arch/x86/kernel/cpu/bugs.c: * mismatch. If the boot CPU is locked update_gds_msr() will
arch/x86/kernel/cpu/bugs.c: update_gds_msr();
arch/x86/kernel/cpu/bugs.c:static int __init gds_parse_cmdline(char *str)
arch/x86/kernel/cpu/bugs.c:static ssize_t gds_show_state(char *buf)
arch/x86/kernel/cpu/bugs.c: return gds_show_state(buf);
Versus a targeted, obvious, untuitive search for all mitigation_
functions related to GDS:
starship:~/tip> git grep -E 'mitigation_.*_gds' arch/x86/kernel/cpu/bugs.c
arch/x86/kernel/cpu/bugs.c:static void __init mitigation_select_gds(void);
arch/x86/kernel/cpu/bugs.c: mitigation_select_gds();
arch/x86/kernel/cpu/bugs.c:static void __init mitigation_select_gds(void)
Because a proper namespace is a far more easier to search.
Hierarchical lexicographic organization of function names is basically
a code organization 101 concept, and I didn't think it would be
particularly controversial. :-)
Thanks,
Ingo
Powered by blists - more mailing lists