Re: [bug report] printk: ringbuffer: Add KUnit test

[Thomas Weißschuh]

On Wed, Jun 25, 2025 at 10:22:19AM -0500, Dan Carpenter wrote:
> Hello Thomas Weißschuh,
> 
> The patch 5ea2bcdfbf46: "printk: ringbuffer: Add KUnit test" from Jun
> 12, 2025, leads to the following static checker warning:
> 
>       kernel/printk/printk_ringbuffer_kunit_test.c:91 prbtest_check_data()
>       (unpublished script worries this an off by one)
> 
> kernel/printk/printk_ringbuffer_kunit_test.c
>     83 static bool prbtest_check_data(const struct prbtest_rbdata *dat)
>     84 {
>     85        unsigned int len;
>     86 
>     87        /* Sane length? */
>     88        if (dat->len < 1 || dat->len > MAX_RBDATA_TEXT_SIZE)
>     89                return false;
>     90 
> --> 91        if (dat->text[dat->len] != '\0')
>     92                return false;
>     93 
> 
> My question is that the prbtest_rbdata structure is declared like this:
> 
>     53  /* test data structure */
>     54  struct prbtest_rbdata {
>     55          unsigned int len;
>     56          char text[] __counted_by(len);
>     57  };
> 
> The size of text is not really counted by len, it's "MAX_RBDATA_TEXT_SIZE
> + 1".  The condition "if (dat->text[dat->len] != '\0')" is reading one
> element beyond the __counted_by() value so something should complain if
> we enable all the debugging, right?

You are right, we are reading past the __counted_by().
But I don't get any complains with CONFIG_FORTIFY_SOURCE=y and CONFIG_KASAN=y
on either clang or gcc.
We could remove the __counted_by, but I assume somebody will try to add it back
at some point.
Or we account for the terminator in dat->len:

diff --git a/kernel/printk/printk_ringbuffer_kunit_test.c 
b/kernel/printk/printk_ringbuffer_kunit_test.c
index ef4a2beea57a..106f4c7ffc86 100644
--- a/kernel/printk/printk_ringbuffer_kunit_test.c
+++ b/kernel/printk/printk_ringbuffer_kunit_test.c
@@ -85,14 +85,15 @@ 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)
+       if (dat->len < 2 || dat->len > MAX_RBDATA_TEXT_SIZE + 1)
                return false;
 
-       if (dat->text[dat->len] != '\0')
+       len = dat->len - 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 +115,9 @@ 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, +1 for terminator */
+               text_size = get_random_u32_inclusive(1, MAX_RBDATA_TEXT_SIZE) + 
1;
+               record_size = sizeof(struct prbtest_rbdata) + text_size;
                WARN_ON_ONCE(record_size > MAX_PRB_RECORD_SIZE);
 
                /* specify the text sizes for reservation */
@@ -142,7 +142,7 @@ static int prbtest_writer(void *data)
                        dat = (struct prbtest_rbdata *)r.text_buf;
                        dat->len = text_size;
                        memset(dat->text, text_id, text_size);
-                       dat->text[text_size] = 0;
+                       dat->text[text_size - 1] = '\0';
 
                        prb_commit(&e);