[2/2] debugobject: add unit test for static debug object
Commit Message
Add test case to enusre that static debug object correctness.
Tested on little-endian arm64 qemu, result:
[ 2.385735] KTAP version 1
[ 2.385860] 1..1
[ 2.386406] KTAP version 1
[ 2.386658] # Subtest: static debugobject init
[ 2.386726] 1..1
[ 2.401777] ok 1 static_debugobject_test
[ 2.402455] ok 1 static debugobject init
Signed-off-by: Schspa Shi <schspa@gmail.com>
---
MAINTAINERS | 5 ++
lib/Kconfig.debug | 14 ++++
lib/Makefile | 2 +
lib/test_static_debug_object.c | 125 +++++++++++++++++++++++++++++++++
4 files changed, 146 insertions(+)
create mode 100644 lib/test_static_debug_object.c
Comments
Schspa Shi <schspa@gmail.com> writes:
> Add test case to enusre that static debug object correctness.
>
> Tested on little-endian arm64 qemu, result:
>
> [ 2.385735] KTAP version 1
> [ 2.385860] 1..1
> [ 2.386406] KTAP version 1
> [ 2.386658] # Subtest: static debugobject init
> [ 2.386726] 1..1
> [ 2.401777] ok 1 static_debugobject_test
> [ 2.402455] ok 1 static debugobject init
>
> Signed-off-by: Schspa Shi <schspa@gmail.com>
> ---
> MAINTAINERS | 5 ++
> lib/Kconfig.debug | 14 ++++
> lib/Makefile | 2 +
> lib/test_static_debug_object.c | 125 +++++++++++++++++++++++++++++++++
> 4 files changed, 146 insertions(+)
> create mode 100644 lib/test_static_debug_object.c
Hi tglx:
What do you think about this test case? Should we need it ? There are
some platform compatibility issues here that need a little optimization.
On Thu, Mar 23 2023 at 11:16, Schspa Shi wrote:
> Schspa Shi <schspa@gmail.com> writes:
>> MAINTAINERS | 5 ++
>> lib/Kconfig.debug | 14 ++++
>> lib/Makefile | 2 +
>> lib/test_static_debug_object.c | 125 +++++++++++++++++++++++++++++++++
>> 4 files changed, 146 insertions(+)
>> create mode 100644 lib/test_static_debug_object.c
>
> What do you think about this test case? Should we need it ? There are
> some platform compatibility issues here that need a little optimization.
What does it buy over the existing self test. Nothing AFACIT aside of
extra code.
Thanks,
tglx
Thomas Gleixner <tglx@linutronix.de> writes:
> On Thu, Mar 23 2023 at 11:16, Schspa Shi wrote:
>> Schspa Shi <schspa@gmail.com> writes:
>>> MAINTAINERS | 5 ++
>>> lib/Kconfig.debug | 14 ++++
>>> lib/Makefile | 2 +
>>> lib/test_static_debug_object.c | 125 +++++++++++++++++++++++++++++++++
>>> 4 files changed, 146 insertions(+)
>>> create mode 100644 lib/test_static_debug_object.c
>>
>> What do you think about this test case? Should we need it ? There are
>> some platform compatibility issues here that need a little optimization.
>
> What does it buy over the existing self test. Nothing AFACIT aside of
> extra code.
>
It checks the race of the is_static_object() call in the previous
BUG. This test can used to make sure the new fix patch works. The
existing self test have no ability to check this.
> Thanks,
>
> tglx
On Thu, Mar 23 2023 at 16:44, Schspa Shi wrote:
> Thomas Gleixner <tglx@linutronix.de> writes:
>> On Thu, Mar 23 2023 at 11:16, Schspa Shi wrote:
>>> What do you think about this test case? Should we need it ? There are
>>> some platform compatibility issues here that need a little optimization.
>>
>> What does it buy over the existing self test. Nothing AFACIT aside of
>> extra code.
>>
>
> It checks the race of the is_static_object() call in the previous
> BUG. This test can used to make sure the new fix patch works. The
> existing self test have no ability to check this.
Sure it can somehow make sure that it works.
Don't misunderstand me. I'm all for self tests, but this one really
falls into the category of testing the obvious. There are tons of more
interesting places in the kernel which lack self tests than this
particular oddity which is well understood and more than unlikely to
come back.
That said, it would be valuable if you could add your Tested-by to the
final patch, which will be queued up soonish.
Thanks,
tglx
@@ -23202,6 +23202,11 @@ L: linux-mm@kvack.org
S: Maintained
F: mm/zswap.c
+STATIC DEBUGOBJECT TEST
+M: Schspa Shi <schspa@gmail.com>
+S: Maintained
+F: lib/test_static_debug_object.c
+
THE REST
M: Linus Torvalds <torvalds@linux-foundation.org>
L: linux-kernel@vger.kernel.org
@@ -2801,6 +2801,20 @@ config TEST_CLOCKSOURCE_WATCHDOG
If unsure, say N.
+config TEST_STATIC_DEBUGOBJECT
+ tristate "KUnit test for static debugobject"
+ depends on KUNIT
+ select KPROBES
+ select DEBUG_OBJECTS
+ select DEBUG_OBJECTS_TIMERS
+ help
+ This builds the static debugobject unit test, which runs on boot.
+ Tests the static debug object correctness.
+ For more information on KUnit and unit tests in general please refer
+ to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+ If unsure, say N.
+
endif # RUNTIME_TESTING_MENU
config ARCH_USE_MEMTEST
@@ -427,3 +427,5 @@ $(obj)/$(TEST_FORTIFY_LOG): $(addprefix $(obj)/, $(TEST_FORTIFY_LOGS)) FORCE
ifeq ($(CONFIG_FORTIFY_SOURCE),y)
$(obj)/string.o: $(obj)/$(TEST_FORTIFY_LOG)
endif
+
+obj-$(CONFIG_TEST_STATIC_DEBUGOBJECT) += test_static_debug_object.o
new file mode 100644
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * THis module tests the static debugobject via a static timer instance. This
+ * test use kretprobe to inject some delay to make the problem easier to
+ * reproduce.
+ *
+ * Copyright (c) 2023, Schspa Shi <schspa@gmail.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/sched.h>
+#include <linux/timer.h>
+#include <linux/delay.h>
+#include <linux/kprobes.h>
+#include <linux/workqueue.h>
+#include <linux/cpu.h>
+#include <kunit/test.h>
+
+static void ktest_timer_func(struct timer_list *);
+
+static DEFINE_TIMER(ktest_timer, ktest_timer_func);
+static int timer_stop;
+DEFINE_SPINLOCK(tlock);
+
+static DEFINE_PER_CPU(struct work_struct, timer_debugobject_test_work);
+
+static void timer_debugobject_workfn(struct work_struct *work)
+{
+ mod_timer(&ktest_timer, jiffies + (5 * HZ));
+}
+
+/*
+ * Reaper for links from keyrings to dead keys.
+ */
+static void ktest_timer_func(struct timer_list *t)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&tlock, flags);
+ if (!timer_stop)
+ mod_timer(&ktest_timer, jiffies + (1 * HZ));
+ spin_unlock_irqrestore(&tlock, flags);
+}
+
+
+static int static_object_check_handler(
+ struct kretprobe_instance *ri, struct pt_regs *regs)
+{
+ void *address;
+
+ address = (void *)regs_get_register(regs, 0);
+
+ if (address == &ktest_timer) {
+ int this_cpu = raw_smp_processor_id();
+ /*
+ * This hook point adds an extra delay to make the problem
+ * easier to reproduce. We need different delay for
+ * differenct processor.
+ */
+ mdelay(this_cpu * 100);
+ }
+
+ return 0;
+}
+
+
+static struct kretprobe is_static_kretprobes = {
+ .entry_handler = static_object_check_handler,
+ .data_size = 0,
+ /* Probe up to 512 instances concurrently. */
+ .maxactive = 512,
+ .kp = {
+ .symbol_name = "timer_is_static_object",
+ }
+};
+
+
+static void static_debugobject_test(struct kunit *test)
+{
+ unsigned long flags;
+ int cpu;
+ int ret;
+
+ ret = register_kretprobe(&is_static_kretprobes);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ /* Do test */
+ cpus_read_lock();
+ for_each_online_cpu(cpu) {
+ struct work_struct *work =
+ &per_cpu(timer_debugobject_test_work, cpu);
+ INIT_WORK(work, timer_debugobject_workfn);
+ schedule_work_on(cpu, work);
+ }
+
+ for_each_online_cpu(cpu) {
+ struct work_struct *work =
+ &per_cpu(timer_debugobject_test_work, cpu);
+ flush_work(work);
+ }
+ cpus_read_unlock();
+
+ spin_lock_irqsave(&tlock, flags);
+ timer_stop = 0;
+ spin_unlock_irqrestore(&tlock, flags);
+
+ del_timer_sync(&ktest_timer);
+
+ unregister_kretprobe(&is_static_kretprobes);
+}
+
+static struct kunit_case static_debugobject_init_cases[] = {
+ KUNIT_CASE(static_debugobject_test),
+ {}
+};
+
+static struct kunit_suite static_debugobject_suite = {
+ .name = "static debugobject init",
+ .test_cases = static_debugobject_init_cases,
+};
+
+kunit_test_suite(static_debugobject_suite);
+MODULE_AUTHOR("Schspa <schspa@gmail.com>");
+MODULE_LICENSE("GPL");