Message-ID: <20090529104901.GA5997@nowhere>
Date:	Fri, 29 May 2009 12:49:03 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	"K.Prasad" <prasad@...ux.vnet.ibm.com>
Cc:	David Gibson <dwg@....ibm.com>,
	Alan Stern <stern@...land.harvard.edu>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ingo Molnar <mingo@...e.hu>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Benjamin Herrenschmidt <benh@....ibm.com>,
	maneesh@...ux.vnet.ibm.com, Roland McGrath <roland@...hat.com>,
	Masami Hiramatsu <mhiramat@...hat.com>
Subject: Re: [Patch 06/12] Use the new wrapper routines to access debug
	registers in process/thread code

On Fri, May 29, 2009 at 02:31:46PM +0530, K.Prasad wrote:
> On Thu, May 28, 2009 at 04:42:38PM +1000, David Gibson wrote:
> > On Mon, May 11, 2009 at 05:23:44PM +0530, K.Prasad wrote:
> > > From: Alan Stern <stern@...land.harvard.edu>
> > > 
> > > This patch enables the use of abstract debug registers in
> > > process-handling routines.
> > 
> > [snip]
> > >  
> > > +	p->thread.io_bitmap_ptr = NULL;
> > 
> > Why is manipulating the io_bitmap_ptr relevant to debug register
> > handling?
> 
> I *re-read* the patch but was unable to find how this change had sneaked
> in. It shouldn't be there although it is harmless.


When I reviewed this patch, I also ended stucked on it.
But actually I guess I found the sense, this is only for
convenience.

Look at the current copy_thread() in arch/x86/kernel/process32.c

If p->thread.io_bitmap_ptr fails to be duplicated, we set
p->thread.io_bitmap_max = 0 and return -ENOMEM

Now look at the patch.
If we fail to copy the hardware thread virtual registers we
want to exit with io_bitmap_ptr = NULL
If we fail to copy the io_bitmap, we want to free the breakpoint
and exit.
If we fail further, we want to free breakpoints and io_bitmap_ptr

The out section then tries to:

-free the breakpoints
-free p->thread.io_bitmap_ptr


So it's important to set io_bitmap_ptr to NULL so that
we know whether we have to release it or not.

 
> Hi Frederic,
> 	I am attaching a new version of this patch 06/12 that:
> 
> - removes the line that assigns NULL to "p->thread.io_bitmap_ptr"


Dangerous. Unless p->thread.io_bitmap_ptr is already zeroed out
at this stage?

 
> - Updates the comment in __switch_to() function which was stale (was
>   relevant when 'last_debugged_task' was used to detect lazy debug
>   register switching).
> 
> Kindly integrate this version in lieu of the older version sent here:
> http://lkml.org/lkml/2009/5/21/149.


Ok. Well it would be nice if you resend the whole series actually :)
Do you have another fix pending?

Thanks!

> Hi Alan,
> 	I'm retaining the
> "Reviewed-by: Alan Stern <stern@...land.harvard.edu>" tag on the patch
> even after the above changes and guess it is acceptable to you. Please
> let me know if you have any concerns about that.
> 
> Thanks,
> K.Prasad
> 
> 
> Use the new wrapper routines to access debug registers in process/thread code
> 
> This patch enables the use of abstract debug registers in
> process-handling routines.
> 
> Original-patch-by: Alan Stern <stern@...land.harvard.edu>
> Signed-off-by: K.Prasad <prasad@...ux.vnet.ibm.com>
> Reviewed-by: Alan Stern <stern@...land.harvard.edu>
> ---
>  arch/x86/kernel/process.c    |   23 ++++++-----------------
>  arch/x86/kernel/process_32.c |   27 +++++++++++++++++++++++++++
>  arch/x86/kernel/process_64.c |   30 ++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 17 deletions(-)
> 
> Index: linux-2.6-tip.hbkpt/arch/x86/kernel/process.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/arch/x86/kernel/process.c
> +++ linux-2.6-tip.hbkpt/arch/x86/kernel/process.c
> @@ -17,6 +17,8 @@
>  #include <asm/uaccess.h>
>  #include <asm/i387.h>
>  #include <asm/ds.h>
> +#include <asm/debugreg.h>
> +#include <asm/hw_breakpoint.h>
>  
>  unsigned long idle_halt;
>  EXPORT_SYMBOL(idle_halt);
> @@ -48,6 +50,8 @@ void free_thread_xstate(struct task_stru
>  		kmem_cache_free(task_xstate_cachep, tsk->thread.xstate);
>  		tsk->thread.xstate = NULL;
>  	}
> +	if (unlikely(test_tsk_thread_flag(tsk, TIF_DEBUG)))
> +		flush_thread_hw_breakpoint(tsk);
>  
>  	WARN(tsk->thread.ds_ctx, "leaking DS context\n");
>  }
> @@ -106,14 +110,9 @@ void flush_thread(void)
>  	}
>  #endif
>  
> -	clear_tsk_thread_flag(tsk, TIF_DEBUG);
> +	if (unlikely(test_tsk_thread_flag(tsk, TIF_DEBUG)))
> +		flush_thread_hw_breakpoint(tsk);
>  
> -	tsk->thread.debugreg0 = 0;
> -	tsk->thread.debugreg1 = 0;
> -	tsk->thread.debugreg2 = 0;
> -	tsk->thread.debugreg3 = 0;
> -	tsk->thread.debugreg6 = 0;
> -	tsk->thread.debugreg7 = 0;
>  	memset(tsk->thread.tls_array, 0, sizeof(tsk->thread.tls_array));
>  	/*
>  	 * Forget coprocessor state..
> @@ -195,16 +194,6 @@ void __switch_to_xtra(struct task_struct
>  	else if (next->debugctlmsr != prev->debugctlmsr)
>  		update_debugctlmsr(next->debugctlmsr);
>  
> -	if (test_tsk_thread_flag(next_p, TIF_DEBUG)) {
> -		set_debugreg(next->debugreg0, 0);
> -		set_debugreg(next->debugreg1, 1);
> -		set_debugreg(next->debugreg2, 2);
> -		set_debugreg(next->debugreg3, 3);
> -		/* no 4 and 5 */
> -		set_debugreg(next->debugreg6, 6);
> -		set_debugreg(next->debugreg7, 7);
> -	}
> -
>  	if (test_tsk_thread_flag(prev_p, TIF_NOTSC) ^
>  	    test_tsk_thread_flag(next_p, TIF_NOTSC)) {
>  		/* prev and next are different */
> Index: linux-2.6-tip.hbkpt/arch/x86/kernel/process_32.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/arch/x86/kernel/process_32.c
> +++ linux-2.6-tip.hbkpt/arch/x86/kernel/process_32.c
> @@ -58,6 +58,8 @@
>  #include <asm/idle.h>
>  #include <asm/syscalls.h>
>  #include <asm/ds.h>
> +#include <asm/debugreg.h>
> +#include <asm/hw_breakpoint.h>
>  
>  asmlinkage void ret_from_fork(void) __asm__("ret_from_fork");
>  
> @@ -263,6 +265,11 @@ int copy_thread(unsigned long clone_flag
>  	task_user_gs(p) = get_user_gs(regs);
>  
>  	tsk = current;
> +	err = -ENOMEM;
> +	if (unlikely(test_tsk_thread_flag(tsk, TIF_DEBUG)))
> +		if (copy_thread_hw_breakpoint(tsk, p, clone_flags))
> +			goto out;
> +
>  	if (unlikely(test_tsk_thread_flag(tsk, TIF_IO_BITMAP))) {
>  		p->thread.io_bitmap_ptr = kmemdup(tsk->thread.io_bitmap_ptr,
>  						IO_BITMAP_BYTES, GFP_KERNEL);
> @@ -282,10 +289,13 @@ int copy_thread(unsigned long clone_flag
>  		err = do_set_thread_area(p, -1,
>  			(struct user_desc __user *)childregs->si, 0);
>  
> +out:
>  	if (err && p->thread.io_bitmap_ptr) {
>  		kfree(p->thread.io_bitmap_ptr);
>  		p->thread.io_bitmap_max = 0;
>  	}
> +	if (err)
> +		flush_thread_hw_breakpoint(p);
>  
>  	clear_tsk_thread_flag(p, TIF_DS_AREA_MSR);
>  	p->thread.ds_ctx = NULL;
> @@ -424,6 +434,23 @@ __switch_to(struct task_struct *prev_p, 
>  		lazy_load_gs(next->gs);
>  
>  	percpu_write(current_task, next_p);
> +	/*
> +	 * There's a problem with moving the arch_install_thread_hw_breakpoint()
> +	 * call before current is updated.  Suppose a kernel breakpoint is
> +	 * triggered in between the two, the hw-breakpoint handler will see that
> +	 * the 'current' task does not have TIF_DEBUG flag set and will think it
> +	 * is leftover from an old task (lazy switching) and will erase it. Then
> +	 * until the next context switch, no user-breakpoints will be installed.
> +	 *
> +	 * The real problem is that it's impossible to update both current and
> +	 * physical debug registers at the same instant, so there will always be
> +	 * a window in which they disagree and a breakpoint might get triggered.
> +	 * Since we use lazy switching, we are forced to assume that a
> +	 * disagreement means that current is correct and the exception is due
> +	 * to lazy debug register switching.
> +	 */
> +	if (unlikely(test_tsk_thread_flag(next_p, TIF_DEBUG)))
> +		arch_install_thread_hw_breakpoint(next_p);
>  
>  	return prev_p;
>  }
> Index: linux-2.6-tip.hbkpt/arch/x86/kernel/process_64.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/arch/x86/kernel/process_64.c
> +++ linux-2.6-tip.hbkpt/arch/x86/kernel/process_64.c
> @@ -52,6 +52,8 @@
>  #include <asm/idle.h>
>  #include <asm/syscalls.h>
>  #include <asm/ds.h>
> +#include <asm/debugreg.h>
> +#include <asm/hw_breakpoint.h>
>  
>  asmlinkage extern void ret_from_fork(void);
>  
> @@ -245,6 +247,8 @@ void release_thread(struct task_struct *
>  			BUG();
>  		}
>  	}
> +	if (unlikely(dead_task->thread.debugreg7))
> +		flush_thread_hw_breakpoint(dead_task);
>  }
>  
>  static inline void set_32bit_tls(struct task_struct *t, int tls, u32 addr)
> @@ -306,6 +310,11 @@ int copy_thread(unsigned long clone_flag
>  	savesegment(es, p->thread.es);
>  	savesegment(ds, p->thread.ds);
>  
> +	err = -ENOMEM;
> +	if (unlikely(test_tsk_thread_flag(me, TIF_DEBUG)))
> +		if (copy_thread_hw_breakpoint(me, p, clone_flags))
> +			goto out;
> +
>  	if (unlikely(test_tsk_thread_flag(me, TIF_IO_BITMAP))) {
>  		p->thread.io_bitmap_ptr = kmalloc(IO_BITMAP_BYTES, GFP_KERNEL);
>  		if (!p->thread.io_bitmap_ptr) {
> @@ -344,6 +353,9 @@ out:
>  		kfree(p->thread.io_bitmap_ptr);
>  		p->thread.io_bitmap_max = 0;
>  	}
> +	if (err)
> +		flush_thread_hw_breakpoint(p);
> +
>  	return err;
>  }
>  
> @@ -489,6 +501,24 @@ __switch_to(struct task_struct *prev_p, 
>  	 */
>  	if (tsk_used_math(next_p) && next_p->fpu_counter > 5)
>  		math_state_restore();
> +	/*
> +	 * There's a problem with moving the arch_install_thread_hw_breakpoint()
> +	 * call before current is updated.  Suppose a kernel breakpoint is
> +	 * triggered in between the two, the hw-breakpoint handler will see that
> +	 * the 'current' task does not have TIF_DEBUG flag set and will think it
> +	 * is leftover from an old task (lazy switching) and will erase it. Then
> +	 * until the next context switch, no user-breakpoints will be installed.
> +	 *
> +	 * The real problem is that it's impossible to update both current and
> +	 * physical debug registers at the same instant, so there will always be
> +	 * a window in which they disagree and a breakpoint might get triggered.
> +	 * Since we use lazy switching, we are forced to assume that a
> +	 * disagreement means that current is correct and the exception is due
> +	 * to lazy debug register switching.
> +	 */
> +	if (unlikely(test_tsk_thread_flag(next_p, TIF_DEBUG)))
> +		arch_install_thread_hw_breakpoint(next_p);
> +
>  	return prev_p;
>  }
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists