lkml.org 
[lkml]   [2011]   [Mar]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: + x86-mm-handle-mm_fault_error-in-kernel-space.patch added to -mm tree
On 03/10/2011 05:28 PM, Oleg Nesterov wrote:
> (add cc's)
>
>> Subject: x86/mm: handle mm_fault_error() in kernel space
>> From: Andrey Vagin<avagin@openvz.org>
>>
>> mm_fault_error() should not execute oom-killer, if page fault occurs in
>> kernel space. E.g. in copy_from_user/copy_to_user.
> Why? I don't understand this part.
I thought for a bit more...

I think we should not execute out_of_memory() in this case at all,
because when we return from page fault, we execute the same command and
provoke the "same" page fault again. Now pls think what is the
difference between these page faults? It has been generated from one
place and the program do nothing between those. I think we try to handle
one error two times and it isn't good. If handle_mm_fault() returns
VM_FAULT_OOM and pagefault occurred from userspace, the current task
should be killed by SIGKILL, then we should return from page fault back
to userspace for the task to handle the signal. If handle_mm_fault()
returns VM_FAULT_OOM and pagefault occurred in kernel space, we should
execute no_context() to return from syscall.

Also note that out_of_memory is usually called from handle_mm_fault() ->
... -> alloc_page()->...->out_of_memory().

>> This would happen if we find ourselves in OOM on a copy_to_user(), or a
>> copy_from_user() which faults.
>>
>> Without this patch, the kernels hangs up in copy_from_user, because OOM
>> killer sends SIG_KILL to current process,
> This depends. OOM can choose another victim, and if it does we shouldn't
> return -EFAULT.
>
>> but it can't handle a signal
>> while in syscall, then the kernel returns to copy_from_user, reexcute
>> current command and provokes page_fault again.
> Yes. This is buggy.
>
>> --- a/arch/x86/mm/fault.c~x86-mm-handle-mm_fault_error-in-kernel-space
>> +++ a/arch/x86/mm/fault.c
>> @@ -827,6 +827,13 @@ mm_fault_error(struct pt_regs *regs, uns
>> unsigned long address, unsigned int fault)
>> {
>> if (fault& VM_FAULT_OOM) {
>> + /* Kernel mode? Handle exceptions or die: */
>> + if (!(error_code& PF_USER)) {
>> + up_read(&current->mm->mmap_sem);
>> + no_context(regs, error_code, address);
>> + return;
>> + }
>> +
> At first glance, this is not optimal...
>
> Perhaps I missed something, but afaics it is better to call
> out_of_memory() first, then check if current was killed. In this case
> no_context() is fine, we are not going to return to the user-mode.
It's not first oom, so I perfere don't ca
> IOW, what do you think about the (untested/uncompiled) patch below?
>
> Oleg.
>
> --- x/arch/x86/mm/fault.c
> +++ x/arch/x86/mm/fault.c
> @@ -829,6 +829,11 @@ mm_fault_error(struct pt_regs *regs, uns
> {
> if (fault& VM_FAULT_OOM) {
> out_of_memory(regs, error_code, address);
> +
> + if (!(error_code& PF_USER)&& fatal_signal_pending(current)) {
> + no_context(regs, error_code, address);
> + return;
> + }
> } else {
> if (fault& (VM_FAULT_SIGBUS|VM_FAULT_HWPOISON|
> VM_FAULT_HWPOISON_LARGE))
>
>



\
 
 \ /
  Last update: 2011-03-10 20:33    [W:0.083 / U:0.740 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site