This has us pass in the thread's name during creation in kernel_thread.
Signed-off-by: Mike Christie <[email protected]>
---
kernel/kthread.c | 35 ++++++++++++++---------------------
1 file changed, 14 insertions(+), 21 deletions(-)
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 63574cee925e..831a55b406d8 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -38,6 +38,7 @@ struct task_struct *kthreadd_task;
struct kthread_create_info
{
/* Information passed to kthread() from kthreadd. */
+ char *full_name;
int (*threadfn)(void *data);
void *data;
int node;
@@ -343,10 +344,15 @@ static int kthread(void *_create)
/* Release the structure when caller killed by a fatal signal. */
done = xchg(&create->done, NULL);
if (!done) {
+ kfree(create->full_name);
kfree(create);
kthread_exit(-EINTR);
}
+ if (strlen(create->full_name) >= TASK_COMM_LEN)
+ self->full_name = create->full_name;
+ else
+ kfree(create->full_name);
self->threadfn = threadfn;
self->data = data;
@@ -396,12 +402,13 @@ static void create_kthread(struct kthread_create_info *create)
current->pref_node_fork = create->node;
#endif
/* We want our own signal handler (we take no signals by default). */
- pid = kernel_thread(kthread, create, NULL,
+ pid = kernel_thread(kthread, create, create->full_name,
CLONE_FS | CLONE_FILES | SIGCHLD);
if (pid < 0) {
/* Release the structure when caller killed by a fatal signal. */
struct completion *done = xchg(&create->done, NULL);
+ kfree(create->full_name);
if (!done) {
kfree(create);
return;
@@ -428,6 +435,11 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
create->data = data;
create->node = node;
create->done = &done;
+ create->full_name = kvasprintf(GFP_KERNEL, namefmt, args);
+ if (!create->full_name) {
+ task = ERR_PTR(-ENOMEM);
+ goto free_create;
+ }
spin_lock(&kthread_create_lock);
list_add_tail(&create->list, &kthread_create_list);
@@ -454,26 +466,7 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
wait_for_completion(&done);
}
task = create->result;
- if (!IS_ERR(task)) {
- char name[TASK_COMM_LEN];
- va_list aq;
- int len;
-
- /*
- * task is already visible to other tasks, so updating
- * COMM must be protected.
- */
- 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);
-
- /* leave it truncated when out of memory. */
- kthread->full_name = kvasprintf(GFP_KERNEL, namefmt, args);
- }
- set_task_comm(task, name);
- }
+free_create:
kfree(create);
return task;
}
--
2.25.1
On Fri, Mar 10, 2023 at 04:03:24PM -0600, Mike Christie wrote:
> This has us pass in the thread's name during creation in kernel_thread.
>
> Signed-off-by: Mike Christie <[email protected]>
> ---
> kernel/kthread.c | 35 ++++++++++++++---------------------
> 1 file changed, 14 insertions(+), 21 deletions(-)
>
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 63574cee925e..831a55b406d8 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -38,6 +38,7 @@ struct task_struct *kthreadd_task;
> struct kthread_create_info
> {
> /* Information passed to kthread() from kthreadd. */
> + char *full_name;
> int (*threadfn)(void *data);
> void *data;
> int node;
> @@ -343,10 +344,15 @@ static int kthread(void *_create)
> /* Release the structure when caller killed by a fatal signal. */
> done = xchg(&create->done, NULL);
> if (!done) {
> + kfree(create->full_name);
> kfree(create);
> kthread_exit(-EINTR);
> }
>
> + if (strlen(create->full_name) >= TASK_COMM_LEN)
> + self->full_name = create->full_name;
> + else
> + kfree(create->full_name);
This is monir but wwiw, this looks suspicious when reading it without
more context. It took me a while to see that kthread->full_name is
intended to store the untruncated name only if truncation actually needs
to happen. So either we should always initialize this or we should add a
comment. You can just send a tiny patch that I can fold into this one so
you don't have to resend the whole series...
On 3/11/23 2:53 AM, Christian Brauner wrote:
> On Fri, Mar 10, 2023 at 04:03:24PM -0600, Mike Christie wrote:
>> This has us pass in the thread's name during creation in kernel_thread.
>>
>> Signed-off-by: Mike Christie <[email protected]>
>> ---
>> kernel/kthread.c | 35 ++++++++++++++---------------------
>> 1 file changed, 14 insertions(+), 21 deletions(-)
>>
>> diff --git a/kernel/kthread.c b/kernel/kthread.c
>> index 63574cee925e..831a55b406d8 100644
>> --- a/kernel/kthread.c
>> +++ b/kernel/kthread.c
>> @@ -38,6 +38,7 @@ struct task_struct *kthreadd_task;
>> struct kthread_create_info
>> {
>> /* Information passed to kthread() from kthreadd. */
>> + char *full_name;
>> int (*threadfn)(void *data);
>> void *data;
>> int node;
>> @@ -343,10 +344,15 @@ static int kthread(void *_create)
>> /* Release the structure when caller killed by a fatal signal. */
>> done = xchg(&create->done, NULL);
>> if (!done) {
>> + kfree(create->full_name);
>> kfree(create);
>> kthread_exit(-EINTR);
>> }
>>
>> + if (strlen(create->full_name) >= TASK_COMM_LEN)
>> + self->full_name = create->full_name;
>> + else
>> + kfree(create->full_name);
>
> This is monir but wwiw, this looks suspicious when reading it without
> more context. It took me a while to see that kthread->full_name is
> intended to store the untruncated name only if truncation actually needs
> to happen. So either we should always initialize this or we should add a
> comment. You can just send a tiny patch that I can fold into this one so
> you don't have to resend the whole series...
Ok. Thanks. I think always initializing it would make the code a lot easier
to understand and be nicer.
I don't think it would be too much extra memory used, and we don't have
to worry about some random userspace app breaking because it wanted to
configure the thread but the name got truncated.
On 3/11/23 10:11 AM, [email protected] wrote:
> On 3/11/23 2:53 AM, Christian Brauner wrote:
>> On Fri, Mar 10, 2023 at 04:03:24PM -0600, Mike Christie wrote:
>>> This has us pass in the thread's name during creation in kernel_thread.
>>>
>>> Signed-off-by: Mike Christie <[email protected]>
>>> ---
>>> kernel/kthread.c | 35 ++++++++++++++---------------------
>>> 1 file changed, 14 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/kernel/kthread.c b/kernel/kthread.c
>>> index 63574cee925e..831a55b406d8 100644
>>> --- a/kernel/kthread.c
>>> +++ b/kernel/kthread.c
>>> @@ -38,6 +38,7 @@ struct task_struct *kthreadd_task;
>>> struct kthread_create_info
>>> {
>>> /* Information passed to kthread() from kthreadd. */
>>> + char *full_name;
>>> int (*threadfn)(void *data);
>>> void *data;
>>> int node;
>>> @@ -343,10 +344,15 @@ static int kthread(void *_create)
>>> /* Release the structure when caller killed by a fatal signal. */
>>> done = xchg(&create->done, NULL);
>>> if (!done) {
>>> + kfree(create->full_name);
>>> kfree(create);
>>> kthread_exit(-EINTR);
>>> }
>>>
>>> + if (strlen(create->full_name) >= TASK_COMM_LEN)
>>> + self->full_name = create->full_name;
>>> + else
>>> + kfree(create->full_name);
>>
>> This is monir but wwiw, this looks suspicious when reading it without
>> more context. It took me a while to see that kthread->full_name is
>> intended to store the untruncated name only if truncation actually needs
>> to happen. So either we should always initialize this or we should add a
>> comment. You can just send a tiny patch that I can fold into this one so
>> you don't have to resend the whole series...
Hey Christian, here is a patch you can fold into the original. Thanks
for your help.
From ac82986ec4e7faae245ec48cb9213a4ca1c1d4d6 Mon Sep 17 00:00:00 2001
From: Mike Christie <[email protected]>
Date: Sat, 11 Mar 2023 16:14:24 -0600
Subject: [PATCH] kthread: Always save the full_name
Simplify the kthread name handling by always using the full_name.
---
kernel/kthread.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 831a55b406d8..5596ec3f75cf 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -349,10 +349,7 @@ static int kthread(void *_create)
kthread_exit(-EINTR);
}
- if (strlen(create->full_name) >= TASK_COMM_LEN)
- self->full_name = create->full_name;
- else
- kfree(create->full_name);
+ self->full_name = create->full_name;
self->threadfn = threadfn;
self->data = data;