From: Roland McGrath on
> Roland, All I found after double checking is:
>
> > For now, only first 8 bytes of the sw_usable_bytes[464..467]
>
> should be
>
> > For now, only first 8 bytes of the sw_usable_bytes[464..471]

I didn't notice that one, because I didn't check any of your arithmetic.

> Let me know if I am overlooking something.

I only meant the pure English errors. Here is the comment with some
grammar and punctuation fixed:

+/*
+ * The structure layout used in PTRACE_GETXSTATEREGS/PTRACE_SETXSTATEREGS is
+ * the same as the memory layout of xsave used by the processor (except
+ * for the bytes 464..511, which can be used by the software). The size
+ * of the structure that users need to use for these two interfaces can be
+ * obtained by doing:
+ * cpuid_count(0xd, 0, &eax, &ptrace_xstateregs_struct_size, &ecx, &edx);
+ * i.e., cpuid.(eax=0xd,ecx=0).ebx will be the size that user (debuggers, etc.)
+ * need to use.
+ *
+ * The format of this structure will be like:
+ * struct {
+ * fxsave_bytes[0..463]
+ * sw_usable_bytes[464..511]
+ * xsave_hdr_bytes[512..575]
+ * avx_bytes[576..831]
+ * future_state etc
+ * }
+ *
+ * The same memory layout will be used for the core-dump NT_X86_XSTATE
+ * note representing the xstate registers.
+ *
+ * For now, only the first 8 bytes of the sw_usable_bytes[464..471] will
+ * be used and will be set to OS enabled xstate mask (which is same as the
+ * 64bit mask returned by the xgetbv's xCR0). Users (analyzing core dump
+ * remotely, etc.) can use this mask as well as the mask saved in the
+ * xstate_hdr bytes and interpret what states the processor/OS supports
+ * and what states are in modified/initialized conditions for the
+ * particular process/thread.
+ *
+ * Also when the user modifies certain state FP/SSE/etc through this
+ * PTRACE_SETXSTATEREGS, they must ensure that the xsave_hdr.xstate_bv
+ * bytes[512..519] of the above memory layout are updated correspondingly.
+ * i.e., for example when FP state is modified to a non-init state,
+ * xsave_hdr.xstate_bv's bit 0 must be set to '1', when SSE is modified to
+ * non-init state, xsave_hdr.xstate_bv's bit 1 must to be set to '1', etc.
+ */

> Yes. No size limit as of now.

Ok. Thanks for the clear answer.

> Ok. I think I can agree, if we are ok with giving room for the ptrace
> (or any other user of the API) to make a mistake and corrupt reg-state
> of the process under debug, if it doesn't follow rules.

This is not new, and it's not a problem. The bottom line is that ptrace
can do whatever the process could have done to itself.

> Thought some of them might be only relevant to core-dump or based on
> permissions etc. But I guess get/set routines of the regset should be
> able to take care of this?

The whole point of user_regset is that whatever user-space machine state
there is to be had can be accessed consistently by whatever means. If
there is something that you don't want debuggers to be able to access, then
user_regset is not where it belongs. There is no rationale I can imagine
for having something in a core dump but not readable by debuggers when the
process is still alive.

The only issue that I can imagine you might be referring to when you say
"based on permissions etc." is state that you cannot set arbitrarily from
user mode. The only such example we have is NT_386_IOPERM, which you just
cannot set at all via user_regset. If we were to allow debuggers to change
that data at all, then its .set function would need to do some permission
checking.

For everything else that is really part of the user-mode register state per
se, it just doesn't make sense to think about any kind of "permission
checking" beyond the simple masks applied to eflags, mxcsr, etc. The
debugger can make the user process do anything that the process could do
itself, which of course includes changing all its registers.

> So in the example you provided before:
>
> struct iovec iov = { mybuffer, mylength };
> ret = ptrace(PTRACE_GETREGSET, NT_X86_XSTATE, &iov);
>
> You wanted to propose common data format (iov) for all of the NT_* ?

I'm not sure I understand your question. The iovec is just API trivia,
part of communicating "I want this regset and I want these bytes of it".
This has nothing to do with the actual data format of each regset.


Thanks,
Roland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Roland McGrath on
> #define PTRACE_GETREGS(r) (((r) << 16) | PTRACE_GETREGS_CMD)
>
> ... or something like that?

(You can't use that exact name, it's taken.) IMHO this is some spurious
obfuscation that is not warranted by saving the two get_user calls in the
kernel. (OTOH, my suggestion requires a whole extra 5 lines of code or so
in compat_sys_ptrace because the indirection in the ABI is sensitive to
userland word size.) But I don't feel strongly about the particulars of
the ptrace API addition, just that it be generic to cover any regset and
not be prone to implicit buffer-size miscommunications. I'll leave it to
whatever Oleg wants to implement.


Thanks,
Roland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Suresh Siddha on
On Fri, 2010-02-05 at 13:15 -0800, Roland McGrath wrote:
> > #define PTRACE_GETREGS(r) (((r) << 16) | PTRACE_GETREGS_CMD)
> >
> > ... or something like that?
>
> (You can't use that exact name, it's taken.) IMHO this is some spurious
> obfuscation that is not warranted by saving the two get_user calls in the
> kernel.

Also value of NT_PRXFPREG complicates things bit more

#define NT_PRXFPREG 0x46e62b7f /* copied from gdb5.1/include/elf/common.h */

In this context, we have to perhaps use bits 31 and 29 to represent this
generic ptrace interface and the corresponding GET/SET commands.

> (OTOH, my suggestion requires a whole extra 5 lines of code or so
> in compat_sys_ptrace because the indirection in the ABI is sensitive to
> userland word size.) But I don't feel strongly about the particulars of
> the ptrace API addition, just that it be generic to cover any regset and
> not be prone to implicit buffer-size miscommunications. I'll leave it to
> whatever Oleg wants to implement.

Ok. I will split the previous patch in to two patches and re-post it. I
can help Oleg with reviewing and testing the generic implementation
whenever it is ready.

thanks,
suresh

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on
Sorry for delay!

On 02/05, Suresh Siddha wrote:
>
> On Fri, 2010-02-05 at 13:15 -0800, Roland McGrath wrote:
>
> > (OTOH, my suggestion requires a whole extra 5 lines of code or so
> > in compat_sys_ptrace because the indirection in the ABI is sensitive to
> > userland word size.) But I don't feel strongly about the particulars of
> > the ptrace API addition, just that it be generic to cover any regset and
> > not be prone to implicit buffer-size miscommunications. I'll leave it to
> > whatever Oleg wants to implement.
>
> Ok. I will split the previous patch in to two patches and re-post it. I
> can help Oleg with reviewing and testing the generic implementation
> whenever it is ready.

OK, I'll try to do this "asap", this generic implementation looks easy.

Except, I am not sure I really understand the problem, need to read
the regset code and then re-read this thread ;)

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/