From 5ea2bcdfbf46fc3aac239ea371a9561053cc977a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= Date: Thu, 12 Jun 2025 08:29:07 +0200 Subject: [PATCH 1/5] printk: ringbuffer: Add KUnit test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The KUnit test validates the correct operation of the ringbuffer. A separate dedicated ringbuffer is used so that the global printk ringbuffer is not touched. Co-developed-by: John Ogness Signed-off-by: John Ogness Signed-off-by: Thomas Weißschuh Reviewed-by: Petr Mladek Link: https://patch.msgid.link/20250612-printk-ringbuffer-test-v3-1-550c088ee368@linutronix.de Signed-off-by: Petr Mladek --- init/Kconfig | 12 + kernel/printk/.kunitconfig | 3 + kernel/printk/Makefile | 2 + kernel/printk/printk_ringbuffer.c | 5 + kernel/printk/printk_ringbuffer_kunit_test.c | 295 +++++++++++++++++++ 5 files changed, 317 insertions(+) create mode 100644 kernel/printk/.kunitconfig create mode 100644 kernel/printk/printk_ringbuffer_kunit_test.c diff --git a/init/Kconfig b/init/Kconfig index af4c2f085455..73e64172b564 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -1667,6 +1667,18 @@ config PRINTK very difficult to diagnose system problems, saying N here is strongly discouraged. +config PRINTK_RINGBUFFER_KUNIT_TEST + tristate "KUnit Test for the printk ringbuffer" if !KUNIT_ALL_TESTS + depends on PRINTK && KUNIT + default KUNIT_ALL_TESTS + help + This builds the printk ringbuffer KUnit test suite. + + For more information on KUnit and unit tests in general, please refer + to the KUnit documentation. + + If unsure, say N. + config BUG bool "BUG() support" if EXPERT default y diff --git a/kernel/printk/.kunitconfig b/kernel/printk/.kunitconfig new file mode 100644 index 000000000000..f31458fd1a92 --- /dev/null +++ b/kernel/printk/.kunitconfig @@ -0,0 +1,3 @@ +CONFIG_KUNIT=y +CONFIG_PRINTK=y +CONFIG_PRINTK_RINGBUFFER_KUNIT_TEST=y diff --git a/kernel/printk/Makefile b/kernel/printk/Makefile index 39a2b61c7232..f8004ac3983d 100644 --- a/kernel/printk/Makefile +++ b/kernel/printk/Makefile @@ -7,3 +7,5 @@ obj-$(CONFIG_PRINTK_INDEX) += index.o obj-$(CONFIG_PRINTK) += printk_support.o printk_support-y := printk_ringbuffer.o printk_support-$(CONFIG_SYSCTL) += sysctl.o + +obj-$(CONFIG_PRINTK_RINGBUFFER_KUNIT_TEST) += printk_ringbuffer_kunit_test.o diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c index d9fb053cff67..bc811de18316 100644 --- a/kernel/printk/printk_ringbuffer.c +++ b/kernel/printk/printk_ringbuffer.c @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 +#include #include #include #include @@ -1685,6 +1686,7 @@ fail: memset(r, 0, sizeof(*r)); return false; } +EXPORT_SYMBOL_IF_KUNIT(prb_reserve); /* Commit the data (possibly finalizing it) and restore interrupts. */ static void _prb_commit(struct prb_reserved_entry *e, unsigned long state_val) @@ -1759,6 +1761,7 @@ void prb_commit(struct prb_reserved_entry *e) if (head_id != e->id) desc_make_final(e->rb, e->id); } +EXPORT_SYMBOL_IF_KUNIT(prb_commit); /** * prb_final_commit() - Commit and finalize (previously reserved) data to @@ -2184,6 +2187,7 @@ bool prb_read_valid(struct printk_ringbuffer *rb, u64 seq, { return _prb_read_valid(rb, &seq, r, NULL); } +EXPORT_SYMBOL_IF_KUNIT(prb_read_valid); /** * prb_read_valid_info() - Non-blocking read of meta data for a requested @@ -2333,6 +2337,7 @@ void prb_init(struct printk_ringbuffer *rb, infos[0].seq = -(u64)_DESCS_COUNT(descbits); infos[_DESCS_COUNT(descbits) - 1].seq = 0; } +EXPORT_SYMBOL_IF_KUNIT(prb_init); /** * prb_record_text_space() - Query the full actual used ringbuffer space for diff --git a/kernel/printk/printk_ringbuffer_kunit_test.c b/kernel/printk/printk_ringbuffer_kunit_test.c new file mode 100644 index 000000000000..4081ae051d8e --- /dev/null +++ b/kernel/printk/printk_ringbuffer_kunit_test.c @@ -0,0 +1,295 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +#include "printk_ringbuffer.h" + +/* + * This KUnit tests the data integrity of the lockless printk_ringbuffer. + * From multiple CPUs it writes messages of varying length and content while + * a reader validates the correctness of the messages. + * + * IMPORTANT: The more CPUs you can use for this KUnit, the better! + * + * The test works by starting "num_online_cpus() - 1" writer threads, each + * pinned to their own CPU. Each writer thread loops, writing data of varying + * length into a printk_ringbuffer as fast as possible. The data content is + * an embedded data struct followed by string content repeating the byte: + * + * 'A' + CPUID + * + * The reader is running on the remaining online CPU, or if there is only one + * CPU on the same as the writer. + * It ensures that the embedded struct content is consistent with the string + * and that the string * is terminated and is composed of the same repeating + * byte as its first byte. + * + * Because the threads are running in such tight loops, they will call + * cond_resched() from time to time so the system stays functional. + * + * If the reader encounters an error, the test is aborted and some + * information about the error is reported. + * The runtime of the test can be configured with the runtime_ms module parameter. + * + * Note that the test is performed on a separate printk_ringbuffer instance + * and not the instance used by printk(). + */ + +static unsigned long runtime_ms = 10 * MSEC_PER_SEC; +module_param(runtime_ms, ulong, 0400); + +/* test data structure */ +struct prbtest_rbdata { + unsigned int len; + char text[] __counted_by(len); +}; + +#define MAX_RBDATA_TEXT_SIZE 0x7f +/* +1 for terminator. */ +#define MAX_PRB_RECORD_SIZE (sizeof(struct prbtest_rbdata) + MAX_RBDATA_TEXT_SIZE + 1) + +struct prbtest_data { + struct kunit *test; + struct printk_ringbuffer *ringbuffer; + /* used by writers to signal reader of new records */ + wait_queue_head_t new_record_wait; +}; + +struct prbtest_thread_data { + unsigned long num; + struct prbtest_data *test_data; +}; + +static void prbtest_fail_record(struct kunit *test, const struct prbtest_rbdata *dat, u64 seq) +{ + KUNIT_FAIL(test, "BAD RECORD: seq=%llu len=%u text=%.*s\n", + seq, dat->len, + dat->len <= MAX_RBDATA_TEXT_SIZE ? dat->len : -1, + dat->len <= MAX_RBDATA_TEXT_SIZE ? dat->text : ""); +} + +static bool prbtest_check_data(const struct prbtest_rbdata *dat) +{ + unsigned int len; + + /* Sane length? */ + if (dat->len < 1 || dat->len > MAX_RBDATA_TEXT_SIZE) + return false; + + if (dat->text[dat->len] != '\0') + return false; + + /* String repeats with the same character? */ + len = dat->len; + while (len--) { + if (dat->text[len] != dat->text[0]) + return false; + } + + return true; +} + +static int prbtest_writer(void *data) +{ + struct prbtest_thread_data *tr = data; + char text_id = 'A' + tr->num; + struct prb_reserved_entry e; + struct prbtest_rbdata *dat; + u32 record_size, text_size; + unsigned long count = 0; + struct printk_record r; + + kunit_info(tr->test_data->test, "start thread %03lu (writer)\n", tr->num); + + for (;;) { + /* ensure at least 1 character */ + text_size = get_random_u32_inclusive(1, MAX_RBDATA_TEXT_SIZE); + /* +1 for terminator. */ + record_size = sizeof(struct prbtest_rbdata) + text_size + 1; + WARN_ON_ONCE(record_size > MAX_PRB_RECORD_SIZE); + + /* specify the text sizes for reservation */ + prb_rec_init_wr(&r, record_size); + + if (prb_reserve(&e, tr->test_data->ringbuffer, &r)) { + r.info->text_len = record_size; + + dat = (struct prbtest_rbdata *)r.text_buf; + dat->len = text_size; + memset(dat->text, text_id, text_size); + dat->text[text_size] = 0; + + prb_commit(&e); + + wake_up_interruptible(&tr->test_data->new_record_wait); + } + + if ((count++ & 0x3fff) == 0) + cond_resched(); + + if (kthread_should_stop()) + break; + } + + kunit_info(tr->test_data->test, "end thread %03lu: wrote=%lu\n", tr->num, count); + + return 0; +} + +struct prbtest_wakeup_timer { + struct timer_list timer; + struct task_struct *task; +}; + +static void prbtest_wakeup_callback(struct timer_list *timer) +{ + struct prbtest_wakeup_timer *wakeup = timer_container_of(wakeup, timer, timer); + + set_tsk_thread_flag(wakeup->task, TIF_NOTIFY_SIGNAL); + wake_up_process(wakeup->task); +} + +static int prbtest_reader(struct prbtest_data *test_data, unsigned long timeout_ms) +{ + struct prbtest_wakeup_timer wakeup; + char text_buf[MAX_PRB_RECORD_SIZE]; + unsigned long count = 0; + struct printk_info info; + struct printk_record r; + u64 seq = 0; + + wakeup.task = current; + timer_setup_on_stack(&wakeup.timer, prbtest_wakeup_callback, 0); + mod_timer(&wakeup.timer, jiffies + msecs_to_jiffies(timeout_ms)); + + prb_rec_init_rd(&r, &info, text_buf, sizeof(text_buf)); + + kunit_info(test_data->test, "start reader\n"); + + while (!wait_event_interruptible(test_data->new_record_wait, + prb_read_valid(test_data->ringbuffer, seq, &r))) { + /* check/track the sequence */ + if (info.seq < seq) + KUNIT_FAIL(test_data->test, "BAD SEQ READ: request=%llu read=%llu\n", + seq, info.seq); + + if (!prbtest_check_data((struct prbtest_rbdata *)r.text_buf)) + prbtest_fail_record(test_data->test, + (struct prbtest_rbdata *)r.text_buf, info.seq); + + if ((count++ & 0x3fff) == 0) + cond_resched(); + + seq = info.seq + 1; + } + + timer_delete_sync(&wakeup.timer); + timer_destroy_on_stack(&wakeup.timer); + + kunit_info(test_data->test, "end reader: read=%lu seq=%llu\n", count, info.seq); + + return 0; +} + +KUNIT_DEFINE_ACTION_WRAPPER(prbtest_kthread_cleanup, kthread_stop, struct task_struct *); + +static void prbtest_add_kthread_cleanup(struct kunit *test, struct task_struct *kthread) +{ + int err; + + err = kunit_add_action_or_reset(test, prbtest_kthread_cleanup, kthread); + KUNIT_ASSERT_EQ(test, err, 0); +} + +static inline void prbtest_prb_reinit(struct printk_ringbuffer *rb) +{ + prb_init(rb, rb->text_data_ring.data, rb->text_data_ring.size_bits, rb->desc_ring.descs, + rb->desc_ring.count_bits, rb->desc_ring.infos); +} + +static void test_readerwriter(struct kunit *test) +{ + /* Equivalent to CONFIG_LOG_BUF_SHIFT=13 */ + DEFINE_PRINTKRB(test_rb, 8, 5); + + struct prbtest_thread_data *thread_data; + struct prbtest_data *test_data; + struct task_struct *thread; + cpumask_t test_cpus; + int cpu, reader_cpu; + + cpus_read_lock(); + /* + * Failure of KUNIT_ASSERT() kills the current task + * so it can not be called while the CPU hotplug lock is held. + * Instead use a snapshot of the online CPUs. + * If they change during test execution it is unfortunate but not a grave error. + */ + cpumask_copy(&test_cpus, cpu_online_mask); + cpus_read_unlock(); + + /* One CPU is for the reader, all others are writers */ + reader_cpu = cpumask_first(&test_cpus); + if (cpumask_weight(&test_cpus) == 1) + kunit_warn(test, "more than one CPU is recommended"); + else + cpumask_clear_cpu(reader_cpu, &test_cpus); + + /* KUnit test can get restarted more times. */ + prbtest_prb_reinit(&test_rb); + + test_data = kunit_kmalloc(test, sizeof(*test_data), GFP_KERNEL); + KUNIT_ASSERT_NOT_NULL(test, test_data); + test_data->test = test; + test_data->ringbuffer = &test_rb; + init_waitqueue_head(&test_data->new_record_wait); + + kunit_info(test, "running for %lu ms\n", runtime_ms); + + for_each_cpu(cpu, &test_cpus) { + thread_data = kunit_kmalloc(test, sizeof(*thread_data), GFP_KERNEL); + KUNIT_ASSERT_NOT_NULL(test, thread_data); + thread_data->test_data = test_data; + thread_data->num = cpu; + + thread = kthread_run_on_cpu(prbtest_writer, thread_data, cpu, + "prbtest writer %u"); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, thread); + prbtest_add_kthread_cleanup(test, thread); + } + + kunit_info(test, "starting test\n"); + + set_cpus_allowed_ptr(current, cpumask_of(reader_cpu)); + prbtest_reader(test_data, runtime_ms); + + kunit_info(test, "completed test\n"); +} + +static struct kunit_case prb_test_cases[] = { + KUNIT_CASE_SLOW(test_readerwriter), + {} +}; + +static struct kunit_suite prb_test_suite = { + .name = "printk-ringbuffer", + .test_cases = prb_test_cases, +}; +kunit_test_suite(prb_test_suite); + +MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING"); +MODULE_AUTHOR("John Ogness "); +MODULE_DESCRIPTION("printk_ringbuffer KUnit test"); +MODULE_LICENSE("GPL"); From 254e8fb5e67643a19a8dd6e142262ec83b30c3c7 Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Wed, 2 Jul 2025 11:51:55 +0200 Subject: [PATCH 2/5] printk: ringbuffer: Explain why the KUnit test ignores failed writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The KUnit test ignores prb_reserve() failures on purpose. It tries to push the ringbuffer beyond limits. Note that it is a know problem that writes might fail in this situation. printk() tries to prevent this problem by: + allocating big enough data buffer, see log_buf_add_cpu(). + allocating enough descriptors by using small enough average record, see PRB_AVGBITS. + storing the record with disabled interrupts, see vprintk_store(). Also the amount of printk() messages is always somehow bound in practice. And they are serialized when they are printed from many CPUs on purpose, for example, when printing backtraces. Reviewed-by: John Ogness Reviewed-by: Thomas Weißschuh Link: https://patch.msgid.link/20250702095157.110916-2-pmladek@suse.com Signed-off-by: Petr Mladek --- kernel/printk/printk_ringbuffer_kunit_test.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/kernel/printk/printk_ringbuffer_kunit_test.c b/kernel/printk/printk_ringbuffer_kunit_test.c index 4081ae051d8e..217dcc14670c 100644 --- a/kernel/printk/printk_ringbuffer_kunit_test.c +++ b/kernel/printk/printk_ringbuffer_kunit_test.c @@ -123,6 +123,19 @@ static int prbtest_writer(void *data) /* specify the text sizes for reservation */ prb_rec_init_wr(&r, record_size); + /* + * Reservation can fail if: + * + * - No free descriptor is available. + * - The buffer is full, and the oldest record is reserved + * but not yet committed. + * + * It actually happens in this test because all CPUs are trying + * to write an unbounded number of messages in a tight loop. + * These failures are intentionally ignored because this test + * focuses on races, ringbuffer consistency, and pushing system + * usability limits. + */ if (prb_reserve(&e, tr->test_data->ringbuffer, &r)) { r.info->text_len = record_size; From d18d7989e3da1f2753d49cb24d916f357e340f76 Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Wed, 2 Jul 2025 11:51:57 +0200 Subject: [PATCH 3/5] printk: kunit: Fix __counted_by() in struct prbtest_rbdata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit __counted_by() has to point to a variable which defines the size of the related array. The code must never access the array beyond this limit. struct prbtest_rbdata currently stores the length of the string. And the code access the array beyond the limit when writing or reading the trailing '\0'. Store the size of the string, including the trailing '\0' if we wanted to keep __counted_by(). Consistently use "_size" suffix when the trailing '\0' is counted. Note that MAX_RBDATA_TEXT_SIZE was originally used to limit the text length. When touching the code, make sure that @text_size produced by get_random_u32_inclusive() stays within the limits. Reported-by: Dan Carpenter Closes: https://lore.kernel.org/r/eaea66b9-266a-46e7-980d-33f40ad4b215@sabinyo.mountain Suggested-by: Thomas Weißschuh Reviewed-by: John Ogness Reviewed-by: Thomas Weißschuh Link: https://patch.msgid.link/20250702095157.110916-4-pmladek@suse.com Signed-off-by: Petr Mladek --- kernel/printk/printk_ringbuffer_kunit_test.c | 47 +++++++++++--------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/kernel/printk/printk_ringbuffer_kunit_test.c b/kernel/printk/printk_ringbuffer_kunit_test.c index 217dcc14670c..e67e1815f4c8 100644 --- a/kernel/printk/printk_ringbuffer_kunit_test.c +++ b/kernel/printk/printk_ringbuffer_kunit_test.c @@ -52,13 +52,12 @@ module_param(runtime_ms, ulong, 0400); /* test data structure */ struct prbtest_rbdata { - unsigned int len; - char text[] __counted_by(len); + unsigned int size; + char text[] __counted_by(size); }; -#define MAX_RBDATA_TEXT_SIZE 0x7f -/* +1 for terminator. */ -#define MAX_PRB_RECORD_SIZE (sizeof(struct prbtest_rbdata) + MAX_RBDATA_TEXT_SIZE + 1) +#define MAX_RBDATA_TEXT_SIZE 0x80 +#define MAX_PRB_RECORD_SIZE (sizeof(struct prbtest_rbdata) + MAX_RBDATA_TEXT_SIZE) struct prbtest_data { struct kunit *test; @@ -74,25 +73,29 @@ struct prbtest_thread_data { static void prbtest_fail_record(struct kunit *test, const struct prbtest_rbdata *dat, u64 seq) { - KUNIT_FAIL(test, "BAD RECORD: seq=%llu len=%u text=%.*s\n", - seq, dat->len, - dat->len <= MAX_RBDATA_TEXT_SIZE ? dat->len : -1, - dat->len <= MAX_RBDATA_TEXT_SIZE ? dat->text : ""); + unsigned int len; + + len = dat->size - 1; + + KUNIT_FAIL(test, "BAD RECORD: seq=%llu size=%u text=%.*s\n", + seq, dat->size, + len < MAX_RBDATA_TEXT_SIZE ? len : -1, + len < MAX_RBDATA_TEXT_SIZE ? dat->text : ""); } static bool prbtest_check_data(const struct prbtest_rbdata *dat) { unsigned int len; - /* Sane length? */ - if (dat->len < 1 || dat->len > MAX_RBDATA_TEXT_SIZE) + /* Sane size? At least one character + trailing '\0' */ + if (dat->size < 2 || dat->size > MAX_RBDATA_TEXT_SIZE) return false; - if (dat->text[dat->len] != '\0') + len = dat->size - 1; + if (dat->text[len] != '\0') return false; /* String repeats with the same character? */ - len = dat->len; while (len--) { if (dat->text[len] != dat->text[0]) return false; @@ -114,10 +117,14 @@ static int prbtest_writer(void *data) kunit_info(tr->test_data->test, "start thread %03lu (writer)\n", tr->num); for (;;) { - /* ensure at least 1 character */ - text_size = get_random_u32_inclusive(1, MAX_RBDATA_TEXT_SIZE); - /* +1 for terminator. */ - record_size = sizeof(struct prbtest_rbdata) + text_size + 1; + /* ensure at least 1 character + trailing '\0' */ + text_size = get_random_u32_inclusive(2, MAX_RBDATA_TEXT_SIZE); + if (WARN_ON_ONCE(text_size < 2)) + text_size = 2; + if (WARN_ON_ONCE(text_size > MAX_RBDATA_TEXT_SIZE)) + text_size = MAX_RBDATA_TEXT_SIZE; + + record_size = sizeof(struct prbtest_rbdata) + text_size; WARN_ON_ONCE(record_size > MAX_PRB_RECORD_SIZE); /* specify the text sizes for reservation */ @@ -140,9 +147,9 @@ static int prbtest_writer(void *data) r.info->text_len = record_size; dat = (struct prbtest_rbdata *)r.text_buf; - dat->len = text_size; - memset(dat->text, text_id, text_size); - dat->text[text_size] = 0; + dat->size = text_size; + memset(dat->text, text_id, text_size - 1); + dat->text[text_size - 1] = '\0'; prb_commit(&e); From bf42df09b6aa4ebb596ecba66cf35b75362b55c7 Mon Sep 17 00:00:00 2001 From: Arnd Bergmann Date: Tue, 2 Sep 2025 16:03:26 +0200 Subject: [PATCH 4/5] printk: kunit: support offstack cpumask For large values of CONFIG_NR_CPUS, the newly added kunit test fails to build: kernel/printk/printk_ringbuffer_kunit_test.c: In function 'test_readerwriter': kernel/printk/printk_ringbuffer_kunit_test.c:279:1: error: the frame size of 1432 bytes is larger than 1280 bytes [-Werror=frame-larger-than=] Change this to use cpumask_var_t and allocate it dynamically when CONFIG_CPUMASK_OFFSTACK is set. The variable has to be released via a KUnit action wrapper so that it is freed when the test fails and gets aborted. The parameter type is hardcoded to "struct cpumask *" because the macro KUNIT_DEFINE_ACTION_WRAPPER() does not accept an array. But the function does nothing when CONFIG_CPUMASK_OFFSTACK is not set anyway. Fixes: 5ea2bcdfbf46 ("printk: ringbuffer: Add KUnit test") Signed-off-by: Arnd Bergmann Link: https://lore.kernel.org/all/20250620192554.2234184-1-arnd@kernel.org # v1 [pmladek@suse.com: Correctly handle allocation failures and freeing using KUnit test API.] Link: https://lore.kernel.org/all/20250702095157.110916-3-pmladek@suse.com # v2 Signed-off-by: Petr Mladek --- kernel/printk/printk_ringbuffer_kunit_test.c | 24 +++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/kernel/printk/printk_ringbuffer_kunit_test.c b/kernel/printk/printk_ringbuffer_kunit_test.c index e67e1815f4c8..2282348e869a 100644 --- a/kernel/printk/printk_ringbuffer_kunit_test.c +++ b/kernel/printk/printk_ringbuffer_kunit_test.c @@ -223,8 +223,17 @@ static int prbtest_reader(struct prbtest_data *test_data, unsigned long timeout_ return 0; } +KUNIT_DEFINE_ACTION_WRAPPER(prbtest_cpumask_cleanup, free_cpumask_var, struct cpumask *); KUNIT_DEFINE_ACTION_WRAPPER(prbtest_kthread_cleanup, kthread_stop, struct task_struct *); +static void prbtest_add_cpumask_cleanup(struct kunit *test, cpumask_var_t mask) +{ + int err; + + err = kunit_add_action_or_reset(test, prbtest_cpumask_cleanup, mask); + KUNIT_ASSERT_EQ(test, err, 0); +} + static void prbtest_add_kthread_cleanup(struct kunit *test, struct task_struct *kthread) { int err; @@ -247,9 +256,12 @@ static void test_readerwriter(struct kunit *test) struct prbtest_thread_data *thread_data; struct prbtest_data *test_data; struct task_struct *thread; - cpumask_t test_cpus; + cpumask_var_t test_cpus; int cpu, reader_cpu; + KUNIT_ASSERT_TRUE(test, alloc_cpumask_var(&test_cpus, GFP_KERNEL)); + prbtest_add_cpumask_cleanup(test, test_cpus); + cpus_read_lock(); /* * Failure of KUNIT_ASSERT() kills the current task @@ -257,15 +269,15 @@ static void test_readerwriter(struct kunit *test) * Instead use a snapshot of the online CPUs. * If they change during test execution it is unfortunate but not a grave error. */ - cpumask_copy(&test_cpus, cpu_online_mask); + cpumask_copy(test_cpus, cpu_online_mask); cpus_read_unlock(); /* One CPU is for the reader, all others are writers */ - reader_cpu = cpumask_first(&test_cpus); - if (cpumask_weight(&test_cpus) == 1) + reader_cpu = cpumask_first(test_cpus); + if (cpumask_weight(test_cpus) == 1) kunit_warn(test, "more than one CPU is recommended"); else - cpumask_clear_cpu(reader_cpu, &test_cpus); + cpumask_clear_cpu(reader_cpu, test_cpus); /* KUnit test can get restarted more times. */ prbtest_prb_reinit(&test_rb); @@ -278,7 +290,7 @@ static void test_readerwriter(struct kunit *test) kunit_info(test, "running for %lu ms\n", runtime_ms); - for_each_cpu(cpu, &test_cpus) { + for_each_cpu(cpu, test_cpus) { thread_data = kunit_kmalloc(test, sizeof(*thread_data), GFP_KERNEL); KUNIT_ASSERT_NOT_NULL(test, thread_data); thread_data->test_data = test_data; From 4d164e08cd8457ebcd5346f612ac2c04e80b6bea Mon Sep 17 00:00:00 2001 From: John Ogness Date: Fri, 26 Sep 2025 00:55:59 +0206 Subject: [PATCH 5/5] printk: ringbuffer: Fix data block max size check Currently data_check_size() limits data blocks to a maximum size of the full buffer minus an ID (long integer): max_size <= DATA_SIZE(data_ring) - sizeof(long) However, this is not an appropriate limit due to the nature of wrapping data blocks. For example, if a data block is larger than half the buffer: size = (DATA_SIZE(data_ring) / 2) + 8 and begins exactly in the middle of the buffer, then: - the data block will wrap - the ID will be stored at exactly half of the buffer - the record data begins at the beginning of the buffer - the record data ends 8 bytes _past_ exactly half of the buffer The record overwrites itself, i.e. needs more space than the full buffer! Luckily printk() is not vulnerable to this problem because truncate_msg() limits printk-messages to 1/4 of the ringbuffer. Indeed, by adjusting the printk_ringbuffer KUnit test, which does not use printk() and its truncate_msg() check, it is easy to see that the ringbuffer becomes corrupted for records larger than half the buffer size. The corruption occurs because data_push_tail() expects it will never be requested to push the tail beyond the head. Avoid this problem by adjusting data_check_size() to limit record sizes to half the buffer size. Also add WARN_ON_ONCE() before relevant data_push_tail() calls to validate that there are no such illegal requests. WARN_ON_ONCE() is used, rather than just adding extra checks to data_push_tail() because it is considered a bug to attempt such illegal actions. Link: https://lore.kernel.org/lkml/aMLrGCQSyC8odlFZ@pathway.suse.cz Signed-off-by: John Ogness Reviewed-by: Petr Mladek Signed-off-by: Petr Mladek --- kernel/printk/printk_ringbuffer.c | 43 +++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c index d9fb053cff67..96df860e6f7c 100644 --- a/kernel/printk/printk_ringbuffer.c +++ b/kernel/printk/printk_ringbuffer.c @@ -393,25 +393,21 @@ static unsigned int to_blk_size(unsigned int size) * Sanity checker for reserve size. The ringbuffer code assumes that a data * block does not exceed the maximum possible size that could fit within the * ringbuffer. This function provides that basic size check so that the - * assumption is safe. + * assumption is safe. In particular, it guarantees that data_push_tail() will + * never attempt to push the tail beyond the head. */ static bool data_check_size(struct prb_data_ring *data_ring, unsigned int size) { - struct prb_data_block *db = NULL; - + /* Data-less blocks take no space. */ if (size == 0) return true; /* - * Ensure the alignment padded size could possibly fit in the data - * array. The largest possible data block must still leave room for - * at least the ID of the next block. + * If data blocks were allowed to be larger than half the data ring + * size, a wrapping data block could require more space than the full + * ringbuffer. */ - size = to_blk_size(size); - if (size > DATA_SIZE(data_ring) - sizeof(db->id)) - return false; - - return true; + return to_blk_size(size) <= DATA_SIZE(data_ring) / 2; } /* Query the state of a descriptor. */ @@ -1051,8 +1047,17 @@ static char *data_alloc(struct printk_ringbuffer *rb, unsigned int size, do { next_lpos = get_next_lpos(data_ring, begin_lpos, size); - if (!data_push_tail(rb, next_lpos - DATA_SIZE(data_ring))) { - /* Failed to allocate, specify a data-less block. */ + /* + * data_check_size() prevents data block allocation that could + * cause illegal ringbuffer states. But double check that the + * used space will not be bigger than the ring buffer. Wrapped + * messages need to reserve more space, see get_next_lpos(). + * + * Specify a data-less block when the check or the allocation + * fails. + */ + if (WARN_ON_ONCE(next_lpos - begin_lpos > DATA_SIZE(data_ring)) || + !data_push_tail(rb, next_lpos - DATA_SIZE(data_ring))) { blk_lpos->begin = FAILED_LPOS; blk_lpos->next = FAILED_LPOS; return NULL; @@ -1140,8 +1145,18 @@ static char *data_realloc(struct printk_ringbuffer *rb, unsigned int size, return &blk->data[0]; } - if (!data_push_tail(rb, next_lpos - DATA_SIZE(data_ring))) + /* + * data_check_size() prevents data block reallocation that could + * cause illegal ringbuffer states. But double check that the + * new used space will not be bigger than the ring buffer. Wrapped + * messages need to reserve more space, see get_next_lpos(). + * + * Specify failure when the check or the allocation fails. + */ + if (WARN_ON_ONCE(next_lpos - blk_lpos->begin > DATA_SIZE(data_ring)) || + !data_push_tail(rb, next_lpos - DATA_SIZE(data_ring))) { return NULL; + } /* The memory barrier involvement is the same as data_alloc:A. */ if (!atomic_long_try_cmpxchg(&data_ring->head_lpos, &head_lpos,