On Thu, Oct 28, 2021 at 4:10 AM Petr Mladek [off-list ref] wrote:

On Mon 2021-10-25 14:35:42, Kees Cook wrote:

quoted

On Mon, Oct 25, 2021 at 08:33:15AM +0000, Yafang Shao wrote:

quoted

Show a warning if task comm is truncated. Below is the result
of my test case:

truncated kthread comm:I-am-a-kthread-with-lon, pid:14 by 6 characters

Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Yafang Shao <redacted>
Reviewed-by: Kees Cook <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Andrii Nakryiko <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
Cc: Petr Mladek <pmladek@suse.com>
---
 kernel/kthread.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 5b37a8567168..46b924c92078 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c

@@ -399,12 +399,17 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
    if (!IS_ERR(task)) {
            static const struct sched_param param = { .sched_priority = 0 };
            char name[TASK_COMM_LEN];
+           int len;

            /*
             * task is already visible to other tasks, so updating
             * COMM must be protected.
             */
-           vsnprintf(name, sizeof(name), namefmt, args);
+           len = vsnprintf(name, sizeof(name), namefmt, args);
+           if (len >= TASK_COMM_LEN) {

And since this failure case is slow-path, we could improve the warning
as other had kind of suggested earlier with something like this instead:

                      char *full_comm;

                      full_comm = kvasprintf(GFP_KERNEL, namefmt, args);

You need to use va_copy()/va_end() if you want to use the same va_args
twice.

For example, see how kvasprintf() is implemented. It calls
vsnprintf() twice and it uses va_copy()/va_end() around the the first call.

Does it mean that if we want to call vsnprintf() three times, we must
use va_copy()/va_end() around the first call and the second call ?
IOW, if we call vsnprintf() multiple times, all the calls except the
last call should be protected by va_copy()/va_end().
Actually I don't quite understand why we should do it like this. I
will try to understand it, and appreciate it if you could explain it
in detail.

BTW,  can we use va_copy()/va_end() in vsnprintf(), then the caller
doesn't need to care how many times it will call vsnprintf().

kvasprintf() could also return NULL if there is not enough memory.

Right. We need to do the NULL check.

quoted

                      pr_warn("truncated kthread comm '%s' to '%s' (pid:%d)\n",
                              full_comm, name);

BTW: Is this message printed during normal boot? I did not tried the
patchset myself.

Yes, it will be printed at boot time.

We should add this warning only if there is a good solution how to
avoid the truncated names. And we should me sure that the most common
kthreads/workqueues do not trigger it. It would be ugly to print many
warnings during boot if people could not get rid of them easily.

As we have extended task comm to 24, there's no such warning printed
for the existing kthreads/workqueues.
IOW, it will only print for the newly introduced one if it has a long name.
That means this printing is under control.

quoted

                      kfree(full_comm);
              }

quoted

            set_task_comm(task, name);
            /*
             * root may have changed our (kthreadd's) priority or CPU mask.

Best Regards,
Petr

-- 
Thanks
Yafang