Re: [PATCH v8 13/30] powerpc: Add a probe_user_read_inst() function
On Thu, May 14, 2020 at 3:46 PM Christophe Leroy
wrote:
>
>
>
> Le 06/05/2020 à 05:40, Jordan Niethe a écrit :
> > Introduce a probe_user_read_inst() function to use in cases where
> > probe_user_read() is used for getting an instruction. This will be more
> > useful for prefixed instructions.
> >
> > Reviewed-by: Alistair Popple
> > Signed-off-by: Jordan Niethe
> > ---
> > v6: - New to series
> > ---
> > arch/powerpc/include/asm/inst.h | 3 +++
> > arch/powerpc/lib/Makefile | 2 +-
> > arch/powerpc/lib/inst.c | 18 ++
> > arch/powerpc/mm/fault.c | 2 +-
> > 4 files changed, 23 insertions(+), 2 deletions(-)
> > create mode 100644 arch/powerpc/lib/inst.c
> >
> > diff --git a/arch/powerpc/include/asm/inst.h
> > b/arch/powerpc/include/asm/inst.h
> > index 552e953bf04f..3e9a58420151 100644
> > --- a/arch/powerpc/include/asm/inst.h
> > +++ b/arch/powerpc/include/asm/inst.h
> > @@ -37,4 +37,7 @@ static inline bool ppc_inst_equal(struct ppc_inst x,
> > struct ppc_inst y)
> > return ppc_inst_val(x) == ppc_inst_val(y);
> > }
> >
> > +int probe_user_read_inst(struct ppc_inst *inst,
> > + struct ppc_inst *nip);
> > +
> > #endif /* _ASM_INST_H */
> > diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
> > index b8de3be10eb4..546591848219 100644
> > --- a/arch/powerpc/lib/Makefile
> > +++ b/arch/powerpc/lib/Makefile
> > @@ -16,7 +16,7 @@ CFLAGS_code-patching.o += -DDISABLE_BRANCH_PROFILING
> > CFLAGS_feature-fixups.o += -DDISABLE_BRANCH_PROFILING
> > endif
> >
> > -obj-y += alloc.o code-patching.o feature-fixups.o pmem.o
> > +obj-y += alloc.o code-patching.o feature-fixups.o pmem.o inst.o
> >
> > ifndef CONFIG_KASAN
> > obj-y += string.o memcmp_$(BITS).o
> > diff --git a/arch/powerpc/lib/inst.c b/arch/powerpc/lib/inst.c
> > new file mode 100644
> > index ..eaf786afad2b
> > --- /dev/null
> > +++ b/arch/powerpc/lib/inst.c
> > @@ -0,0 +1,18 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright 2020, IBM Corporation.
> > + */
> > +
> > +#include
> > +#include
> > +
> > +int probe_user_read_inst(struct ppc_inst *inst,
> > + struct ppc_inst *nip)
> > +{
> > + unsigned int val;
> > + int err;
> > +
> > + err = probe_user_read(, nip, sizeof(val));
> > + *inst = ppc_inst(val);
> > + return err;
> > +}
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index 4a50f125ec18..f3a943eae305 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -281,7 +281,7 @@ static bool bad_stack_expansion(struct pt_regs *regs,
> > unsigned long address,
> > access_ok(nip, sizeof(*nip))) {
> > struct ppc_inst inst;
> >
> > - if (!probe_user_read(, nip, sizeof(inst)))
> > + if (!probe_user_read_inst(, (struct ppc_inst
> > __user *)nip))
>
> Shouldn't 'nip' become de 'struct ppc_inst __user *' instead of casting ?
>
> > return !store_updates_sp(inst);
> > *must_retry = true;
> > }
> >
Yeah it would make more sense to do it like this.
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -256,7 +256,7 @@ static bool bad_stack_expansion(struct pt_regs
*regs, unsigned long address,
* expand to 1MB without further checks.
*/
if (address + 0x10 < vma->vm_end) {
-unsigned int __user *nip = (unsigned int __user *)regs->nip;
+struct ppc_inst __user *nip = (struct ppc_inst __user *)regs->nip;
/* get user regs even if this fault is in kernel mode */
struct pt_regs *uregs = current->thread.regs;
if (uregs == NULL)
@@ -281,7 +281,7 @@ static bool bad_stack_expansion(struct pt_regs
*regs, unsigned long address,
access_ok(nip, sizeof(*nip))) {
struct ppc_inst inst;
-if (!probe_user_read_inst(, (struct ppc_inst __user *)nip))
+if (!probe_user_read_inst(, nip))
return !store_updates_sp(inst);
*must_retry = true;
}
--
2.17.1
>
> Christophe
Re: [PATCH v8 13/30] powerpc: Add a probe_user_read_inst() function
Le 06/05/2020 à 05:40, Jordan Niethe a écrit :
Introduce a probe_user_read_inst() function to use in cases where
probe_user_read() is used for getting an instruction. This will be more
useful for prefixed instructions.
Reviewed-by: Alistair Popple
Signed-off-by: Jordan Niethe
---
v6: - New to series
---
arch/powerpc/include/asm/inst.h | 3 +++
arch/powerpc/lib/Makefile | 2 +-
arch/powerpc/lib/inst.c | 18 ++
arch/powerpc/mm/fault.c | 2 +-
4 files changed, 23 insertions(+), 2 deletions(-)
create mode 100644 arch/powerpc/lib/inst.c
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
index 552e953bf04f..3e9a58420151 100644
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h
@@ -37,4 +37,7 @@ static inline bool ppc_inst_equal(struct ppc_inst x, struct
ppc_inst y)
return ppc_inst_val(x) == ppc_inst_val(y);
}
+int probe_user_read_inst(struct ppc_inst *inst,
+struct ppc_inst *nip);
+
#endif /* _ASM_INST_H */
diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
index b8de3be10eb4..546591848219 100644
--- a/arch/powerpc/lib/Makefile
+++ b/arch/powerpc/lib/Makefile
@@ -16,7 +16,7 @@ CFLAGS_code-patching.o += -DDISABLE_BRANCH_PROFILING
CFLAGS_feature-fixups.o += -DDISABLE_BRANCH_PROFILING
endif
-obj-y += alloc.o code-patching.o feature-fixups.o pmem.o
+obj-y += alloc.o code-patching.o feature-fixups.o pmem.o inst.o
ifndef CONFIG_KASAN
obj-y += string.o memcmp_$(BITS).o
diff --git a/arch/powerpc/lib/inst.c b/arch/powerpc/lib/inst.c
new file mode 100644
index ..eaf786afad2b
--- /dev/null
+++ b/arch/powerpc/lib/inst.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2020, IBM Corporation.
+ */
+
+#include
+#include
+
+int probe_user_read_inst(struct ppc_inst *inst,
+struct ppc_inst *nip)
+{
+ unsigned int val;
+ int err;
+
+ err = probe_user_read(, nip, sizeof(val));
+ *inst = ppc_inst(val);
+ return err;
+}
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 4a50f125ec18..f3a943eae305 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -281,7 +281,7 @@ static bool bad_stack_expansion(struct pt_regs *regs,
unsigned long address,
access_ok(nip, sizeof(*nip))) {
struct ppc_inst inst;
- if (!probe_user_read(, nip, sizeof(inst)))
+ if (!probe_user_read_inst(, (struct ppc_inst
__user *)nip))
Shouldn't 'nip' become de 'struct ppc_inst __user *' instead of casting ?
return !store_updates_sp(inst);
*must_retry = true;
}
Christophe
Re: [PATCH v8 13/30] powerpc: Add a probe_user_read_inst() function
On Wed, May 13, 2020 at 10:52 PM Michael Ellerman wrote:
>
> Jordan Niethe writes:
> > diff --git a/arch/powerpc/lib/inst.c b/arch/powerpc/lib/inst.c
> > new file mode 100644
> > index ..eaf786afad2b
> > --- /dev/null
> > +++ b/arch/powerpc/lib/inst.c
> > @@ -0,0 +1,18 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright 2020, IBM Corporation.
> > + */
> > +
> > +#include
> > +#include
> > +
> > +int probe_user_read_inst(struct ppc_inst *inst,
> > + struct ppc_inst *nip)
> > +{
> > + unsigned int val;
> > + int err;
> > +
> > + err = probe_user_read(, nip, sizeof(val));
> > + *inst = ppc_inst(val);
>
> We shouldn't be storing to *inst if the read failed?
Good point.
>
> I changed it to:
>
> + if (!err)
> + *inst = ppc_inst(val);
> +
>
> Similarly for probe_kernel_read_inst().
Thanks.
>
> cheers
Re: [PATCH v8 13/30] powerpc: Add a probe_user_read_inst() function
Jordan Niethe writes:
> diff --git a/arch/powerpc/lib/inst.c b/arch/powerpc/lib/inst.c
> new file mode 100644
> index ..eaf786afad2b
> --- /dev/null
> +++ b/arch/powerpc/lib/inst.c
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2020, IBM Corporation.
> + */
> +
> +#include
> +#include
> +
> +int probe_user_read_inst(struct ppc_inst *inst,
> + struct ppc_inst *nip)
> +{
> + unsigned int val;
> + int err;
> +
> + err = probe_user_read(, nip, sizeof(val));
> + *inst = ppc_inst(val);
We shouldn't be storing to *inst if the read failed?
I changed it to:
+ if (!err)
+ *inst = ppc_inst(val);
+
Similarly for probe_kernel_read_inst().
cheers
