Re: [PATCH] kthread: dynamically allocate memory to store kthread's full name

On Wed 2021-11-10 10:12:17, Yafang Shao wrote:

On Wed, Nov 10, 2021 at 1:34 AM Kees Cook [off-list ref] wrote:

quoted

On Mon, Nov 08, 2021 at 08:41:42AM +0000, Yafang Shao wrote:

quoted

When I was implementing a new per-cpu kthread cfs_migration, I found the
comm of it "cfs_migration/%u" is truncated due to the limitation of
TASK_COMM_LEN. For example, the comm of the percpu thread on CPU10~19 are
all with the same name "cfs_migration/1", which will confuse the user. This
issue is not critical, because we can get the corresponding CPU from the
task's Cpus_allowed. But for kthreads correspoinding to other hardware
devices, it is not easy to get the detailed device info from task comm,
for example,

After this change, the full name of these truncated kthreads will be
displayed via /proc/[pid]/comm:

--- a/kernel/kthread.c
+++ b/kernel/kthread.c

@@ -399,12 +414,27 @@ 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];
+             va_list aq;
+             int len;

              /*
               * task is already visible to other tasks, so updating
               * COMM must be protected.
               */
-             vsnprintf(name, sizeof(name), namefmt, args);
+             va_copy(aq, args);
+             len = vsnprintf(name, sizeof(name), namefmt, aq);
+             va_end(aq);
+             if (len >= TASK_COMM_LEN) {
+                     struct kthread *kthread = to_kthread(task);
+                     char *full_name;
+
+                     full_name = kvasprintf(GFP_KERNEL, namefmt, args);
+                     if (!full_name) {
+                             kfree(create);
+                             return ERR_PTR(-ENOMEM);

I'm not a fan of this out-of-line free/return. Why not just leave it
truncated when out of memory? For example just do:

                        struct kthread *kthread = to_kthread(task);

                        kthread->full_name = kvasprintf(GFP_KERNEL, namefmt, args);

It is OK for me.

I agree. It is perfectly fine to continue here. The truncated name is
a reasonable fallback.

I will do it as you suggested and show a warning for this case.

Yup. Just please, use only the truncated name in the warning. It is
not important enough to add another va_copy() for this.

quoted

quoted

+                     }
+                     kthread->full_name = full_name;
+             }
              set_task_comm(task, name);
              /*
               * root may have changed our (kthreadd's) priority or CPU mask.

Best Regards,
Petr