[PATCH] arch/tile: provide PT_FLAGS_COMPAT value in pt_regs

[PATCH] arch/tile: provide PT_FLAGS_COMPAT value in pt_regs

[Chris Metcalf]

This flag is set for ptrace GETREGS or PEEKUSER for processes
that are COMPAT, i.e. 32-bit.  This allows things like strace
to easily discover what personality to use, for example.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
 arch/tile/include/uapi/asm/ptrace.h |    6 ++++
 arch/tile/kernel/ptrace.c           |   57 ++++++++++++++++++++++++-----------
 2 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/arch/tile/include/uapi/asm/ptrace.h b/arch/tile/include/uapi/asm/ptrace.h
index c717d0f..0d22088 100644
--- a/arch/tile/include/uapi/asm/ptrace.h
+++ b/arch/tile/include/uapi/asm/ptrace.h
@@ -84,5 +84,11 @@ struct pt_regs {
 #define PTRACE_O_TRACEMIGRATE	0x00010000
 #define PTRACE_EVENT_MIGRATE	16
 
+/*
+ * Flag bits in pt_regs.flags that are part of the ptrace API.
+ * We start our numbering higher up to avoid confusion with the
+ * non-ABI kernel-internal values that use the low 16 bits.
+ */
+#define PT_FLAGS_COMPAT		0x10000  /* process is an -m32 compat process */
 
 #endif /* _UAPI_ASM_TILE_PTRACE_H */
diff --git a/arch/tile/kernel/ptrace.c b/arch/tile/kernel/ptrace.c
index e92e405..64ba102 100644
--- a/arch/tile/kernel/ptrace.c
+++ b/arch/tile/kernel/ptrace.c
@@ -45,6 +45,41 @@ void ptrace_disable(struct task_struct *child)
 	clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
 }
 
+/*
+ * Get registers from task and ready the result for userspace.
+ * Note that we localize the API issues to getregs() and putregs() at
+ * some cost in performance, e.g. we need a full pt_regs copy for
+ * PEEKUSR, and two copies for POKEUSR.  But in general we expect
+ * GETREGS/PUTREGS to be the API of choice anyway.
+ */
+static char *getregs(struct task_struct *child, struct pt_regs *uregs)
+{
+	*uregs = *task_pt_regs(child);
+
+	/* Set up flags ABI bits. */
+	uregs->flags = 0;
+#ifdef CONFIG_COMPAT
+	if (task_thread_info(child)->status & TS_COMPAT)
+		uregs->flags |= PT_FLAGS_COMPAT;
+#endif
+
+	return (char *)uregs;
+}
+
+/* Put registers back to task. */
+static void putregs(struct task_struct *child, struct pt_regs *uregs)
+{
+	struct pt_regs *regs = task_pt_regs(child);
+
+	/* Don't allow overwriting the kernel-internal flags word. */
+	uregs->flags = regs->flags;
+
+	/* Only allow setting the ICS bit in the ex1 word. */
+	uregs->ex1 = PL_ICS_EX1(USER_PL, EX1_ICS(uregs->ex1));
+
+	*regs = *uregs;
+}
+
 long arch_ptrace(struct task_struct *child, long request,
 		 unsigned long addr, unsigned long data)
 {
@@ -53,14 +88,13 @@ long arch_ptrace(struct task_struct *child, long request,
 	long ret = -EIO;
 	char *childreg;
 	struct pt_regs copyregs;
-	int ex1_offset;
 
 	switch (request) {
 
 	case PTRACE_PEEKUSR:  /* Read register from pt_regs. */
 		if (addr >= PTREGS_SIZE)
 			break;
-		childreg = (char *)task_pt_regs(child) + addr;
+		childreg = getregs(child, &copyregs) + addr;
 #ifdef CONFIG_COMPAT
 		if (is_compat_task()) {
 			if (addr & (sizeof(compat_long_t)-1))
@@ -79,17 +113,7 @@ long arch_ptrace(struct task_struct *child, long request,
 	case PTRACE_POKEUSR:  /* Write register in pt_regs. */
 		if (addr >= PTREGS_SIZE)
 			break;
-		childreg = (char *)task_pt_regs(child) + addr;
-
-		/* Guard against overwrites of the privilege level. */
-		ex1_offset = PTREGS_OFFSET_EX1;
-#if defined(CONFIG_COMPAT) && defined(__BIG_ENDIAN)
-		if (is_compat_task())   /* point at low word */
-			ex1_offset += sizeof(compat_long_t);
-#endif
-		if (addr == ex1_offset)
-			data = PL_ICS_EX1(USER_PL, EX1_ICS(data));
-
+		childreg = getregs(child, &copyregs) + addr;
 #ifdef CONFIG_COMPAT
 		if (is_compat_task()) {
 			if (addr & (sizeof(compat_long_t)-1))
@@ -102,11 +126,12 @@ long arch_ptrace(struct task_struct *child, long request,
 				break;
 			*(long *)childreg = data;
 		}
+		putregs(child, &copyregs);
 		ret = 0;
 		break;
 
 	case PTRACE_GETREGS:  /* Get all registers from the child. */
-		if (copy_to_user(datap, task_pt_regs(child),
+		if (copy_to_user(datap, getregs(child, &copyregs),
 				 sizeof(struct pt_regs)) == 0) {
 			ret = 0;
 		}
@@ -115,9 +140,7 @@ long arch_ptrace(struct task_struct *child, long request,
 	case PTRACE_SETREGS:  /* Set all registers in the child. */
 		if (copy_from_user(&copyregs, datap,
 				   sizeof(struct pt_regs)) == 0) {
-			copyregs.ex1 =
-				PL_ICS_EX1(USER_PL, EX1_ICS(copyregs.ex1));
-			*task_pt_regs(child) = copyregs;
+			putregs(child, &copyregs);
 			ret = 0;
 		}
 		break;
-- 
1.7.10.3

Re: [PATCH] arch/tile: provide PT_FLAGS_COMPAT value in pt_regs

[Oleg Nesterov]

Hi Chris,

it is too late for me to even try to read this patch, but...

On 12/12, Chris Metcalf wrote:
>
> This flag is set for ptrace GETREGS or PEEKUSER for processes
> that are COMPAT, i.e. 32-bit.
           ^^^^^^^^^^^^^^^^^^^

at least on x86 this is not the same. TS_COMPAT can also be set if a 64-bit
task calls the 32-bit syscall.

> --- a/arch/tile/include/uapi/asm/ptrace.h
> +++ b/arch/tile/include/uapi/asm/ptrace.h
> @@ -84,5 +84,11 @@ struct pt_regs {
>  #define PTRACE_O_TRACEMIGRATE	0x00010000
>  #define PTRACE_EVENT_MIGRATE	16
>
> +/*
> + * Flag bits in pt_regs.flags that are part of the ptrace API.
> + * We start our numbering higher up to avoid confusion with the
> + * non-ABI kernel-internal values that use the low 16 bits.
> + */
> +#define PT_FLAGS_COMPAT		0x10000  /* process is an -m32 compat process */

Can't understand how this connects to ptrace (I mean task->ptrace).

OK, let it live in asm/ptrace.h, but it seems that it is similar to
(say) PT_FLAGS_RESTORE_REGS and thus it should be 8?

And. arch/tile/kernel/ptrace.c:arch_ptrace() does

	case PTRACE_SETOPTIONS:
		/* Support TILE-specific ptrace options. */
		child->ptrace &= ~PT_TRACE_MASK_TILE;
		tmp = data & PTRACE_O_MASK_TILE;
		data &= ~PTRACE_O_MASK_TILE;

AFAICS we need something like BUILD_BUG_ON(PTRACE_O_MASK_TILE & PTRACE_O_MASK),
and

		ret = ptrace_request(child, request, addr, data);
		if (tmp & PTRACE_O_TRACEMIGRATE)
			child->ptrace |= PT_TRACE_MIGRATE;

this also needs "ret == 0" check, and "&= ~PT_TRACE_MASK_TILE" abobe should
be moved here, no?

OTOH using /bin/grep I can't see where do we check ">ptrace & PT_TRACE_MIGRATE".


In short: confused ;)

Oleg.

Re: [PATCH] arch/tile: provide PT_FLAGS_COMPAT value in pt_regs

[Chris Metcalf]

On 12/12/2012 6:43 PM, Oleg Nesterov wrote:
> Hi Chris,
>
> it is too late for me to even try to read this patch, but...

Thanks for the review!

> On 12/12, Chris Metcalf wrote:
>> This flag is set for ptrace GETREGS or PEEKUSER for processes
>> that are COMPAT, i.e. 32-bit.
>            ^^^^^^^^^^^^^^^^^^^
>
> at least on x86 this is not the same. TS_COMPAT can also be set if a 64-bit
> task calls the 32-bit syscall.

There's no way on tile for that to happen.  The OS chooses which syscall implementation to provide based on the Elf class of the loaded executable; the syscall instruction in userspace is the same either way.

In fact, SO MUCH is identical in the two environments that I couldn't think of a good way for strace to be able to easily figure out whether it was a 32- or 64-bit environment - thus this patch :-)

>> --- a/arch/tile/include/uapi/asm/ptrace.h
>> +++ b/arch/tile/include/uapi/asm/ptrace.h
>> @@ -84,5 +84,11 @@ struct pt_regs {
>>  #define PTRACE_O_TRACEMIGRATE	0x00010000
>>  #define PTRACE_EVENT_MIGRATE	16
>>
>> +/*
>> + * Flag bits in pt_regs.flags that are part of the ptrace API.
>> + * We start our numbering higher up to avoid confusion with the
>> + * non-ABI kernel-internal values that use the low 16 bits.
>> + */
>> +#define PT_FLAGS_COMPAT		0x10000  /* process is an -m32 compat process */
> Can't understand how this connects to ptrace (I mean task->ptrace).

The idea is that while other architectures have things in their registers that identify a 32-bit execution environment, tile doesn't.  For example, PPC has a bit in the MSR and x86 has a different value in the CS register.  So for tile I just synthesize a bit to report in the existing "flags" word of the struct pt_regs.

> OK, let it live in asm/ptrace.h, but it seems that it is similar to
> (say) PT_FLAGS_RESTORE_REGS and thus it should be 8?

The other bits that live in that word are kernel-internal only, e.g. PT_FLAGS_RESTORE_REGS.  So they are not in the uapi header.  And in fact, we don't even report them out through the GETREGS API; we just set the single user-visible bit.  In principle we could even overlap the numbering and make PT_FLAGS_COMPAT have value "1" as well, but that seemed excessively confusing.

> And. arch/tile/kernel/ptrace.c:arch_ptrace() does
>
> 	case PTRACE_SETOPTIONS:
> 		/* Support TILE-specific ptrace options. */
> 		child->ptrace &= ~PT_TRACE_MASK_TILE;
> 		tmp = data & PTRACE_O_MASK_TILE;
> 		data &= ~PTRACE_O_MASK_TILE;
>
> AFAICS we need something like BUILD_BUG_ON(PTRACE_O_MASK_TILE & PTRACE_O_MASK),

I don't think so.  These are disjoint namespaces anyway.  PTRACE_O_MASK_TILE is for the actual PTRACE_SETOPTIONS ABI values.  PT_TRACE_MASK_TILE is for the values stored in task->ptrace.

> and
>
> 		ret = ptrace_request(child, request, addr, data);
> 		if (tmp & PTRACE_O_TRACEMIGRATE)
> 			child->ptrace |= PT_TRACE_MIGRATE;
>
> this also needs "ret == 0" check

The question is, what happens if we pass some illegal bit to the generic ptrace_request(), and also pass a valid PTRACE_O_MASK_TILE bit?  Currently we set the tile-specific bit, then report the error.  This is consistent with how ptrace_setoptions() handles a mix of legal and illegal bits.

> and "&= ~PT_TRACE_MASK_TILE" abobe should be moved here, no?

We could move it, but I don't think there's a correctness argument here.  Are you just seeing it would be easier to understand if the manipulation of child->ptrace was all on adjacent lines of code?  I agree that does seem a bit clearer; I'll post a separate patch for that.

> OTOH using /bin/grep I can't see where do we check ">ptrace & PT_TRACE_MIGRATE".

Yes, in our internal tree, this is part of the "dynamic homecache" code that lets us adjust the owner cache for some pages of a task's address space when it migrates to a new core.  It's not actually strictly dependent on that functionality but just got bundled in with it for convenience.  And since we haven't yet tried to return that code (it's somewhat intrusive to the core mm stuff) there's not much point to the ptrace extension. :-)  Oh well, we'll try to push it at some point.

> In short: confused ;)
>
> Oleg.

I hope this clears it up a bit.  Let me know if the patch makes sense to you now! :-)

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] arch/tile: provide PT_FLAGS_COMPAT value in pt_regs

[Oleg Nesterov]

On 12/13, Chris Metcalf wrote:
>
> On 12/12/2012 6:43 PM, Oleg Nesterov wrote:
>
> > On 12/12, Chris Metcalf wrote:
> >> This flag is set for ptrace GETREGS or PEEKUSER for processes
> >> that are COMPAT, i.e. 32-bit.
> >            ^^^^^^^^^^^^^^^^^^^
> >
> > at least on x86 this is not the same. TS_COMPAT can also be set if a 64-bit
> > task calls the 32-bit syscall.
>
> There's no way on tile for that to happen.

OK,

> >> --- a/arch/tile/include/uapi/asm/ptrace.h
> >> +++ b/arch/tile/include/uapi/asm/ptrace.h
> >> @@ -84,5 +84,11 @@ struct pt_regs {
> >>  #define PTRACE_O_TRACEMIGRATE	0x00010000
> >>  #define PTRACE_EVENT_MIGRATE	16
> >>
> >> +/*
> >> + * Flag bits in pt_regs.flags that are part of the ptrace API.
> >> + * We start our numbering higher up to avoid confusion with the
> >> + * non-ABI kernel-internal values that use the low 16 bits.
> >> + */
> >> +#define PT_FLAGS_COMPAT		0x10000  /* process is an -m32 compat process */
> > Can't understand how this connects to ptrace (I mean task->ptrace).
>
> The idea is that while other architectures have things in their registers
> that identify a 32-bit execution environment, tile doesn't.  For example,
> PPC has a bit in the MSR and x86 has a different value in the CS register.
> So for tile I just synthesize a bit to report in the existing "flags" word
> of the struct pt_regs.
>
> > OK, let it live in asm/ptrace.h, but it seems that it is similar to
> > (say) PT_FLAGS_RESTORE_REGS and thus it should be 8?
>
> The other bits that live in that word are kernel-internal only, e.g.
> PT_FLAGS_RESTORE_REGS.  So they are not in the uapi header.  And in fact,
> we don't even report them out through the GETREGS API;
> we just set the single user-visible bit.

OK,

> > And. arch/tile/kernel/ptrace.c:arch_ptrace() does
> >
> > 	case PTRACE_SETOPTIONS:
> > 		/* Support TILE-specific ptrace options. */
> > 		child->ptrace &= ~PT_TRACE_MASK_TILE;
> > 		tmp = data & PTRACE_O_MASK_TILE;
> > 		data &= ~PTRACE_O_MASK_TILE;
> >
> > AFAICS we need something like BUILD_BUG_ON(PTRACE_O_MASK_TILE & PTRACE_O_MASK),
>
> I don't think so.  These are disjoint namespaces anyway.
> PTRACE_O_MASK_TILE is for the actual PTRACE_SETOPTIONS ABI values.

Yes, and thus it should not intersect with the generic PTRACE_O_MASK, no?

Suppose that we add the new generic ptrace option equal to PT_TRACE_MIGRATE.
then it won't work on tile.

> PT_TRACE_MASK_TILE is for the values stored in task->ptrace.

Yes, and thus it would be better to ensure it can't conflict with other
->ptrace bits.


> > 		ret = ptrace_request(child, request, addr, data);
> > 		if (tmp & PTRACE_O_TRACEMIGRATE)
> > 			child->ptrace |= PT_TRACE_MIGRATE;
> >
> > this also needs "ret == 0" check
>
> The question is, what happens if we pass some illegal bit to the generic
> ptrace_request(), and also pass a valid PTRACE_O_MASK_TILE bit?
> Currently we set the tile-specific bit, then report the error.
> This is consistent with how ptrace_setoptions() handles a mix of legal and
> illegal bits.

But ptrace_setoptions() returns EINVAL? it doesn't accept illegal bits.

> > and "&= ~PT_TRACE_MASK_TILE" abobe should be moved here, no?
>
> We could move it, but I don't think there's a correctness argument here.
> Are you just seeing it would be easier to understand if the manipulation of
> child->ptrace was all on adjacent lines of code?  I agree that does seem a bit
> clearer; I'll post a separate patch for that.

I agree this is minor, and up to you. Just it doesn't look consistent to me.

> > OTOH using /bin/grep I can't see where do we check ">ptrace & PT_TRACE_MIGRATE".
>
> Yes, in our internal tree,

OK. And again, somehow it would be nice to check that PTRACE_EVENT_MIGRATE
doesn't conflict with the generic PTRACE_EVENT_'s.

But this all is offtopic,

> > In short: confused ;)
> >
> I hope this clears it up a bit.  Let me know if the patch makes sense to you now! :-)

Oh, I do not understand this code with or without the patch ;)

So I'd say it looks fine to me.

Oleg.

Re: [PATCH] arch/tile: provide PT_FLAGS_COMPAT value in pt_regs

[Chris Metcalf]

On 12/13/2012 10:49 AM, Oleg Nesterov wrote:
> On 12/13, Chris Metcalf wrote:
>> On 12/12/2012 6:43 PM, Oleg Nesterov wrote:
>>> And. arch/tile/kernel/ptrace.c:arch_ptrace() does
>>>
>>> 	case PTRACE_SETOPTIONS:
>>> 		/* Support TILE-specific ptrace options. */
>>> 		child->ptrace &= ~PT_TRACE_MASK_TILE;
>>> 		tmp = data & PTRACE_O_MASK_TILE;
>>> 		data &= ~PTRACE_O_MASK_TILE;
>>>
>>> AFAICS we need something like BUILD_BUG_ON(PTRACE_O_MASK_TILE & PTRACE_O_MASK),
>> I don't think so.  These are disjoint namespaces anyway.
>> PTRACE_O_MASK_TILE is for the actual PTRACE_SETOPTIONS ABI values.
> Yes, and thus it should not intersect with the generic PTRACE_O_MASK, no?

Yes, I misunderstood your original suggestion (I read PTRACE_O_MASK as PT_TRACE_MASK_TILE - oops).  You're quite right that it's a good build bug; I'll add it to the cleanup patch that will also move the task->ptrace bit clear.

>>> 		ret = ptrace_request(child, request, addr, data);
>>> 		if (tmp & PTRACE_O_TRACEMIGRATE)
>>> 			child->ptrace |= PT_TRACE_MIGRATE;
>>>
>>> this also needs "ret == 0" check
>> The question is, what happens if we pass some illegal bit to the generic
>> ptrace_request(), and also pass a valid PTRACE_O_MASK_TILE bit?
>> Currently we set the tile-specific bit, then report the error.
>> This is consistent with how ptrace_setoptions() handles a mix of legal and
>> illegal bits.
> But ptrace_setoptions() returns EINVAL? it doesn't accept illegal bits.

It does return EINVAL - but if it gets both legal and illegal bits, it honors all the legal bits first.  So we honor the PT_TRACE_MIGRATE bit in this code, even if ptrace_request() returns EINVAL.

> So I'd say it looks fine to me.

Thanks!  Should I convert that to a Reviewed-by or Acked-by on the patch?

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] arch/tile: provide PT_FLAGS_COMPAT value in pt_regs

[Oleg Nesterov]

On 12/13, Chris Metcalf wrote:
>
> On 12/13/2012 10:49 AM, Oleg Nesterov wrote:
> > But ptrace_setoptions() returns EINVAL? it doesn't accept illegal bits.
>
> It does return EINVAL - but if it gets both legal and illegal bits,
> it honors all the legal bits first.

This was true in the past, but now we have

	static int ptrace_setoptions(struct task_struct *child, unsigned long data)
	{
		unsigned flags;

		if (data & ~(unsigned long)PTRACE_O_MASK)
			return -EINVAL;

		/* Avoid intermediate state when all opts are cleared */
		flags = child->ptrace;
		flags &= ~(PTRACE_O_MASK << PT_OPT_FLAG_SHIFT);
		flags |= (data << PT_OPT_FLAG_SHIFT);
		child->ptrace = flags;

		return 0;
	}

> > So I'd say it looks fine to me.
>
> Thanks!  Should I convert that to a Reviewed-by or Acked-by on the patch?

Heh ;) Please feel free to add Acked-by: Oleg Nesterov <oleg@redhat.com>

Oleg.

[PATCH] arch/tile: clean up tile-specific PTRACE_SETOPTIONS

[Chris Metcalf]

Use the newer idioms for setting PTRACE_O_xxx and PT_TRACE_xxx flags.
Only set/clear tile-specific flags if the generic routine returns
success, since otherwise we want to avoid setting any flags at all.
Atomically update the ptrace flags with the new values.  Eliminate
the PT_TRACE_MASK_TILE bitmask and just shift PTRACE_O_MASK_TILE.
Add a BUILD_BUG_ON to avoid overlapping with generic bits.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
 arch/tile/include/asm/ptrace.h      |    3 +--
 arch/tile/include/uapi/asm/ptrace.h |    2 +-
 arch/tile/kernel/ptrace.c           |   10 +++++++---
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/tile/include/asm/ptrace.h b/arch/tile/include/asm/ptrace.h
index 1a4fd9a..5ce052e 100644
--- a/arch/tile/include/asm/ptrace.h
+++ b/arch/tile/include/asm/ptrace.h
@@ -24,8 +24,7 @@ typedef unsigned long pt_reg_t;
 #include <uapi/asm/ptrace.h>
 
 #define PTRACE_O_MASK_TILE	(PTRACE_O_TRACEMIGRATE)
-#define PT_TRACE_MIGRATE	0x00080000
-#define PT_TRACE_MASK_TILE	(PT_TRACE_MIGRATE)
+#define PT_TRACE_MIGRATE	PT_EVENT_FLAG(PTRACE_EVENT_MIGRATE)
 
 /* Flag bits in pt_regs.flags */
 #define PT_FLAGS_DISABLE_IRQ    1  /* on return to kernel, disable irqs */
diff --git a/arch/tile/include/uapi/asm/ptrace.h b/arch/tile/include/uapi/asm/ptrace.h
index 0d22088..7757e19 100644
--- a/arch/tile/include/uapi/asm/ptrace.h
+++ b/arch/tile/include/uapi/asm/ptrace.h
@@ -81,8 +81,8 @@ struct pt_regs {
 #define PTRACE_SETFPREGS	15
 
 /* Support TILE-specific ptrace options, with events starting at 16. */
-#define PTRACE_O_TRACEMIGRATE	0x00010000
 #define PTRACE_EVENT_MIGRATE	16
+#define PTRACE_O_TRACEMIGRATE	(1 << PTRACE_EVENT_MIGRATE)
 
 /*
  * Flag bits in pt_regs.flags that are part of the ptrace API.
diff --git a/arch/tile/kernel/ptrace.c b/arch/tile/kernel/ptrace.c
index 64ba102..b32bc3f 100644
--- a/arch/tile/kernel/ptrace.c
+++ b/arch/tile/kernel/ptrace.c
@@ -151,12 +151,16 @@ long arch_ptrace(struct task_struct *child, long request,
 
 	case PTRACE_SETOPTIONS:
 		/* Support TILE-specific ptrace options. */
-		child->ptrace &= ~PT_TRACE_MASK_TILE;
+		BUILD_BUG_ON(PTRACE_O_MASK_TILE & PTRACE_O_MASK);
 		tmp = data & PTRACE_O_MASK_TILE;
 		data &= ~PTRACE_O_MASK_TILE;
 		ret = ptrace_request(child, request, addr, data);
-		if (tmp & PTRACE_O_TRACEMIGRATE)
-			child->ptrace |= PT_TRACE_MIGRATE;
+		if (ret == 0) {
+			unsigned int flags = child->ptrace;
+			flags &= ~(PTRACE_O_MASK_TILE << PT_OPT_FLAG_SHIFT);
+			flags |= (tmp << PT_OPT_FLAG_SHIFT);
+			child->ptrace = flags;
+		}
 		break;
 
 	default:
-- 
1.7.10.3

Re: [PATCH] arch/tile: provide PT_FLAGS_COMPAT value in pt_regs

[Chris Metcalf]

On 12/13/2012 11:27 AM, Oleg Nesterov wrote:
> On 12/13, Chris Metcalf wrote:
>> On 12/13/2012 10:49 AM, Oleg Nesterov wrote:
>>> But ptrace_setoptions() returns EINVAL? it doesn't accept illegal bits.
>> It does return EINVAL - but if it gets both legal and illegal bits,
>> it honors all the legal bits first.
> This was true in the past, but now we have
>
> 	static int ptrace_setoptions(struct task_struct *child, unsigned long data)
> 	{
> 		unsigned flags;
>
> 		if (data & ~(unsigned long)PTRACE_O_MASK)
> 			return -EINVAL;
>
> 		/* Avoid intermediate state when all opts are cleared */
> 		flags = child->ptrace;
> 		flags &= ~(PTRACE_O_MASK << PT_OPT_FLAG_SHIFT);
> 		flags |= (data << PT_OPT_FLAG_SHIFT);
> 		child->ptrace = flags;
>
> 		return 0;
> 	}

Yup, I was looking at an older version of the code by mistake.  See patch to follow.  Thanks!

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] arch/tile: clean up tile-specific PTRACE_SETOPTIONS

[Oleg Nesterov]

On 12/13, Chris Metcalf wrote:
>
> Use the newer idioms for setting PTRACE_O_xxx and PT_TRACE_xxx flags.
> Only set/clear tile-specific flags if the generic routine returns
> success, since otherwise we want to avoid setting any flags at all.
> Atomically update the ptrace flags with the new values.  Eliminate
> the PT_TRACE_MASK_TILE bitmask and just shift PTRACE_O_MASK_TILE.
> Add a BUILD_BUG_ON to avoid overlapping with generic bits.
>
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>

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

> ---
>  arch/tile/include/asm/ptrace.h      |    3 +--
>  arch/tile/include/uapi/asm/ptrace.h |    2 +-
>  arch/tile/kernel/ptrace.c           |   10 +++++++---
>  3 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/tile/include/asm/ptrace.h b/arch/tile/include/asm/ptrace.h
> index 1a4fd9a..5ce052e 100644
> --- a/arch/tile/include/asm/ptrace.h
> +++ b/arch/tile/include/asm/ptrace.h
> @@ -24,8 +24,7 @@ typedef unsigned long pt_reg_t;
>  #include <uapi/asm/ptrace.h>
>  
>  #define PTRACE_O_MASK_TILE	(PTRACE_O_TRACEMIGRATE)
> -#define PT_TRACE_MIGRATE	0x00080000
> -#define PT_TRACE_MASK_TILE	(PT_TRACE_MIGRATE)
> +#define PT_TRACE_MIGRATE	PT_EVENT_FLAG(PTRACE_EVENT_MIGRATE)
>  
>  /* Flag bits in pt_regs.flags */
>  #define PT_FLAGS_DISABLE_IRQ    1  /* on return to kernel, disable irqs */
> diff --git a/arch/tile/include/uapi/asm/ptrace.h b/arch/tile/include/uapi/asm/ptrace.h
> index 0d22088..7757e19 100644
> --- a/arch/tile/include/uapi/asm/ptrace.h
> +++ b/arch/tile/include/uapi/asm/ptrace.h
> @@ -81,8 +81,8 @@ struct pt_regs {
>  #define PTRACE_SETFPREGS	15
>  
>  /* Support TILE-specific ptrace options, with events starting at 16. */
> -#define PTRACE_O_TRACEMIGRATE	0x00010000
>  #define PTRACE_EVENT_MIGRATE	16
> +#define PTRACE_O_TRACEMIGRATE	(1 << PTRACE_EVENT_MIGRATE)
>  
>  /*
>   * Flag bits in pt_regs.flags that are part of the ptrace API.
> diff --git a/arch/tile/kernel/ptrace.c b/arch/tile/kernel/ptrace.c
> index 64ba102..b32bc3f 100644
> --- a/arch/tile/kernel/ptrace.c
> +++ b/arch/tile/kernel/ptrace.c
> @@ -151,12 +151,16 @@ long arch_ptrace(struct task_struct *child, long request,
>  
>  	case PTRACE_SETOPTIONS:
>  		/* Support TILE-specific ptrace options. */
> -		child->ptrace &= ~PT_TRACE_MASK_TILE;
> +		BUILD_BUG_ON(PTRACE_O_MASK_TILE & PTRACE_O_MASK);
>  		tmp = data & PTRACE_O_MASK_TILE;
>  		data &= ~PTRACE_O_MASK_TILE;
>  		ret = ptrace_request(child, request, addr, data);
> -		if (tmp & PTRACE_O_TRACEMIGRATE)
> -			child->ptrace |= PT_TRACE_MIGRATE;
> +		if (ret == 0) {
> +			unsigned int flags = child->ptrace;
> +			flags &= ~(PTRACE_O_MASK_TILE << PT_OPT_FLAG_SHIFT);
> +			flags |= (tmp << PT_OPT_FLAG_SHIFT);
> +			child->ptrace = flags;
> +		}
>  		break;
>  
>  	default:
> -- 
> 1.7.10.3
>