Removal of most do_exit calls

My s390 assembly is not particularly good so I have read the history of the reference to do_exit copy_thread and have been able to verify that do_exit is not used.

The general argument is that s390 has been changed to use the generic kernel_thread and kernel_execve and the generic versions do not call do_exit. So it is strange to see a do_exit reference sitting there.

The history of the do_exit reference in s390's version of copy_thread seems conclusive that the do_exit reference is something that lingers and should have been removed several years ago.

Remove this dead reference to do_exit to make it clear that s390 is not doing anything with do_exit in copy_thread.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 arch/s390/kernel/process.c | 1 -
 1 file changed, 1 deletion(-)

Quoted 11 lines · Eric W. Biederman

My s390 assembly is not particularly good so I have read the history of the reference to do_exit copy_thread and have been able to verify that do_exit is not used.

The general argument is that s390 has been changed to use the generic kernel_thread and kernel_execve and the generic versions do not call do_exit. So it is strange to see a do_exit reference sitting there.

The history of the do_exit reference in s390's version of copy_thread seems conclusive that the do_exit reference is something that lingers and should have been removed several years ago.

...

Quoted 3 lines

Remove this dead reference to do_exit to make it clear that s390 is not doing anything with do_exit in copy_thread.

Quoted 4 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 arch/s390/kernel/process.c | 1 -
 1 file changed, 1 deletion(-)

Applied to s390 tree. Just in case you want to apply this to your tree too:

Acked-by Heiko Carstens <[email protected]>

Quoted 11 lines · Eric W. Biederman

My s390 assembly is not particularly good so I have read the history of the reference to do_exit copy_thread and have been able to verify that do_exit is not used.

The general argument is that s390 has been changed to use the generic kernel_thread and kernel_execve and the generic versions do not call do_exit. So it is strange to see a do_exit reference sitting there.

The history of the do_exit reference in s390's version of copy_thread seems conclusive that the do_exit reference is something that lingers and should have been removed several years ago.

...

Quoted 3 lines

Remove this dead reference to do_exit to make it clear that s390 is not doing anything with do_exit in copy_thread.

Quoted 4 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 arch/s390/kernel/process.c | 1 -
 1 file changed, 1 deletion(-)

Applied to s390 tree. Just in case you want to apply this to your tree too:

Acked-by Heiko Carstens <[email protected]>

There are two big uses of do_exit. The first is it's design use to be the guts of the exit(2) system call. The second use is to terminate a task after something catastrophic has happened like a NULL pointer in kernel code.

Add a function make_task_dead that is initialy exactly the same as do_exit to cover the cases where do_exit is called to handle catastrophic failure. In time this can probably be reduced to just a light wrapper around do_task_dead. For now keep it exactly the same so that there will be no behavioral differences introducing this new concept.

Replace all of the uses of do_exit that use it for catastraphic task cleanup with make_task_dead to make it clear what the code is doing.

As part of this rename rewind_stack_do_exit
rewind_stack_and_make_dead.

Quoted 16 lines · Eric W. Biederman

There are two big uses of do_exit. The first is it's design use to be the guts of the exit(2) system call. The second use is to terminate a task after something catastrophic has happened like a NULL pointer in kernel code.

Add a function make_task_dead that is initialy exactly the same as do_exit to cover the cases where do_exit is called to handle catastrophic failure. In time this can probably be reduced to just a light wrapper around do_task_dead. For now keep it exactly the same so that there will be no behavioral differences introducing this new concept.

Replace all of the uses of do_exit that use it for catastraphic task cleanup with make_task_dead to make it clear what the code is doing.

Quoted 2 lines

As part of this rename rewind_stack_do_exit
rewind_stack_and_make_dead.

Umm... What about .Linvalid_mask: in arch/xtensa/kernel/entry.S?
That's an obvious case for your make_task_dead().

Quoted 20 lines · Al Viro

Quoted 18 lines · Eric W. Biederman

There are two big uses of do_exit. The first is it's design use to be the guts of the exit(2) system call. The second use is to terminate a task after something catastrophic has happened like a NULL pointer in kernel code.

Add a function make_task_dead that is initialy exactly the same as do_exit to cover the cases where do_exit is called to handle catastrophic failure. In time this can probably be reduced to just a light wrapper around do_task_dead. For now keep it exactly the same so that there will be no behavioral differences introducing this new concept.

Replace all of the uses of do_exit that use it for catastraphic task cleanup with make_task_dead to make it clear what the code is doing.

As part of this rename rewind_stack_do_exit
rewind_stack_and_make_dead.

Quoted 2 lines

Umm... What about .Linvalid_mask: in arch/xtensa/kernel/entry.S?
That's an obvious case for your make_task_dead().

Good catch.

Being in assembly it did not have anything after the name do_exit so it hid from my regex "[^A-Za-z0-9_]do_exit[^A-Za-z0-9]". Thank you for finding that.

Quoted 4 lines · Eric W. Biederman

Being in assembly it did not have anything after the name do_exit so it hid from my regex "[^A-Za-z0-9_]do_exit[^A-Za-z0-9]". Thank you for finding that.

Umm... What's wrong with '\<do_exit\>'?

Quoted 8 lines · Al Viro

Quoted 4 lines · Eric W. Biederman

Being in assembly it did not have anything after the name do_exit so it hid from my regex "[^A-Za-z0-9_]do_exit[^A-Za-z0-9]". Thank you for finding that.

Umm... What's wrong with '\<do_exit\>'?

Christ people, you both make it so complicated.

If you want to search for 'do_exit', just do

git grep -w do_exit

where that '-w' does exactly that "word boundary" thing.

I thought everybody knew about this, because it's such a common thing to do - checking my shell history, more than a third of my "git grep" uses use '-w', exactly because it's very convenient for identifier lookup

But yes, in more complex cases where you have other parts to the pattern (ie you're not looking *just* for a single word), by all means use '\<' and/or '\>'.

The beginning of do_exit has become cluttered and difficult to read as it is filled with checks to handle things that can only happen when the kernel is operating improperly.

Now that we have a dedicated function for cleaning up a task when the kernel is operating improperly move the checks there.

Quoted 6 lines · Eric W. Biederman

The beginning of do_exit has become cluttered and difficult to read as it is filled with checks to handle things that can only happen when the kernel is operating improperly.

Now that we have a dedicated function for cleaning up a task when the kernel is operating improperly move the checks there.

Umm... I would probably take profile_task_exit() crap out before that point.
1) the damn thing is dead - nothing registers notifiers there
2) blocking_notifier_call_chain() is not a nice thing to do on oops...

I'll post a patch ripping the dead parts of kernel/profile.c out tomorrow morning (there's also profile_handoff_task(), equally useless these days and complicating things for __put_task_struct()).

Quoted 6 lines · Eric W. Biederman

The beginning of do_exit has become cluttered and difficult to read as it is filled with checks to handle things that can only happen when the kernel is operating improperly.

Now that we have a dedicated function for cleaning up a task when the kernel is operating improperly move the checks there.

Umm... I would probably take profile_task_exit() crap out before that point.
1) the damn thing is dead - nothing registers notifiers there
2) blocking_notifier_call_chain() is not a nice thing to do on oops...

I'll post a patch ripping the dead parts of kernel/profile.c out tomorrow morning (there's also profile_handoff_task(), equally useless these days and complicating things for __put_task_struct()).

Quoted 13 lines · Al Viro

Quoted 6 lines · Eric W. Biederman

The beginning of do_exit has become cluttered and difficult to read as it is filled with checks to handle things that can only happen when the kernel is operating improperly.

Now that we have a dedicated function for cleaning up a task when the kernel is operating improperly move the checks there.

Umm... I would probably take profile_task_exit() crap out before that point.
1) the damn thing is dead - nothing registers notifiers there
2) blocking_notifier_call_chain() is not a nice thing to do on oops...

Quoted 3 lines

I'll post a patch ripping the dead parts of kernel/profile.c out tomorrow morning (there's also profile_handoff_task(), equally useless these days and complicating things for __put_task_struct()).

Argh... OK, so your subsequent series had pretty much the same thing. My apologies - still digging myself out from mail pile that had accumulated over two months ;-/

kernel/exit.c void __noreturn do_exit(long code) · line 759

Quoted 8 lines · Eric W. Biederman

kernel/exit.c +0 -8

-	/*
-	 * If do_exit is called because this processes oopsed, it's possible
-	 * that get_fs() was left as KERNEL_DS, so reset it to USER_DS before
-	 * continuing. Amongst other possible reasons, this is to prevent
-	 * mm_release()->clear_child_tid() from writing to a user-controlled
-	 * kernel address.
-	 */
-	force_uaccess_begin();

Are you sure about that one? It shouldn't matter, but... it's a potential change for do_exit() from a kernel thread. As it is, we have that force_uaccess_begin() for exiting threads and for kernel ones it's not a no-op. I'm not concerned about attempted userland access after that point for those, obviously, but I'm not sure you won't step into something subtle here.

I would prefer to split that particular change off into a separate commit...

Assuming it won't be too much longer before the rest of the arches have set_fs/get_fs removed it looks like it makes sense to leave the force_uaccess_begin where it is, and just let force_uaccess_begin be removed when set_fs/get_fs are removed from the tree.

Christoph does it look like the set_fs/get_fs removal work is going to stall indefinitely on some architectures? If so I think we want to find a way to get kernel threads to run with set_fs(USER_DS) on the stalled architectures. Otherwise I think we have a real hazard of introducing bugs that will only show up on the stalled architectures.

Quoted 10 lines · Eric W. Biederman

Assuming it won't be too much longer before the rest of the arches have set_fs/get_fs removed it looks like it makes sense to leave the force_uaccess_begin where it is, and just let force_uaccess_begin be removed when set_fs/get_fs are removed from the tree.

Christoph does it look like the set_fs/get_fs removal work is going to stall indefinitely on some architectures? If so I think we want to find a way to get kernel threads to run with set_fs(USER_DS) on the stalled architectures. Otherwise I think we have a real hazard of introducing bugs that will only show up on the stalled architectures.

I really need help from the arch maintainers to finish the set_fs removal. There have been very few arch maintainers helping with that work (arm, arm64, parisc, m68k) in addition to the ones I did because I have the test setups and knowledge. I'll send out another ping,

Quoted 4 lines

I really need help from the arch maintainers to finish the set_fs removal. There have been very few arch maintainers helping with that work (arm, arm64, parisc, m68k) in addition to the ones I did because I have the test setups and knowledge. I'll send out another ping,

Just in case you missed it: s390 was converted with commit 87d598634521 ("s390/mm: remove set_fs / rework address space handling").

Quoted 10 lines · Eric W. Biederman

Assuming it won't be too much longer before the rest of the arches have set_fs/get_fs removed it looks like it makes sense to leave the force_uaccess_begin where it is, and just let force_uaccess_begin be removed when set_fs/get_fs are removed from the tree.

Christoph does it look like the set_fs/get_fs removal work is going to stall indefinitely on some architectures? If so I think we want to find a way to get kernel threads to run with set_fs(USER_DS) on the stalled architectures. Otherwise I think we have a real hazard of introducing bugs that will only show up on the stalled architectures.

I really need help from the arch maintainers to finish the set_fs removal. There have been very few arch maintainers helping with that work (arm, arm64, parisc, m68k) in addition to the ones I did because I have the test setups and knowledge. I'll send out another ping, for necrotic architectures like ia64 and sh I have very little hope.

From: Arnd Bergmann <[email protected]>

I picked microblaze as one of the architectures that still use set_fs() and converted it not to.

I can't see any issue with the patch when I run it on real HW.

Tested-by Michal Simek <[email protected]>

Christoph: Is there any recommended test suite which I should run?

Quoted 4 lines · Michal Simek

I can't see any issue with the patch when I run it on real HW.

Tested-by Michal Simek <[email protected]>

Christoph: Is there any recommended test suite which I should run?

No. For architectures that already didn't use set_fs internally there is nothing specific to test. Running some perf or backtrace tests might be useful to check if the non-faulting kernel helpers work properly.

Hi Arnd,

po 17. 1. 2022 v 14:28 odesílatel Arnd Bergmann <[email protected]> napsal:

Quoted 1 line

Quoted 4 lines

From: Arnd Bergmann <[email protected]>

I picked microblaze as one of the architectures that still use set_fs() and converted it not to.

Can you please update the commit message because what is above is not the right one?

Quoted 7 lines · Michal Simek

Hi Arnd,

po 17. 1. 2022 v 14:28 odesílatel Arnd Bergmann <[email protected]> napsal:

Quoted 3 lines

From: Arnd Bergmann <[email protected]>

Quoted 5 lines

Quoted 2 lines

I picked microblaze as one of the architectures that still use set_fs() and converted it not to.

Can you please update the commit message because what is above is not the right one?

Ah, sorry about that. I think you can copy from the openrisc patch, see https://lore.kernel.org/lkml/[email protected]/

Quoted 11 lines · Arnd Bergmann

Quoted 10 lines · Michal Simek

Hi Arnd,

po 17. 1. 2022 v 14:28 odesílatel Arnd Bergmann <[email protected]> napsal:

Quoted 5 lines

From: Arnd Bergmann <[email protected]>

I picked microblaze as one of the architectures that still use set_fs() and converted it not to.

Quoted 5 lines

Quoted 2 lines

Can you please update the commit message because what is above is not the right one?

Ah, sorry about that. I think you can copy from the openrisc patch, see https://lore.kernel.org/lkml/[email protected]/

Please do it. You are the author of this patch and we should follow the process.

Quoted 18 lines · Michal Simek

Quoted 16 lines · Arnd Bergmann

Quoted 5 lines · Michal Simek

Hi Arnd,

po 17. 1. 2022 v 14:28 odesílatel Arnd Bergmann <[email protected]> napsal:

Quoted 1 line

Quoted 7 lines

Quoted 4 lines

From: Arnd Bergmann <[email protected]>

I picked microblaze as one of the architectures that still use set_fs() and converted it not to.

Can you please update the commit message because what is above is not the right one?

Ah, sorry about that. I think you can copy from the openrisc patch, see https://lore.kernel.org/lkml/[email protected]/

Quoted 1 line

Please do it. You are the author of this patch and we should follow the process.

Done.

Looking at it again, I wonder if it would help to use the __get_kernel_nofault() and __get_kernel_nofault() helpers as the default in include/asm-generic/uaccess.h.

I see it's identical to the openrisc version and would probably be the same for some of the other architectures that have no other use for set_fs(). That may
help to do a bulk remove of set_fs for alpha, arc, csky, h8300, hexagon, nds32, nios2, um and extensa, leaving only ia64, sparc and sh.

Quoted 19 lines · Arnd Bergmann

Quoted 18 lines · Michal Simek

Quoted 16 lines · Arnd Bergmann

Quoted 5 lines · Michal Simek

Hi Arnd,

po 17. 1. 2022 v 14:28 odesílatel Arnd Bergmann <[email protected]> napsal:

Quoted 1 line

Quoted 7 lines

Quoted 4 lines

From: Arnd Bergmann <[email protected]>

I picked microblaze as one of the architectures that still use set_fs() and converted it not to.

Can you please update the commit message because what is above is not the right one?

Ah, sorry about that. I think you can copy from the openrisc patch, see https://lore.kernel.org/lkml/[email protected]/

Quoted 7 lines

Quoted 1 line

Please do it. You are the author of this patch and we should follow the process.

Done.

Looking at it again, I wonder if it would help to use the __get_kernel_nofault() and __get_kernel_nofault() helpers as the default in include/asm-generic/uaccess.h.

That would make sense. Perhaps also the __range_ok() function from OpenRISC could move there as I think other architectures would also want to use that.

Quoted 5 lines

I see it's identical to the openrisc version and would probably be the same for some of the other architectures that have no other use for set_fs(). That may
help to do a bulk remove of set_fs for alpha, arc, csky, h8300, hexagon, nds32, nios2, um and extensa, leaving only ia64, sparc and sh.

If you could add it into include/asm-generic/uaccess.h I can test changing my patch to use it.

Quoted 6 lines · Stafford Horne

Quoted 3 lines

Looking at it again, I wonder if it would help to use the __get_kernel_nofault() and __get_kernel_nofault() helpers as the default in include/asm-generic/uaccess.h.

That would make sense. Perhaps also the __range_ok() function from OpenRISC could move there as I think other architectures would also want to use that.

Quoted 6 lines

Quoted 5 lines

I see it's identical to the openrisc version and would probably be the same for some of the other architectures that have no other use for set_fs(). That may
help to do a bulk remove of set_fs for alpha, arc, csky, h8300, hexagon, nds32, nios2, um and extensa, leaving only ia64, sparc and sh.

Quoted 2 lines

If you could add it into include/asm-generic/uaccess.h I can test changing my patch to use it.

Note, I would be happy to do the work to move these into include/asm-generic/uaccess.h. But as I see it the existing include/asm-generic/uaccess.h is for NOMMU. How should we go about having an MMU and NOMMU version? Should we move uaccess.h to uaccess-nommu.h? Or add more ifdefs to uaccess.h?

I have now uploaded a cleanup series to
https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=set_fs

This uses the same access_ok() function across almost all architectures, with the exception of those that need something else, and I then I went further and killed off set_fs for everything other than ia64.

Quoted 8 lines · Arnd Bergmann

I have now uploaded a cleanup series to
https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=set_fs

This uses the same access_ok() function across almost all architectures, with the exception of those that need something else, and I then I went further and killed off set_fs for everything other than ia64.

Thanks, looks good to me.

Can you say why you didn't convert ia64? I don't see any set_fs() use there, except for the unaligned handler, which looks trivial to remove. It looks like the only reason for it is kernel-mode unaligned exceptions, which we should just turn fatal, I suspect (they already get logged).

And ia64 people could make the unaligned handling do the kernel mode case in emulate_load/store_int() - it doesn't look *that* painful.

But maybe you noticed something else?

It would be really good to just be able to say that set_fs() no longer exists at all.

Quoted 18 lines · Linus Torvalds

Quoted 8 lines · Arnd Bergmann

I have now uploaded a cleanup series to
https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=set_fs

This uses the same access_ok() function across almost all architectures, with the exception of those that need something else, and I then I went further and killed off set_fs for everything other than ia64.

Thanks, looks good to me.

Can you say why you didn't convert ia64? I don't see any set_fs() use there, except for the unaligned handler, which looks trivial to remove. It looks like the only reason for it is kernel-mode unaligned exceptions, which we should just turn fatal, I suspect (they already get logged).

Quoted 7 lines

And ia64 people could make the unaligned handling do the kernel mode case in emulate_load/store_int() - it doesn't look *that* painful.

But maybe you noticed something else?

It would be really good to just be able to say that set_fs() no longer exists at all.

I had previously gotten stuck at ia64, but gave it another go now and uploaded an updated branch with ia64 taken care of and another patch to clean up bits afterwards.

I only gave it light testing so far, mainly building the defconfig for every architecture. I'll post the series once the build bots are happy with the branch overall.

Quoted 5 lines · Arnd Bergmann

I had previously gotten stuck at ia64, but gave it another go now and uploaded an updated branch with ia64 taken care of and another patch to clean up bits afterwards.

Quoted 4 lines

I only gave it light testing so far, mainly building the defconfig for every architecture. I'll post the series once the build bots are happy with the branch overall.

Thank you so much for doing this work.

Can you say why you didn't convert ia64? I don't see any set_fs() use there, except for the unaligned handler, which looks trivial to remove. It looks like the only reason for it is kernel-mode unaligned exceptions, which we should just turn fatal, I suspect (they already get logged).

And ia64 people could make the unaligned handling do the kernel mode case in emulate_load/store_int() - it doesn't look *that* painful.

I like the series a lot.

Superficial comments:

for nds32 is there any good reason why __get_user / __set_user check the address limit directly? Maybe we should unify this and make it work like the other architectures.

With "uaccess: add generic __{get,put}_kernel_nofault" we should be able to remove HAVE_GET_KERNEL_NOFAULT entirely and just check if the helpers are already defined in linux/uaccess.h.

The new generic __access_ok, and the 3 fixed up version early on have a whole lot of superflous braces.

When the kernel detects it is oops or otherwise force killing a task while it exits the code poorly attempts to permanently stop the task from scheduling.

I say poorly because it is possible for a task in TASK_UINTERRUPTIBLE to be woken up.

As it makes no sense for the task to continue call do_task_dead instead which actually does the work and permanently removes the task from the scheduler. Guaranteeing the task will never be woken up again.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 kernel/exit.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

kernel/exit.c +1 -2

index d0ec6f6b41cb..f975cd8a2ed8 100644
@@ -886,8 +886,7 @@ void __noreturn make_task_dead(int signr)
 	if (unlikely(tsk->flags & PF_EXITING)) {
 		pr_alert("Fixing recursive fault but reboot is needed!\n");
 		futex_exit_recursive(tsk);
-		set_current_state(TASK_UNINTERRUPTIBLE);
-		schedule();
+		do_task_dead();
 	}
 
 	do_exit(signr);

Signature · 1 line

2.29.2

Quoted 11 lines · Eric W. Biederman

When the kernel detects it is oops or otherwise force killing a task while it exits the code poorly attempts to permanently stop the task from scheduling.

I say poorly because it is possible for a task in TASK_UINTERRUPTIBLE to be woken up.

As it makes no sense for the task to continue call do_task_dead instead which actually does the work and permanently removes the task from the scheduler. Guaranteeing the task will never be woken up again.

NAK. This is not all do_task_dead() leads to - see what finish_task_switch() does upon seeing TASK_DEAD:

                /* Task is done with its stack. */
		put_task_stack(prev);
		put_task_struct_rcu_user(prev);

Now take a look at the comment just before that check for PF_EXITING - the point is to leave the task leaked, rather than proceeding with freeing the sucker.

We are not going through the normal "turn zombie" motions, including waking wait(2) callers up, etc. Going ahead and freeing it could fuck the things up quite badly.

Now that there are no more modular uses of do_exit remove the EXPORT_SYMBOL.

Suggested-by Christoph Hellwig <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 kernel/exit.c | 1 -
 1 file changed, 1 deletion(-)

kernel/exit.c +0 -0

index f975cd8a2ed8..57afac845a0a 100644

Quoted 8 lines · Eric W. Biederman

Now that there are no more modular uses of do_exit remove the EXPORT_SYMBOL.

Suggested-by Christoph Hellwig <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 kernel/exit.c | 1 -
 1 file changed, 1 deletion(-)

kernel/exit.c void __noreturn do_exit(long code) · line 846

Quoted 9 lines

kernel/exit.c +0 -1

index f975cd8a2ed8..57afac845a0a 100644
@@ -843,7 +843,6 @@ void __noreturn do_exit(long code)
 	lockdep_free_task(tsk);
 	do_task_dead();
 }
-EXPORT_SYMBOL_GPL(do_exit);

"Now" in the commit message is misleading, AFAICS - there's no such users in the mainline right now (and yes, that one could be moved all the way up).

kernel/kthread.c +14 -0

+/**
+ * kthread_exit - Cause the current kthread return @result to kthread_stop().
+ * @result: The integer value to return to kthread_stop().
+ *
+ * While kthread_exit can be called directly, it exists so that
+ * functions which do some additional work in non-modular code such as
+ * module_put_and_kthread_exit can be implemented.
+ *
+ * Does not return.
+ */
+void __noreturn kthread_exit(long result)
+{
+	do_exit(result);
+}
 
 static int kthread(void *_create)
 {
 	static const struct sched_param param = { .sched_priority = 0 };

IMO the right way to handle that would be
1) turn these two do_exit() into do_exit(0), to reduce
confusion
2) deal with all do_exit() in kthread payloads. Your
name for the primitive is fine, IMO.
3) make that primitive pass the return value by way of
a field in struct kthread, adjusting kthread_stop() accordingly and passing 0 to do_exit() in kthread_exit() itself.

(2) is not as trivial as you seem to hope, though. Your patches in drivers/staging/rt*/ had papered over the problem in there, but hadn't really solved it.

thread_exit() should've been shot, all right, but it really ought to have been complete_and_exit() there. The thing is, complete()
+ return does *not* guarantee that driver won't get unloaded before the thread terminates. Possibly freeing its .code and leaving a thread to resume running in there as soon as it regains CPU.

The point of complete_and_exit() is that it's noreturn *and* in core kernel. So it can be safely used in a modular kthread, if paired with wait_for_completion() in or before module_exit. complete() + do_exit() (or complete + return as you've gotten there) doesn't give such guarantees at all.

I'm (re)crawling through that zoo right now, will post when I get more details.

Sent: 08 January 2022 18:36

Quoted 19 lines · Al Viro

IMO the right way to handle that would be
1) turn these two do_exit() into do_exit(0), to reduce
confusion
2) deal with all do_exit() in kthread payloads. Your
name for the primitive is fine, IMO.
3) make that primitive pass the return value by way of
a field in struct kthread, adjusting kthread_stop() accordingly and passing 0 to do_exit() in kthread_exit() itself.

(2) is not as trivial as you seem to hope, though. Your patches in drivers/staging/rt*/ had papered over the problem in there, but hadn't really solved it.

thread_exit() should've been shot, all right, but it really ought to have been complete_and_exit() there. The thing is, complete()
+ return does *not* guarantee that driver won't get unloaded before the thread terminates. Possibly freeing its .code and leaving a thread to resume running in there as soon as it regains CPU.

Quoted 5 lines

The point of complete_and_exit() is that it's noreturn *and* in core kernel. So it can be safely used in a modular kthread, if paired with wait_for_completion() in or before module_exit. complete() + do_exit() (or complete + return as you've gotten there) doesn't give such guarantees at all.

I think we are mostly in agreement here.

There are kernel threads started by modules that do:

	complete(...);
        return 0;

That should be at a minimum calling complete_and_exit. Possibly should be restructured to use kthread_stop().

From: Eric W. Biederman

Quoted 36 lines

Sent: 08 January 2022 18:36

Quoted 19 lines · Al Viro

IMO the right way to handle that would be
1) turn these two do_exit() into do_exit(0), to reduce
confusion
2) deal with all do_exit() in kthread payloads. Your
name for the primitive is fine, IMO.
3) make that primitive pass the return value by way of
a field in struct kthread, adjusting kthread_stop() accordingly and passing 0 to do_exit() in kthread_exit() itself.

(2) is not as trivial as you seem to hope, though. Your patches in drivers/staging/rt*/ had papered over the problem in there, but hadn't really solved it.

thread_exit() should've been shot, all right, but it really ought to have been complete_and_exit() there. The thing is, complete()
+ return does *not* guarantee that driver won't get unloaded before the thread terminates. Possibly freeing its .code and leaving a thread to resume running in there as soon as it regains CPU.

Quoted 5 lines

The point of complete_and_exit() is that it's noreturn *and* in core kernel. So it can be safely used in a modular kthread, if paired with wait_for_completion() in or before module_exit. complete() + do_exit() (or complete + return as you've gotten there) doesn't give such guarantees at all.

I think we are mostly in agreement here.

There are kernel threads started by modules that do:

	complete(...);
        return 0;

Quoted 2 lines

That should be at a minimum calling complete_and_exit. Possibly should be restructured to use kthread_stop().

There is also module_put_and_exit(0);
Which must have an implied THIS_MODULE.

There are kernel threads started by modules that do:

	complete(...);
        return 0;

That should be at a minimum calling complete_and_exit. Possibly should be restructured to use kthread_stop().

Some of those users of the now removed thread_exit() in staging are among the offenders.

However thread_exit() was implemented as:

	#define thread_exit() complete_and_exit(NULL, 0)

Which does nothing with a completion, it was just a really funny way to spell "do_exit(0)".

While I agree digging through all of the kernel threads and finding the ones that should be calling complete_and_exit is a fine idea. It is a concern independent of these patches.

Quoted 7 lines · Eric W. Biederman

There are kernel threads started by modules that do:

	complete(...);
        return 0;

That should be at a minimum calling complete_and_exit. Possibly should be restructured to use kthread_stop().

Quoted 8 lines

Some of those users of the now removed thread_exit() in staging are among the offenders.

However thread_exit() was implemented as:

	#define thread_exit() complete_and_exit(NULL, 0)

Which does nothing with a completion, it was just a really funny way to spell "do_exit(0)".

Yes. And there's a plenty of cargo-culting in that area.

Quoted 3 lines

While I agree digging through all of the kernel threads and finding the ones that should be calling complete_and_exit is a fine idea. It is a concern independent of these patches.

BTW, could somebody explain how could this
/*
* Prevent the kthread exits directly, and make sure when kthread_stop()
* is called to stop a kthread, it is still alive. If a kthread might be
* stopped by CACHE_SET_IO_DISABLE bit set, wait_for_kthread_stop() is
* necessary before the kthread returns.
*/
static inline void wait_for_kthread_stop(void)

{ 
        while (!kthread_should_stop()) {
                set_current_state(TASK_INTERRUPTIBLE);
                schedule();
        }
} 

in drivers/md/bcache/bcache.h possibly avoid losing wakeups?

AFAICS, it can be called while in TASK_RUNNING. Suppose kthread_stop() gets called just after the check for kthread_should_stop(). Our thread is still in TASK_RUNNING; kthread_stop() sets the flag for the next kthread_should_stop() to observe and does wake_up_process() to our thread. Which does nothing. Now our thread goes into TASK_INTERRUPTIBLE and calls schedule(). Sure, as soon as it gets woken up it'll call kthread_should_stop(), get true from it and that's it. What's going to wake it up, though?

The same goes for e.g. fs/btrfs/disk-io.c:cleaner_kthread():

                if (kthread_should_stop())
                        return 0;
                if (!again) {
                        set_current_state(TASK_INTERRUPTIBLE);
                        schedule();
                        __set_current_state(TASK_RUNNING);
                }

can't be right. Similar fun exists in e.g. fs/jfs, etc.

Am I missing something?

Today the rules are a bit iffy and arbitrary about which kernel threads have struct kthread present. Both idle threads and thread started with create_kthread want struct kthread present so that is effectively all kernel threads. Make the rule that if PF_KTHREAD and the task is running then struct kthread is present.

This will allow the kernel thread code to using tsk->exit_code with different semantics from ordinary processes.

To make ensure that struct kthread is present for all kernel threads move it's allocation into copy_process.

Add a deallocation of struct kthread in exec for processes that were kernel threads.

Move the allocation of struct kthread for the initial thread earlier so that it is not repeated for each additional idle thread.

Move the initialization of struct kthread into set_kthread_struct so that the structure is always and reliably initailized.

Clear set_child_tid in free_kthread_struct to ensure the kthread struct is reliably freed during exec. The function free_kthread_struct does not need to clear vfork_done during exec as exec_mm_release called from exec_mmap has already cleared vfork_done.

Signed-off-by "Eric W. Biederman" <[email protected]>

Hi Eric,

Quoted 22 lines · Eric W. Biederman

Today the rules are a bit iffy and arbitrary about which kernel threads have struct kthread present. Both idle threads and thread started with create_kthread want struct kthread present so that is effectively all kernel threads. Make the rule that if PF_KTHREAD and the task is running then struct kthread is present.

This will allow the kernel thread code to using tsk->exit_code with different semantics from ordinary processes.

To make ensure that struct kthread is present for all kernel threads move it's allocation into copy_process.

Add a deallocation of struct kthread in exec for processes that were kernel threads.

Move the allocation of struct kthread for the initial thread earlier so that it is not repeated for each additional idle thread.

Move the initialization of struct kthread into set_kthread_struct so that the structure is always and reliably initailized.

Quoted 6 lines

Clear set_child_tid in free_kthread_struct to ensure the kthread struct is reliably freed during exec. The function free_kthread_struct does not need to clear vfork_done during exec as exec_mm_release called from exec_mmap has already cleared vfork_done.

Signed-off-by "Eric W. Biederman" <[email protected]>

This patch as commit 40966e316f86 ("kthread: Ensure struct kthread is present for all kthreads") in -next causes an ARCH=arm multi_v5_defconfig kernel to fail to boot in QEMU. I had to apply commit 6692c98c7df5 ("fork: Stop protecting back_fork_cleanup_cgroup_lock with CONFIG_NUMA") to get it to build and I applied commit dd621ee0cf8e ("kthread: Warn about failed allocations for the init kthread") to avoid the known runtime warning.

$ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- distclean multi_v5_defconfig all

$ qemu-system-arm \
    -initrd rootfs.cpio \
    -append earlycon \
    -machine palmetto-bmc \
    -no-reboot \
    -dtb arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dtb \
    -display none \
    -kernel arch/arm/boot/zImage \
    -m 512m \
    -nodefaults \
    -serial mon:stdio
qemu-system-arm: warning: nic ftgmac100.0 has no peer
qemu-system-arm: warning: nic ftgmac100.1 has no peer
Booting Linux on physical CPU 0x0
Linux version 5.16.0-rc1-00016-g40966e316f86-dirty (nathan@archlinux-ax161) (arm-linux-gnueabi-gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2) #1 PREEMPT Wed Dec 22 18:08:53 UTC 2021
CPU: ARM926EJ-S [41069265] revision 5 (ARMv5TEJ), cr=00093177
CPU: VIVT data cache, VIVT instruction cache
OF: fdt: Machine model: Palmetto BMC
earlycon: ns16550a0 at MMIO 0x1e784000 (options '')
printk: bootconsole [ns16550a0] enabled
Memory policy: Data cache writethrough
cma: Reserved 16 MiB at 0x5b000000
Zone ranges:
  DMA      [mem 0x0000000040000000-0x000000005edfffff]
  Normal   empty
  HighMem  [mem 0x000000005ee00000-0x000000005fffffff]
Movable zone start for each node
Early memory node ranges
  node   0: [mem 0x0000000040000000-0x000000005bffffff]
  node   0: [mem 0x000000005c000000-0x000000005dffffff]
  node   0: [mem 0x000000005e000000-0x000000005edfffff]
  node   0: [mem 0x000000005ee00000-0x000000005fffffff]
Initmem setup node 0 [mem 0x0000000040000000-0x000000005fffffff]
Built 1 zonelists, mobility grouping on.  Total pages: 130084
Kernel command line: earlycon
Dentry cache hash table entries: 65536 (order: 6, 262144 bytes, linear)
Inode-cache hash table entries: 32768 (order: 5, 131072 bytes, linear)
mem auto-init: stack:off, heap alloc:off, heap free:off
Memory: 433140K/524288K available (9628K kernel code, 2019K rwdata, 2368K rodata, 340K init, 661K bss, 74764K reserved, 16384K cma-reserved, 0K highmem)
SLUB: HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
rcu: Preemptible hierarchical RCU implementation.
rcu:    RCU event tracing is enabled.
        Trampoline variant of Tasks RCU enabled.
rcu: RCU calculated value of scheduler-enlistment delay is 10 jiffies.
NR_IRQS: 16, nr_irqs: 16, preallocated irqs: 16
i2c controller registered, irq 16
random: get_random_bytes called from start_kernel+0x408/0x624 with crng_init=0
clocksource: FTTMR010-TIMER2: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 79635851949 ns
sched_clock: 32 bits at 24MHz, resolution 41ns, wraps every 89478484971ns
Switching to timer-based delay loop, resolution 41ns
Console: colour dummy device 80x30
printk: console [tty0] enabled
printk: bootconsole [ns16550a0] disabled

After that, it just hangs.

The rootfs is available at https://github.com/ClangBuiltLinux/boot-utils
in the images/arm folder.

If there is any more information that I can provide or changes to test, please let me know.

Quoted 96 lines · Nathan Chancellor

Hi Eric,

Quoted 22 lines · Eric W. Biederman

Today the rules are a bit iffy and arbitrary about which kernel threads have struct kthread present. Both idle threads and thread started with create_kthread want struct kthread present so that is effectively all kernel threads. Make the rule that if PF_KTHREAD and the task is running then struct kthread is present.

This will allow the kernel thread code to using tsk->exit_code with different semantics from ordinary processes.

To make ensure that struct kthread is present for all kernel threads move it's allocation into copy_process.

Add a deallocation of struct kthread in exec for processes that were kernel threads.

Move the allocation of struct kthread for the initial thread earlier so that it is not repeated for each additional idle thread.

Move the initialization of struct kthread into set_kthread_struct so that the structure is always and reliably initailized.

Quoted 6 lines

Clear set_child_tid in free_kthread_struct to ensure the kthread struct is reliably freed during exec. The function free_kthread_struct does not need to clear vfork_done during exec as exec_mm_release called from exec_mmap has already cleared vfork_done.

Signed-off-by "Eric W. Biederman" <[email protected]>

This patch as commit 40966e316f86 ("kthread: Ensure struct kthread is present for all kthreads") in -next causes an ARCH=arm multi_v5_defconfig kernel to fail to boot in QEMU. I had to apply commit 6692c98c7df5 ("fork: Stop protecting back_fork_cleanup_cgroup_lock with CONFIG_NUMA") to get it to build and I applied commit dd621ee0cf8e ("kthread: Warn about failed allocations for the init kthread") to avoid the known runtime warning.

$ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- distclean multi_v5_defconfig all

$ qemu-system-arm \
    -initrd rootfs.cpio \
    -append earlycon \
    -machine palmetto-bmc \
    -no-reboot \
    -dtb arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dtb \
    -display none \
    -kernel arch/arm/boot/zImage \
    -m 512m \
    -nodefaults \
    -serial mon:stdio
qemu-system-arm: warning: nic ftgmac100.0 has no peer
qemu-system-arm: warning: nic ftgmac100.1 has no peer
Booting Linux on physical CPU 0x0
Linux version 5.16.0-rc1-00016-g40966e316f86-dirty (nathan@archlinux-ax161) (arm-linux-gnueabi-gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2) #1 PREEMPT Wed Dec 22 18:08:53 UTC 2021
CPU: ARM926EJ-S [41069265] revision 5 (ARMv5TEJ), cr=00093177
CPU: VIVT data cache, VIVT instruction cache
OF: fdt: Machine model: Palmetto BMC
earlycon: ns16550a0 at MMIO 0x1e784000 (options '')
printk: bootconsole [ns16550a0] enabled
Memory policy: Data cache writethrough
cma: Reserved 16 MiB at 0x5b000000
Zone ranges:
  DMA      [mem 0x0000000040000000-0x000000005edfffff]
  Normal   empty
  HighMem  [mem 0x000000005ee00000-0x000000005fffffff]
Movable zone start for each node
Early memory node ranges
  node   0: [mem 0x0000000040000000-0x000000005bffffff]
  node   0: [mem 0x000000005c000000-0x000000005dffffff]
  node   0: [mem 0x000000005e000000-0x000000005edfffff]
  node   0: [mem 0x000000005ee00000-0x000000005fffffff]
Initmem setup node 0 [mem 0x0000000040000000-0x000000005fffffff]
Built 1 zonelists, mobility grouping on.  Total pages: 130084
Kernel command line: earlycon
Dentry cache hash table entries: 65536 (order: 6, 262144 bytes, linear)
Inode-cache hash table entries: 32768 (order: 5, 131072 bytes, linear)
mem auto-init: stack:off, heap alloc:off, heap free:off
Memory: 433140K/524288K available (9628K kernel code, 2019K rwdata, 2368K rodata, 340K init, 661K bss, 74764K reserved, 16384K cma-reserved, 0K highmem)
SLUB: HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
rcu: Preemptible hierarchical RCU implementation.
rcu:    RCU event tracing is enabled.
        Trampoline variant of Tasks RCU enabled.
rcu: RCU calculated value of scheduler-enlistment delay is 10 jiffies.
NR_IRQS: 16, nr_irqs: 16, preallocated irqs: 16
i2c controller registered, irq 16
random: get_random_bytes called from start_kernel+0x408/0x624 with crng_init=0
clocksource: FTTMR010-TIMER2: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 79635851949 ns
sched_clock: 32 bits at 24MHz, resolution 41ns, wraps every 89478484971ns
Switching to timer-based delay loop, resolution 41ns
Console: colour dummy device 80x30
printk: console [tty0] enabled
printk: bootconsole [ns16550a0] disabled

Quoted 7 lines

After that, it just hangs.

The rootfs is available at https://github.com/ClangBuiltLinux/boot-utils
in the images/arm folder.

If there is any more information that I can provide or changes to test, please let me know.

Well crap. I hate to hear my code is causing problems like this.

This is however a very good bug report, which I very much appreciate.

I think I have enough information. I will see if I can reproduce this and track down what is happening.

Have you by any chance tried linux-next with just these changes backed out?

Quoted 98 lines · Eric W. Biederman

Quoted 96 lines · Nathan Chancellor

Hi Eric,

Quoted 22 lines · Eric W. Biederman

Today the rules are a bit iffy and arbitrary about which kernel threads have struct kthread present. Both idle threads and thread started with create_kthread want struct kthread present so that is effectively all kernel threads. Make the rule that if PF_KTHREAD and the task is running then struct kthread is present.

This will allow the kernel thread code to using tsk->exit_code with different semantics from ordinary processes.

To make ensure that struct kthread is present for all kernel threads move it's allocation into copy_process.

Add a deallocation of struct kthread in exec for processes that were kernel threads.

Move the allocation of struct kthread for the initial thread earlier so that it is not repeated for each additional idle thread.

Move the initialization of struct kthread into set_kthread_struct so that the structure is always and reliably initailized.

Quoted 6 lines

Clear set_child_tid in free_kthread_struct to ensure the kthread struct is reliably freed during exec. The function free_kthread_struct does not need to clear vfork_done during exec as exec_mm_release called from exec_mmap has already cleared vfork_done.

Signed-off-by "Eric W. Biederman" <[email protected]>

This patch as commit 40966e316f86 ("kthread: Ensure struct kthread is present for all kthreads") in -next causes an ARCH=arm multi_v5_defconfig kernel to fail to boot in QEMU. I had to apply commit 6692c98c7df5 ("fork: Stop protecting back_fork_cleanup_cgroup_lock with CONFIG_NUMA") to get it to build and I applied commit dd621ee0cf8e ("kthread: Warn about failed allocations for the init kthread") to avoid the known runtime warning.

$ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- distclean multi_v5_defconfig all

$ qemu-system-arm \
    -initrd rootfs.cpio \
    -append earlycon \
    -machine palmetto-bmc \
    -no-reboot \
    -dtb arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dtb \
    -display none \
    -kernel arch/arm/boot/zImage \
    -m 512m \
    -nodefaults \
    -serial mon:stdio
qemu-system-arm: warning: nic ftgmac100.0 has no peer
qemu-system-arm: warning: nic ftgmac100.1 has no peer
Booting Linux on physical CPU 0x0
Linux version 5.16.0-rc1-00016-g40966e316f86-dirty (nathan@archlinux-ax161) (arm-linux-gnueabi-gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2) #1 PREEMPT Wed Dec 22 18:08:53 UTC 2021
CPU: ARM926EJ-S [41069265] revision 5 (ARMv5TEJ), cr=00093177
CPU: VIVT data cache, VIVT instruction cache
OF: fdt: Machine model: Palmetto BMC
earlycon: ns16550a0 at MMIO 0x1e784000 (options '')
printk: bootconsole [ns16550a0] enabled
Memory policy: Data cache writethrough
cma: Reserved 16 MiB at 0x5b000000
Zone ranges:
  DMA      [mem 0x0000000040000000-0x000000005edfffff]
  Normal   empty
  HighMem  [mem 0x000000005ee00000-0x000000005fffffff]
Movable zone start for each node
Early memory node ranges
  node   0: [mem 0x0000000040000000-0x000000005bffffff]
  node   0: [mem 0x000000005c000000-0x000000005dffffff]
  node   0: [mem 0x000000005e000000-0x000000005edfffff]
  node   0: [mem 0x000000005ee00000-0x000000005fffffff]
Initmem setup node 0 [mem 0x0000000040000000-0x000000005fffffff]
Built 1 zonelists, mobility grouping on.  Total pages: 130084
Kernel command line: earlycon
Dentry cache hash table entries: 65536 (order: 6, 262144 bytes, linear)
Inode-cache hash table entries: 32768 (order: 5, 131072 bytes, linear)
mem auto-init: stack:off, heap alloc:off, heap free:off
Memory: 433140K/524288K available (9628K kernel code, 2019K rwdata, 2368K rodata, 340K init, 661K bss, 74764K reserved, 16384K cma-reserved, 0K highmem)
SLUB: HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
rcu: Preemptible hierarchical RCU implementation.
rcu:    RCU event tracing is enabled.
        Trampoline variant of Tasks RCU enabled.
rcu: RCU calculated value of scheduler-enlistment delay is 10 jiffies.
NR_IRQS: 16, nr_irqs: 16, preallocated irqs: 16
i2c controller registered, irq 16
random: get_random_bytes called from start_kernel+0x408/0x624 with crng_init=0
clocksource: FTTMR010-TIMER2: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 79635851949 ns
sched_clock: 32 bits at 24MHz, resolution 41ns, wraps every 89478484971ns
Switching to timer-based delay loop, resolution 41ns
Console: colour dummy device 80x30
printk: console [tty0] enabled
printk: bootconsole [ns16550a0] disabled

Quoted 7 lines

Quoted 7 lines

After that, it just hangs.

The rootfs is available at https://github.com/ClangBuiltLinux/boot-utils
in the images/arm folder.

If there is any more information that I can provide or changes to test, please let me know.

Quoted 99 lines · Nathan Chancellor

Quoted 98 lines · Eric W. Biederman

Quoted 96 lines · Nathan Chancellor

Hi Eric,

Quoted 22 lines · Eric W. Biederman

Today the rules are a bit iffy and arbitrary about which kernel threads have struct kthread present. Both idle threads and thread started with create_kthread want struct kthread present so that is effectively all kernel threads. Make the rule that if PF_KTHREAD and the task is running then struct kthread is present.

This will allow the kernel thread code to using tsk->exit_code with different semantics from ordinary processes.

To make ensure that struct kthread is present for all kernel threads move it's allocation into copy_process.

Add a deallocation of struct kthread in exec for processes that were kernel threads.

Move the allocation of struct kthread for the initial thread earlier so that it is not repeated for each additional idle thread.

Move the initialization of struct kthread into set_kthread_struct so that the structure is always and reliably initailized.

Quoted 6 lines

Clear set_child_tid in free_kthread_struct to ensure the kthread struct is reliably freed during exec. The function free_kthread_struct does not need to clear vfork_done during exec as exec_mm_release called from exec_mmap has already cleared vfork_done.

Signed-off-by "Eric W. Biederman" <[email protected]>

This patch as commit 40966e316f86 ("kthread: Ensure struct kthread is present for all kthreads") in -next causes an ARCH=arm multi_v5_defconfig kernel to fail to boot in QEMU. I had to apply commit 6692c98c7df5 ("fork: Stop protecting back_fork_cleanup_cgroup_lock with CONFIG_NUMA") to get it to build and I applied commit dd621ee0cf8e ("kthread: Warn about failed allocations for the init kthread") to avoid the known runtime warning.

$ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- distclean multi_v5_defconfig all

$ qemu-system-arm \
    -initrd rootfs.cpio \
    -append earlycon \
    -machine palmetto-bmc \
    -no-reboot \
    -dtb arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dtb \
    -display none \
    -kernel arch/arm/boot/zImage \
    -m 512m \
    -nodefaults \
    -serial mon:stdio
qemu-system-arm: warning: nic ftgmac100.0 has no peer
qemu-system-arm: warning: nic ftgmac100.1 has no peer
Booting Linux on physical CPU 0x0
Linux version 5.16.0-rc1-00016-g40966e316f86-dirty (nathan@archlinux-ax161) (arm-linux-gnueabi-gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2) #1 PREEMPT Wed Dec 22 18:08:53 UTC 2021
CPU: ARM926EJ-S [41069265] revision 5 (ARMv5TEJ), cr=00093177
CPU: VIVT data cache, VIVT instruction cache
OF: fdt: Machine model: Palmetto BMC
earlycon: ns16550a0 at MMIO 0x1e784000 (options '')
printk: bootconsole [ns16550a0] enabled
Memory policy: Data cache writethrough
cma: Reserved 16 MiB at 0x5b000000
Zone ranges:
  DMA      [mem 0x0000000040000000-0x000000005edfffff]
  Normal   empty
  HighMem  [mem 0x000000005ee00000-0x000000005fffffff]
Movable zone start for each node
Early memory node ranges
  node   0: [mem 0x0000000040000000-0x000000005bffffff]
  node   0: [mem 0x000000005c000000-0x000000005dffffff]
  node   0: [mem 0x000000005e000000-0x000000005edfffff]
  node   0: [mem 0x000000005ee00000-0x000000005fffffff]
Initmem setup node 0 [mem 0x0000000040000000-0x000000005fffffff]
Built 1 zonelists, mobility grouping on.  Total pages: 130084
Kernel command line: earlycon
Dentry cache hash table entries: 65536 (order: 6, 262144 bytes, linear)
Inode-cache hash table entries: 32768 (order: 5, 131072 bytes, linear)
mem auto-init: stack:off, heap alloc:off, heap free:off
Memory: 433140K/524288K available (9628K kernel code, 2019K rwdata, 2368K rodata, 340K init, 661K bss, 74764K reserved, 16384K cma-reserved, 0K highmem)
SLUB: HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
rcu: Preemptible hierarchical RCU implementation.
rcu:    RCU event tracing is enabled.
        Trampoline variant of Tasks RCU enabled.
rcu: RCU calculated value of scheduler-enlistment delay is 10 jiffies.
NR_IRQS: 16, nr_irqs: 16, preallocated irqs: 16
i2c controller registered, irq 16
random: get_random_bytes called from start_kernel+0x408/0x624 with crng_init=0
clocksource: FTTMR010-TIMER2: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 79635851949 ns
sched_clock: 32 bits at 24MHz, resolution 41ns, wraps every 89478484971ns
Switching to timer-based delay loop, resolution 41ns
Console: colour dummy device 80x30
printk: console [tty0] enabled
printk: bootconsole [ns16550a0] disabled

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

After that, it just hangs.

The rootfs is available at https://github.com/ClangBuiltLinux/boot-utils
in the images/arm folder.

If there is any more information that I can provide or changes to test, please let me know.

I have managed to reproduce, fix and verify my fix, please see below.

Subject: [PATCH] kthread: Never put_user the set_child_tid address

Kernel threads abuse set_child_tid. Historically that has been fine as set_child_tid was initialized after the kernel thread had been forked. Unfortunately storing struct kthread in set_child_tid after the thread is running makes struct kthread being unusable for storing result codes of the thread.

When set_child_tid is set to struct kthread during fork that results in schedule_tail writing the thread id to the beggining of struct kthread (if put_user does not realize it is a kernel address).

Solve this by skipping the put_user for all kthreads.

Reported-by Nathan Chancellor <[email protected]>

Link https://lkml.kernel.org/r/YcNsG0Lp94V13whH@archlinux-ax161 open here

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 kernel/sched/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/sched/core.c +1 -1

index ee222b89c692..d8adbea77be1 100644
@@ -4908,7 +4908,7 @@ asmlinkage __visible void schedule_tail(struct task_struct *prev)
 	finish_task_switch(prev);
 	preempt_enable();
 
-	if (current->set_child_tid)
+	if (!(current->flags & PF_KTHREAD) && current->set_child_tid)
 		put_user(task_pid_vnr(current), current->set_child_tid);
 
 	calculate_sigpending();

Signature · 4 lines

2.29.2


Eric

Solve this by skipping the put_user for all kthreads.

Quoted 2 lines · Eric W. Biederman

Solve this by skipping the put_user for all kthreads.

Ugh.

While this fixes the problem, could we please just not mis-use that 'set_child_tid' as that kthread pointer any more?

It was always kind of hacky. I think a new pointer with the proper 'struct kthread *' type would be an improvement.

One of the "arguments" in the comment for re-using that set_child_tid pointer was that 'fork()' used to not wrongly copy it, but your patch literally now does that "allocate new kthread struct" at fork-time, so that argument is actually bogus now.

Instead of continuing to use the set_child_tid field of task_struct generalize the pf_io_worker field of task_struct and use it to hold the kthread pointer.

Today the rules are a bit iffy and arbitrary about which kernel threads have struct kthread present. Both idle threads and thread started with create_kthread want struct kthread present so that is effectively all kernel threads. Make the rule that if PF_KTHREAD and the task is running then struct kthread is present.

This will allow the kernel thread code to using tsk->exit_code with different semantics from ordinary processes.

Quoted 8 lines · Eric W. Biederman

Today the rules are a bit iffy and arbitrary about which kernel threads have struct kthread present. Both idle threads and thread started with create_kthread want struct kthread present so that is effectively all kernel threads. Make the rule that if PF_KTHREAD and the task is running then struct kthread is present.

This will allow the kernel thread code to using tsk->exit_code with different semantics from ordinary processes.

Getting rid of ->exit_code abuse is independent from this. I'm not saying that this change is a bad idea, but it's an independent thing. Simply turn these two failure exits into do_exit(0) in 06/10 and that's it. Then this one would get rid of if (!self) and the second of those two calls, but it won't be nailed to that point of queue.

The exit code of kernel threads has different semantics than the exit_code of userspace tasks. To avoid confusion and allow the userspace implementation to change as needed move the kernel thread exit code into struct kthread.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 kernel/kthread.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

kernel/kthread.c +4 -1

index 8e5f44bed027..9c6c532047c4 100644
@@ -52,6 +52,7 @@ struct kthread_create_info
 struct kthread {
 	unsigned long flags;
 	unsigned int cpu;
+	int result;
 	int (*threadfn)(void *);
 	void *data;
 	mm_segment_t oldfs;

@@ -287,7 +288,9 @@ EXPORT_SYMBOL_GPL(kthread_parkme);
  */
 void __noreturn kthread_exit(long result)
 {
-	do_exit(result);
+	struct kthread *kthread = to_kthread(current);
+	kthread->result = result;
+	do_exit(0);
 }
 
 /**