Re: [PATCH v3 1/2] powerpc/ptrace: simplify gpr_get/tm_cgpr_get

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 19/11/2020 à 17:02, Oleg Nesterov a écrit :
> gpr_get() does membuf_write() twice to override pt_regs->msr in between.

Is there anything wrong with that ?

> We can call membuf_write() once and change ->msr in the kernel buffer,
> this simplifies the code and the next fix.
> 
> The patch adds a new simple helper, membuf_at(offs), it returns the new
> membuf which can be safely used after membuf_write().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>   arch/powerpc/kernel/ptrace/ptrace-tm.c   | 13 +++++--------
>   arch/powerpc/kernel/ptrace/ptrace-view.c | 13 +++++--------
>   include/linux/regset.h                   | 12 ++++++++++++
>   3 files changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-tm.c b/arch/powerpc/kernel/ptrace/ptrace-tm.c
> index 54f2d076206f..f8fcbd85d4cb 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-tm.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-tm.c
> @@ -86,6 +86,8 @@ int tm_cgpr_active(struct task_struct *target, const struct user_regset *regset)
>   int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
>   		struct membuf to)
>   {
> +	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
> +
>   	if (!cpu_has_feature(CPU_FTR_TM))
>   		return -ENODEV;
>   
> @@ -97,17 +99,12 @@ int tm_cgpr_get(struct task_struct *target, const struct user_regset *regset,
>   	flush_altivec_to_thread(target);
>   
>   	membuf_write(&to, &target->thread.ckpt_regs,
> -			offsetof(struct pt_regs, msr));
> -	membuf_store(&to, get_user_ckpt_msr(target));
> +				sizeof(struct user_pt_regs));

This looks mis-aligned. But it should fit on a single line, now we allow up to 100 chars on a line.

>   
> -	BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
> -		     offsetof(struct pt_regs, msr) + sizeof(long));
> +	membuf_store(&to_msr, get_user_ckpt_msr(target));
>   
> -	membuf_write(&to, &target->thread.ckpt_regs.orig_gpr3,
> -			sizeof(struct user_pt_regs) -
> -			offsetof(struct pt_regs, orig_gpr3));
>   	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
> -			sizeof(struct user_pt_regs));
> +				sizeof(struct user_pt_regs));

I can't see any change here except the alignment. Can you leave it as is ?


>   }
>   
>   /*
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c
> index 7e6478e7ed07..39686ede40b3 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-view.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
> @@ -217,6 +217,7 @@ int ptrace_put_reg(struct task_struct *task, int regno, unsigned long data)
>   static int gpr_get(struct task_struct *target, const struct user_regset *regset,
>   		   struct membuf to)
>   {
> +	struct membuf to_msr = membuf_at(&to, offsetof(struct pt_regs, msr));
>   	int i;
>   
>   	if (target->thread.regs == NULL)
> @@ -228,17 +229,13 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
>   			target->thread.regs->gpr[i] = NV_REG_POISON;
>   	}
>   
> -	membuf_write(&to, target->thread.regs, offsetof(struct pt_regs, msr));
> -	membuf_store(&to, get_user_msr(target));
> +	membuf_write(&to, target->thread.regs,
> +				sizeof(struct user_pt_regs));

This should fit on a single line.

>   
> -	BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
> -		     offsetof(struct pt_regs, msr) + sizeof(long));
> +	membuf_store(&to_msr, get_user_msr(target));
>   
> -	membuf_write(&to, &target->thread.regs->orig_gpr3,
> -			sizeof(struct user_pt_regs) -
> -			offsetof(struct pt_regs, orig_gpr3));
>   	return membuf_zero(&to, ELF_NGREG * sizeof(unsigned long) -
> -				 sizeof(struct user_pt_regs));
> +				sizeof(struct user_pt_regs));

This should not change, it's not part of the changes for this patch.

>   }
>   
>   static int gpr_set(struct task_struct *target, const struct user_regset *regset,
> diff --git a/include/linux/regset.h b/include/linux/regset.h
> index c3403f328257..a00765f0e8cf 100644
> --- a/include/linux/regset.h
> +++ b/include/linux/regset.h
> @@ -46,6 +46,18 @@ static inline int membuf_write(struct membuf *s, const void *v, size_t size)
>   	return s->left;
>   }
>   
> +static inline struct membuf membuf_at(const struct membuf *s, size_t offs)
> +{
> +	struct membuf n = *s;

Is there any point in using a struct membuf * instaed of a struct membuf as parameter ?

> +
> +	if (offs > n.left)
> +		offs = n.left;
> +	n.p += offs;
> +	n.left -= offs;
> +
> +	return n;
> +}
> +
>   /* current s->p must be aligned for v; v must be a scalar */
>   #define membuf_store(s, v)				\
>   ({							\
> 

Christophe