[V3] x86/split_lock: Add sysctl to control the misery mode
Commit Message
Commit b041b525dab9 ("x86/split_lock: Make life miserable for split lockers")
changed the way the split lock detector works when in "warn" mode;
basically, not only it shows the warn message, but also intentionally
introduces a slowdown (through sleeping plus serialization mechanism)
on such task. Based on discussions in [0], seems the warning alone
wasn't enough motivation for userspace developers to fix their
applications.
Happens that originally the proposal in [0] was to add a new mode
which would warns + slowdown the "split locking" task, keeping the
old warn mode untouched. In the end, that idea was discarded and
the regular/default "warn" mode now slowdowns the applications. This
is quite aggressive with regards proprietary/legacy programs that
basically are unable to properly run in kernel with this change.
While it is understandable that a malicious application could DoS
by split locking, it seems unacceptable to regress old/proprietary
userspace programs through a default configuration that previously
worked. An example of such breakage was reported in [1].
So let's add a sysctl to allow controlling the "misery mode" behavior,
as per Thomas suggestion on [2]. This way, users running legacy and/or
proprietary software are allowed to still execute them with a decent
performance while still observe the warning messages on kernel log.
[0] https://lore.kernel.org/lkml/20220217012721.9694-1-tony.luck@intel.com/
[1] https://github.com/doitsujin/dxvk/issues/2938
[2] https://lore.kernel.org/lkml/87pmf4bter.ffs@tglx/
Fixes: b041b525dab9 ("x86/split_lock: Make life miserable for split lockers")
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Paul Gofman <pgofman@codeweavers.com>
Cc: Pavel Machek <pavel@denx.de>
Cc: Pierre-Loup Griffais <pgriffais@valvesoftware.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Zebediah Figura <zfigura@codeweavers.com>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Andre Almeida <andrealmeid@igalia.com>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
V3:
- Reworked with the suggestions from Bagas and Dave - thanks!
- Rebased to v6.1-rc2.
Dave, in the end I kept the 2 delayed workers, seems easier than
encompass the worker in a new struct and have a bool embedded there
(we can't use data directly in the work struct, since it keeps the
workqueue state). Lemme know if you (or others) really prefer the
other approach and I can rework if required.
Thanks again for comments/reviews!
Cheers,
Guilherme
Documentation/admin-guide/sysctl/kernel.rst | 23 ++++++++
arch/x86/kernel/cpu/intel.c | 63 +++++++++++++++++----
2 files changed, 76 insertions(+), 10 deletions(-)
Comments
On Mon, Oct 24, 2022 at 05:02:54PM -0300, Guilherme G. Piccoli wrote:
> +split_lock_mitigate (x86 only)
> +==============================
> +
> +On x86, each "split lock" imposes a system-wide performance penalty. On larger
> +systems, large numbers of split locks from unprivileged users can result in
> +denials of service to well-behaved and potentially more important users.
> +
DoS which harms legitimate users?
> +The kernel mitigates these bad users by detecting split locks and imposing
> +penalties: forcing them to wait and only allowing one core to execute split
> +locks at a time.
> +
> +These mitigations can make those bad applications unbearably slow. Setting
> +split_lock_mitigate=0 may restore some application performance, but will also
> +increase system exposure to denial of service attacks from split lock users.
> +
> += ===================================================================
> +0 Disable the mitigation mode - just warns the split lock on kernel log
> + and exposes the system to denials of service from the split lockers.
> +1 Enable the mitigation mode (this is the default) - penalizes the split
> + lockers with intentional performance degradation.
> += ===================================================================
> +
> +
The prose can be improved:
---- >8 ----
diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
index c2c64c1b706ff6..cdc5b75adb4633 100644
--- a/Documentation/admin-guide/sysctl/kernel.rst
+++ b/Documentation/admin-guide/sysctl/kernel.rst
@@ -1319,22 +1319,21 @@ split_lock_mitigate (x86 only)
On x86, each "split lock" imposes a system-wide performance penalty. On larger
systems, large numbers of split locks from unprivileged users can result in
-denials of service to well-behaved and potentially more important users.
+denials of service attack.
-The kernel mitigates these bad users by detecting split locks and imposing
+The kernel mitigates that condition by detecting split locks and imposing
penalties: forcing them to wait and only allowing one core to execute split
locks at a time.
-These mitigations can make those bad applications unbearably slow. Setting
-split_lock_mitigate=0 may restore some application performance, but will also
-increase system exposure to denial of service attacks from split lock users.
+These mitigations can make applications which rely on split locks unbearably
+slow. Setting split_lock_mitigate=0 may restore performance, but will also
+increase likelihood of DoS caused by split locks.
-= ===================================================================
-0 Disable the mitigation mode - just warns the split lock on kernel log
- and exposes the system to denials of service from the split lockers.
-1 Enable the mitigation mode (this is the default) - penalizes the split
- lockers with intentional performance degradation.
-= ===================================================================
+= ==================================================================
+0 Disable the mitigation - just warns the split lock on kernel log.
+1 Enable the mitigation (default) - penalizes the split lockers with
+ intentional performance degradation.
+= ==================================================================
stack_erasing
Thanks.
On 10/24/22 20:46, Bagas Sanjaya wrote:
> The prose can be improved:
I'm not sure that's a universal improvement. It chops out some valuable
context.
On 10/25/22 11:10, Dave Hansen wrote:
> On 10/24/22 20:46, Bagas Sanjaya wrote:
>> The prose can be improved:
>
> I'm not sure that's a universal improvement. It chops out some valuable
> context.
Seems like you forgot to quote my suggested diff.
Other than that, is this version good enough in your opinion Dave?
Personally I think Bagas could submit a patch later and propose the
changes to the docs as well.
Cheers,
Guilherme
On 24/10/2022 17:02, Guilherme G. Piccoli wrote:
> Commit b041b525dab9 ("x86/split_lock: Make life miserable for split lockers")
> changed the way the split lock detector works when in "warn" mode;
> basically, not only it shows the warn message, but also intentionally
> introduces a slowdown (through sleeping plus serialization mechanism)
> on such task. Based on discussions in [0], seems the warning alone
> wasn't enough motivation for userspace developers to fix their
> applications.
>
> Happens that originally the proposal in [0] was to add a new mode
> which would warns + slowdown the "split locking" task, keeping the
> old warn mode untouched. In the end, that idea was discarded and
> the regular/default "warn" mode now slowdowns the applications. This
> is quite aggressive with regards proprietary/legacy programs that
> basically are unable to properly run in kernel with this change.
> While it is understandable that a malicious application could DoS
> by split locking, it seems unacceptable to regress old/proprietary
> userspace programs through a default configuration that previously
> worked. An example of such breakage was reported in [1].
>
> So let's add a sysctl to allow controlling the "misery mode" behavior,
> as per Thomas suggestion on [2]. This way, users running legacy and/or
> proprietary software are allowed to still execute them with a decent
> performance while still observe the warning messages on kernel log.
>
> [0] https://lore.kernel.org/lkml/20220217012721.9694-1-tony.luck@intel.com/
>
> [1] https://github.com/doitsujin/dxvk/issues/2938
>
> [2] https://lore.kernel.org/lkml/87pmf4bter.ffs@tglx/
>
> Fixes: b041b525dab9 ("x86/split_lock: Make life miserable for split lockers")
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Joshua Ashton <joshua@froggi.es>
> Cc: Melissa Wen <mwen@igalia.com>
> Cc: Paul Gofman <pgofman@codeweavers.com>
> Cc: Pavel Machek <pavel@denx.de>
> Cc: Pierre-Loup Griffais <pgriffais@valvesoftware.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Zebediah Figura <zfigura@codeweavers.com>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Tested-by: Andre Almeida <andrealmeid@igalia.com>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
Hi Thomas / Tony, is there anything else to improve here? Suggestions /
reviews are greatly appreciated.
In case this version is good enough, I'd like to ask if it's possible to
get it merged during the rc cycle for 6.1 (I'll also backport it to
stable 6.0), so we can have this fix available for a bigger public.
Thanks,
Guilherme
On Mon, Oct 24, 2022 at 05:02:54PM -0300, Guilherme G. Piccoli wrote:
> Commit b041b525dab9 ("x86/split_lock: Make life miserable for split lockers")
> changed the way the split lock detector works when in "warn" mode;
> basically, not only it shows the warn message, but also intentionally
> introduces a slowdown (through sleeping plus serialization mechanism)
> on such task. Based on discussions in [0], seems the warning alone
> wasn't enough motivation for userspace developers to fix their
> applications.
>
> Happens that originally the proposal in [0] was to add a new mode
> which would warns + slowdown the "split locking" task, keeping the
> old warn mode untouched. In the end, that idea was discarded and
> the regular/default "warn" mode now slowdowns the applications. This
> is quite aggressive with regards proprietary/legacy programs that
> basically are unable to properly run in kernel with this change.
> While it is understandable that a malicious application could DoS
> by split locking, it seems unacceptable to regress old/proprietary
> userspace programs through a default configuration that previously
> worked. An example of such breakage was reported in [1].
>
> So let's add a sysctl to allow controlling the "misery mode" behavior,
> as per Thomas suggestion on [2]. This way, users running legacy and/or
> proprietary software are allowed to still execute them with a decent
> performance while still observe the warning messages on kernel log.
Reviewed-by: Tony Luck <tony.luck@intel.com>
-Tony
@@ -1314,6 +1314,29 @@ watchdog work to be queued by the watchdog timer function, otherwise the NMI
watchdog — if enabled — can detect a hard lockup condition.
+split_lock_mitigate (x86 only)
+==============================
+
+On x86, each "split lock" imposes a system-wide performance penalty. On larger
+systems, large numbers of split locks from unprivileged users can result in
+denials of service to well-behaved and potentially more important users.
+
+The kernel mitigates these bad users by detecting split locks and imposing
+penalties: forcing them to wait and only allowing one core to execute split
+locks at a time.
+
+These mitigations can make those bad applications unbearably slow. Setting
+split_lock_mitigate=0 may restore some application performance, but will also
+increase system exposure to denial of service attacks from split lock users.
+
+= ===================================================================
+0 Disable the mitigation mode - just warns the split lock on kernel log
+ and exposes the system to denials of service from the split lockers.
+1 Enable the mitigation mode (this is the default) - penalizes the split
+ lockers with intentional performance degradation.
+= ===================================================================
+
+
stack_erasing
=============
@@ -1034,8 +1034,32 @@ static const struct {
static struct ratelimit_state bld_ratelimit;
+static unsigned int sysctl_sld_mitigate = 1;
static DEFINE_SEMAPHORE(buslock_sem);
+#ifdef CONFIG_PROC_SYSCTL
+static struct ctl_table sld_sysctls[] = {
+ {
+ .procname = "split_lock_mitigate",
+ .data = &sysctl_sld_mitigate,
+ .maxlen = sizeof(unsigned int),
+ .mode = 0644,
+ .proc_handler = proc_douintvec_minmax,
+ .extra1 = SYSCTL_ZERO,
+ .extra2 = SYSCTL_ONE,
+ },
+ {}
+};
+
+static int __init sld_mitigate_sysctl_init(void)
+{
+ register_sysctl_init("kernel", sld_sysctls);
+ return 0;
+}
+
+late_initcall(sld_mitigate_sysctl_init);
+#endif
+
static inline bool match_option(const char *arg, int arglen, const char *opt)
{
int len = strlen(opt), ratelimit;
@@ -1146,12 +1170,20 @@ static void split_lock_init(void)
split_lock_verify_msr(sld_state != sld_off);
}
-static void __split_lock_reenable(struct work_struct *work)
+static void __split_lock_reenable_unlock(struct work_struct *work)
{
sld_update_msr(true);
up(&buslock_sem);
}
+static DECLARE_DELAYED_WORK(sl_reenable_unlock, __split_lock_reenable_unlock);
+
+static void __split_lock_reenable(struct work_struct *work)
+{
+ sld_update_msr(true);
+}
+static DECLARE_DELAYED_WORK(sl_reenable, __split_lock_reenable);
+
/*
* If a CPU goes offline with pending delayed work to re-enable split lock
* detection then the delayed work will be executed on some other CPU. That
@@ -1169,10 +1201,9 @@ static int splitlock_cpu_offline(unsigned int cpu)
return 0;
}
-static DECLARE_DELAYED_WORK(split_lock_reenable, __split_lock_reenable);
-
static void split_lock_warn(unsigned long ip)
{
+ struct delayed_work *work;
int cpu;
if (!current->reported_split_lock)
@@ -1180,14 +1211,26 @@ static void split_lock_warn(unsigned long ip)
current->comm, current->pid, ip);
current->reported_split_lock = 1;
- /* misery factor #1, sleep 10ms before trying to execute split lock */
- if (msleep_interruptible(10) > 0)
- return;
- /* Misery factor #2, only allow one buslocked disabled core at a time */
- if (down_interruptible(&buslock_sem) == -EINTR)
- return;
+ if (sysctl_sld_mitigate) {
+ /*
+ * misery factor #1:
+ * sleep 10ms before trying to execute split lock.
+ */
+ if (msleep_interruptible(10) > 0)
+ return;
+ /*
+ * Misery factor #2:
+ * only allow one buslocked disabled core at a time.
+ */
+ if (down_interruptible(&buslock_sem) == -EINTR)
+ return;
+ work = &sl_reenable_unlock;
+ } else {
+ work = &sl_reenable;
+ }
+
cpu = get_cpu();
- schedule_delayed_work_on(cpu, &split_lock_reenable, 2);
+ schedule_delayed_work_on(cpu, work, 2);
/* Disable split lock detection on this CPU to make progress */
sld_update_msr(false);