Linux Virtualization - Mar 2023 - [PATCH 03/11] kthread: Pass in the thread's name during creation

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 <michael.christie at oracle.com>
>> ---
>>  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, michael.christie at oracle.com
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 <michael.christie at oracle.com>
>>> ---
>>>  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 <michael.christie at oracle.com>
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;