[PATCH] x86/ptrace: Add comments to x86_regset about empty slots

[PATCH] x86/ptrace: Add comments to x86_regset about empty slots

[Yu-cheng Yu]

find_regset() goes through regsets sequentially.  Empty slots
in regset arrays causes mismatch.  Add comments to x86_regset
enum.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 arch/x86/kernel/ptrace.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index e2ee403865eb..130ca4f27a17 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -42,6 +42,11 @@
 
 #include "tls.h"
 
+/*
+ * find_regset() goes through regsets sequentially.
+ * Do not create empty slots in x86_64_regsets[] or
+ * x86_32_regsets[] below.
+ */
 enum x86_regset {
 	REGSET_GENERAL,
 	REGSET_FP,
-- 
2.17.1

Re: [PATCH] x86/ptrace: Add comments to x86_regset about empty slots

[Ingo Molnar]

* Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:

> find_regset() goes through regsets sequentially.  Empty slots
> in regset arrays causes mismatch.  Add comments to x86_regset
> enum.
> 
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
>  arch/x86/kernel/ptrace.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> index e2ee403865eb..130ca4f27a17 100644
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@ -42,6 +42,11 @@
>  
>  #include "tls.h"
>  
> +/*
> + * find_regset() goes through regsets sequentially.
> + * Do not create empty slots in x86_64_regsets[] or
> + * x86_32_regsets[] below.
> + */
>  enum x86_regset {
>  	REGSET_GENERAL,
>  	REGSET_FP,

What mismatch exactly? The logic in find_regset() is:

        for (n = 0; n < view->n; ++n) {
                regset = view->regsets + n;
                if (regset->core_note_type == type)
                        return regset;
        }


and an 'empty' slot would have .core_note_type of 0 - which would be easy to skip 
or warn about.

It appears to me user-space ptrace users can control 'type' via PTRACE_GETREGSET, 
so it might make sense to filter out the value of 0 there.

Or something like that. The patch doesn't really explain the problem, and I 
maintain my argument that the current code of relying on no empty slots and not 
having any mechanism other than human review ensuring it is both ugly and fragile.

Thanks,

	Ingo