[PATCH] ptrace: unify FDPIC implementations

[PATCH] ptrace: unify FDPIC implementations

[Mike Frysinger]

The Blackfin/FRV/SuperH guys all have the same exact FDPIC ptrace code in
their arch handlers (since they were probably copied & pasted).  Since
these ptrace interfaces are an arch independent aspect of the FDPIC code,
unify them in the common ptrace code so new FDPIC ports don't need to copy
and paste this fundamental stuff yet again.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 arch/blackfin/kernel/ptrace.c |   33 ++++++++-------------------------
 arch/frv/kernel/ptrace.c      |   20 --------------------
 arch/sh/kernel/ptrace_32.c    |   23 -----------------------
 kernel/ptrace.c               |   20 ++++++++++++++++++++
 4 files changed, 28 insertions(+), 68 deletions(-)

diff --git a/arch/blackfin/kernel/ptrace.c b/arch/blackfin/kernel/ptrace.c
index 65567dc..0ae2f1e 100644
--- a/arch/blackfin/kernel/ptrace.c
+++ b/arch/blackfin/kernel/ptrace.c
@@ -261,11 +261,15 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 			} else if (addr = (sizeof(struct pt_regs) + 8)) {
 				/* PT_DATA_ADDR */
 				tmp = child->mm->start_data;
-#ifdef CONFIG_BINFMT_ELF_FDPIC
+#ifdef CONFIG_BINFMT_ELF_FDPIC	/* backwards compat */
 			} else if (addr = (sizeof(struct pt_regs) + 12)) {
-				goto case_PTRACE_GETFDPIC_EXEC;
+				request = PTRACE_GETFDPIC;
+				addr = PTRACE_GETFDPIC_EXEC;
+				goto case_default;
 			} else if (addr = (sizeof(struct pt_regs) + 16)) {
-				goto case_PTRACE_GETFDPIC_INTERP;
+				request = PTRACE_GETFDPIC;
+				addr = PTRACE_GETFDPIC_INTERP;
+				goto case_default;
 #endif
 			} else {
 				tmp = get_reg(child, addr);
@@ -274,28 +278,6 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 			break;
 		}
 
-#ifdef CONFIG_BINFMT_ELF_FDPIC
-	case PTRACE_GETFDPIC: {
-		unsigned long tmp = 0;
-
-		switch (addr) {
-		case_PTRACE_GETFDPIC_EXEC:
-		case PTRACE_GETFDPIC_EXEC:
-			tmp = child->mm->context.exec_fdpic_loadmap;
-			break;
-		case_PTRACE_GETFDPIC_INTERP:
-		case PTRACE_GETFDPIC_INTERP:
-			tmp = child->mm->context.interp_fdpic_loadmap;
-			break;
-		default:
-			break;
-		}
-
-		ret = put_user(tmp, datap);
-		break;
-	}
-#endif
-
 		/* when I and D space are separate, this will have to be fixed. */
 	case PTRACE_POKEDATA:
 		pr_debug("ptrace: PTRACE_PEEKDATA\n");
@@ -409,6 +391,7 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 		ret = 0;
 		break;
 
+	case_default:
 	default:
 		ret = ptrace_request(child, request, addr, data);
 		break;
diff --git a/arch/frv/kernel/ptrace.c b/arch/frv/kernel/ptrace.c
index 60eeed3..fac0289 100644
--- a/arch/frv/kernel/ptrace.c
+++ b/arch/frv/kernel/ptrace.c
@@ -344,26 +344,6 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 					     0, sizeof(child->thread.user->f),
 					     (const void __user *)data);
 
-	case PTRACE_GETFDPIC:
-		tmp = 0;
-		switch (addr) {
-		case PTRACE_GETFDPIC_EXEC:
-			tmp = child->mm->context.exec_fdpic_loadmap;
-			break;
-		case PTRACE_GETFDPIC_INTERP:
-			tmp = child->mm->context.interp_fdpic_loadmap;
-			break;
-		default:
-			break;
-		}
-
-		ret = 0;
-		if (put_user(tmp, (unsigned long *) data)) {
-			ret = -EFAULT;
-			break;
-		}
-		break;
-
 	default:
 		ret = ptrace_request(child, request, addr, data);
 		break;
diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c
index 9be35f3..ab8123b 100644
--- a/arch/sh/kernel/ptrace_32.c
+++ b/arch/sh/kernel/ptrace_32.c
@@ -405,29 +405,6 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 					     0, sizeof(struct pt_dspregs),
 					     (const void __user *)data);
 #endif
-#ifdef CONFIG_BINFMT_ELF_FDPIC
-	case PTRACE_GETFDPIC: {
-		unsigned long tmp = 0;
-
-		switch (addr) {
-		case PTRACE_GETFDPIC_EXEC:
-			tmp = child->mm->context.exec_fdpic_loadmap;
-			break;
-		case PTRACE_GETFDPIC_INTERP:
-			tmp = child->mm->context.interp_fdpic_loadmap;
-			break;
-		default:
-			break;
-		}
-
-		ret = 0;
-		if (put_user(tmp, datap)) {
-			ret = -EFAULT;
-			break;
-		}
-		break;
-	}
-#endif
 	default:
 		ret = ptrace_request(child, request, addr, data);
 		break;
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 23bd09c..60fe465 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -554,6 +554,26 @@ int ptrace_request(struct task_struct *child, long request,
 		ret = ptrace_detach(child, data);
 		break;
 
+#ifdef CONFIG_BINFMT_ELF_FDPIC
+	case PTRACE_GETFDPIC: {
+		unsigned long tmp = 0;
+
+		switch (addr) {
+		case PTRACE_GETFDPIC_EXEC:
+			tmp = child->mm->context.exec_fdpic_loadmap;
+			break;
+		case PTRACE_GETFDPIC_INTERP:
+			tmp = child->mm->context.interp_fdpic_loadmap;
+			break;
+		default:
+			break;
+		}
+
+		ret = put_user(tmp, (unsigned long __user *) data);
+		break;
+	}
+#endif
+
 #ifdef PTRACE_SINGLESTEP
 	case PTRACE_SINGLESTEP:
 #endif
-- 
1.6.6.1

Re: [PATCH] ptrace: unify FDPIC implementations

[Roland McGrath]

Acked-by: Roland McGrath <roland@redhat.com>

Re: [PATCH] ptrace: unify FDPIC implementations

[David Howells]

Mike Frysinger <vapier@gentoo.org> wrote:

> The Blackfin/FRV/SuperH guys all have the same exact FDPIC ptrace code in
> their arch handlers (since they were probably copied & pasted).  Since
> these ptrace interfaces are an arch independent aspect of the FDPIC code,
> unify them in the common ptrace code so new FDPIC ports don't need to copy
> and paste this fundamental stuff yet again.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH] ptrace: unify FDPIC implementations

[Paul Mundt]

On Thu, Feb 11, 2010 at 04:36:00AM -0500, Mike Frysinger wrote:
> The Blackfin/FRV/SuperH guys all have the same exact FDPIC ptrace code in
> their arch handlers (since they were probably copied & pasted).  Since
> these ptrace interfaces are an arch independent aspect of the FDPIC code,
> unify them in the common ptrace code so new FDPIC ports don't need to copy
> and paste this fundamental stuff yet again.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

Acked-by: Paul Mundt  <lethal@linux-sh.org>

[PATCH] ptrace: unify FDPIC implementations

[Mike Frysinger]

The Blackfin/FRV/SuperH guys all have the same exact FDPIC ptrace code in
their arch handlers (since they were probably copied & pasted).  Since
these ptrace interfaces are an arch independent aspect of the FDPIC code,
unify them in the common ptrace code so new FDPIC ports don't need to copy
and paste this fundamental stuff yet again.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
Acked-by: Roland McGrath <roland@redhat.com>
Acked-by: David Howells <dhowells@redhat.com>
Acked-by: Paul Mundt <lethal@linux-sh.org>
---
Andrew: could you pick this up ?  everyone concerned has acked it ...

 arch/blackfin/kernel/ptrace.c |   33 ++++++++-------------------------
 arch/frv/kernel/ptrace.c      |   20 --------------------
 arch/sh/kernel/ptrace_32.c    |   23 -----------------------
 kernel/ptrace.c               |   20 ++++++++++++++++++++
 4 files changed, 28 insertions(+), 68 deletions(-)

diff --git a/arch/blackfin/kernel/ptrace.c b/arch/blackfin/kernel/ptrace.c
index 65567dc..0ae2f1e 100644
--- a/arch/blackfin/kernel/ptrace.c
+++ b/arch/blackfin/kernel/ptrace.c
@@ -261,11 +261,15 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 			} else if (addr = (sizeof(struct pt_regs) + 8)) {
 				/* PT_DATA_ADDR */
 				tmp = child->mm->start_data;
-#ifdef CONFIG_BINFMT_ELF_FDPIC
+#ifdef CONFIG_BINFMT_ELF_FDPIC	/* backwards compat */
 			} else if (addr = (sizeof(struct pt_regs) + 12)) {
-				goto case_PTRACE_GETFDPIC_EXEC;
+				request = PTRACE_GETFDPIC;
+				addr = PTRACE_GETFDPIC_EXEC;
+				goto case_default;
 			} else if (addr = (sizeof(struct pt_regs) + 16)) {
-				goto case_PTRACE_GETFDPIC_INTERP;
+				request = PTRACE_GETFDPIC;
+				addr = PTRACE_GETFDPIC_INTERP;
+				goto case_default;
 #endif
 			} else {
 				tmp = get_reg(child, addr);
@@ -274,28 +278,6 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 			break;
 		}
 
-#ifdef CONFIG_BINFMT_ELF_FDPIC
-	case PTRACE_GETFDPIC: {
-		unsigned long tmp = 0;
-
-		switch (addr) {
-		case_PTRACE_GETFDPIC_EXEC:
-		case PTRACE_GETFDPIC_EXEC:
-			tmp = child->mm->context.exec_fdpic_loadmap;
-			break;
-		case_PTRACE_GETFDPIC_INTERP:
-		case PTRACE_GETFDPIC_INTERP:
-			tmp = child->mm->context.interp_fdpic_loadmap;
-			break;
-		default:
-			break;
-		}
-
-		ret = put_user(tmp, datap);
-		break;
-	}
-#endif
-
 		/* when I and D space are separate, this will have to be fixed. */
 	case PTRACE_POKEDATA:
 		pr_debug("ptrace: PTRACE_PEEKDATA\n");
@@ -409,6 +391,7 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 		ret = 0;
 		break;
 
+	case_default:
 	default:
 		ret = ptrace_request(child, request, addr, data);
 		break;
diff --git a/arch/frv/kernel/ptrace.c b/arch/frv/kernel/ptrace.c
index 60eeed3..fac0289 100644
--- a/arch/frv/kernel/ptrace.c
+++ b/arch/frv/kernel/ptrace.c
@@ -344,26 +344,6 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 					     0, sizeof(child->thread.user->f),
 					     (const void __user *)data);
 
-	case PTRACE_GETFDPIC:
-		tmp = 0;
-		switch (addr) {
-		case PTRACE_GETFDPIC_EXEC:
-			tmp = child->mm->context.exec_fdpic_loadmap;
-			break;
-		case PTRACE_GETFDPIC_INTERP:
-			tmp = child->mm->context.interp_fdpic_loadmap;
-			break;
-		default:
-			break;
-		}
-
-		ret = 0;
-		if (put_user(tmp, (unsigned long *) data)) {
-			ret = -EFAULT;
-			break;
-		}
-		break;
-
 	default:
 		ret = ptrace_request(child, request, addr, data);
 		break;
diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c
index 9be35f3..ab8123b 100644
--- a/arch/sh/kernel/ptrace_32.c
+++ b/arch/sh/kernel/ptrace_32.c
@@ -405,29 +405,6 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 					     0, sizeof(struct pt_dspregs),
 					     (const void __user *)data);
 #endif
-#ifdef CONFIG_BINFMT_ELF_FDPIC
-	case PTRACE_GETFDPIC: {
-		unsigned long tmp = 0;
-
-		switch (addr) {
-		case PTRACE_GETFDPIC_EXEC:
-			tmp = child->mm->context.exec_fdpic_loadmap;
-			break;
-		case PTRACE_GETFDPIC_INTERP:
-			tmp = child->mm->context.interp_fdpic_loadmap;
-			break;
-		default:
-			break;
-		}
-
-		ret = 0;
-		if (put_user(tmp, datap)) {
-			ret = -EFAULT;
-			break;
-		}
-		break;
-	}
-#endif
 	default:
 		ret = ptrace_request(child, request, addr, data);
 		break;
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 23bd09c..60fe465 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -554,6 +554,26 @@ int ptrace_request(struct task_struct *child, long request,
 		ret = ptrace_detach(child, data);
 		break;
 
+#ifdef CONFIG_BINFMT_ELF_FDPIC
+	case PTRACE_GETFDPIC: {
+		unsigned long tmp = 0;
+
+		switch (addr) {
+		case PTRACE_GETFDPIC_EXEC:
+			tmp = child->mm->context.exec_fdpic_loadmap;
+			break;
+		case PTRACE_GETFDPIC_INTERP:
+			tmp = child->mm->context.interp_fdpic_loadmap;
+			break;
+		default:
+			break;
+		}
+
+		ret = put_user(tmp, (unsigned long __user *) data);
+		break;
+	}
+#endif
+
 #ifdef PTRACE_SINGLESTEP
 	case PTRACE_SINGLESTEP:
 #endif
-- 
1.7.0

Re: [PATCH] ptrace: unify FDPIC implementations

[Paul Mundt]

On Mon, Feb 15, 2010 at 07:30:29PM -0500, Mike Frysinger wrote:
> The Blackfin/FRV/SuperH guys all have the same exact FDPIC ptrace code in
> their arch handlers (since they were probably copied & pasted).  Since
> these ptrace interfaces are an arch independent aspect of the FDPIC code,
> unify them in the common ptrace code so new FDPIC ports don't need to copy
> and paste this fundamental stuff yet again.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> Acked-by: Roland McGrath <roland@redhat.com>
> Acked-by: David Howells <dhowells@redhat.com>
> Acked-by: Paul Mundt <lethal@linux-sh.org>

Just to follow up on this, this code obviously expects consistent entries
in mm_context_t. Presently include/asm-generic/mmu.h is aimed at nommu,
so it would probably be worthwhile stubbing in the FDPIC loadmap entries
there to make things easier for future ports.

Re: [PATCH] ptrace: unify FDPIC implementations

[Mike Frysinger]

On Mon, Feb 15, 2010 at 20:10, Paul Mundt wrote:
> On Mon, Feb 15, 2010 at 07:30:29PM -0500, Mike Frysinger wrote:
>> The Blackfin/FRV/SuperH guys all have the same exact FDPIC ptrace code in
>> their arch handlers (since they were probably copied & pasted).  Since
>> these ptrace interfaces are an arch independent aspect of the FDPIC code,
>> unify them in the common ptrace code so new FDPIC ports don't need to copy
>> and paste this fundamental stuff yet again.
>>
>> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>> Acked-by: Roland McGrath <roland@redhat.com>
>> Acked-by: David Howells <dhowells@redhat.com>
>> Acked-by: Paul Mundt <lethal@linux-sh.org>
>
> Just to follow up on this, this code obviously expects consistent entries
> in mm_context_t. Presently include/asm-generic/mmu.h is aimed at nommu,
> so it would probably be worthwhile stubbing in the FDPIC loadmap entries
> there to make things easier for future ports.

probably be a good idea.  for FDPIC in general, the header also needs
"stack_start".
-mike

[PATCH v2] ptrace: unify FDPIC implementations

[Mike Frysinger]

The Blackfin/FRV/SuperH guys all have the same exact FDPIC ptrace code in
their arch handlers (since they were probably copied & pasted).  Since
these ptrace interfaces are an arch independent aspect of the FDPIC code,
unify them in the common ptrace code so new FDPIC ports don't need to copy
and paste this fundamental stuff yet again.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
Acked-by: Roland McGrath <roland@redhat.com>
Acked-by: David Howells <dhowells@redhat.com>
Acked-by: Paul Mundt <lethal@linux-sh.org>
---
v2
	- rebased onto latest mainline

 arch/blackfin/kernel/ptrace.c |   33 +++++++++------------------------
 arch/frv/kernel/ptrace.c      |   20 --------------------
 arch/sh/kernel/ptrace_32.c    |   23 -----------------------
 kernel/ptrace.c               |   20 ++++++++++++++++++++
 4 files changed, 29 insertions(+), 67 deletions(-)

diff --git a/arch/blackfin/kernel/ptrace.c b/arch/blackfin/kernel/ptrace.c
index 43eb969..6ec7768 100644
--- a/arch/blackfin/kernel/ptrace.c
+++ b/arch/blackfin/kernel/ptrace.c
@@ -292,28 +292,6 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 			break;
 		}
 
-#ifdef CONFIG_BINFMT_ELF_FDPIC
-	case PTRACE_GETFDPIC: {
-		unsigned long tmp = 0;
-
-		switch (addr) {
-		case_PTRACE_GETFDPIC_EXEC:
-		case PTRACE_GETFDPIC_EXEC:
-			tmp = child->mm->context.exec_fdpic_loadmap;
-			break;
-		case_PTRACE_GETFDPIC_INTERP:
-		case PTRACE_GETFDPIC_INTERP:
-			tmp = child->mm->context.interp_fdpic_loadmap;
-			break;
-		default:
-			break;
-		}
-
-		ret = put_user(tmp, datap);
-		break;
-	}
-#endif
-
 		/* when I and D space are separate, this will have to be fixed. */
 	case PTRACE_POKEDATA:
 		pr_debug("ptrace: PTRACE_PEEKDATA\n");
@@ -357,8 +335,14 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 	case PTRACE_PEEKUSR:
 		switch (addr) {
 #ifdef CONFIG_BINFMT_ELF_FDPIC	/* backwards compat */
-		case PT_FDPIC_EXEC:   goto case_PTRACE_GETFDPIC_EXEC;
-		case PT_FDPIC_INTERP: goto case_PTRACE_GETFDPIC_INTERP;
+		case PT_FDPIC_EXEC:
+			request = PTRACE_GETFDPIC;
+			addr = PTRACE_GETFDPIC_EXEC;
+			goto case_default;
+		case PT_FDPIC_INTERP:
+			request = PTRACE_GETFDPIC;
+			addr = PTRACE_GETFDPIC_INTERP;
+			goto case_default;
 #endif
 		default:
 			ret = get_reg(child, addr, datap);
@@ -385,6 +369,7 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 					     0, sizeof(struct pt_regs),
 					     (const void __user *)data);
 
+	case_default:
 	default:
 		ret = ptrace_request(child, request, addr, data);
 		break;
diff --git a/arch/frv/kernel/ptrace.c b/arch/frv/kernel/ptrace.c
index 60eeed3..fac0289 100644
--- a/arch/frv/kernel/ptrace.c
+++ b/arch/frv/kernel/ptrace.c
@@ -344,26 +344,6 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 					     0, sizeof(child->thread.user->f),
 					     (const void __user *)data);
 
-	case PTRACE_GETFDPIC:
-		tmp = 0;
-		switch (addr) {
-		case PTRACE_GETFDPIC_EXEC:
-			tmp = child->mm->context.exec_fdpic_loadmap;
-			break;
-		case PTRACE_GETFDPIC_INTERP:
-			tmp = child->mm->context.interp_fdpic_loadmap;
-			break;
-		default:
-			break;
-		}
-
-		ret = 0;
-		if (put_user(tmp, (unsigned long *) data)) {
-			ret = -EFAULT;
-			break;
-		}
-		break;
-
 	default:
 		ret = ptrace_request(child, request, addr, data);
 		break;
diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c
index 7759a9a..75b74b2 100644
--- a/arch/sh/kernel/ptrace_32.c
+++ b/arch/sh/kernel/ptrace_32.c
@@ -436,29 +436,6 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 					     0, sizeof(struct pt_dspregs),
 					     (const void __user *)data);
 #endif
-#ifdef CONFIG_BINFMT_ELF_FDPIC
-	case PTRACE_GETFDPIC: {
-		unsigned long tmp = 0;
-
-		switch (addr) {
-		case PTRACE_GETFDPIC_EXEC:
-			tmp = child->mm->context.exec_fdpic_loadmap;
-			break;
-		case PTRACE_GETFDPIC_INTERP:
-			tmp = child->mm->context.interp_fdpic_loadmap;
-			break;
-		default:
-			break;
-		}
-
-		ret = 0;
-		if (put_user(tmp, datap)) {
-			ret = -EFAULT;
-			break;
-		}
-		break;
-	}
-#endif
 	default:
 		ret = ptrace_request(child, request, addr, data);
 		break;
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 42ad8ae..3ecb0a1 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -596,6 +596,26 @@ int ptrace_request(struct task_struct *child, long request,
 		ret = ptrace_detach(child, data);
 		break;
 
+#ifdef CONFIG_BINFMT_ELF_FDPIC
+	case PTRACE_GETFDPIC: {
+		unsigned long tmp = 0;
+
+		switch (addr) {
+		case PTRACE_GETFDPIC_EXEC:
+			tmp = child->mm->context.exec_fdpic_loadmap;
+			break;
+		case PTRACE_GETFDPIC_INTERP:
+			tmp = child->mm->context.interp_fdpic_loadmap;
+			break;
+		default:
+			break;
+		}
+
+		ret = put_user(tmp, (unsigned long __user *) data);
+		break;
+	}
+#endif
+
 #ifdef PTRACE_SINGLESTEP
 	case PTRACE_SINGLESTEP:
 #endif
-- 
1.7.1

Re: [PATCH v2] ptrace: unify FDPIC implementations

[Oleg Nesterov]

On 05/21, Mike Frysinger wrote:
>
> The Blackfin/FRV/SuperH guys all have the same exact FDPIC ptrace code in
> their arch handlers (since they were probably copied & pasted).  Since
> these ptrace interfaces are an arch independent aspect of the FDPIC code,
> unify them in the common ptrace code so new FDPIC ports don't need to copy
> and paste this fundamental stuff yet again.
>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> Acked-by: Roland McGrath <roland@redhat.com>
> Acked-by: David Howells <dhowells@redhat.com>
> Acked-by: Paul Mundt <lethal@linux-sh.org>

I think the patch is nice too.

But,

> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -596,6 +596,26 @@ int ptrace_request(struct task_struct *child, long request,
>  		ret = ptrace_detach(child, data);
>  		break;
>
> +#ifdef CONFIG_BINFMT_ELF_FDPIC
> +	case PTRACE_GETFDPIC: {
> +		unsigned long tmp = 0;
> +
> +		switch (addr) {
> +		case PTRACE_GETFDPIC_EXEC:
> +			tmp = child->mm->context.exec_fdpic_loadmap;
> +			break;
> +		case PTRACE_GETFDPIC_INTERP:
> +			tmp = child->mm->context.interp_fdpic_loadmap;

This looks unsafe. What protect child->mm if ptrace() races with SIGKILL ?

Of course, I do not blame this patch, the code was copied from arch/, but
I think we need another patch which checks ->mm != NULL under task_lock()
on top of this one?

Oleg.

Re: [PATCH v2] ptrace: unify FDPIC implementations

[Roland McGrath]

> This looks unsafe. What protect child->mm if ptrace() races with SIGKILL ?
> 
> Of course, I do not blame this patch, the code was copied from arch/, but
> I think we need another patch which checks ->mm != NULL under task_lock()
> on top of this one?

Agreed, unless things are somehow different on nommu so there aren't such
races.  But, as you mention, this is a long-standing issue that is entirely 
unrelated to the cleanup here and should not delay merging this patch.


Thanks,
Roland

[PATCH -mm 0/1] (Was: ptrace: unify FDPIC implementations)

[Oleg Nesterov]

On 05/21, Roland McGrath wrote:
>
> > This looks unsafe. What protect child->mm if ptrace() races with SIGKILL ?
> >
> > Of course, I do not blame this patch, the code was copied from arch/, but
> > I think we need another patch which checks ->mm != NULL under task_lock()
> > on top of this one?
>
> Agreed, unless things are somehow different on nommu so there aren't such
> races.

I don't this !CONFG_MMU can make any difference, nothing can protect us
from exit_mm() afaics.

> But, as you mention, this is a long-standing issue that is entirely
> unrelated to the cleanup here and should not delay merging this patch.

Yes, yes, agreed.

Please see the patch on top of ptrace-unify-fdpic-implementations.patch
Untested, but hopefully trivial enough.

Oleg.

[PATCH -mm 1/1] ptrace: PTRACE_GETFDPIC: fix the unsafe usage of

[Oleg Nesterov]

Now that Mike Frysinger unified the FDPIC ptrace code, we can fix
the unsafe usage of child->mm in ptrace_request(PTRACE_GETFDPIC).

We have the reference to task_struct, and ptrace_check_attach()
verified the tracee is stopped. But nothing can protect from
SIGKILL after that, we must not assume child->mm != NULL.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 kernel/ptrace.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

--- 34-rc1/kernel/ptrace.c~PTRACE_FDPIC	2010-05-22 18:04:47.000000000 +0200
+++ 34-rc1/kernel/ptrace.c	2010-05-22 18:35:35.000000000 +0200
@@ -598,18 +598,24 @@ int ptrace_request(struct task_struct *c
 
 #ifdef CONFIG_BINFMT_ELF_FDPIC
 	case PTRACE_GETFDPIC: {
+		struct mm_struct *mm = get_task_mm(child);
 		unsigned long tmp = 0;
 
+		ret = -ESRCH;
+		if (!mm)
+			break;
+
 		switch (addr) {
 		case PTRACE_GETFDPIC_EXEC:
-			tmp = child->mm->context.exec_fdpic_loadmap;
+			tmp = mm->context.exec_fdpic_loadmap;
 			break;
 		case PTRACE_GETFDPIC_INTERP:
-			tmp = child->mm->context.interp_fdpic_loadmap;
+			tmp = mm->context.interp_fdpic_loadmap;
 			break;
 		default:
 			break;
 		}
+		mmput(mm);
 
 		ret = put_user(tmp, (unsigned long __user *) data);
 		break;

Re: [Uclinux-dist-devel] [PATCH -mm 1/1] ptrace: PTRACE_GETFDPIC: fix

[Mike Frysinger]

On Sat, May 22, 2010 at 12:54, Oleg Nesterov wrote:
> Now that Mike Frysinger unified the FDPIC ptrace code, we can fix
> the unsafe usage of child->mm in ptrace_request(PTRACE_GETFDPIC).
>
> We have the reference to task_struct, and ptrace_check_attach()
> verified the tracee is stopped. But nothing can protect from
> SIGKILL after that, we must not assume child->mm != NULL.

gdb still works with FDPIC apps after this, so presumably it works
fine :).  ive added it to the Blackfin tree to get more testing on our
side (not that i'll be pushing it out or anything).

thanks!
-mike

Re: [PATCH -mm 1/1] ptrace: PTRACE_GETFDPIC: fix the unsafe usage of

[Roland McGrath]

Acked-by: Roland McGrath <roland@redhat.com>

Re: [PATCH -mm 1/1] ptrace: PTRACE_GETFDPIC: fix the unsafe usage of child->mm

[David Howells]

Oleg Nesterov <oleg@redhat.com> wrote:

> Now that Mike Frysinger unified the FDPIC ptrace code, we can fix
> the unsafe usage of child->mm in ptrace_request(PTRACE_GETFDPIC).
> 
> We have the reference to task_struct, and ptrace_check_attach()
> verified the tracee is stopped. But nothing can protect from
> SIGKILL after that, we must not assume child->mm != NULL.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: David Howells <dhowells@redhat.com>

Does it make sense to move the call to get_task_mm() up to sys_ptrace() since
several ptrace functions use it?  The mm pointer could then be handed down the
ptrace hierarchy.

David

Re: [PATCH -mm 1/1] ptrace: PTRACE_GETFDPIC: fix the unsafe usage

[Oleg Nesterov]

On 05/24, David Howells wrote:
>
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > Now that Mike Frysinger unified the FDPIC ptrace code, we can fix
> > the unsafe usage of child->mm in ptrace_request(PTRACE_GETFDPIC).
> >
> > We have the reference to task_struct, and ptrace_check_attach()
> > verified the tracee is stopped. But nothing can protect from
> > SIGKILL after that, we must not assume child->mm != NULL.
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>
> Acked-by: David Howells <dhowells@redhat.com>
>
> Does it make sense to move the call to get_task_mm() up to sys_ptrace() since
> several ptrace functions use it?  The mm pointer could then be handed down the
> ptrace hierarchy.

You mean, pass it to arch_ptrace() ?

grep, grep, grep. I guess I understand you. We have more unsafe code
like this in arch/*/kernel/ptrace.c. Of course, it can be fixed without
doing get_task_mm() in sys_ptrace(), but perhaps it would be more clean
to do what you suggest.

Roland, what do you think?

Oleg.

Re: [PATCH -mm 1/1] ptrace: PTRACE_GETFDPIC: fix the unsafe usage

[Roland McGrath]

> You mean, pass it to arch_ptrace() ?
> 
> grep, grep, grep. I guess I understand you. We have more unsafe code
> like this in arch/*/kernel/ptrace.c. Of course, it can be fixed without
> doing get_task_mm() in sys_ptrace(), but perhaps it would be more clean
> to do what you suggest.
> 
> Roland, what do you think?

The mm pointer is only used by these uncommon ptrace operations that exist
only in certain unusual arch's (and they're all ill-advised old arch ptrace
ABI additions, at that).  It doesn't seem wise to pay the overhead for
get_task_mm()/mmput() on every ptrace call, 99.44% of which don't use it
(and 100% on 90% of machines).

If you were to make any change to the signature of arch_ptrace() it should
be one big change to use a struct ptrace_params or suchlike, also passed
down to ptrace_request().  Then any future needs to pass around more
information won't require changing the code in all the arch code that
doesn't look at the new parameter.


Thanks,
Roland

Re: [Uclinux-dist-devel] [PATCH -mm 1/1] ptrace: PTRACE_GETFDPIC: fix

[Mike Frysinger]

On Mon, May 24, 2010 at 19:42, Roland McGrath wrote:
>> You mean, pass it to arch_ptrace() ?
>>
>> grep, grep, grep. I guess I understand you. We have more unsafe code
>> like this in arch/*/kernel/ptrace.c. Of course, it can be fixed without
>> doing get_task_mm() in sys_ptrace(), but perhaps it would be more clean
>> to do what you suggest.
>>
>> Roland, what do you think?
>
> The mm pointer is only used by these uncommon ptrace operations that exist
> only in certain unusual arch's (and they're all ill-advised old arch ptrace
> ABI additions, at that).

are you suggesting the FDPIC additions are ill-advised as well ?
curious how you would have implemented this if that's the case ...
-mike

Re: [Uclinux-dist-devel] [PATCH -mm 1/1] ptrace: PTRACE_GETFDPIC: fix

[Roland McGrath]

> are you suggesting the FDPIC additions are ill-advised as well ?
> curious how you would have implemented this if that's the case ...

They are certainly not as dismal as the arch PTRACE_PEEKUSR delivering
mm->fields.  But, frankly I don't think ptrace is the right mechanism for
delivering any read-only, process-shared information.  You can use some
/proc/pid/foo for that.  (The "dismal" cases deliver things like
start_code, which is already available in /proc/pid/stat.)


Thanks,
Roland

Re: [PATCH -mm 1/1] ptrace: PTRACE_GETFDPIC: fix the unsafe usage of child->mm

[David Howells]

Roland McGrath <roland@redhat.com> wrote:

> The mm pointer is only used by these uncommon ptrace operations

Like PEEKTEXT and POKETEXT?

David

Re: [PATCH -mm 1/1] ptrace: PTRACE_GETFDPIC: fix the unsafe usage

[Oleg Nesterov]

On 05/25, David Howells wrote:
>
> Roland McGrath <roland@redhat.com> wrote:
>
> > The mm pointer is only used by these uncommon ptrace operations
>
> Like PEEKTEXT and POKETEXT?

They use access_process_vm().

According to grep, mm is only use to read a couple of members.

Perhaps can even add the simple helper

	struct mm_xxx {
		unsigned long start_code, end_code, start_data, end_data;
		... some more ...
	};

	int get_mm_xxx(struct task_struct *tracee, struct mm_xxx *mm_xxx)
	{
		struct mm_struct *mm = get_task_mm(tracee);
		...
	}

Except:

	- arch/um/kernel/ptrace.c PTRACE_SWITCH_MM does something
	  really strange

	- arch/blackfin/kernel/ptrace.c:is_user_addr_valid()
	  needs mmap_sem around find_vma()

	  The lockless access to mm->context.sram_list doesn't look
	  safe to me.

	  If we add get_task_mm() - this protects us against
	  destroy_context() only. What is the tracee's sub-thread
	  does sys_sram_alloc() or sys_sram_free() in parallel?

Oleg.

Re: [PATCH -mm 1/1] ptrace: PTRACE_GETFDPIC: fix the unsafe usage of

[Mike Frysinger]

On Tue, May 25, 2010 at 06:23, Oleg Nesterov wrote:
>        - arch/blackfin/kernel/ptrace.c:is_user_addr_valid()
>          needs mmap_sem around find_vma()
>
>          The lockless access to mm->context.sram_list doesn't look
>          safe to me.
>
>          If we add get_task_mm() - this protects us against
>          destroy_context() only. What is the tracee's sub-thread
>          does sys_sram_alloc() or sys_sram_free() in parallel?

i dont believe there are any code paths in UP systems where this would
be a practical problem because sram_list is only updated by syscalls
from userspace.  we probably should add proper locking to this
structure though.  i'll open a tracker item about it, thanks !
-mike

Re: [PATCH -mm 1/1] ptrace: PTRACE_GETFDPIC: fix the unsafe usage

[Oleg Nesterov]

On 05/25, Mike Frysinger wrote:
>
> On Tue, May 25, 2010 at 06:23, Oleg Nesterov wrote:
> >        - arch/blackfin/kernel/ptrace.c:is_user_addr_valid()
> >          needs mmap_sem around find_vma()
> >
> >          The lockless access to mm->context.sram_list doesn't look
> >          safe to me.
> >
> >          If we add get_task_mm() - this protects us against
> >          destroy_context() only. What is the tracee's sub-thread
> >          does sys_sram_alloc() or sys_sram_free() in parallel?
>
> i dont believe there are any code paths in UP systems where this would
> be a practical problem because sram_list is only updated by syscalls
> from userspace.

Yes sure, UP && !PREEMPT is safe.

> we probably should add proper locking to this
> structure though.

Agreed. I'll try to make the trivial patch tomorrow. I think we
can just use mm->mmap_sem, is_user_addr_valid() needs this lock
for find_vma() anyway.

Oleg.

[PATCH 0/2] blackfin: ptrace mm/sram_list fixes

[Oleg Nesterov]

On 05/26, Oleg Nesterov wrote:
>
> On 05/25, Mike Frysinger wrote:
> >
> > we probably should add proper locking to this
> > structure though.
>
> Agreed. I'll try to make the trivial patch tomorrow. I think we
> can just use mm->mmap_sem, is_user_addr_valid() needs this lock
> for find_vma() anyway.

please see the patches.

UNTESTED! the second one certainly needs the review from someone
who knows what this code does ;)

BTW. Obviously sys_sram_alloc() can create multiple sram_list_struct
nodes with the same ->addr (with or without this patch), I hope this
is fine.

Oleg.

[PATCH 1/2] blackfin: ptrace: fix the unsafe usage of mm/find_vma

[Oleg Nesterov]

1. is_user_addr_valid() must not assume child->mm != NULL, we can
   race with SIGKILL. Use get_task_mm().

2. find_vma() needs mm->mmap_sem, we can race with another thread
   which shares ->mm with the tracee.

3. Move the simple FIXED_CODE_ checks up, before other potentially
   costly checks, and thus outside of mmap_sem.

4. Uninline it, it is fat and has 2 callers.

Note: we scan mm->context.sram_list under mmap_sem too. Currently this
is unnecessary, but see the next patch.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 arch/blackfin/kernel/ptrace.c |   27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

--- 34-rc1/arch/blackfin/kernel/ptrace.c~IUAV_1_GET_MM_TAKE_SEM	2010-05-27 17:52:36.000000000 +0200
+++ 34-rc1/arch/blackfin/kernel/ptrace.c	2010-05-27 20:07:10.000000000 +0200
@@ -113,29 +113,40 @@ put_reg(struct task_struct *task, long r
 /*
  * check that an address falls within the bounds of the target process's memory mappings
  */
-static inline int is_user_addr_valid(struct task_struct *child,
+static int is_user_addr_valid(struct task_struct *child,
 				     unsigned long start, unsigned long len)
 {
+	struct mm_struct *mm;
 	struct vm_area_struct *vma;
 	struct sram_list_struct *sraml;
+	int ret = 0;
 
 	/* overflow */
 	if (start + len < start)
 		return -EIO;
+	if (start >= FIXED_CODE_START && start + len < FIXED_CODE_END)
+		return 0;
 
-	vma = find_vma(child->mm, start);
+	mm = get_task_mm(child);
+	if (!mm)
+		return -EIO;
+
+	down_read(&mm->mmap_sem);
+	vma = find_vma(mm, start);
 	if (vma && start >= vma->vm_start && start + len <= vma->vm_end)
-			return 0;
+			goto out;
 
-	for (sraml = child->mm->context.sram_list; sraml; sraml = sraml->next)
+	for (sraml = mm->context.sram_list; sraml; sraml = sraml->next)
 		if (start >= (unsigned long)sraml->addr
 		    && start + len < (unsigned long)sraml->addr + sraml->length)
-			return 0;
+			goto out;
 
-	if (start >= FIXED_CODE_START && start + len < FIXED_CODE_END)
-		return 0;
+	ret = -EIO;
+out:
+	up_read(&mm->mmap_sem);
+	mmput(mm);
 
-	return -EIO;
+	return ret;
 }
 
 /*

[PATCH 2/2] blackfin: use ->mmap_sem to protect

[Oleg Nesterov]

Any usage of mm->context.sram_list is not safe, is_user_addr_valid()
and sram_free_with_lsl/sram_free_with_lsl can race with each other.

Change sram_free_with_lsl/sram_free_with_lsl to take mm->mmap_sem
for writing, is_user_addr_valid() was already modified to take it
for reading and it doesn't modify this list.

destroy_context() reaps this list lockless, this is OK.

This patch assumes that sram_free() doesn't need to be serialized
(afaics it does the locking correctly), and it is safe to call
sram_free(addr) after its sram_list_struct was removed from list.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 arch/blackfin/mm/sram-alloc.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

--- 34-rc1/arch/blackfin/mm/sram-alloc.c~IUAV_2_SRAM_LIST_LOCKING	2010-05-27 17:51:48.000000000 +0200
+++ 34-rc1/arch/blackfin/mm/sram-alloc.c	2010-05-27 21:31:01.000000000 +0200
@@ -702,19 +702,23 @@ EXPORT_SYMBOL(l2_sram_free);
 
 int sram_free_with_lsl(const void *addr)
 {
-	struct sram_list_struct *lsl, **tmp;
+	struct sram_list_struct *lsl = NULL, **tmp;
 	struct mm_struct *mm = current->mm;
 
+	down_write(&mm->mmap_sem);
 	for (tmp = &mm->context.sram_list; *tmp; tmp = &(*tmp)->next)
-		if ((*tmp)->addr = addr)
-			goto found;
-	return -1;
-found:
-	lsl = *tmp;
+		if ((*tmp)->addr = addr) {
+			lsl = *tmp;
+			*tmp = lsl->next;
+			break;
+		}
+	up_write(&mm->mmap_sem);
+
+	if (!lsl)
+		return -1;
+
 	sram_free(addr);
-	*tmp = lsl->next;
 	kfree(lsl);
-
 	return 0;
 }
 EXPORT_SYMBOL(sram_free_with_lsl);
@@ -749,10 +753,14 @@ void *sram_alloc_with_lsl(size_t size, u
 		kfree(lsl);
 		return NULL;
 	}
+
+	down_write(&mm->mmap_sem);
 	lsl->addr = addr;
 	lsl->length = size;
 	lsl->next = mm->context.sram_list;
 	mm->context.sram_list = lsl;
+	up_write(&mm->mmap_sem);
+
 	return addr;
 }
 EXPORT_SYMBOL(sram_alloc_with_lsl);

Re: [PATCH 0/2] blackfin: ptrace mm/sram_list fixes

[Mike Frysinger]

On Thu, May 27, 2010 at 15:55, Oleg Nesterov wrote:
> On 05/26, Oleg Nesterov wrote:
>> On 05/25, Mike Frysinger wrote:
>> > we probably should add proper locking to this
>> > structure though.
>>
>> Agreed. I'll try to make the trivial patch tomorrow. I think we
>> can just use mm->mmap_sem, is_user_addr_valid() needs this lock
>> for find_vma() anyway.
>
> please see the patches.

awesome !  wasnt expecting someone to do the work for us :).

> UNTESTED! the second one certainly needs the review from someone
> who knows what this code does ;)

np.  after i look them over, i'll suck them into the Blackfin tree and
let them settle for a release for our test suites to shake out
problems.

> BTW. Obviously sys_sram_alloc() can create multiple sram_list_struct
> nodes with the same ->addr (with or without this patch), I hope this
> is fine.

how so ?  Blackfin is a nommu arch, so there should be no aliasing
issues.  each of the individual L1 sub-allocators manage a different
address range, and none of those should return an address that is
already in use.  it could happen that the code calling these funcs
calls a free func directly instead of going through the lsl free, but
that's a bug in the usage, not this code.
-mike

Re: [PATCH 0/2] blackfin: ptrace mm/sram_list fixes

[Oleg Nesterov]

On 05/27, Mike Frysinger wrote:
>
> > BTW. Obviously sys_sram_alloc() can create multiple sram_list_struct
> > nodes with the same ->addr (with or without this patch), I hope this
> > is fine.
>
> how so ?

OOPS. Of course it can't, I was wrong.

> Blackfin is a nommu arch, so there should be no aliasing
> issues.  each of the individual L1 sub-allocators manage a different
> address range, and none of those should return an address that is
> already in use.

Yes, I greatly misunderstood this code, to the point I didn't notice
where this "addr" actually comes from.

Thanks for your explanation,

Oleg.

Re: [PATCH -mm 1/1] ptrace: PTRACE_GETFDPIC: fix the unsafe usage of child->mm

[David Howells]

Oleg Nesterov <oleg@redhat.com> wrote:

> > Like PEEKTEXT and POKETEXT?
> 
> They use access_process_vm().

Which needs to get the mm:

	int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, int len, int write)
	{
		struct vm_area_struct *vma;
		struct mm_struct *mm;

		if (addr + len < addr)
			return 0;

		mm = get_task_mm(tsk);
		if (!mm)
			return 0;

David

Re: [PATCH -mm 1/1] ptrace: PTRACE_GETFDPIC: fix the unsafe usage

[Oleg Nesterov]

On 05/25, David Howells wrote:
>
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > > Like PEEKTEXT and POKETEXT?
> >
> > They use access_process_vm().
>
> Which needs to get the mm:
>
> 	int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, int len, int write)
> 	{
> 		struct vm_area_struct *vma;
> 		struct mm_struct *mm;
>
> 		if (addr + len < addr)
> 			return 0;
>
> 		mm = get_task_mm(tsk);

Yes sure,

But I do not think it makes any sense to change the signature of
access_process_vm() as well, it has a lot of callers. And it is
complex, it does get_user_pages(). Compared to that get_task_mm()
inside of access_process_vm() is nothing.

Oleg.