lkml.org 
[lkml]   [2009]   [May]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@rowland.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@rowland.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@rowland.harvard.edu>
    > Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
    > Reviewed-by: Alan Stern <stern@rowland.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;
    > }
    >



    \
     
     \ /
      Last update: 2009-05-29 12:51    [W:0.045 / U:29.352 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site