[PATCH] enable interrupts in user path of page fault.

[PATCH] enable interrupts in user path of page fault.

[Steven Rostedt]

This is a minor fix, but what is currently there is essentially wrong.
In do_page_fault, if the faulting address from user code happens to be
in kernel address space (int *p = (int*)-1; p = 0xbed;)  then the
do_page_fault handler will jump over the local_irq_enable with the

  goto bad_area_nosemaphore;

But the first line there sees this is user code and goes through the
process of sending a signal to send SIGSEGV to the user task. This whole
time interrupts are disabled and the task can not be preempted by a
higher priority task.

This patch always enables interrupts in the user path of the
bad_area_nosemaphore.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/arch/i386/mm/fault.c b/arch/i386/mm/fault.c
index 29d7d61..1ecb3e4 100644
--- a/arch/i386/mm/fault.c
+++ b/arch/i386/mm/fault.c
@@ -458,6 +458,11 @@ bad_area:
 bad_area_nosemaphore:
 	/* User mode accesses just cause a SIGSEGV */
 	if (error_code & 4) {
+		/*
+		 * It's possible to have interrupts off here.
+		 */
+		local_irq_enable();
+
 		/* 
 		 * Valid to do another page fault here because this one came 
 		 * from user space.
diff --git a/arch/x86_64/mm/fault.c b/arch/x86_64/mm/fault.c
index bfb62a1..635e58d 100644
--- a/arch/x86_64/mm/fault.c
+++ b/arch/x86_64/mm/fault.c
@@ -476,6 +476,12 @@ bad_area:
 bad_area_nosemaphore:
 	/* User mode accesses just cause a SIGSEGV */
 	if (error_code & PF_USER) {
+
+		/*
+		 * It's possible to have interrupts off here.
+		 */
+		local_irq_enable();
+
 		if (is_prefetch(regs, address, error_code))
 			return;
 

Re: [PATCH] enable interrupts in user path of page fault.

[Andrew Morton]

On Wed, 06 Jun 2007 23:34:04 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> This is a minor fix, but what is currently there is essentially wrong.
> In do_page_fault, if the faulting address from user code happens to be
> in kernel address space (int *p = (int*)-1; p = 0xbed;)  then the
> do_page_fault handler will jump over the local_irq_enable with the
> 
>   goto bad_area_nosemaphore;
> 
> But the first line there sees this is user code and goes through the
> process of sending a signal to send SIGSEGV to the user task. This whole
> time interrupts are disabled and the task can not be preempted by a
> higher priority task.
> 
> This patch always enables interrupts in the user path of the
> bad_area_nosemaphore.
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> diff --git a/arch/i386/mm/fault.c b/arch/i386/mm/fault.c
> index 29d7d61..1ecb3e4 100644
> --- a/arch/i386/mm/fault.c
> +++ b/arch/i386/mm/fault.c
> @@ -458,6 +458,11 @@ bad_area:
>  bad_area_nosemaphore:
>  	/* User mode accesses just cause a SIGSEGV */
>  	if (error_code & 4) {
> +		/*
> +		 * It's possible to have interrupts off here.
> +		 */
> +		local_irq_enable();
> +

Interrupts got disabled here because do_page_fault() is an
interrupt-disabling trap, yes?

The patch looks reasonable to me: a slight reduction in interrupt-off
latency when really weird things are happening.

The patch also breaks things, I think: if userspace is running with
interrupts disabled and tries to access kernel memory it will presently
whizz through the kernel without ever enabling interrupts.  With this
change, the kernel will now enable interrupts, which is presumably not what
the application wanted.

However it's surely already the case that most pagefaults will go and
enable interrupts on this process anyway, so no big loss there.  I'd expect
the kernel to spit piles of might_sleep() warnings when all this happens, so
maybe it just doesn't happen for some reason.

Re: [PATCH] enable interrupts in user path of page fault.

[Steven Rostedt]

On Thu, 2007-06-07 at 18:58 -0700, Andrew Morton wrote:
> On Wed, 06 Jun 2007 23:34:04 -0400

> Interrupts got disabled here because do_page_fault() is an
> interrupt-disabling trap, yes?

Correct.

> 
> The patch looks reasonable to me: a slight reduction in interrupt-off
> latency when really weird things are happening.
> 
> The patch also breaks things, I think: if userspace is running with
> interrupts disabled and tries to access kernel memory it will presently
> whizz through the kernel without ever enabling interrupts.  With this
> change, the kernel will now enable interrupts, which is presumably not what
> the application wanted.

I didn't realize that userspace was allowed to run with interrupts
disabled. If this becomes a problem, we can add the same check that's
above where do_page_fault does enable interrupts, but is skipped because
the faulting address was above PAGE_OFFSET.


ie. (i386)
        if (regs->eflags & (X86_EFLAGS_IF|VM_MASK))
                local_irq_enable();


> 
> However it's surely already the case that most pagefaults will go and
> enable interrupts on this process anyway, so no big loss there.  I'd expect
> the kernel to spit piles of might_sleep() warnings when all this happens, so
> maybe it just doesn't happen for some reason.

Actually it does on the RT kernel. Hence why I found it ;-)

-- Steve

Re: [PATCH] enable interrupts in user path of page fault.

[Linus Torvalds]

On Thu, 7 Jun 2007, Andrew Morton wrote:
> 
> Interrupts got disabled here because do_page_fault() is an
> interrupt-disabling trap, yes?

Yes - and it has to be: we want to disable preemption and interrupts that 
can fault on the vmalloc space, until we've at least saved away %cr2. We 
had bugs in that area before.

> The patch looks reasonable to me: a slight reduction in interrupt-off
> latency when really weird things are happening.

I applied it as obviously correct.

> The patch also breaks things, I think: if userspace is running with
> interrupts disabled and tries to access kernel memory it will presently
> whizz through the kernel without ever enabling interrupts.  With this
> change, the kernel will now enable interrupts, which is presumably not what
> the application wanted.

Well, we *do* enable interrupts for real page faults anyway, and this 
whole code just triggers for the case where we'd send a SIGSEGV. If some 
silly app really thought it could do that with interrupts disabled, it was 
wrong before too (we'd hit a reschedule point and enable them there 
anyway).

		Linus

Re: [PATCH] enable interrupts in user path of page fault.

[Linus Torvalds]

On Thu, 7 Jun 2007, Steven Rostedt wrote:
> 
> I didn't realize that userspace was allowed to run with interrupts
> disabled.

It isn't, normally. You *can* try to do it by using iopl(), but it's not 
practical. It's certainly not practical to expect a page fault to not 
enable them again, since we obviously need interrupts enabled just to 
handle any IO. 

So don't worry about it. 

		Linus