| Message ID | 20201119160221.GA5188@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 640586f8af356096e084d69a9909d217852bde48 |
| Headers | show |
| Series | powerpc/ptrace: Hard wire PT_SOFTE value to 1 in gpr_get() too | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (9d1aa2f025c6cc516125c42c70f6a9ce087c49ea) |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 1 checks, 74 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
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
On 11/19, Christophe Leroy wrote: > > > 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 ? Nothing wrong, but imo the code and 2/2 looks simpler after this patch. I tried to explain this in the changelog. > > 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. OK, I can change this. > >- 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 ? I just tried to make tm_cgpr_get() and gpr_get() look similar. Sure, I can leave it as is. Better yet, could you please fix this problem somehow so that I could forget about the bug assigned to me? I know nothing about powerpc, and personally I do not care about this (minor) bug, I agree with any changes. > >- 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. > > > 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. See above, I can leave it as is. > >--- 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 ? This matches other membuf_ helpers. Oleg.
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)); - 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)); } /* 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)); - 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)); } 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; + + 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) \ ({ \
gpr_get() does membuf_write() twice to override pt_regs->msr in between. 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(-)