Re: [PATCH v3 1/2] powerpc/ptrace: simplify gpr_get/tm_cgpr_get
From: Christophe Leroy <hidden>
Date: 2020-11-19 17:16:42
Also in:
lkml
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 ?
quoted hunk ↗ jump to hunk
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 ?
quoted hunk ↗ jump to hunk
} /*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.
quoted hunk ↗ jump to hunk
} 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