linux-kernel - Re: [PATCH] x86/split_lock: Zap the unwieldy switch-case in sld_state

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20260108103850.GAaV-JOgVZNHuZIkBh@fat_crate.local>
Date: Thu, 8 Jan 2026 11:38:50 +0100
From: Borislav Petkov <bp@...en8.de>
To: "Luck, Tony" <tony.luck@...el.com>
Cc: "Li, Xiaoyao" <xiaoyao.li@...el.com>, Rong Zhang <i@...g.moe>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	"x86@...nel.org" <x86@...nel.org>, "H. Peter Anvin" <hpa@...or.com>,
	Fenghua Yu <fenghuay@...dia.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/split_lock: Zap the unwieldy switch-case in
 sld_state_show()

On Wed, Jan 07, 2026 at 04:41:22PM +0000, Luck, Tony wrote:
> It matters because the mode is shared by both hyper-threads, and you may
> only offline one of them.
> 
> The offline handler isn't needed in the fatal case because split lock is
> always enabled. So the "Unconditionally re-enable detection here."
> won't change anything.

/me goes and reads

b041b525dab9 ("x86/split_lock: Make life miserable for split lockers")

Oh wow, that's a fancy dance there :)
 
> That said, it wouldn't break anything to run that. So if it makes the setup
> code easier to read, it's OK to do this. But should have a comment to
> describe what it going on.

Right, exactly.

My goal is to have it be more readable by dialing down on the repetitive
stuff.

IOW, this (pasting the whole function):

static void sld_state_show(void)
{
        const char *action = "warning";
 
        if (!boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT) &&
            !boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
                return;
 
        if (sld_state == sld_off) {
                pr_info("disabled\n");
                return;
        } else if (sld_state == sld_ratelimit) {
                if (boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT))
                        pr_info("#DB: setting system wide bus lock rate limit to %u/sec\n", bld_ratelimit.burst);
                return;
        }
 
        if (sld_state == sld_fatal)
                action = "sending SIGBUS";
 
        if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {
                pr_info("#AC: crashing the kernel on kernel split_locks and %s on user-space split_locks\n", action);
 
                /*
                 * This is handling the case where a CPU goes offline at the
                 * moment where split lock detection is disabled in the warn
                 * setting, see split_lock_warn(). It doesn't have any effect
                 * in the fatal case.
                 */
                if (cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
                                      "x86/splitlock", NULL, splitlock_cpu_offline) < 0)
                        pr_warn("No splitlock CPU offline handler\n");
        } else if (boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT)) {
                pr_info("#DB: %s on user-space bus_locks\n", action);
        }
}

Btw, looking some more, that MSR writing function could save a write if not
necessary:

diff --git a/arch/x86/kernel/cpu/bus_lock.c b/arch/x86/kernel/cpu/bus_lock.c
index 811f87906c1e..bdf518d28310 100644
--- a/arch/x86/kernel/cpu/bus_lock.c
+++ b/arch/x86/kernel/cpu/bus_lock.c
@@ -164,7 +164,10 @@ static void sld_update_msr(bool on)
        if (on)
                test_ctrl_val |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
 
-       wrmsrq(MSR_TEST_CTRL, test_ctrl_val);
+       if (test_ctrl_val != msr_test_ctrl_cache) {
+               wrmsrq(MSR_TEST_CTRL, test_ctrl_val);
+               msr_test_ctrl_cache = test_ctrl_val;
+       }
 }
 
 void split_lock_init(void)

---

but maybe that's not important as slow path...

Also, from the looks of it, that cached value could be dropped in favor of
using msr_{set,clear}_bit(). But maybe something for another day...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette