Re: [Kgdb-bugreport] [PATCH] Solving the preempt count problem

[Amit S. K. <ami...@li...>]

Looks good.
-Amit

On Thursday 07 June 2007 10:02, Jason Wessel wrote:
> Well since source forge is eating patches of my again...  I use cut and
> paste.
>
> --------------------------------------------------------
> It is necessary to restore the preempt count in a preempt kernel
> because there can be any number of faults that generate one or more
> exceptions.
>
> Generally there will only be one for the die handler, but the safest
> case is to restore the preempt count to what it was when the context
> was saved.
>
> This patch will handle it in an architecture independent way.
>
> Signed-off-by: Jason Wessel <jas...@wi...>
>
> ---
>  kernel/kgdb.c |   56
> +++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 39 insertions(+), 17 deletions(-)
>
> Index: linux-2.6.21-standard/kernel/kgdb.c
> ===================================================================
> --- linux-2.6.21-standard.orig/kernel/kgdb.c
> +++ linux-2.6.21-standard/kernel/kgdb.c
> @@ -69,6 +69,10 @@ int kgdb_connected;
>  /* Could we be about to try and access a bad memory location? If so we
>   * also need to flag this has happend. */
>  int kgdb_may_fault;
> +#ifdef CONFIG_PREEMPT
> +static int kgdb_fault_preempt_count;
> +#endif
> +
>  /* All the KGDB handlers are installed */
>  int kgdb_from_module_registered = 0;
>  /* Guard for recursive entry */
> @@ -232,6 +236,20 @@ static void get_packet(char *buffer)
>      } while (checksum != xmitcsum);
>  }
>
> +static void kgdb_set_may_fault(void) {
> +    kgdb_may_fault = 1;
> +#ifdef CONFIG_PREEMPT
> +    kgdb_fault_preempt_count = preempt_count();
> +#endif
> +}
> +
> +static void kgdb_unset_may_fault(void) {
> +    kgdb_may_fault = 0;
> +#ifdef CONFIG_PREEMPT
> +    preempt_count() = kgdb_fault_preempt_count;
> +#endif
> +}
> +
>  /*
>   * Send the packet in buffer.
>   * Check for gdb connection if asked for.
> @@ -291,9 +309,9 @@ static void put_packet(char *buffer)
>   */
>  char *kgdb_mem2hex(char *mem, char *buf, int count)
>  {
> -    kgdb_may_fault = 1;
> +    kgdb_set_may_fault();
>      if ((kgdb_fault_setjmp(kgdb_fault_jmp_regs)) != 0) {
> -        kgdb_may_fault = 0;
> +        kgdb_unset_may_fault();
>          return ERR_PTR(-EINVAL);
>      }
>      /* Accessing some registers in a single load instruction is
> @@ -382,7 +400,7 @@ char *kgdb_mem2hex(char *mem, char *buf,
>              *buf++ = hexchars[ch & 0xf];
>          }
>      }
> -    kgdb_may_fault = 0;
> +    kgdb_unset_may_fault();
>      *buf = 0;
>      return (buf);
>  }
> @@ -394,9 +412,9 @@ char *kgdb_mem2hex(char *mem, char *buf,
>   */
>  static char *kgdb_ebin2mem(char *buf, char *mem, int count)
>  {
> -    kgdb_may_fault = 1;
> +    kgdb_set_may_fault();
>      if ((kgdb_fault_setjmp(kgdb_fault_jmp_regs)) != 0) {
> -        kgdb_may_fault = 0;
> +        kgdb_unset_may_fault();
>          return ERR_PTR(-EINVAL);
>      }
>      for (; count > 0; count--, buf++) {
> @@ -405,7 +423,7 @@ static char *kgdb_ebin2mem(char *buf, ch
>          else
>              *mem++ = *buf;
>      }
> -    kgdb_may_fault = 0;
> +    kgdb_unset_may_fault();
>      return mem;
>  }
>
> @@ -416,9 +434,9 @@ static char *kgdb_ebin2mem(char *buf, ch
>   */
>  char *kgdb_hex2mem(char *buf, char *mem, int count)
>  {
> -    kgdb_may_fault = 1;
> +    kgdb_set_may_fault();
>      if ((kgdb_fault_setjmp(kgdb_fault_jmp_regs)) != 0) {
> -        kgdb_may_fault = 0;
> +        kgdb_unset_may_fault();
>          return ERR_PTR(-EINVAL);
>      }
>      if ((count == 2) && (((long)mem & 1) == 0)) {
> @@ -467,7 +485,7 @@ char *kgdb_hex2mem(char *buf, char *mem,
>              *mem++ = ch;
>          }
>      }
> -    kgdb_may_fault = 0;
> +    kgdb_unset_may_fault();
>      return (mem);
>  }
>
> @@ -622,35 +640,39 @@ static void kgdb_wait(struct pt_regs *re
>
>  int kgdb_get_mem(char *addr, unsigned char *buf, int count)
>  {
> -    kgdb_may_fault = 1;
> +    kgdb_set_may_fault();
>      if ((kgdb_fault_setjmp(kgdb_fault_jmp_regs)) != 0) {
> -        kgdb_may_fault = 0;
> +        kgdb_unset_may_fault();
>          return -EINVAL;
>      }
>      while (count) {
> -        if ((unsigned long)addr < TASK_SIZE)
> +        if ((unsigned long)addr < TASK_SIZE) {
> +            kgdb_unset_may_fault();
>              return -EINVAL;
> +        }
>          *buf++ = *addr++;
>          count--;
>      }
> -    kgdb_may_fault = 0;
> +    kgdb_unset_may_fault();
>      return 0;
>  }
>
>  int kgdb_set_mem(char *addr, unsigned char *buf, int count)
>  {
> -    kgdb_may_fault = 1;
> +    kgdb_set_may_fault();
>      if ((kgdb_fault_setjmp(kgdb_fault_jmp_regs)) != 0) {
> -        kgdb_may_fault = 0;
> +        kgdb_unset_may_fault();
>          return -EINVAL;
>      }
>      while (count) {
> -        if ((unsigned long)addr < TASK_SIZE)
> +        if ((unsigned long)addr < TASK_SIZE) {
> +            kgdb_unset_may_fault();
>              return -EINVAL;
> +        }
>          *addr++ = *buf++;
>          count--;
>      }
> -    kgdb_may_fault = 0;
> +    kgdb_unset_may_fault();
>      return 0;
>  }
>  int kgdb_activate_sw_breakpoints(void)
>
> Jason Wessel wrote:
> > I discovered that the preempt_count problem exists in different forms
> > due to the way faults are handled are different archs when the
> > kgdb_may_fault and long jump occur to restore the system context.
> > This means that a general solution was needed vs a per arch solution.
> >
> > Attached is the patch to fix the kgdb_may_fault memory access issues
> > when using a kernel that cares about the preempt count.  This will
> > avoid random crashes of threads to the abort to trigger from a
> > bad/unexpected preempt count change.  This patch also fixes a missed
> > case where a return can happen in an error condition where
> > kgdb_may_fault is still set.  Of course the system will most
> > definitely crash when kgdb_may_fault is set and a resume occurs.
> >
> > If there are no objections I will merge this patch soon and make a
> > final call for contributions to the 2.6.21 uprev.  The next step is on
> > to 2.6.22 and updating the kernel.org tree.
> >
> > Thanks,
> > Jason.
> > ------------------------------------------------------------------------
> >
> > -------------------------------------------------------------------------
> > This SF.net email is sponsored by DB2 Express
> > Download DB2 Express C - the FREE version of DB2 express and take
> > control of your XML. No limits. Just data. Click to get it now.
> > http://sourceforge.net/powerbar/db2/
> > ------------------------------------------------------------------------
> >
> > _______________________________________________
> > Kgdb-bugreport mailing list
> > Kgd...@li...
> > https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by DB2 Express
> Download DB2 Express C - the FREE version of DB2 express and take
> control of your XML. No limits. Just data. Click to get it now.
> http://sourceforge.net/powerbar/db2/
> _______________________________________________
> Kgdb-bugreport mailing list
> Kgd...@li...
> https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport