diff mbox

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

Message ID 1299011204.28753.8.camel@w500 (mailing list archive)
State Changes Requested
Headers show

Commit Message

Michael Wolf March 1, 2011, 8:26 p.m. UTC 
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>

----------

Comments

Benjamin Herrenschmidt March 2, 2011, 3:08 a.m. UTC | #1 
> --- 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
Michael Wolf March 2, 2011, 3:44 p.m. UTC | #2 
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
> 
>
diff mbox

Patch

--- 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++];