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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
