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,
jbd2/nvme0n1p2-
xfs-reclaim/sdf
Currently there are so many truncated kthreads:
rcu_tasks_kthre
rcu_tasks_rude_
rcu_tasks_trace
poll_mpt3sas0_s
ext4-rsv-conver
xfs-reclaim/sd{a, b, c, ...}
xfs-blockgc/sd{a, b, c, ...}
xfs-inodegc/sd{a, b, c, ...}
audit_send_repl
ecryptfs-kthrea
vfio-irqfd-clea
jbd2/nvme0n1p2-
...
We can shorten these names to work around this problem, but it may be
not applied to all of the truncated kthreads. Take 'jbd2/nvme0n1p2-' for
example, it is a nice name, and it is not a good idea to shorten it.
One possible way to fix this issue is extending the task comm size, but
as task->comm is used in lots of places, that may cause some potential
buffer overflows. Another more conservative approach is introducing a new
pointer to store kthread's full name if it is truncated, which won't
introduce too much overhead as it is in the non-critical path. Finally we
make a dicision to use the second approach. See also the discussions in
this thread:
https://lore.kernel.org/lkml/[email protected]/
After this change, the full name of these truncated kthreads will be
displayed via /proc/[pid]/comm:
rcu_tasks_kthread
rcu_tasks_rude_kthread
rcu_tasks_trace_kthread
poll_mpt3sas0_statu
ext4-rsv-conversion
xfs-reclaim/sdf1
xfs-blockgc/sdf1
xfs-inodegc/sdf1
audit_send_reply
ecryptfs-kthread
vfio-irqfd-cleanup
jbd2/nvme0n1p2-8
Suggested-by: Petr Mladek <[email protected]>
Suggested-by: Steven Rostedt <[email protected]>
Signed-off-by: Yafang Shao <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Andrii Nakryiko <[email protected]>
Cc: Michal Miroslaw <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Petr Mladek <[email protected]>
---
TODO: will cleanup worker comm in the next step.
---
fs/proc/array.c | 3 +++
include/linux/kthread.h | 1 +
kernel/kthread.c | 32 +++++++++++++++++++++++++++++++-
3 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/fs/proc/array.c b/fs/proc/array.c
index 49be8c8ef555..860e4deafa65 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -92,6 +92,7 @@
#include <linux/string_helpers.h>
#include <linux/user_namespace.h>
#include <linux/fs_struct.h>
+#include <linux/kthread.h>
#include <asm/processor.h>
#include "internal.h"
@@ -102,6 +103,8 @@ void proc_task_name(struct seq_file *m, struct task_struct *p, bool escape)
if (p->flags & PF_WQ_WORKER)
wq_worker_comm(tcomm, sizeof(tcomm), p);
+ else if (p->flags & PF_KTHREAD)
+ get_kthread_comm(tcomm, sizeof(tcomm), p);
else
__get_task_comm(tcomm, sizeof(tcomm), p);
diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 346b0f269161..2a5c04494663 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -33,6 +33,7 @@ struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
unsigned int cpu,
const char *namefmt);
+void get_kthread_comm(char *buf, size_t buf_size, struct task_struct *tsk);
void set_kthread_struct(struct task_struct *p);
void kthread_set_per_cpu(struct task_struct *k, int cpu);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 5b37a8567168..ce8258231eea 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -60,6 +60,8 @@ struct kthread {
#ifdef CONFIG_BLK_CGROUP
struct cgroup_subsys_state *blkcg_css;
#endif
+ /* To store the full name if task comm is truncated. */
+ char *full_name;
};
enum KTHREAD_BITS {
@@ -93,6 +95,18 @@ static inline struct kthread *__to_kthread(struct task_struct *p)
return kthread;
}
+void get_kthread_comm(char *buf, size_t buf_size, struct task_struct *tsk)
+{
+ struct kthread *kthread = to_kthread(tsk);
+
+ if (!kthread || !kthread->full_name) {
+ __get_task_comm(buf, buf_size, tsk);
+ return;
+ }
+
+ strscpy_pad(buf, kthread->full_name, buf_size);
+}
+
void set_kthread_struct(struct task_struct *p)
{
struct kthread *kthread;
@@ -121,6 +135,7 @@ void free_kthread_struct(struct task_struct *k)
#ifdef CONFIG_BLK_CGROUP
WARN_ON_ONCE(kthread && kthread->blkcg_css);
#endif
+ kfree(kthread->full_name);
kfree(kthread);
}
@@ -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);
+ }
+ kthread->full_name = full_name;
+ }
set_task_comm(task, name);
/*
* root may have changed our (kthreadd's) priority or CPU mask.
--
2.17.1
On Mon, Nov 08, 2021 at 08:41:42AM +0000, Yafang Shao wrote:
> 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,
>
> jbd2/nvme0n1p2-
> xfs-reclaim/sdf
>
> Currently there are so many truncated kthreads:
>
> rcu_tasks_kthre
> rcu_tasks_rude_
> rcu_tasks_trace
> poll_mpt3sas0_s
> ext4-rsv-conver
> xfs-reclaim/sd{a, b, c, ...}
> xfs-blockgc/sd{a, b, c, ...}
> xfs-inodegc/sd{a, b, c, ...}
> audit_send_repl
> ecryptfs-kthrea
> vfio-irqfd-clea
> jbd2/nvme0n1p2-
> ...
>
> We can shorten these names to work around this problem, but it may be
> not applied to all of the truncated kthreads. Take 'jbd2/nvme0n1p2-' for
> example, it is a nice name, and it is not a good idea to shorten it.
>
> One possible way to fix this issue is extending the task comm size, but
> as task->comm is used in lots of places, that may cause some potential
> buffer overflows. Another more conservative approach is introducing a new
> pointer to store kthread's full name if it is truncated, which won't
> introduce too much overhead as it is in the non-critical path. Finally we
> make a dicision to use the second approach. See also the discussions in
> this thread:
> https://lore.kernel.org/lkml/[email protected]/
>
> After this change, the full name of these truncated kthreads will be
> displayed via /proc/[pid]/comm:
>
> rcu_tasks_kthread
> rcu_tasks_rude_kthread
> rcu_tasks_trace_kthread
> poll_mpt3sas0_statu
> ext4-rsv-conversion
> xfs-reclaim/sdf1
> xfs-blockgc/sdf1
> xfs-inodegc/sdf1
> audit_send_reply
> ecryptfs-kthread
> vfio-irqfd-cleanup
> jbd2/nvme0n1p2-8
>
> Suggested-by: Petr Mladek <[email protected]>
> Suggested-by: Steven Rostedt <[email protected]>
> Signed-off-by: Yafang Shao <[email protected]>
> Cc: Mathieu Desnoyers <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Andrii Nakryiko <[email protected]>
> Cc: Michal Miroslaw <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: David Hildenbrand <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Petr Mladek <[email protected]>
> ---
> TODO: will cleanup worker comm in the next step.
>
> ---
> fs/proc/array.c | 3 +++
> include/linux/kthread.h | 1 +
> kernel/kthread.c | 32 +++++++++++++++++++++++++++++++-
> 3 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index 49be8c8ef555..860e4deafa65 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -92,6 +92,7 @@
> #include <linux/string_helpers.h>
> #include <linux/user_namespace.h>
> #include <linux/fs_struct.h>
> +#include <linux/kthread.h>
>
> #include <asm/processor.h>
> #include "internal.h"
> @@ -102,6 +103,8 @@ void proc_task_name(struct seq_file *m, struct task_struct *p, bool escape)
>
> if (p->flags & PF_WQ_WORKER)
> wq_worker_comm(tcomm, sizeof(tcomm), p);
> + else if (p->flags & PF_KTHREAD)
> + get_kthread_comm(tcomm, sizeof(tcomm), p);
> else
> __get_task_comm(tcomm, sizeof(tcomm), p);
>
> diff --git a/include/linux/kthread.h b/include/linux/kthread.h
> index 346b0f269161..2a5c04494663 100644
> --- a/include/linux/kthread.h
> +++ b/include/linux/kthread.h
> @@ -33,6 +33,7 @@ struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
> unsigned int cpu,
> const char *namefmt);
>
> +void get_kthread_comm(char *buf, size_t buf_size, struct task_struct *tsk);
> void set_kthread_struct(struct task_struct *p);
>
> void kthread_set_per_cpu(struct task_struct *k, int cpu);
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 5b37a8567168..ce8258231eea 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -60,6 +60,8 @@ struct kthread {
> #ifdef CONFIG_BLK_CGROUP
> struct cgroup_subsys_state *blkcg_css;
> #endif
> + /* To store the full name if task comm is truncated. */
> + char *full_name;
> };
>
> enum KTHREAD_BITS {
> @@ -93,6 +95,18 @@ static inline struct kthread *__to_kthread(struct task_struct *p)
> return kthread;
> }
>
> +void get_kthread_comm(char *buf, size_t buf_size, struct task_struct *tsk)
> +{
> + struct kthread *kthread = to_kthread(tsk);
> +
> + if (!kthread || !kthread->full_name) {
> + __get_task_comm(buf, buf_size, tsk);
> + return;
> + }
> +
> + strscpy_pad(buf, kthread->full_name, buf_size);
> +}
> +
> void set_kthread_struct(struct task_struct *p)
> {
> struct kthread *kthread;
> @@ -121,6 +135,7 @@ void free_kthread_struct(struct task_struct *k)
> #ifdef CONFIG_BLK_CGROUP
> WARN_ON_ONCE(kthread && kthread->blkcg_css);
> #endif
> + kfree(kthread->full_name);
> kfree(kthread);
> }
>
> @@ -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);
> + }
> + kthread->full_name = full_name;
> + }
> set_task_comm(task, name);
> /*
> * root may have changed our (kthreadd's) priority or CPU mask.
> --
> 2.17.1
>
--
Kees Cook
On Wed, Nov 10, 2021 at 1:34 AM Kees Cook <[email protected]> wrote:
>
> On Mon, Nov 08, 2021 at 08:41:42AM +0000, Yafang Shao wrote:
> > 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,
> >
> > jbd2/nvme0n1p2-
> > xfs-reclaim/sdf
> >
> > Currently there are so many truncated kthreads:
> >
> > rcu_tasks_kthre
> > rcu_tasks_rude_
> > rcu_tasks_trace
> > poll_mpt3sas0_s
> > ext4-rsv-conver
> > xfs-reclaim/sd{a, b, c, ...}
> > xfs-blockgc/sd{a, b, c, ...}
> > xfs-inodegc/sd{a, b, c, ...}
> > audit_send_repl
> > ecryptfs-kthrea
> > vfio-irqfd-clea
> > jbd2/nvme0n1p2-
> > ...
> >
> > We can shorten these names to work around this problem, but it may be
> > not applied to all of the truncated kthreads. Take 'jbd2/nvme0n1p2-' for
> > example, it is a nice name, and it is not a good idea to shorten it.
> >
> > One possible way to fix this issue is extending the task comm size, but
> > as task->comm is used in lots of places, that may cause some potential
> > buffer overflows. Another more conservative approach is introducing a new
> > pointer to store kthread's full name if it is truncated, which won't
> > introduce too much overhead as it is in the non-critical path. Finally we
> > make a dicision to use the second approach. See also the discussions in
> > this thread:
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > After this change, the full name of these truncated kthreads will be
> > displayed via /proc/[pid]/comm:
> >
> > rcu_tasks_kthread
> > rcu_tasks_rude_kthread
> > rcu_tasks_trace_kthread
> > poll_mpt3sas0_statu
> > ext4-rsv-conversion
> > xfs-reclaim/sdf1
> > xfs-blockgc/sdf1
> > xfs-inodegc/sdf1
> > audit_send_reply
> > ecryptfs-kthread
> > vfio-irqfd-cleanup
> > jbd2/nvme0n1p2-8
> >
> > Suggested-by: Petr Mladek <[email protected]>
> > Suggested-by: Steven Rostedt <[email protected]>
> > Signed-off-by: Yafang Shao <[email protected]>
> > Cc: Mathieu Desnoyers <[email protected]>
> > Cc: Arnaldo Carvalho de Melo <[email protected]>
> > Cc: Alexei Starovoitov <[email protected]>
> > Cc: Andrii Nakryiko <[email protected]>
> > Cc: Michal Miroslaw <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Steven Rostedt <[email protected]>
> > Cc: Matthew Wilcox <[email protected]>
> > Cc: David Hildenbrand <[email protected]>
> > Cc: Al Viro <[email protected]>
> > Cc: Kees Cook <[email protected]>
> > Cc: Petr Mladek <[email protected]>
> > ---
> > TODO: will cleanup worker comm in the next step.
> >
> > ---
> > fs/proc/array.c | 3 +++
> > include/linux/kthread.h | 1 +
> > kernel/kthread.c | 32 +++++++++++++++++++++++++++++++-
> > 3 files changed, 35 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/proc/array.c b/fs/proc/array.c
> > index 49be8c8ef555..860e4deafa65 100644
> > --- a/fs/proc/array.c
> > +++ b/fs/proc/array.c
> > @@ -92,6 +92,7 @@
> > #include <linux/string_helpers.h>
> > #include <linux/user_namespace.h>
> > #include <linux/fs_struct.h>
> > +#include <linux/kthread.h>
> >
> > #include <asm/processor.h>
> > #include "internal.h"
> > @@ -102,6 +103,8 @@ void proc_task_name(struct seq_file *m, struct task_struct *p, bool escape)
> >
> > if (p->flags & PF_WQ_WORKER)
> > wq_worker_comm(tcomm, sizeof(tcomm), p);
> > + else if (p->flags & PF_KTHREAD)
> > + get_kthread_comm(tcomm, sizeof(tcomm), p);
> > else
> > __get_task_comm(tcomm, sizeof(tcomm), p);
> >
> > diff --git a/include/linux/kthread.h b/include/linux/kthread.h
> > index 346b0f269161..2a5c04494663 100644
> > --- a/include/linux/kthread.h
> > +++ b/include/linux/kthread.h
> > @@ -33,6 +33,7 @@ struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
> > unsigned int cpu,
> > const char *namefmt);
> >
> > +void get_kthread_comm(char *buf, size_t buf_size, struct task_struct *tsk);
> > void set_kthread_struct(struct task_struct *p);
> >
> > void kthread_set_per_cpu(struct task_struct *k, int cpu);
> > diff --git a/kernel/kthread.c b/kernel/kthread.c
> > index 5b37a8567168..ce8258231eea 100644
> > --- a/kernel/kthread.c
> > +++ b/kernel/kthread.c
> > @@ -60,6 +60,8 @@ struct kthread {
> > #ifdef CONFIG_BLK_CGROUP
> > struct cgroup_subsys_state *blkcg_css;
> > #endif
> > + /* To store the full name if task comm is truncated. */
> > + char *full_name;
> > };
> >
> > enum KTHREAD_BITS {
> > @@ -93,6 +95,18 @@ static inline struct kthread *__to_kthread(struct task_struct *p)
> > return kthread;
> > }
> >
> > +void get_kthread_comm(char *buf, size_t buf_size, struct task_struct *tsk)
> > +{
> > + struct kthread *kthread = to_kthread(tsk);
> > +
> > + if (!kthread || !kthread->full_name) {
> > + __get_task_comm(buf, buf_size, tsk);
> > + return;
> > + }
> > +
> > + strscpy_pad(buf, kthread->full_name, buf_size);
> > +}
> > +
> > void set_kthread_struct(struct task_struct *p)
> > {
> > struct kthread *kthread;
> > @@ -121,6 +135,7 @@ void free_kthread_struct(struct task_struct *k)
> > #ifdef CONFIG_BLK_CGROUP
> > WARN_ON_ONCE(kthread && kthread->blkcg_css);
> > #endif
> > + kfree(kthread->full_name);
> > kfree(kthread);
> > }
> >
> > @@ -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:
>
It is OK for me.
I will do it as you suggested and show a warning for this case.
> struct kthread *kthread = to_kthread(task);
>
> kthread->full_name = kvasprintf(GFP_KERNEL, namefmt, args);
>
> > + }
> > + kthread->full_name = full_name;
> > + }
> > set_task_comm(task, name);
> > /*
> > * root may have changed our (kthreadd's) priority or CPU mask.
> > --
> > 2.17.1
> >
>
> --
> Kees Cook
--
Thanks
Yafang
On Mon 2021-11-08 08:41:42, Yafang Shao wrote:
> 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/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -102,6 +103,8 @@ void proc_task_name(struct seq_file *m, struct task_struct *p, bool escape)
>
> if (p->flags & PF_WQ_WORKER)
> wq_worker_comm(tcomm, sizeof(tcomm), p);
Just for record. I though that this patch obsoleted wq_worker_comm()
but it did not. wq_worker_comm() returns different values
depending on the last proceed work item and has to stay.
> + else if (p->flags & PF_KTHREAD)
> + get_kthread_comm(tcomm, sizeof(tcomm), p);
> else
> __get_task_comm(tcomm, sizeof(tcomm), p);
>
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -121,6 +135,7 @@ void free_kthread_struct(struct task_struct *k)
Hmm, there is the following comment:
/*
* Can be NULL if this kthread was created by kernel_thread()
* or if kmalloc() in kthread() failed.
*/
kthread = to_kthread(k);
And indeed, set_kthread_struct() is called only by kthread()
and init_idle().
For example, call_usermodehelper_exec_sync() calls kernel_thread()
but given @fn does not call set_kthread_struct(). Also init_idle()
continues even when the allocation failed.
> #ifdef CONFIG_BLK_CGROUP
> WARN_ON_ONCE(kthread && kthread->blkcg_css);
> #endif
> + kfree(kthread->full_name);
Hence, we have to make sure that it is not NULL here. I suggest
something like:
void free_kthread_struct(struct task_struct *k)
{
struct kthread *kthread;
/*
* Can be NULL if this kthread was created by kernel_thread()
* or if kmalloc() in kthread() failed.
*/
kthread = to_kthread(k);
if (!kthread)
return;
#ifdef CONFIG_BLK_CGROUP
WARN_ON_ONCE(kthread->blkcg_css);
#endif
kfree(kthread->full_name);
kfree(kthread);
}
Side note: The possible NULL pointer looks dangerous to
me. to_kthread() is dereferenced without any check on
several locations.
For example, kthread_create_on_cpu() looks safe. It is a kthread
crated by kthread(). It will exists only when the allocation
succeeded.
kthread_stop() is probably safe only because it used only for
the classic kthreads created by kthread(). But the API
is not safe.
kthread_use_mm() is probably used only by classic kthreads as
well. But it is less clear to me.
All this unsafe APIs looks like a ticking bomb to me. But
it is beyond this patchset.
> kfree(kthread);
> }
>
Best Regards,
Petr
On Wed 2021-11-10 10:12:17, Yafang Shao wrote:
> On Wed, Nov 10, 2021 at 1:34 AM Kees Cook <[email protected]> wrote:
> >
> > On Mon, Nov 08, 2021 at 08:41:42AM +0000, Yafang Shao wrote:
> > > 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.
> >
> > > + }
> > > + kthread->full_name = full_name;
> > > + }
> > > set_task_comm(task, name);
> > > /*
> > > * root may have changed our (kthreadd's) priority or CPU mask.
Best Regards,
Petr
On Fri, Nov 12, 2021 at 11:34 PM Petr Mladek <[email protected]> wrote:
>
> On Mon 2021-11-08 08:41:42, Yafang Shao wrote:
> > 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/fs/proc/array.c
> > +++ b/fs/proc/array.c
> > @@ -102,6 +103,8 @@ void proc_task_name(struct seq_file *m, struct task_struct *p, bool escape)
> >
> > if (p->flags & PF_WQ_WORKER)
> > wq_worker_comm(tcomm, sizeof(tcomm), p);
>
> Just for record. I though that this patch obsoleted wq_worker_comm()
> but it did not. wq_worker_comm() returns different values
> depending on the last proceed work item and has to stay.
>
Right. worker comm is changed dynamically, which is combined by
(task_comm+worker_desc) or (task_comm-worker_desc).
I planned to remove the whole worker->desc and set it dynamically to
the new kthread full_name but I found it may not be a good idea.
> > + else if (p->flags & PF_KTHREAD)
> > + get_kthread_comm(tcomm, sizeof(tcomm), p);
> > else
> > __get_task_comm(tcomm, sizeof(tcomm), p);
> >
> > --- a/kernel/kthread.c
> > +++ b/kernel/kthread.c
> > @@ -121,6 +135,7 @@ void free_kthread_struct(struct task_struct *k)
>
> Hmm, there is the following comment:
>
> /*
> * Can be NULL if this kthread was created by kernel_thread()
> * or if kmalloc() in kthread() failed.
> */
> kthread = to_kthread(k);
>
> And indeed, set_kthread_struct() is called only by kthread()
> and init_idle().
>
> For example, call_usermodehelper_exec_sync() calls kernel_thread()
> but given @fn does not call set_kthread_struct(). Also init_idle()
> continues even when the allocation failed.
>
Yes, it really can be NULL.
>
> > #ifdef CONFIG_BLK_CGROUP
> > WARN_ON_ONCE(kthread && kthread->blkcg_css);
> > #endif
> > + kfree(kthread->full_name);
>
> Hence, we have to make sure that it is not NULL here. I suggest
> something like:
>
Agreed. I will do it.
> void free_kthread_struct(struct task_struct *k)
> {
> struct kthread *kthread;
>
> /*
> * Can be NULL if this kthread was created by kernel_thread()
> * or if kmalloc() in kthread() failed.
> */
> kthread = to_kthread(k);
> if (!kthread)
> return;
>
> #ifdef CONFIG_BLK_CGROUP
> WARN_ON_ONCE(kthread->blkcg_css);
> #endif
> kfree(kthread->full_name);
> kfree(kthread);
> }
>
>
> Side note: The possible NULL pointer looks dangerous to
> me. to_kthread() is dereferenced without any check on
> several locations.
>
> For example, kthread_create_on_cpu() looks safe. It is a kthread
> crated by kthread(). It will exists only when the allocation
> succeeded.
>
> kthread_stop() is probably safe only because it used only for
> the classic kthreads created by kthread(). But the API
> is not safe.
>
> kthread_use_mm() is probably used only by classic kthreads as
> well. But it is less clear to me.
>
> All this unsafe APIs looks like a ticking bomb to me. But
> it is beyond this patchset.
>
I will analyze it in depth and try to dismantle this ticking bomb.
--
Thanks
Yafang