[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20231012170548.o5vi4kgpvpjyld7s@treble>
Date: Thu, 12 Oct 2023 10:05:48 -0700
From: Josh Poimboeuf <jpoimboe@...nel.org>
To: Breno Leitao <leitao@...ian.org>
Cc: Ingo Molnar <mingo@...nel.org>, tglx@...utronix.de, bp@...en8.de,
Ingo Molnar <mingo@...hat.com>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
"H. Peter Anvin" <hpa@...or.com>,
Peter Zijlstra <peterz@...radead.org>,
Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>, leit@...a.com,
"open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation
On Thu, Oct 12, 2023 at 06:02:41AM -0700, Breno Leitao wrote:
> > Yeah, so this #ifdeffery is unnecessarily ugly - we can actually assign
> > integer values in the Kconfig language and use that for initialization.
> >
> > Is there a reason why we wouldn't want to do something like:
> >
> > static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init = CONFIG_BOOT_DEFAULT_X86_MITIGATE_RETBLEED;
> >
> > ... or so?
>
> Yes. There are two reasons rigth now:
>
> 1) How to avoid the "undefined" behaviour when
> CONFIG_BOOT_DEFAULT_X86_MITIGATE_RETBLEED is not defined ? Something as:
>
> error: ‘CONFIG_BOOT_DEFAULT_X86_MITIGATE_RETBLEED’ undeclared (first use in this function)
>
> 2) Right now, these _cmd values are all different by default. Here are a few
> examples when the kernel is compiled with the mitigations:
>
> retbleed_cmd = RETBLEED_CMD_AUTO (1)
> spectre_v2_mitigation_cmd = SPECTRE_V2_CMD_AUTO (1)
> ssb_mitigation_cmd = SPEC_STORE_BYPASS_CMD_AUTO (1)
> l1tf_mitigation = L1TF_MITIGATION_FLUSH(2)
> mds_mitigation = MDS_MITIGATION_FULL(1)
> taa_mitigation = TAA_MITIGATION_VERW (2)
> mmio_mitigation = MMIO_MITIGATION_VERW (2)
> gds_mitigation = GDS_MITIGATION_FULL (3)
>
> If there is a solution for 1, then I _think_ we can probably reorder the
> enums, so, the "AUTO" value is always 1?!
I'd rather avoid hard-coding enums as that adds fragility into the mix.
Another way to avoid ifdeffery:
static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
IS_ENABLED(CONFIG_MITIGATION_RETBLEED) ? RETBLEED_CMD_AUTO : RETBLEED_CMD_OFF;
> > 3)
> >
> > And yes, now that the rush of CPU vulnerabilities seems to be ebbing, we
> > should probably consider unifying the existing hodgepodge of mitigation
> > Kconfig options as well, to not build up even more technical debt.
>
> What do you mean by unifying the existing hodgepodge of mitigation
> Kconfigs? If you are implying to just have fewer config options, I think
> that is the opposite of what Linus has suggested previously:
>
> https://lore.kernel.org/all/CAHk-=wjTHeQjsqtHcBGvy9TaJQ5uAm5HrCDuOD9v7qA9U1Xr4w@mail.gmail.com/
I read that as Ingo agreeing with me that we should rename all the
existing options for consistency.
> > 4)
> >
> > Fourth, I think we should inform users (in the boot log) when a kernel
> > .config changes a mitigation default value compared from what the upstream
> > kernel thinks is a suitable default.
> >
> > Sometimes it can be a simple configuration mistake, or a user might have
> > different opinion about the importance of a particular mitigation. Nothing
> > heavy-handed, just a simple pr_info() table of changes?
>
> That could be done, but, right now messages are printed in regard to the
> mitigations. Aren't these enough?
>
> Here are some examples:
>
> pr_info("MDS: %s\n", mds_strings[mds_mitigation]);
> pr_info("TAA: %s\n", taa_strings[taa_mitigation]);
> pr_info("MMIO Stale Data: %s\n", mmio_strings[mmio_mitigation]);
> pr_info("MMIO Stale Data: Unknown: No mitigations\n");
> pr_info("%s\n", srbds_strings[srbds_mitigation]);
> pr_info("%s\n", gds_strings[gds_mitigation]);
> pr_info("%s\n", spectre_v1_strings[spectre_v1_mitigation]);
> pr_info("%s\n", spectre_v2_user_strings[mode]);
> pr_info("%s\n", retbleed_strings[retbleed_mitigation]);
> pr_info("%s\n", ssb_strings[ssb_mode]);
But notice many/most of those functions exit early if the mitigation is
turned off, thereby skipping the pr_info(). It might be a matter of
just tweaking the print behavior and making it consistent across all the
mitigations.
--
Josh
Powered by blists - more mailing lists