kfence: optimize timer scheduling

The allocation wait timeout was initially added because of warnings due
to CONFIG_DETECT_HUNG_TASK=y [1]. While the 1 sec timeout is sufficient
to resolve the warnings (given the hung task timeout must be 1 sec or
larger) it may cause unnecessary wake-ups if the system is idle.
[1] https://lkml.kernel.org/r/CADYN=9J0DQhizAGB0-jz4HOBBh+05kMBXb4c0cXMS7Qi5NAJiw@mail.gmail.com

Fix it by computing the timeout duration in terms of the current sysctl_hung_task_timeout_secs value.

Signed-off-by Marco Elver <[email protected]>

---
  mm/kfence/core.c | 12 +++++++++++-
  1 file changed, 11 insertions(+), 1 deletion(-)

mm/kfence/core.c +11 -1

index 235d726f88bc..9742649f3f88 100644
@@ -20,6 +20,7 @@
  #include <linux/moduleparam.h>
  #include <linux/random.h>
  #include <linux/rcupdate.h>
+#include <linux/sched/sysctl.h>
  #include <linux/seq_file.h>
  #include <linux/slab.h>
  #include <linux/spinlock.h>

@@ -621,7 +622,16 @@ static void toggle_allocation_gate(struct work_struct *work)
  	/* Enable static key, and await allocation to happen. */
  	static_branch_enable(&kfence_allocation_key);
 
-	wait_event_timeout(allocation_wait, atomic_read(&kfence_allocation_gate), HZ);
+	if (sysctl_hung_task_timeout_secs) {
+		/*
+		 * During low activity with no allocations we might wait a
+		 * while; let's avoid the hung task warning.
+		 */
+		wait_event_timeout(allocation_wait, atomic_read(&kfence_allocation_gate),
+				   sysctl_hung_task_timeout_secs * HZ / 2);
+	} else {
+		wait_event(allocation_wait, atomic_read(&kfence_allocation_gate));
+	}
 
  	/* Disable static key and reset timer. */
  	static_branch_disable(&kfence_allocation_key);

Quoted 45 lines · Marco Elver

The allocation wait timeout was initially added because of warnings due
to CONFIG_DETECT_HUNG_TASK=y [1]. While the 1 sec timeout is sufficient
to resolve the warnings (given the hung task timeout must be 1 sec or
larger) it may cause unnecessary wake-ups if the system is idle.
[1] https://lkml.kernel.org/r/CADYN=9J0DQhizAGB0-jz4HOBBh+05kMBXb4c0cXMS7Qi5NAJiw@mail.gmail.com

Fix it by computing the timeout duration in terms of the current sysctl_hung_task_timeout_secs value.

Signed-off-by Marco Elver <[email protected]>

---
  mm/kfence/core.c | 12 +++++++++++-

  1 file changed, 11 insertions(+), 1 deletion(-)

mm/kfence/core.c +11 -0

index 235d726f88bc..9742649f3f88 100644
@@ -20,6 +20,7 @@
   #include <linux/moduleparam.h>
   #include <linux/random.h>
   #include <linux/rcupdate.h>
+#include <linux/sched/sysctl.h>
   #include <linux/seq_file.h>
   #include <linux/slab.h>
   #include <linux/spinlock.h>

@@ -621,7 +622,16 @@ static void toggle_allocation_gate(struct 
 work_struct *work)
       /* Enable static key, and await allocation to happen. */
       static_branch_enable(&kfence_allocation_key);
   -    wait_event_timeout(allocation_wait, atomic_read(&kfence_allocation_gate), HZ);
+    if (sysctl_hung_task_timeout_secs) {
+        /*
+         * During low activity with no allocations we might wait a
+         * while; let's avoid the hung task warning.
+         */
+        wait_event_timeout(allocation_wait, 
 atomic_read(&kfence_allocation_gate),
+                   sysctl_hung_task_timeout_secs * HZ / 2);
+    } else {
+        wait_event(allocation_wait, 
 atomic_read(&kfence_allocation_gate));
+    }

        /* Disable static key and reset timer. */ static_branch_disable(&kfence_allocation_key);

Hi Marco,

We found kfence_test will fails on ARM64 with this patch with/without CONFIG_DETECT_HUNG_TASK,

Any thought ?

Quoted 6 lines · Kefeng Wang

Hi Marco,

We found kfence_test will fails on ARM64 with this patch with/without CONFIG_DETECT_HUNG_TASK,

Any thought ?

Please share log and instructions to reproduce if possible. Also, if possible, please share bisection log that led you to this patch.

I currently do not see how this patch would cause that, it only increases the timeout duration.

I know that under QEMU TCG mode, there are occasionally timeouts in the test simply due to QEMU being extremely slow or other weirdness.

.

Quoted 7 lines · Marco Elver

Quoted 6 lines · Kefeng Wang

Hi Marco,

We found kfence_test will fails on ARM64 with this patch with/without CONFIG_DETECT_HUNG_TASK,

Any thought ?

Quoted 10 lines

Please share log and instructions to reproduce if possible. Also, if possible, please share bisection log that led you to this patch.

I currently do not see how this patch would cause that, it only increases the timeout duration.

I know that under QEMU TCG mode, there are occasionally timeouts in the test simply due to QEMU being extremely slow or other weirdness.

Hi Marco,

There are some of the results of the current test:
1. Using qemu-kvm on arm64 machine, all testcase can pass.
2. Using qemu-system-aarch64 on x86_64 machine, randomly some testcases fail.
3. Using qemu-system-aarch64 on x86_64, but removing the judgment of kfence_allocation_key in kfence_alloc(), all testcase can pass.

I add some printing to the kernel and get very strange results.
I add a new variable kfence_allocation_key_gate to track the state of kfence_allocation_key. As shown in the following code, theoretically, if kfence_allocation_key_gate is zero, then kfence_allocation_key must be enabled, so the value of variable error in kfence_alloc() should always be zero. In fact, all the passed testcases fit this point. But as shown in the following failed log, although kfence_allocation_key has been enabled, it's still check failed here.

So I think static_key might be problematic in my qemu environment.
The change of timeout is not a problem but caused us to observe this problem. I tried changing the wait_event to a loop. I set timeout to HZ and re-enable/disabled in each loop, then the failed testcase disappears.

Quoted 35 lines · Liu Shixin

Quoted 7 lines · Marco Elver

Quoted 6 lines · Kefeng Wang

Hi Marco,

We found kfence_test will fails on ARM64 with this patch with/without CONFIG_DETECT_HUNG_TASK,

Any thought ?

Quoted 10 lines

Please share log and instructions to reproduce if possible. Also, if possible, please share bisection log that led you to this patch.

I currently do not see how this patch would cause that, it only increases the timeout duration.

I know that under QEMU TCG mode, there are occasionally timeouts in the test simply due to QEMU being extremely slow or other weirdness.

Hi Marco,

There are some of the results of the current test:
1. Using qemu-kvm on arm64 machine, all testcase can pass.
2. Using qemu-system-aarch64 on x86_64 machine, randomly some testcases fail.
3. Using qemu-system-aarch64 on x86_64, but removing the judgment of kfence_allocation_key in kfence_alloc(), all testcase can pass.

I add some printing to the kernel and get very strange results.
I add a new variable kfence_allocation_key_gate to track the state of kfence_allocation_key. As shown in the following code, theoretically, if kfence_allocation_key_gate is zero, then kfence_allocation_key must be enabled, so the value of variable error in kfence_alloc() should always be zero. In fact, all the passed testcases fit this point. But as shown in the following failed log, although kfence_allocation_key has been enabled, it's still check failed here.

Quoted 4 lines

So I think static_key might be problematic in my qemu environment.
The change of timeout is not a problem but caused us to observe this problem. I tried changing the wait_event to a loop. I set timeout to HZ and re-enable/disabled in each loop, then the failed testcase disappears.

Nice analysis, thanks! What I gather is that static_keys/jump_labels are somehow broken in QEMU.

This does remind me that I found a bug in QEMU that might be relevant:
https://bugs.launchpad.net/qemu/+bug/1920934
Looks like it was never fixed. :-/

The failures I encountered caused the kernel to crash, but never saw the kfence test to fail due to that (never managed to get that far). Though the bug I saw was on x86 TCG mode, and I never tried arm64. If

you can, try to build a QEMU with ASan and see if you also get the same use-after-free bug.

Unless we observe the problem on a real machine, I think for now we can conclude with fairly high confidence that QEMU TCG still has issues and cannot be fully trusted here (see bug above).

Thanks,
-- Marco

Sent: 16 September 2021 02:21

We found kfence_test will fails  on ARM64 with this patch with/without CONFIG_DETECT_HUNG_TASK,

Any thought ?

Quoted 7 lines

Quoted 7 lines

      /* Enable static key, and await allocation to happen. */ static_branch_enable(&kfence_allocation_key);
  -    wait_event_timeout(allocation_wait, atomic_read(&kfence_allocation_gate), HZ); +    if (sysctl_hung_task_timeout_secs) {
+        /*
+         * During low activity with no allocations we might wait a
+         * while; let's avoid the hung task warning.

Quoted 8 lines

Quoted 8 lines

+         */
+        wait_event_timeout(allocation_wait, atomic_read(&kfence_allocation_gate), +                   sysctl_hung_task_timeout_secs * HZ / 2);
+    } else {
+        wait_event(allocation_wait, atomic_read(&kfence_allocation_gate)); +    }
        /* Disable static key and reset timer. */ static_branch_disable(&kfence_allocation_key);

From: Kefeng Wang

Quoted 7 lines

Sent: 16 September 2021 02:21

We found kfence_test will fails on ARM64 with this patch with/without CONFIG_DETECT_HUNG_TASK,

Any thought ?

...

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

      /* Enable static key, and await allocation to happen. */
      static_branch_enable(&kfence_allocation_key);

- wait_event_timeout(allocation_wait, atomic_read(&kfence_allocation_gate), HZ);

mm/kfence/core.c +4 -0

+    if (sysctl_hung_task_timeout_secs) {
+        /*
+         * During low activity with no allocations we might wait a
+         * while; let's avoid the hung task warning.

Quoted 8 lines

Quoted 8 lines

mm/kfence/core.c static void toggle_allocation_gate(struct work_struct *work) · line 637

Quoted 8 lines

mm/kfence/core.c +6 -0

+         */
+        wait_event_timeout(allocation_wait, atomic_read(&kfence_allocation_gate),
+                   sysctl_hung_task_timeout_secs * HZ / 2);
+    } else {
+        wait_event(allocation_wait, atomic_read(&kfence_allocation_gate));
+    }
        /* Disable static key and reset timer. */
      static_branch_disable(&kfence_allocation_key);

It has replaced a wait_event_timeout() with a wait_event().

That probably isn't intended.
Although I'd expect their to be some test for the wait being signalled or timing out.