diff mbox series

powerpc: don't try to copy ppc for task with NULL pt_regs

Message ID d9f63344-fe7c-56ae-b420-4a1a04a2ae4c@kernel.dk (mailing list archive)
State Accepted
Commit fd7276189450110ed835eb0a334e62d2f1c4e3be
Headers show
Series powerpc: don't try to copy ppc for task with NULL pt_regs | expand

Commit Message

Jens Axboe March 26, 2023, 10:15 p.m. UTC
Powerpc sets up PF_KTHREAD and PF_IO_WORKER with a NULL pt_regs, which
from my (arguably very short) checking is not commonly done for other
archs. This is fine, except when PF_IO_WORKER's have been created and
the task does something that causes a coredump to be generated. Then we
get this crash:

Kernel attempted to read user page (160) - exploit attempt? (uid: 1000)
BUG: Kernel NULL pointer dereference on read at 0x00000160
Faulting instruction address: 0xc0000000000c3a60
Oops: Kernel access of bad area, sig: 11 [#1]
LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=32 NUMA pSeries
Modules linked in: bochs drm_vram_helper drm_kms_helper xts binfmt_misc ecb ctr syscopyarea sysfillrect cbc sysimgblt drm_ttm_helper aes_generic ttm sg libaes evdev joydev virtio_balloon vmx_crypto gf128mul drm dm_mod fuse loop configfs drm_panel_orientation_quirks ip_tables x_tables autofs4 hid_generic usbhid hid xhci_pci xhci_hcd usbcore usb_common sd_mod
CPU: 1 PID: 1982 Comm: ppc-crash Not tainted 6.3.0-rc2+ #88
Hardware name: IBM pSeries (emulated by qemu) POWER9 (raw) 0x4e1202 0xf000005 of:SLOF,HEAD hv:linux,kvm pSeries
NIP:  c0000000000c3a60 LR: c000000000039944 CTR: c0000000000398e0
REGS: c0000000041833b0 TRAP: 0300   Not tainted  (6.3.0-rc2+)
MSR:  800000000280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE>  CR: 88082828  XER: 200400f8
CFAR: c0000000000c386c DAR: 0000000000000160 DSISR: 40000000 IRQMASK: 0
GPR00: c000000000039920 c000000004183650 c00000000175d600 c0000000040f9800
GPR04: 0000000000000160 0000000000000008 c000000000039920 0000000000000000
GPR08: 0000000000000000 0000000000000000 00000003fe060000 0000000000002000
GPR12: c0000000000398e0 c0000003fffff200 c0000000015edbc0 c00000000ba2f648
GPR16: c00000000ba2f600 c000000001616ea8 0000000000000004 00000000ffffffff
GPR20: 0000000000000048 c000000004183918 c000000001410f00 c000000001410ef8
GPR24: c0000000040f9800 c0000000040f9800 c0000000041837b8 c0000000000398e0
GPR28: c00000000cc4cb80 c0000000040f9800 0000000000000008 0000000000000008
NIP [c0000000000c3a60] memcpy_power7+0x200/0x7d0
LR [c000000000039944] ppr_get+0x64/0xb0
Call Trace:
[c000000004183650] [c000000000039920] ppr_get+0x40/0xb0 (unreliable)
[c000000004183690] [c0000000001e5e80] __regset_get+0x180/0x1f0
[c000000004183700] [c0000000001e5f94] regset_get_alloc+0x64/0x90
[c000000004183740] [c0000000007ae638] elf_core_dump+0xb98/0x1b60
[c0000000041839c0] [c0000000007bb564] do_coredump+0x1c34/0x24a0
[c000000004183ba0] [c0000000001acf0c] get_signal+0x71c/0x1410
[c000000004183ce0] [c0000000000228a0] do_notify_resume+0x140/0x6f0
[c000000004183db0] [c0000000000353bc] interrupt_exit_user_prepare_main+0x29c/0x320
[c000000004183e20] [c00000000003579c] interrupt_exit_user_prepare+0x6c/0xa0
[c000000004183e50] [c00000000000c6f4] interrupt_return_srr_user+0x8/0x138
--- interrupt: 300 at 0x183ee09e0
NIP:  0000000183ee09e0 LR: 0000000183ee09dc CTR: 800000000280f033
REGS: c000000004183e80 TRAP: 0300   Not tainted  (6.3.0-rc2+)
MSR:  800000000000d033 <SF,EE,PR,ME,IR,DR,RI,LE>  CR: 22002848  XER: 000000f8
CFAR: 00007ffe6d746aa8 DAR: 0000000000000000 DSISR: 42000000 IRQMASK: 0
GPR00: 0000000183ee09dc 00007ffff20d37c0 0000000183f07f00 0000000000000000
GPR04: 0000000000000000 00007ffff20d37a8 0000000000000000 00007ffe6d9eae00
GPR08: 00007ffff20d3710 0000000000000000 0000000000000000 0000000000000000
GPR12: 0000000000000000 00007ffe6d9eae00 0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000183ee0860
GPR24: 00007ffe6d9df820 00007ffe6d9e0000 00007ffff20d7d98 0000000000000001
GPR28: 0000000183ee0c60 00007ffff20d7924 00007ffff20d7820 0000000000000000
NIP [0000000183ee09e0] 0x183ee09e0
LR [0000000183ee09dc] 0x183ee09dc
--- interrupt: 300
Code: f9030018 38630020 409f001c e8040000 e8c40008 38840010 f8030000 f8c30008 38630010 78a50720 7cb01120 409c001c <80040000> 80c40004 38840008 90030000
---[ end trace 0000000000000000 ]---

note: ppc-crash[1982] exited with irqs disabled

because ppr_get() is trying to copy from a PF_IO_WORKER with a NULL
pt_regs.

Check for a valid pt_regs in both ppc_get/ppr_set, and return an error
if not set. The actual error value doesn't seem to be important here,
so just pick -EINVAL.

Signed-off-by: Jens Axboe <axboe@kernel.dk>

Comments

Nicholas Piggin March 27, 2023, 6:36 a.m. UTC | #1
On Mon Mar 27, 2023 at 8:15 AM AEST, Jens Axboe wrote:
> Powerpc sets up PF_KTHREAD and PF_IO_WORKER with a NULL pt_regs, which
> from my (arguably very short) checking is not commonly done for other
> archs. This is fine, except when PF_IO_WORKER's have been created and
> the task does something that causes a coredump to be generated. Then we
> get this crash:

Hey Jens,

Thanks for the testing and the patch.

I think your patch would work, but I'd be inclined to give the IO worker
a pt_regs so it looks more like other archs and a regular user thread.

Your IO worker bug reminded me to resurrect some copy_thread patches I
had and I think they should do that

https://lists.ozlabs.org/pipermail/linuxppc-dev/2023-March/256271.html

I wouldn't ask you to test it until I've at least tried, do you have a
test case that triggers this?

Thanks,
Nick

>
> Kernel attempted to read user page (160) - exploit attempt? (uid: 1000)
> BUG: Kernel NULL pointer dereference on read at 0x00000160
> Faulting instruction address: 0xc0000000000c3a60
> Oops: Kernel access of bad area, sig: 11 [#1]
> LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=32 NUMA pSeries
> Modules linked in: bochs drm_vram_helper drm_kms_helper xts binfmt_misc ecb ctr syscopyarea sysfillrect cbc sysimgblt drm_ttm_helper aes_generic ttm sg libaes evdev joydev virtio_balloon vmx_crypto gf128mul drm dm_mod fuse loop configfs drm_panel_orientation_quirks ip_tables x_tables autofs4 hid_generic usbhid hid xhci_pci xhci_hcd usbcore usb_common sd_mod
> CPU: 1 PID: 1982 Comm: ppc-crash Not tainted 6.3.0-rc2+ #88
> Hardware name: IBM pSeries (emulated by qemu) POWER9 (raw) 0x4e1202 0xf000005 of:SLOF,HEAD hv:linux,kvm pSeries
> NIP:  c0000000000c3a60 LR: c000000000039944 CTR: c0000000000398e0
> REGS: c0000000041833b0 TRAP: 0300   Not tainted  (6.3.0-rc2+)
> MSR:  800000000280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE>  CR: 88082828  XER: 200400f8
> CFAR: c0000000000c386c DAR: 0000000000000160 DSISR: 40000000 IRQMASK: 0
> GPR00: c000000000039920 c000000004183650 c00000000175d600 c0000000040f9800
> GPR04: 0000000000000160 0000000000000008 c000000000039920 0000000000000000
> GPR08: 0000000000000000 0000000000000000 00000003fe060000 0000000000002000
> GPR12: c0000000000398e0 c0000003fffff200 c0000000015edbc0 c00000000ba2f648
> GPR16: c00000000ba2f600 c000000001616ea8 0000000000000004 00000000ffffffff
> GPR20: 0000000000000048 c000000004183918 c000000001410f00 c000000001410ef8
> GPR24: c0000000040f9800 c0000000040f9800 c0000000041837b8 c0000000000398e0
> GPR28: c00000000cc4cb80 c0000000040f9800 0000000000000008 0000000000000008
> NIP [c0000000000c3a60] memcpy_power7+0x200/0x7d0
> LR [c000000000039944] ppr_get+0x64/0xb0
> Call Trace:
> [c000000004183650] [c000000000039920] ppr_get+0x40/0xb0 (unreliable)
> [c000000004183690] [c0000000001e5e80] __regset_get+0x180/0x1f0
> [c000000004183700] [c0000000001e5f94] regset_get_alloc+0x64/0x90
> [c000000004183740] [c0000000007ae638] elf_core_dump+0xb98/0x1b60
> [c0000000041839c0] [c0000000007bb564] do_coredump+0x1c34/0x24a0
> [c000000004183ba0] [c0000000001acf0c] get_signal+0x71c/0x1410
> [c000000004183ce0] [c0000000000228a0] do_notify_resume+0x140/0x6f0
> [c000000004183db0] [c0000000000353bc] interrupt_exit_user_prepare_main+0x29c/0x320
> [c000000004183e20] [c00000000003579c] interrupt_exit_user_prepare+0x6c/0xa0
> [c000000004183e50] [c00000000000c6f4] interrupt_return_srr_user+0x8/0x138
> --- interrupt: 300 at 0x183ee09e0
> NIP:  0000000183ee09e0 LR: 0000000183ee09dc CTR: 800000000280f033
> REGS: c000000004183e80 TRAP: 0300   Not tainted  (6.3.0-rc2+)
> MSR:  800000000000d033 <SF,EE,PR,ME,IR,DR,RI,LE>  CR: 22002848  XER: 000000f8
> CFAR: 00007ffe6d746aa8 DAR: 0000000000000000 DSISR: 42000000 IRQMASK: 0
> GPR00: 0000000183ee09dc 00007ffff20d37c0 0000000183f07f00 0000000000000000
> GPR04: 0000000000000000 00007ffff20d37a8 0000000000000000 00007ffe6d9eae00
> GPR08: 00007ffff20d3710 0000000000000000 0000000000000000 0000000000000000
> GPR12: 0000000000000000 00007ffe6d9eae00 0000000000000000 0000000000000000
> GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000183ee0860
> GPR24: 00007ffe6d9df820 00007ffe6d9e0000 00007ffff20d7d98 0000000000000001
> GPR28: 0000000183ee0c60 00007ffff20d7924 00007ffff20d7820 0000000000000000
> NIP [0000000183ee09e0] 0x183ee09e0
> LR [0000000183ee09dc] 0x183ee09dc
> --- interrupt: 300
> Code: f9030018 38630020 409f001c e8040000 e8c40008 38840010 f8030000 f8c30008 38630010 78a50720 7cb01120 409c001c <80040000> 80c40004 38840008 90030000
> ---[ end trace 0000000000000000 ]---
>
> note: ppc-crash[1982] exited with irqs disabled
>
> because ppr_get() is trying to copy from a PF_IO_WORKER with a NULL
> pt_regs.
>
> Check for a valid pt_regs in both ppc_get/ppr_set, and return an error
> if not set. The actual error value doesn't seem to be important here,
> so just pick -EINVAL.
>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c
> index 2087a785f05f..80b699dd0d7f 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-view.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
> @@ -290,6 +290,8 @@ static int gpr_set(struct task_struct *target, const struct user_regset *regset,
>  static int ppr_get(struct task_struct *target, const struct user_regset *regset,
>  		   struct membuf to)
>  {
> +	if (!target->thread.regs)
> +		return -EINVAL;
>  	return membuf_write(&to, &target->thread.regs->ppr, sizeof(u64));
>  }
>  
> @@ -297,6 +299,8 @@ static int ppr_set(struct task_struct *target, const struct user_regset *regset,
>  		   unsigned int pos, unsigned int count, const void *kbuf,
>  		   const void __user *ubuf)
>  {
> +	if (!target->thread.regs)
> +		return -EINVAL;
>  	return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
>  				  &target->thread.regs->ppr, 0, sizeof(u64));
>  }
>
> -- 
> Jens Axboe
Christophe Leroy March 27, 2023, 10:26 a.m. UTC | #2
Le 27/03/2023 à 08:36, Nicholas Piggin a écrit :
> On Mon Mar 27, 2023 at 8:15 AM AEST, Jens Axboe wrote:
>> Powerpc sets up PF_KTHREAD and PF_IO_WORKER with a NULL pt_regs, which
>> from my (arguably very short) checking is not commonly done for other
>> archs. This is fine, except when PF_IO_WORKER's have been created and
>> the task does something that causes a coredump to be generated. Then we
>> get this crash:
> 
> Hey Jens,
> 
> Thanks for the testing and the patch.
> 
> I think your patch would work, but I'd be inclined to give the IO worker
> a pt_regs so it looks more like other archs and a regular user thread.
> 
> Your IO worker bug reminded me to resurrect some copy_thread patches I
> had and I think they should do that
> 
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2023-March/256271.html
> 
> I wouldn't ask you to test it until I've at least tried, do you have a
> test case that triggers this?

I fact, most architectures don't have thread.regs, but rely on something 
like:

#define task_pt_regs(p) \
	((struct pt_regs *)(THREAD_SIZE + task_stack_page(p)) - 1)


In powerpc, thread.regs was there because of the regs not being at the 
same place in the stack depending on which interrupt it was.

However with the two commits below, we now have stable fixed location 
for the regs, so thread.regs shouldn't be needed anymore:
- db297c3b07af ("powerpc/32: Don't save thread.regs on interrupt entry")
- b5cfc9cd7b04 ("powerpc/32: Fix critical and debug interrupts on BOOKE")

But in the meantime, thread.regs started to be used for additional 
purpose, like knowing if it is a user thread or a kernel thread by using 
thread.regs nullity.


Now that thread.regs doesn't change anymore at each interrupt, it would 
probably be worth dropping it and falling back to task_pt_regs() as 
defined on most architecture.
Knowing whether a thread is a kernel or user thread can for instance be 
known with PF_KTHREAD flag, so no need of thread.regs for that.

Christophe
Jens Axboe March 27, 2023, 12:42 p.m. UTC | #3
On 3/27/23 12:36?AM, Nicholas Piggin wrote:
> On Mon Mar 27, 2023 at 8:15 AM AEST, Jens Axboe wrote:
>> Powerpc sets up PF_KTHREAD and PF_IO_WORKER with a NULL pt_regs, which
>> from my (arguably very short) checking is not commonly done for other
>> archs. This is fine, except when PF_IO_WORKER's have been created and
>> the task does something that causes a coredump to be generated. Then we
>> get this crash:
> 
> Hey Jens,
> 
> Thanks for the testing and the patch.
> 
> I think your patch would work, but I'd be inclined to give the IO worker
> a pt_regs so it looks more like other archs and a regular user thread.

Yep I think that'd be a better idea. No better way to get a good patch
than to send out a bad one :-)

> Your IO worker bug reminded me to resurrect some copy_thread patches I
> had and I think they should do that
> 
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2023-March/256271.html
> 
> I wouldn't ask you to test it until I've at least tried, do you have a
> test case that triggers this?

I can test them pretty easily. I did write a test case that is 100%
reliable for me, attached. Just do:

$ gcc -Wall -o ppc-crash ppc-crash.c -luring
$ ulimit -c10000000
$ ./ppc-crash

and it'll bomb while trying to write that coredump.
Michael Ellerman March 27, 2023, 1:54 p.m. UTC | #4
"Nicholas Piggin" <npiggin@gmail.com> writes:
> On Mon Mar 27, 2023 at 8:15 AM AEST, Jens Axboe wrote:
>> Powerpc sets up PF_KTHREAD and PF_IO_WORKER with a NULL pt_regs, which
>> from my (arguably very short) checking is not commonly done for other
>> archs. This is fine, except when PF_IO_WORKER's have been created and
>> the task does something that causes a coredump to be generated. Then we
>> get this crash:
>
> Hey Jens,
>
> Thanks for the testing and the patch.
>
> I think your patch would work, but I'd be inclined to give the IO worker
> a pt_regs so it looks more like other archs and a regular user thread.
>
> Your IO worker bug reminded me to resurrect some copy_thread patches I
> had and I think they should do that
>
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2023-March/256271.html
>
> I wouldn't ask you to test it until I've at least tried, do you have a
> test case that triggers this?

I hit it once on one machine while running the mtr test from the other
thread. I'm not sure what leads to it failing that way rather than the
usual case of the mariadb test just printing an error.

I've got your series applied and am testing it now.

cheers
Jens Axboe March 27, 2023, 4:10 p.m. UTC | #5
On 3/27/23 7:54?AM, Michael Ellerman wrote:
> "Nicholas Piggin" <npiggin@gmail.com> writes:
>> On Mon Mar 27, 2023 at 8:15 AM AEST, Jens Axboe wrote:
>>> Powerpc sets up PF_KTHREAD and PF_IO_WORKER with a NULL pt_regs, which
>>> from my (arguably very short) checking is not commonly done for other
>>> archs. This is fine, except when PF_IO_WORKER's have been created and
>>> the task does something that causes a coredump to be generated. Then we
>>> get this crash:
>>
>> Hey Jens,
>>
>> Thanks for the testing and the patch.
>>
>> I think your patch would work, but I'd be inclined to give the IO worker
>> a pt_regs so it looks more like other archs and a regular user thread.
>>
>> Your IO worker bug reminded me to resurrect some copy_thread patches I
>> had and I think they should do that
>>
>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2023-March/256271.html
>>
>> I wouldn't ask you to test it until I've at least tried, do you have a
>> test case that triggers this?
> 
> I hit it once on one machine while running the mtr test from the other
> thread. I'm not sure what leads to it failing that way rather than the
> usual case of the mariadb test just printing an error.

That's how I hit it first too, then I wrote the reproducer to verify and
be able to test a patch.
Nicholas Piggin March 28, 2023, 6:16 a.m. UTC | #6
On Mon Mar 27, 2023 at 8:26 PM AEST, Christophe Leroy wrote:
>
>
> Le 27/03/2023 à 08:36, Nicholas Piggin a écrit :
> > On Mon Mar 27, 2023 at 8:15 AM AEST, Jens Axboe wrote:
> >> Powerpc sets up PF_KTHREAD and PF_IO_WORKER with a NULL pt_regs, which
> >> from my (arguably very short) checking is not commonly done for other
> >> archs. This is fine, except when PF_IO_WORKER's have been created and
> >> the task does something that causes a coredump to be generated. Then we
> >> get this crash:
> > 
> > Hey Jens,
> > 
> > Thanks for the testing and the patch.
> > 
> > I think your patch would work, but I'd be inclined to give the IO worker
> > a pt_regs so it looks more like other archs and a regular user thread.
> > 
> > Your IO worker bug reminded me to resurrect some copy_thread patches I
> > had and I think they should do that
> > 
> > https://lists.ozlabs.org/pipermail/linuxppc-dev/2023-March/256271.html
> > 
> > I wouldn't ask you to test it until I've at least tried, do you have a
> > test case that triggers this?
>
> I fact, most architectures don't have thread.regs, but rely on something 
> like:
>
> #define task_pt_regs(p) \
> 	((struct pt_regs *)(THREAD_SIZE + task_stack_page(p)) - 1)
>
>
> In powerpc, thread.regs was there because of the regs not being at the 
> same place in the stack depending on which interrupt it was.
>
> However with the two commits below, we now have stable fixed location 
> for the regs, so thread.regs shouldn't be needed anymore:
> - db297c3b07af ("powerpc/32: Don't save thread.regs on interrupt entry")
> - b5cfc9cd7b04 ("powerpc/32: Fix critical and debug interrupts on BOOKE")
>
> But in the meantime, thread.regs started to be used for additional 
> purpose, like knowing if it is a user thread or a kernel thread by using 
> thread.regs nullity.
>
>
> Now that thread.regs doesn't change anymore at each interrupt, it would 
> probably be worth dropping it and falling back to task_pt_regs() as 
> defined on most architecture.
> Knowing whether a thread is a kernel or user thread can for instance be 
> known with PF_KTHREAD flag, so no need of thread.regs for that.

That would be nice if we can define regs that way, I agree. We should
look into doing that.

Thanks,
Nick
Michael Ellerman March 28, 2023, 11:32 a.m. UTC | #7
Jens Axboe <axboe@kernel.dk> writes:
> Powerpc sets up PF_KTHREAD and PF_IO_WORKER with a NULL pt_regs, which
> from my (arguably very short) checking is not commonly done for other
> archs. This is fine, except when PF_IO_WORKER's have been created and
> the task does something that causes a coredump to be generated.

Do kthread's ever core dump? I didn't think they did, but I can't find
any logic to prevent it.

Maybe it's always been possible but just never happened due to luck.

As Nick said we should probably have a non-NULL regs for PF_IO_WORKERS,
but I'll still take this as a nice backportable fix for the immediate
crash.

I tagged it as Fixes: pointing back at the commit that added ppr_get(),
even though I don't know for sure the bug was triggerable back then
(v4.8).

cheers
Michael Ellerman March 28, 2023, 11:47 a.m. UTC | #8
"Nicholas Piggin" <npiggin@gmail.com> writes:
> On Mon Mar 27, 2023 at 8:26 PM AEST, Christophe Leroy wrote:
...
>>
>> Now that thread.regs doesn't change anymore at each interrupt, it would 
>> probably be worth dropping it and falling back to task_pt_regs() as 
>> defined on most architecture.
>> Knowing whether a thread is a kernel or user thread can for instance be 
>> known with PF_KTHREAD flag, so no need of thread.regs for that.
>
> That would be nice if we can define regs that way, I agree. We should
> look into doing that.

Yeah it's on the long-list of things that need cleaning up.

I think there's some complication in working out which sites are OK to
use/give-out the value in pt_regs that's potentially a dummy value, vs
cases that actually want to check PF_KTHREAD and do something different.
But that's just my hunch I haven't looked through all the sites.

The thread.regs = NULL for kernel threads goes back to arch/ppc, about
2002 by the looks:

  https://github.com/mpe/linux-fullhistory/commit/2a8e186c384c0c911f91cd12367658eabdc820d8#diff-939b705cff722ee75595fad30d56bb1175dfdce49a69adb4d5656f354be076c6

There's no change log of course :)

Still maybe it doesn't matter why it was originally done that way, if we
can do it differently now.

cheers
Jens Axboe March 28, 2023, 12:39 p.m. UTC | #9
On 3/28/23 5:32?AM, Michael Ellerman wrote:
> Jens Axboe <axboe@kernel.dk> writes:
>> Powerpc sets up PF_KTHREAD and PF_IO_WORKER with a NULL pt_regs, which
>> from my (arguably very short) checking is not commonly done for other
>> archs. This is fine, except when PF_IO_WORKER's have been created and
>> the task does something that causes a coredump to be generated.
> 
> Do kthread's ever core dump? I didn't think they did, but I can't find
> any logic to prevent it.

kthreads aren't associated with the original task, they just exist by
themselves. They also can't take signals. Eg they cannot core dump, just
oops :-)

This is different than io workers that do show up as threads, but they
still don't exit to userspace. That is why it ended being a problem.

> As Nick said we should probably have a non-NULL regs for PF_IO_WORKERS,
> but I'll still take this as a nice backportable fix for the immediate
> crash.
> 
> I tagged it as Fixes: pointing back at the commit that added ppr_get(),
> even though I don't know for sure the bug was triggerable back then
> (v4.8).

Thanks!
Christophe Leroy March 30, 2023, 8:44 a.m. UTC | #10
Le 28/03/2023 à 13:47, Michael Ellerman a écrit :
> "Nicholas Piggin" <npiggin@gmail.com> writes:
>> On Mon Mar 27, 2023 at 8:26 PM AEST, Christophe Leroy wrote:
> ...
>>>
>>> Now that thread.regs doesn't change anymore at each interrupt, it would
>>> probably be worth dropping it and falling back to task_pt_regs() as
>>> defined on most architecture.
>>> Knowing whether a thread is a kernel or user thread can for instance be
>>> known with PF_KTHREAD flag, so no need of thread.regs for that.
>>
>> That would be nice if we can define regs that way, I agree. We should
>> look into doing that.
> 
> Yeah it's on the long-list of things that need cleaning up.
> 
> I think there's some complication in working out which sites are OK to
> use/give-out the value in pt_regs that's potentially a dummy value, vs
> cases that actually want to check PF_KTHREAD and do something different.
> But that's just my hunch I haven't looked through all the sites.
> 
> The thread.regs = NULL for kernel threads goes back to arch/ppc, about
> 2002 by the looks:
> 
>    https://github.com/mpe/linux-fullhistory/commit/2a8e186c384c0c911f91cd12367658eabdc820d8#diff-939b705cff722ee75595fad30d56bb1175dfdce49a69adb4d5656f354be076c6
> 
> There's no change log of course :)
> 
> Still maybe it doesn't matter why it was originally done that way, if we
> can do it differently now.
> 

I have the feeling that our logic is broken after commit 5bd2e97c868a 
("fork: Generalize PF_IO_WORKER handling")

Christophe
Michael Ellerman April 6, 2023, 12:26 a.m. UTC | #11
On Sun, 26 Mar 2023 16:15:57 -0600, Jens Axboe wrote:
> Powerpc sets up PF_KTHREAD and PF_IO_WORKER with a NULL pt_regs, which
> from my (arguably very short) checking is not commonly done for other
> archs. This is fine, except when PF_IO_WORKER's have been created and
> the task does something that causes a coredump to be generated. Then we
> get this crash:
> 
> [...]

Applied to powerpc/fixes.

[1/1] powerpc: don't try to copy ppc for task with NULL pt_regs
      https://git.kernel.org/powerpc/c/fd7276189450110ed835eb0a334e62d2f1c4e3be

cheers
diff mbox series

Patch

diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c
index 2087a785f05f..80b699dd0d7f 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-view.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
@@ -290,6 +290,8 @@  static int gpr_set(struct task_struct *target, const struct user_regset *regset,
 static int ppr_get(struct task_struct *target, const struct user_regset *regset,
 		   struct membuf to)
 {
+	if (!target->thread.regs)
+		return -EINVAL;
 	return membuf_write(&to, &target->thread.regs->ppr, sizeof(u64));
 }
 
@@ -297,6 +299,8 @@  static int ppr_set(struct task_struct *target, const struct user_regset *regset,
 		   unsigned int pos, unsigned int count, const void *kbuf,
 		   const void __user *ubuf)
 {
+	if (!target->thread.regs)
+		return -EINVAL;
 	return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
 				  &target->thread.regs->ppr, 0, sizeof(u64));
 }