Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Thu, 12 Oct 2023 06:02:41 -0700
From:   Breno Leitao <leitao@debian.org>
To:     Ingo Molnar <mingo@kernel.org>
Cc:     tglx@linutronix.de, bp@alien8.de, Ingo Molnar <mingo@redhat.com>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Josh Poimboeuf <jpoimboe@kernel.org>,
        Pawan Gupta <pawan.kumar.gupta@linux.intel.com>, leit@meta.com,
        "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" 
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4] x86/bugs: Add a separate config for each mitigation
Message-ID: <ZSfucR6docdnLznb@gmail.com>
References: <20231010103028.4192223-1-leitao@debian.org>
 <ZSca08rnmZfkONEH@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZSca08rnmZfkONEH@gmail.com>
Precedence: bulk

Hello Ingo,

On Wed, Oct 11, 2023 at 11:59:47PM +0200, Ingo Molnar wrote:
> 
> * Breno Leitao <leitao@debian.org> wrote:

> > +#if IS_ENABLED(CONFIG_MITIGATE_RETBLEED)
> >  static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
> >  	RETBLEED_CMD_AUTO;
> > +#else
> > +static enum retbleed_mitigation_cmd retbleed_cmd __ro_after_init =
> > +	RETBLEED_CMD_OFF;
> > +#endif
> 
> 1)
> 
> 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?!


> 2)
> 
> The new Kconfig namespace should probably be X86_MITIGATE_*, so that we 
> don't crowd the generic kernel's Kconfig namespace.

There is another sub-thread under this patchset that is discussing
naming as well. Boris wants to keep it as simple as it is today:

https://lore.kernel.org/all/20231011190317.GDZSbxdd5TuCIp5+JN@fat_crate.local/

> 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/

> 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]);


Thanks for the review!