Messages in this thread | | | Date | Thu, 12 Oct 2023 10:05:48 -0700 | From | Josh Poimboeuf <> | 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
| |