diff mbox series

[08/10] parisc: fix livelock in uaccess

Message ID Y9l0w4M91DwYLO3N@ZenIV (mailing list archive)
State Awaiting Upstream, archived
Headers show
Series [01/10] alpha: fix livelock in uaccess | expand

Commit Message

Al Viro Jan. 31, 2023, 8:06 p.m. UTC
parisc equivalent of 26178ec11ef3 "x86: mm: consolidate VM_FAULT_RETRY handling"
If e.g. get_user() triggers a page fault and a fatal signal is caught, we might
end up with handle_mm_fault() returning VM_FAULT_RETRY and not doing anything
to page tables.  In such case we must *not* return to the faulting insn -
that would repeat the entire thing without making any progress; what we need
instead is to treat that as failed (user) memory access.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 arch/parisc/mm/fault.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Helge Deller Feb. 6, 2023, 4:58 p.m. UTC | #1
Hi Al,

On 1/31/23 21:06, Al Viro wrote:
> parisc equivalent of 26178ec11ef3 "x86: mm: consolidate VM_FAULT_RETRY handling"
> If e.g. get_user() triggers a page fault and a fatal signal is caught, we might
> end up with handle_mm_fault() returning VM_FAULT_RETRY and not doing anything
> to page tables.  In such case we must *not* return to the faulting insn -
> that would repeat the entire thing without making any progress; what we need
> instead is to treat that as failed (user) memory access.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>   arch/parisc/mm/fault.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c
> index 869204e97ec9..bb30ff6a3e19 100644
> --- a/arch/parisc/mm/fault.c
> +++ b/arch/parisc/mm/fault.c
> @@ -308,8 +308,11 @@ void do_page_fault(struct pt_regs *regs, unsigned long code,
>
>   	fault = handle_mm_fault(vma, address, flags, regs);
>
> -	if (fault_signal_pending(fault, regs))
> +	if (fault_signal_pending(fault, regs)) {
> +		if (!user_mode(regs))
> +			goto no_context;
>   		return;
> +	}

The testcase in
   https://lore.kernel.org/lkml/20170822102527.GA14671@leverpostej/
   https://lore.kernel.org/linux-arch/20210121123140.GD48431@C02TD0UTHF1T.local/
does hang with and without above patch on parisc.
It does not consume CPU in that state and can be killed with ^C.

Any idea?

Helge
Guenter Roeck Feb. 28, 2023, 3:22 p.m. UTC | #2
On Tue, Jan 31, 2023 at 08:06:27PM +0000, Al Viro wrote:
> parisc equivalent of 26178ec11ef3 "x86: mm: consolidate VM_FAULT_RETRY handling"
> If e.g. get_user() triggers a page fault and a fatal signal is caught, we might
> end up with handle_mm_fault() returning VM_FAULT_RETRY and not doing anything
> to page tables.  In such case we must *not* return to the faulting insn -
> that would repeat the entire thing without making any progress; what we need
> instead is to treat that as failed (user) memory access.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  arch/parisc/mm/fault.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c
> index 869204e97ec9..bb30ff6a3e19 100644
> --- a/arch/parisc/mm/fault.c
> +++ b/arch/parisc/mm/fault.c
> @@ -308,8 +308,11 @@ void do_page_fault(struct pt_regs *regs, unsigned long code,
>  
>  	fault = handle_mm_fault(vma, address, flags, regs);
>  
> -	if (fault_signal_pending(fault, regs))
> +	if (fault_signal_pending(fault, regs)) {
> +		if (!user_mode(regs))
> +			goto no_context;

0-day rightfully complains that this leaves 'msg' uninitialized.

arch/parisc/mm/fault.c:427 do_page_fault() error: uninitialized symbol 'msg'

Guenter

>  		return;
> +	}
>  
>  	/* The fault is fully completed (including releasing mmap lock) */
>  	if (fault & VM_FAULT_COMPLETED)
Al Viro Feb. 28, 2023, 5:34 p.m. UTC | #3
On Mon, Feb 06, 2023 at 05:58:02PM +0100, Helge Deller wrote:
> Hi Al,
> 
> On 1/31/23 21:06, Al Viro wrote:
> > parisc equivalent of 26178ec11ef3 "x86: mm: consolidate VM_FAULT_RETRY handling"
> > If e.g. get_user() triggers a page fault and a fatal signal is caught, we might
> > end up with handle_mm_fault() returning VM_FAULT_RETRY and not doing anything
> > to page tables.  In such case we must *not* return to the faulting insn -
> > that would repeat the entire thing without making any progress; what we need
> > instead is to treat that as failed (user) memory access.
> > 
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> >   arch/parisc/mm/fault.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c
> > index 869204e97ec9..bb30ff6a3e19 100644
> > --- a/arch/parisc/mm/fault.c
> > +++ b/arch/parisc/mm/fault.c
> > @@ -308,8 +308,11 @@ void do_page_fault(struct pt_regs *regs, unsigned long code,
> > 
> >   	fault = handle_mm_fault(vma, address, flags, regs);
> > 
> > -	if (fault_signal_pending(fault, regs))
> > +	if (fault_signal_pending(fault, regs)) {
> > +		if (!user_mode(regs))
> > +			goto no_context;
> >   		return;
> > +	}
> 
> The testcase in
>   https://lore.kernel.org/lkml/20170822102527.GA14671@leverpostej/
>   https://lore.kernel.org/linux-arch/20210121123140.GD48431@C02TD0UTHF1T.local/
> does hang with and without above patch on parisc.
> It does not consume CPU in that state and can be killed with ^C.
> 
> Any idea?

	Still trying to resurrect the parisc box to test on it...
FWIW, right now I've locally confirmed that mainline has the bug
in question and that patch fixes it for alpha, sparc32 and sparc64;
hexagon, m68k and riscv got acks from other folks; microblaze,
nios2 and openrisc I can't test at all (no hardware, no qemu setup);
same for parisc64.  Itanic and parisc32 I might be able to test,
if I manage to resurrect the hardware.

	Just to confirm: your "can be killed with ^C" had been on the
mainline parisc kernel (with userfaultfd enable, of course, or it wouldn't
hang up at all), right?  Was it 32bit or 64bit kernel?
Helge Deller Feb. 28, 2023, 6:26 p.m. UTC | #4
On 2/28/23 18:34, Al Viro wrote:
> On Mon, Feb 06, 2023 at 05:58:02PM +0100, Helge Deller wrote:
>> Hi Al,
>>
>> On 1/31/23 21:06, Al Viro wrote:
>>> parisc equivalent of 26178ec11ef3 "x86: mm: consolidate VM_FAULT_RETRY handling"
>>> If e.g. get_user() triggers a page fault and a fatal signal is caught, we might
>>> end up with handle_mm_fault() returning VM_FAULT_RETRY and not doing anything
>>> to page tables.  In such case we must *not* return to the faulting insn -
>>> that would repeat the entire thing without making any progress; what we need
>>> instead is to treat that as failed (user) memory access.
>>>
>>> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>>> ---
>>>    arch/parisc/mm/fault.c | 5 ++++-
>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c
>>> index 869204e97ec9..bb30ff6a3e19 100644
>>> --- a/arch/parisc/mm/fault.c
>>> +++ b/arch/parisc/mm/fault.c
>>> @@ -308,8 +308,11 @@ void do_page_fault(struct pt_regs *regs, unsigned long code,
>>>
>>>    	fault = handle_mm_fault(vma, address, flags, regs);
>>>
>>> -	if (fault_signal_pending(fault, regs))
>>> +	if (fault_signal_pending(fault, regs)) {
>>> +		if (!user_mode(regs))
>>> +			goto no_context;
>>>    		return;
>>> +	}
>>
>> The testcase in
>>    https://lore.kernel.org/lkml/20170822102527.GA14671@leverpostej/
>>    https://lore.kernel.org/linux-arch/20210121123140.GD48431@C02TD0UTHF1T.local/
>> does hang with and without above patch on parisc.
>> It does not consume CPU in that state and can be killed with ^C.
>>
>> Any idea?
>
> 	Still trying to resurrect the parisc box to test on it...
> FWIW, right now I've locally confirmed that mainline has the bug
> in question and that patch fixes it for alpha, sparc32 and sparc64;
> hexagon, m68k and riscv got acks from other folks; microblaze,
> nios2 and openrisc I can't test at all (no hardware, no qemu setup);
> same for parisc64.  Itanic and parisc32 I might be able to test,
> if I manage to resurrect the hardware.

I can test both parisc32 and parisc64.

> 	Just to confirm: your "can be killed with ^C" had been on the
> mainline parisc kernel (with userfaultfd enable, of course, or it wouldn't
> hang up at all), right?

It was a recent mainline kernel with your patch.

> Was it 32bit or 64bit kernel?

I don't remember. I think I tried both.

Helge
Al Viro Feb. 28, 2023, 7:14 p.m. UTC | #5
On Tue, Feb 28, 2023 at 07:26:48PM +0100, Helge Deller wrote:

> I can test both parisc32 and parisc64.
> 
> > 	Just to confirm: your "can be killed with ^C" had been on the
> > mainline parisc kernel (with userfaultfd enable, of course, or it wouldn't
> > hang up at all), right?
> 
> It was a recent mainline kernel with your patch.

Er...  Reproducer *is* supposed to block; the bug is that on the kernel
without this patch it would (AFAICS) be impossible to kill - not even
with kill -9.  With bug fixed the behaviour should be "blocks and is
killable", i.e. what you've reported.

What does it do on unpatched kernel?  *IF* the big is there, it would
block and be unkillable by any means.  Could you verify that?
Michael Schmitz Feb. 28, 2023, 7:18 p.m. UTC | #6
Guenter,

On 1/03/23 04:22, Guenter Roeck wrote:
> On Tue, Jan 31, 2023 at 08:06:27PM +0000, Al Viro wrote:
>> parisc equivalent of 26178ec11ef3 "x86: mm: consolidate VM_FAULT_RETRY handling"
>> If e.g. get_user() triggers a page fault and a fatal signal is caught, we might
>> end up with handle_mm_fault() returning VM_FAULT_RETRY and not doing anything
>> to page tables.  In such case we must *not* return to the faulting insn -
>> that would repeat the entire thing without making any progress; what we need
>> instead is to treat that as failed (user) memory access.
>>
>> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>> ---
>>   arch/parisc/mm/fault.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c
>> index 869204e97ec9..bb30ff6a3e19 100644
>> --- a/arch/parisc/mm/fault.c
>> +++ b/arch/parisc/mm/fault.c
>> @@ -308,8 +308,11 @@ void do_page_fault(struct pt_regs *regs, unsigned long code,
>>   
>>   	fault = handle_mm_fault(vma, address, flags, regs);
>>   
>> -	if (fault_signal_pending(fault, regs))
>> +	if (fault_signal_pending(fault, regs)) {
>> +		if (!user_mode(regs))
>> +			goto no_context;
> 0-day rightfully complains that this leaves 'msg' uninitialized.
>
> arch/parisc/mm/fault.c:427 do_page_fault() error: uninitialized symbol 'msg'
>
> Guenter

What happens if you initialize msg to "Page fault: no context" right at 
the start of do_page_fault (and drop the assignment a few lines down as 
that's now redundant)?

(Wondering if the zero page access on parisc could cause a trip right 
back into do_page_fault, ad infinitum...)

Cheers,

     Michael


>>   		return;
>> +	}
>>   
>>   	/* The fault is fully completed (including releasing mmap lock) */
>>   	if (fault & VM_FAULT_COMPLETED)
Helge Deller Feb. 28, 2023, 7:32 p.m. UTC | #7
Hi Al,

On 2/28/23 20:14, Al Viro wrote:
> On Tue, Feb 28, 2023 at 07:26:48PM +0100, Helge Deller wrote:
>
>> I can test both parisc32 and parisc64.
>>
>>> 	Just to confirm: your "can be killed with ^C" had been on the
>>> mainline parisc kernel (with userfaultfd enable, of course, or it wouldn't
>>> hang up at all), right?
>>
>> It was a recent mainline kernel with your patch.
>
> Er...  Reproducer *is* supposed to block; the bug is that on the kernel
> without this patch it would (AFAICS) be impossible to kill - not even
> with kill -9.  With bug fixed the behaviour should be "blocks and is
> killable", i.e. what you've reported.
>
> What does it do on unpatched kernel?  *IF* the big is there, it would
> block and be unkillable by any means.  Could you verify that?

I just tried 32- and 64-bit kernels.
With and without your patch on top of kernel 6.2.
The result is always the same:
The process hangs, but does not consume CPU and can be killed with ^C or kill command.

strace output is:

userfaultfd(0)                          = 3
ioctl(3, UFFDIO_API, {api=0xaa, features=0 => features=UFFD_FEATURE_EVENT_FORK|UFFD_FEATURE_EVENT_REMAP|UFFD_FEATURE_EVENT_REMOVE|UFFD_FEATURE_MISSING_HUGETLBFS|UFFD_FEATURE_MISSING_SHMEM|UFFD_FEATURE_EVENT_UNMAP|UFFD_FEATURE_SIGBUS|UFFD_FEATURE_THREAD_ID|0x800, ioctls=1<<_UFFDIO_REGISTER|1<<_UFFDIO_UNREGISTER|1<<_UFFDIO_API}) = 0
ioctl(3, UFFDIO_REGISTER, {range={start=0xf7afa000, len=0x1000}, mode=UFFDIO_REGISTER_MODE_MISSING, ioctls=1<<_UFFDIO_WAKE|1<<_UFFDIO_COPY|1<<_UFFDIO_ZEROPAGE}) = 0
fstatfs64(0, 88, {f_type=DEVPTS_SUPER_MAGIC, f_bsize=4096, f_blocks=0, f_bfree=0, f_bavail=0, f_files=0, f_ffree=0, f_fsid={val=[0, 0]}, f_namelen=255, f_frsize=4096, f_flags=ST_VALID|ST_NOSUID|ST_NOEXEC|ST_RELATIME}) = 0

<here it hangs until I kill the process with SIGTERM>

--- SIGTERM {si_signo=SIGTERM, si_code=SI_USER, si_pid=859, si_uid=0} ---
+++ killed by SIGTERM +++
Terminated

It seems the code flow doesn't reaches fault_signal_pending(), or it returns false...

Helge
Helge Deller Feb. 28, 2023, 8 p.m. UTC | #8
On 2/28/23 20:32, Helge Deller wrote:
> Hi Al,
>
> On 2/28/23 20:14, Al Viro wrote:
>> On Tue, Feb 28, 2023 at 07:26:48PM +0100, Helge Deller wrote:
>>
>>> I can test both parisc32 and parisc64.
>>>
>>>>     Just to confirm: your "can be killed with ^C" had been on the
>>>> mainline parisc kernel (with userfaultfd enable, of course, or it wouldn't
>>>> hang up at all), right?
>>>
>>> It was a recent mainline kernel with your patch.
>>
>> Er...  Reproducer *is* supposed to block; the bug is that on the kernel
>> without this patch it would (AFAICS) be impossible to kill - not even
>> with kill -9.  With bug fixed the behaviour should be "blocks and is
>> killable", i.e. what you've reported.
>>
>> What does it do on unpatched kernel?  *IF* the big is there, it would
>> block and be unkillable by any means.  Could you verify that?
>
> I just tried 32- and 64-bit kernels.
> With and without your patch on top of kernel 6.2.
> The result is always the same:
> The process hangs, but does not consume CPU and can be killed with ^C or kill command.
>
> strace output is:
>
> userfaultfd(0)                          = 3
> ioctl(3, UFFDIO_API, {api=0xaa, features=0 => features=UFFD_FEATURE_EVENT_FORK|UFFD_FEATURE_EVENT_REMAP|UFFD_FEATURE_EVENT_REMOVE|UFFD_FEATURE_MISSING_HUGETLBFS|UFFD_FEATURE_MISSING_SHMEM|UFFD_FEATURE_EVENT_UNMAP|UFFD_FEATURE_SIGBUS|UFFD_FEATURE_THREAD_ID|0x800, ioctls=1<<_UFFDIO_REGISTER|1<<_UFFDIO_UNREGISTER|1<<_UFFDIO_API}) = 0
> ioctl(3, UFFDIO_REGISTER, {range={start=0xf7afa000, len=0x1000}, mode=UFFDIO_REGISTER_MODE_MISSING, ioctls=1<<_UFFDIO_WAKE|1<<_UFFDIO_COPY|1<<_UFFDIO_ZEROPAGE}) = 0
> fstatfs64(0, 88, {f_type=DEVPTS_SUPER_MAGIC, f_bsize=4096, f_blocks=0, f_bfree=0, f_bavail=0, f_files=0, f_ffree=0, f_fsid={val=[0, 0]}, f_namelen=255, f_frsize=4096, f_flags=ST_VALID|ST_NOSUID|ST_NOEXEC|ST_RELATIME}) = 0

Ok, one step further...

We get fooled by glibc, which replaces this line in the testcase:
         ret = fstatfs(0, (struct statfs *)mem);
and instead executes the 64-bit variant fstatfs64() inside a wrapper in glibc on another address.

I replaced that line in the testcase to use the 32-bit syscall with the right (userfault-specified) address by
         ret = syscall(__NR_fstatfs, 0, mem);
and now I see:
userfaultfd(0)                          = 3
ioctl(3, UFFDIO_API, {api=0xaa, features=0 => features=UFFD_FEATURE_EVENT_FORK|UFFD_FEATURE_EVENT_REMAP|UFFD_FEATURE_EVENT_REMOVE|UFFD_FEATURE_MISSING_HUGETLBFS|UFFD_FEATURE_MISSING_SHMEM|UFFD_FEATURE_EVENT_UNMAP|UFFD_FEATURE_SIGBUS|UFFD_FEATURE_THREAD_ID|0x800, ioctls=1<<_UFFDIO_REGISTER|1<<_UFFDIO_UNREGISTER|1<<_UFFDIO_API}) = 0
ioctl(3, UFFDIO_REGISTER, {range={start=0xf7afa000, len=0x1000}, mode=UFFDIO_REGISTER_MODE_MISSING, ioctls=1<<_UFFDIO_WAKE|1<<_UFFDIO_COPY|1<<_UFFDIO_ZEROPAGE}) = 0
fstatfs(0,
<here it hangs now and is unkillable>

Needs further testing.

Helge
Helge Deller Feb. 28, 2023, 8:22 p.m. UTC | #9
Hi Al,

On 2/28/23 21:00, Helge Deller wrote:
> On 2/28/23 20:32, Helge Deller wrote:
>> Hi Al,
>>
>> On 2/28/23 20:14, Al Viro wrote:
>>> On Tue, Feb 28, 2023 at 07:26:48PM +0100, Helge Deller wrote:
>>>
>>>> I can test both parisc32 and parisc64.
>>>>
>>>>>     Just to confirm: your "can be killed with ^C" had been on the
>>>>> mainline parisc kernel (with userfaultfd enable, of course, or it wouldn't
>>>>> hang up at all), right?
>>>>
>>>> It was a recent mainline kernel with your patch.
>>>
>>> Er...  Reproducer *is* supposed to block; the bug is that on the kernel
>>> without this patch it would (AFAICS) be impossible to kill - not even
>>> with kill -9.  With bug fixed the behaviour should be "blocks and is
>>> killable", i.e. what you've reported.
>>>
>>> What does it do on unpatched kernel?  *IF* the big is there, it would
>>> block and be unkillable by any means.  Could you verify that?
>>
>> I just tried 32- and 64-bit kernels.
>> With and without your patch on top of kernel 6.2.
>> The result is always the same:
>> The process hangs, but does not consume CPU and can be killed with ^C or kill command.
>>
>> strace output is:
>>
>> userfaultfd(0)                          = 3
>> ioctl(3, UFFDIO_API, {api=0xaa, features=0 => features=UFFD_FEATURE_EVENT_FORK|UFFD_FEATURE_EVENT_REMAP|UFFD_FEATURE_EVENT_REMOVE|UFFD_FEATURE_MISSING_HUGETLBFS|UFFD_FEATURE_MISSING_SHMEM|UFFD_FEATURE_EVENT_UNMAP|UFFD_FEATURE_SIGBUS|UFFD_FEATURE_THREAD_ID|0x800, ioctls=1<<_UFFDIO_REGISTER|1<<_UFFDIO_UNREGISTER|1<<_UFFDIO_API}) = 0
>> ioctl(3, UFFDIO_REGISTER, {range={start=0xf7afa000, len=0x1000}, mode=UFFDIO_REGISTER_MODE_MISSING, ioctls=1<<_UFFDIO_WAKE|1<<_UFFDIO_COPY|1<<_UFFDIO_ZEROPAGE}) = 0
>> fstatfs64(0, 88, {f_type=DEVPTS_SUPER_MAGIC, f_bsize=4096, f_blocks=0, f_bfree=0, f_bavail=0, f_files=0, f_ffree=0, f_fsid={val=[0, 0]}, f_namelen=255, f_frsize=4096, f_flags=ST_VALID|ST_NOSUID|ST_NOEXEC|ST_RELATIME}) = 0
>
> Ok, one step further...
>
> We get fooled by glibc, which replaces this line in the testcase:
>          ret = fstatfs(0, (struct statfs *)mem);
> and instead executes the 64-bit variant fstatfs64() inside a wrapper in glibc on another address.
>
> I replaced that line in the testcase to use the 32-bit syscall with the right (userfault-specified) address by
>          ret = syscall(__NR_fstatfs, 0, mem);
> and now I see:
> userfaultfd(0)                          = 3
> ioctl(3, UFFDIO_API, {api=0xaa, features=0 => features=UFFD_FEATURE_EVENT_FORK|UFFD_FEATURE_EVENT_REMAP|UFFD_FEATURE_EVENT_REMOVE|UFFD_FEATURE_MISSING_HUGETLBFS|UFFD_FEATURE_MISSING_SHMEM|UFFD_FEATURE_EVENT_UNMAP|UFFD_FEATURE_SIGBUS|UFFD_FEATURE_THREAD_ID|0x800, ioctls=1<<_UFFDIO_REGISTER|1<<_UFFDIO_UNREGISTER|1<<_UFFDIO_API}) = 0
> ioctl(3, UFFDIO_REGISTER, {range={start=0xf7afa000, len=0x1000}, mode=UFFDIO_REGISTER_MODE_MISSING, ioctls=1<<_UFFDIO_WAKE|1<<_UFFDIO_COPY|1<<_UFFDIO_ZEROPAGE}) = 0
> fstatfs(0,
> <here it hangs now and is unkillable>
>
> Needs further testing.

Now I can confirm (with the adjusted reproducer), that your patch
allows to kill the process with SIGKILL, while without your patch
it's not possibe to kill the process at all.
I've tested with a 32- and 64-bit parisc kernel.

You may add
Tested-by: Helge Deller <deller@gmx.de> # parisc
to the patch.

If you want me to take the patch (with the warning regarding missing msg variable fixed)
through the parisc tree, please let me know.

Thank you!
Helge
Al Viro Feb. 28, 2023, 10:57 p.m. UTC | #10
On Tue, Feb 28, 2023 at 09:22:31PM +0100, Helge Deller wrote:

> Now I can confirm (with the adjusted reproducer), that your patch
> allows to kill the process with SIGKILL, while without your patch
> it's not possibe to kill the process at all.
> I've tested with a 32- and 64-bit parisc kernel.
> 
> You may add
> Tested-by: Helge Deller <deller@gmx.de> # parisc
> to the patch.
> 
> If you want me to take the patch (with the warning regarding missing msg variable fixed)
> through the parisc tree, please let me know.

What message do you prefer there?  It matters only for the case when
we are hitting an oops there, but...
Helge Deller March 1, 2023, 4 a.m. UTC | #11
On 2/28/23 23:57, Al Viro wrote:
> On Tue, Feb 28, 2023 at 09:22:31PM +0100, Helge Deller wrote:
>
>> Now I can confirm (with the adjusted reproducer), that your patch
>> allows to kill the process with SIGKILL, while without your patch
>> it's not possibe to kill the process at all.
>> I've tested with a 32- and 64-bit parisc kernel.
>>
>> You may add
>> Tested-by: Helge Deller <deller@gmx.de> # parisc
>> to the patch.
>>
>> If you want me to take the patch (with the warning regarding missing msg variable fixed)
>> through the parisc tree, please let me know.
>
> What message do you prefer there?  It matters only for the case when
> we are hitting an oops there, but...

I have no preference on that message. Maybe "Page fault: fault signal on kernel memory"
is too generic, so I'm fine with any better idea/wording you come up with.

Helge
Al Viro March 2, 2023, 5:53 p.m. UTC | #12
On Wed, Mar 01, 2023 at 05:00:11AM +0100, Helge Deller wrote:
> On 2/28/23 23:57, Al Viro wrote:
> > On Tue, Feb 28, 2023 at 09:22:31PM +0100, Helge Deller wrote:
> > 
> > > Now I can confirm (with the adjusted reproducer), that your patch
> > > allows to kill the process with SIGKILL, while without your patch
> > > it's not possibe to kill the process at all.
> > > I've tested with a 32- and 64-bit parisc kernel.
> > > 
> > > You may add
> > > Tested-by: Helge Deller <deller@gmx.de> # parisc
> > > to the patch.
> > > 
> > > If you want me to take the patch (with the warning regarding missing msg variable fixed)
> > > through the parisc tree, please let me know.
> > 
> > What message do you prefer there?  It matters only for the case when
> > we are hitting an oops there, but...
> 
> I have no preference on that message. Maybe "Page fault: fault signal on kernel memory"
> is too generic, so I'm fine with any better idea/wording you come up with.

Ended up going with that...  Series reordered and pushed out in #fixes.
Status:
	m68k, riscv, hexagon, parisc: tested by arch maintainers
	alpha, sparc: locally tested (both sparc32 and sparc64 parts)
	ia64, microblaze, nios2, openrisc: builds, but I have no way to test the
resulting kernels (itanic box appears to be pining for fjords and I've no emulator
setup for that, the other three I don't even have userland to test with).
diff mbox series

Patch

diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c
index 869204e97ec9..bb30ff6a3e19 100644
--- a/arch/parisc/mm/fault.c
+++ b/arch/parisc/mm/fault.c
@@ -308,8 +308,11 @@  void do_page_fault(struct pt_regs *regs, unsigned long code,
 
 	fault = handle_mm_fault(vma, address, flags, regs);
 
-	if (fault_signal_pending(fault, regs))
+	if (fault_signal_pending(fault, regs)) {
+		if (!user_mode(regs))
+			goto no_context;
 		return;
+	}
 
 	/* The fault is fully completed (including releasing mmap lock) */
 	if (fault & VM_FAULT_COMPLETED)