Re: [PATCH RFC] m68k: fix spinlock race in kernel thread creation

Geert Uytterhoeven
2024-05-08T11:53:21Z

On Thu, Apr 11, 2024 at 5:36 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Context switching does take care to retain the correct lock
> owner across the switch from 'prev' to 'next' tasks. This does
> rely on interrupts remaining disabled for the entire duration
> of the switch.
>
> This condition is guaranteed for normal process creation and
> context switching between already running processes, because
> both 'prev' and 'next' already have interrupts disabled in their
> saved copies of the status register.
>
> The situation is different for newly created kernel threads.
> The status register is set to PS_S in copy_thread(), which does
> leave the IPL at 0. Upon restoring the 'next' thread's status
> register in switch_to() aka resume(), interrupts then become
> enabled prematurely. resume() then returns via ret_from_kernel_thread()
> and schedule_tail() where run queue lock is released (see
> finish_task_switch() and finish_lock_switch()).
>
> A timer interrupt calling scheduler_tick() before the lock is
> released in finish_task_switch() will find the lock already taken,
> with the current task as lock owner. This causes a spinlock
> recursion warning as reported by Guenter Roeck.
>
> As far as I can ascertain, this race has been opened in commit
> 533e6903bea0 ("m68k: split ret_from_fork(), simplify kernel_thread()")
> but I haven't done a detailed study of kernel history so it
> may well predate that commit.
>
> Interrupts cannot be disabled in the saved status register copy
> for kernel threads (init will complain about interrupts disabled
> when finally starting user space). Disable interrupts temporarily
> when switching the tasks' register sets in resume().
>
> Note that a simple oriw 0x700,%sr after restoring sr is not enough
> here - this leaves enough of a race for the 'spinlock recursion'
> warning to still be observed.
>
> Tested on ARAnyM and qemu (Quadra 800 emulation).
>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Finn Thain <fthain@linux-m68k.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: stable@kernel.org # 3.6
> Cc: linux-m68k@lists.linux-m68k.org

Reported-by: Guenter Roeck <linux@roeck-us.net>

> Link: https://lore.kernel.org/all/07811b26-677c-4d05-aeb4-996cd880b789@roeck-us.net

s/Link/Closes/

> Fixes: 533e6903bea0 ("m68k: split ret_from_fork(), simplify kernel_thread()")
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
i.e. will queue in the m68k tree for v6.10 (with above tags fixed).

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds