Re: [PATCH v5 00/16] Attack vector controls (part 1)

[Ingo Molnar <mingo@kernel.org>]

* Borislav Petkov <bp@alien8.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