[PATCH] powerpc/ptrace: Remove BUG_ON when full register set not available

[PATCH] powerpc/ptrace: Remove BUG_ON when full register set not available

[Benjamin Herrenschmidt]

From: Mike Wolf <mjw@linux.vnet.ibm.com>

In some cases during a threaded core dump not all the threads will have
a full register set. This happens when the signal causing the core dump
races with a thread exiting.  The race happens when the exiting thread
has entered the kernel for the last time before the signal arrives, but
doesn't get far enough through the exit code to avoid being included
in the core dump.

So we get a thread included in the core dump which is never going to go
out to userspace again and only has a partial register set recorded

Normally we would catch each thread as it is about to go into userspace
and capture the full register set then.

However, this exiting thread is never going to go out to userspace
again, so we have no way to capture its full register set.  It doesn't
really matter, though, as this is a thread which is effectively
already dead.

So instead of hitting a BUG() in this case (a really bad choice of
action in the first place), we use a poison value for the register
values.

[BenH]: Some cosmetic/stylistic changes and fix build on ppc32

Signed-off-by: Mike Wolf <mjw@linux.vnet.ibm.com>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/include/asm/ptrace.h |    2 ++
 arch/powerpc/kernel/ptrace.c      |   15 ++++++++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 0175a67..48223f9 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -125,8 +125,10 @@ extern int ptrace_put_reg(struct task_struct *task, int regno,
 #endif /* ! __powerpc64__ */
 #define TRAP(regs)		((regs)->trap & ~0xF)
 #ifdef __powerpc64__
+#define NV_REG_POISON		0xdeadbeefdeadbeefUL
 #define CHECK_FULL_REGS(regs)	BUG_ON(regs->trap & 1)
 #else
+#define NV_REG_POISON		0xdeadbeef
 #define CHECK_FULL_REGS(regs)						      \
 do {									      \
 	if ((regs)->trap & 1)						      \
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 9065369..895b082 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -229,12 +229,16 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
 		   unsigned int pos, unsigned int count,
 		   void *kbuf, void __user *ubuf)
 {
-	int ret;
+	int i, ret;
 
 	if (target->thread.regs == NULL)
 		return -EIO;
 
-	CHECK_FULL_REGS(target->thread.regs);
+	if (!FULL_REGS(target->thread.regs)) {
+		/* We have a partial register set.  Fill 14-31 with bogus values */
+		for (i = 14; i < 32; i++)
+			target->thread.regs->gpr[i] = NV_REG_POISON;
+	}
 
 	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
 				  target->thread.regs,
@@ -641,11 +645,16 @@ static int gpr32_get(struct task_struct *target,
 	compat_ulong_t *k = kbuf;
 	compat_ulong_t __user *u = ubuf;
 	compat_ulong_t reg;
+	int i;
 
 	if (target->thread.regs == NULL)
 		return -EIO;
 
-	CHECK_FULL_REGS(target->thread.regs);
+	if (!FULL_REGS(target->thread.regs)) {
+		/* We have a partial register set.  Fill 14-31 with bogus values */
+		for (i = 14; i < 32; i++)
+			target->thread.regs->gpr[i] = NV_REG_POISON; 
+	}
 
 	pos /= sizeof(reg);
 	count /= sizeof(reg);

[PATCH] powerpc/ptrace: remove BUG_ON when full register set not available

[Michael Wolf]

In some cases during a threaded core dump not all 
the threads will have a full register set.  This
will cause problems when the sigkill is sent to
the thread.  To solve this problem a poison value
(0xdeadbeef) will be placed in the buffer in place 
of the actual register values.  This will affect
gpr14 to gpr31.

Signed-off-by: Mike Wolf <mjw@linux.vnet.ibm.com>

----------

--- ptracev2.orig/include/linux/regset.h	2011-02-20 12:15:57.000000000 -0600
+++ ptracev2/include/linux/regset.h	2011-02-28 13:33:36.685302349 -0600
@@ -240,6 +240,34 @@ static inline int user_regset_copyout(un
 	}
 	return 0;
 }
+/* want the count to be the registers 14-31 inclusive */
+#define POISON_REG_CNT (PT_R31-PT_R14+1)
+static inline int user_regset_copyout_poison(unsigned int *pos,
+					   unsigned int *count,
+					   void **kbuf, void __user **ubuf,
+					   const int start_pos,
+					   const int end_pos)
+{
+	long poison_data[POISON_REG_CNT] = { [0 ... POISON_REG_CNT-1] = 0xdeadbeefdeadbeefUL };
+
+	if (*count == 0)
+		return 0;
+	BUG_ON(*pos < start_pos);
+	if (end_pos < 0 || *pos < end_pos) {
+		unsigned int copy = (end_pos < 0 ? *count
+				     : min(*count, end_pos - *pos));
+		if (*kbuf) {
+			memset(*kbuf, 0xdeadbeef, copy);
+			*kbuf += copy;
+		} else if (__copy_to_user(*ubuf,poison_data, copy))
+			return -EFAULT;
+		else
+			*ubuf += copy;
+		*pos += copy;
+		*count -= copy;
+	}
+	return 0;
+}
 
 static inline int user_regset_copyin(unsigned int *pos, unsigned int *count,
 				     const void **kbuf,
--- ptracev2.orig/arch/powerpc/kernel/ptrace.c	2011-02-20 12:15:57.000000000 -0600
+++ ptracev2/arch/powerpc/kernel/ptrace.c	2011-03-01 13:49:12.686354353 -0600
@@ -234,11 +234,29 @@ static int gpr_get(struct task_struct *t
 	if (target->thread.regs == NULL)
 		return -EIO;
 
-	CHECK_FULL_REGS(target->thread.regs);
 
-	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
-				  target->thread.regs,
-				  0, offsetof(struct pt_regs, msr));
+	if (!FULL_REGS(target->thread.regs)) {
+                /* Don't have the full register set. Copy out register r0-r13 */
+		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+					  target->thread.regs,
+					  0, sizeof(long)*PT_R14);
+
+		/* Dont want to change the actual register values so rather than read the */
+		/* actual register copy a poison value into the buffer instead		  */
+		if (!ret)
+			ret = user_regset_copyout_poison(&pos, &count, &kbuf, &ubuf,
+						  sizeof(long)*PT_R14, offsetof(struct pt_regs, nip));
+
+		/* Copy out the rest of the registers as usual */
+		if (!ret)
+			ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+						  target->thread.regs,
+						  offsetof(struct pt_regs, nip), offsetof(struct pt_regs, msr));
+		
+	} else 
+		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+					  target->thread.regs,
+					  0, offsetof(struct pt_regs, msr));
 	if (!ret) {
 		unsigned long msr = get_user_msr(target);
 		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &msr,
@@ -645,11 +663,29 @@ static int gpr32_get(struct task_struct 
 	if (target->thread.regs == NULL)
 		return -EIO;
 
-	CHECK_FULL_REGS(target->thread.regs);
-
 	pos /= sizeof(reg);
 	count /= sizeof(reg);
 
+	if(!FULL_REGS(target->thread.regs)) {
+		if (kbuf) {
+                	/* Don't have the full register set. Copy out register r0-r13 */
+			for (; count > 0 && pos < PT_R14; --count)
+				*k++ = regs[pos++];
+
+			/* Dont want to change the actual register values so rather than read the */
+			/* actual register copy a poison value into the buffer instead		  */
+			for (; count > 0 && pos <= PT_R31; --count,pos++)
+				*k++ = 0xdeadbeef;
+
+		} else { 
+			for (; count > 0 && pos < PT_R14; --count)
+				if (__put_user((compat_ulong_t) regs[pos++], u++))
+					return -EFAULT;
+			for (; count > 0 && pos <= PT_R31; --count,pos++)
+				if (__put_user((compat_ulong_t) 0xdeadbeef, u++))
+					return -EFAULT;
+		}
+	}
 	if (kbuf)
 		for (; count > 0 && pos < PT_MSR; --count)
 			*k++ = regs[pos++];

Re: [PATCH] powerpc/ptrace: remove BUG_ON when full register set not available

[Benjamin Herrenschmidt]

> --- ptracev2.orig/include/linux/regset.h	2011-02-20 12:15:57.000000000 -0600
> +++ ptracev2/include/linux/regset.h	2011-02-28 13:33:36.685302349 -0600
> @@ -240,6 +240,34 @@ static inline int user_regset_copyout(un
>  	}
>  	return 0;
>  }
> +/* want the count to be the registers 14-31 inclusive */
> +#define POISON_REG_CNT (PT_R31-PT_R14+1)
> +static inline int user_regset_copyout_poison(unsigned int *pos,
> +					   unsigned int *count,
> +					   void **kbuf, void __user **ubuf,
> +					   const int start_pos,
> +					   const int end_pos)
> +{

I wouldn't put that in a generic location... especially something
like POISON_REG_CNT is very specific to powerpc and our ABI.

.../...

>  static inline int user_regset_copyin(unsigned int *pos, unsigned int *count,
>  				     const void **kbuf,
> --- ptracev2.orig/arch/powerpc/kernel/ptrace.c	2011-02-20 12:15:57.000000000 -0600
> +++ ptracev2/arch/powerpc/kernel/ptrace.c	2011-03-01 13:49:12.686354353 -0600
> @@ -234,11 +234,29 @@ static int gpr_get(struct task_struct *t
>  	if (target->thread.regs == NULL)
>  		return -EIO;
>  
> -	CHECK_FULL_REGS(target->thread.regs);

Wouldn't it be a simpler/cleaner approach to use a single function call
here that checks if the reg set is full and if not, put poison values
in the thread.regs structure itself ?

The resulting code would be more palatable...

Cheers,
Ben.
 
> -	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> -				  target->thread.regs,
> -				  0, offsetof(struct pt_regs, msr));
> +	if (!FULL_REGS(target->thread.regs)) {
> +                /* Don't have the full register set. Copy out register r0-r13 */
> +		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +					  target->thread.regs,
> +					  0, sizeof(long)*PT_R14);
> +
> +		/* Dont want to change the actual register values so rather than read the */
> +		/* actual register copy a poison value into the buffer instead		  */
> +		if (!ret)
> +			ret = user_regset_copyout_poison(&pos, &count, &kbuf, &ubuf,
> +						  sizeof(long)*PT_R14, offsetof(struct pt_regs, nip));
> +
> +		/* Copy out the rest of the registers as usual */
> +		if (!ret)
> +			ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +						  target->thread.regs,
> +						  offsetof(struct pt_regs, nip), offsetof(struct pt_regs, msr));
> +		
> +	} else 
> +		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +					  target->thread.regs,
> +					  0, offsetof(struct pt_regs, msr));
>  	if (!ret) {
>  		unsigned long msr = get_user_msr(target);
>  		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &msr,
> @@ -645,11 +663,29 @@ static int gpr32_get(struct task_struct 
>  	if (target->thread.regs == NULL)
>  		return -EIO;
>  
> -	CHECK_FULL_REGS(target->thread.regs);
> -
>  	pos /= sizeof(reg);
>  	count /= sizeof(reg);
>  
> +	if(!FULL_REGS(target->thread.regs)) {
> +		if (kbuf) {
> +                	/* Don't have the full register set. Copy out register r0-r13 */
> +			for (; count > 0 && pos < PT_R14; --count)
> +				*k++ = regs[pos++];
> +
> +			/* Dont want to change the actual register values so rather than read the */
> +			/* actual register copy a poison value into the buffer instead		  */
> +			for (; count > 0 && pos <= PT_R31; --count,pos++)
> +				*k++ = 0xdeadbeef;
> +
> +		} else { 
> +			for (; count > 0 && pos < PT_R14; --count)
> +				if (__put_user((compat_ulong_t) regs[pos++], u++))
> +					return -EFAULT;
> +			for (; count > 0 && pos <= PT_R31; --count,pos++)
> +				if (__put_user((compat_ulong_t) 0xdeadbeef, u++))
> +					return -EFAULT;
> +		}
> +	}
>  	if (kbuf)
>  		for (; count > 0 && pos < PT_MSR; --count)
>  			*k++ = regs[pos++];
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] powerpc/ptrace: remove BUG_ON when full register set not available

[Michael Wolf]

On Wed, 2011-03-02 at 14:08 +1100, Benjamin Herrenschmidt wrote:
> > --- ptracev2.orig/include/linux/regset.h	2011-02-20 12:15:57.000000000 -0600
> > +++ ptracev2/include/linux/regset.h	2011-02-28 13:33:36.685302349 -0600
> > @@ -240,6 +240,34 @@ static inline int user_regset_copyout(un
> >  	}
> >  	return 0;
> >  }
> > +/* want the count to be the registers 14-31 inclusive */
> > +#define POISON_REG_CNT (PT_R31-PT_R14+1)
> > +static inline int user_regset_copyout_poison(unsigned int *pos,
> > +					   unsigned int *count,
> > +					   void **kbuf, void __user **ubuf,
> > +					   const int start_pos,
> > +					   const int end_pos)
> > +{
> 
> I wouldn't put that in a generic location... especially something
> like POISON_REG_CNT is very specific to powerpc and our ABI.

Yeah I put it there thinking about readability but only use it in the
one place.  If we go forward with this current strategy I will remove
the #define and just comment more.

> 
> .../...
> 
> >  static inline int user_regset_copyin(unsigned int *pos, unsigned int *count,
> >  				     const void **kbuf,
> > --- ptracev2.orig/arch/powerpc/kernel/ptrace.c	2011-02-20 12:15:57.000000000 -0600
> > +++ ptracev2/arch/powerpc/kernel/ptrace.c	2011-03-01 13:49:12.686354353 -0600
> > @@ -234,11 +234,29 @@ static int gpr_get(struct task_struct *t
> >  	if (target->thread.regs == NULL)
> >  		return -EIO;
> >  
> > -	CHECK_FULL_REGS(target->thread.regs);
> 
> Wouldn't it be a simpler/cleaner approach to use a single function call
> here that checks if the reg set is full and if not, put poison values
> in the thread.regs structure itself ?
> 
> The resulting code would be more palatable...

That was actually the way I had done the first patch while debugging the
problem but Paul Mackerras didn't like that I changed the thread struct.

It is not clear to me which is better.  Reporting a poison value that 
isn't actually there or to change the data.  Hopefully a consensus can
be reached and then I can submit a new patch based on that.

> 
> Cheers,
> Ben.
> 
> > -	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> > -				  target->thread.regs,
> > -				  0, offsetof(struct pt_regs, msr));
> > +	if (!FULL_REGS(target->thread.regs)) {
> > +                /* Don't have the full register set. Copy out register r0-r13 */
> > +		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> > +					  target->thread.regs,
> > +					  0, sizeof(long)*PT_R14);
> > +
> > +		/* Dont want to change the actual register values so rather than read the */
> > +		/* actual register copy a poison value into the buffer instead		  */
> > +		if (!ret)
> > +			ret = user_regset_copyout_poison(&pos, &count, &kbuf, &ubuf,
> > +						  sizeof(long)*PT_R14, offsetof(struct pt_regs, nip));
> > +
> > +		/* Copy out the rest of the registers as usual */
> > +		if (!ret)
> > +			ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> > +						  target->thread.regs,
> > +						  offsetof(struct pt_regs, nip), offsetof(struct pt_regs, msr));
> > +		
> > +	} else 
> > +		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> > +					  target->thread.regs,
> > +					  0, offsetof(struct pt_regs, msr));
> >  	if (!ret) {
> >  		unsigned long msr = get_user_msr(target);
> >  		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &msr,
> > @@ -645,11 +663,29 @@ static int gpr32_get(struct task_struct 
> >  	if (target->thread.regs == NULL)
> >  		return -EIO;
> >  
> > -	CHECK_FULL_REGS(target->thread.regs);
> > -
> >  	pos /= sizeof(reg);
> >  	count /= sizeof(reg);
> >  
> > +	if(!FULL_REGS(target->thread.regs)) {
> > +		if (kbuf) {
> > +                	/* Don't have the full register set. Copy out register r0-r13 */
> > +			for (; count > 0 && pos < PT_R14; --count)
> > +				*k++ = regs[pos++];
> > +
> > +			/* Dont want to change the actual register values so rather than read the */
> > +			/* actual register copy a poison value into the buffer instead		  */
> > +			for (; count > 0 && pos <= PT_R31; --count,pos++)
> > +				*k++ = 0xdeadbeef;
> > +
> > +		} else { 
> > +			for (; count > 0 && pos < PT_R14; --count)
> > +				if (__put_user((compat_ulong_t) regs[pos++], u++))
> > +					return -EFAULT;
> > +			for (; count > 0 && pos <= PT_R31; --count,pos++)
> > +				if (__put_user((compat_ulong_t) 0xdeadbeef, u++))
> > +					return -EFAULT;
> > +		}
> > +	}
> >  	if (kbuf)
> >  		for (; count > 0 && pos < PT_MSR; --count)
> >  			*k++ = regs[pos++];
> > 
> > 
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
> 
>