| Message ID | 20191126052141.28009-19-jniethe5@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Initial Prefixed Instruction support | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (2ec2260ce7bce5eb6a8ced0bb78d75c1b3eca306) |
| snowpatch_ozlabs/build-ppc64le | warning | Build succeeded but added 2 new sparse warnings |
| snowpatch_ozlabs/build-ppc64be | warning | Build succeeded but added 2 new sparse warnings |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | warning | Build succeeded but added 2 new sparse warnings |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 1 checks, 62 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Jordan Niethe <jniethe5@gmail.com> writes: > A user-mode access to an address a long way below the stack pointer is > only valid if the instruction is one that would update the stack pointer > to the address accessed. This is checked by directly looking at the > instructions op-code. As a result is does not take into account prefixed > instructions. Instead of looking at the instruction our self, use > analyse_instr() determine if this a store instruction that will update > the stack pointer. > > Something to note is that there currently are not any store with update > prefixed instructions. Actually there is no plan for prefixed > update-form loads and stores. So this patch is probably not needed but > it might be preferable to use analyse_instr() rather than open coding > the test anyway. Yes please. I was looking through this code recently and was horrified. This improves things a lot and I think is justification enough as-is. Regards, Daniel > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > --- > arch/powerpc/mm/fault.c | 39 +++++++++++---------------------------- > 1 file changed, 11 insertions(+), 28 deletions(-) > > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c > index b5047f9b5dec..cb78b3ca1800 100644 > --- a/arch/powerpc/mm/fault.c > +++ b/arch/powerpc/mm/fault.c > @@ -41,37 +41,17 @@ > #include <asm/siginfo.h> > #include <asm/debug.h> > #include <asm/kup.h> > +#include <asm/sstep.h> > > /* > * Check whether the instruction inst is a store using > * an update addressing form which will update r1. > */ > -static bool store_updates_sp(unsigned int inst) > +static bool store_updates_sp(struct instruction_op *op) > { > - /* check for 1 in the rA field */ > - if (((inst >> 16) & 0x1f) != 1) > - return false; > - /* check major opcode */ > - switch (inst >> 26) { > - case OP_STWU: > - case OP_STBU: > - case OP_STHU: > - case OP_STFSU: > - case OP_STFDU: > - return true; > - case OP_STD: /* std or stdu */ > - return (inst & 3) == 1; > - case OP_31: > - /* check minor opcode */ > - switch ((inst >> 1) & 0x3ff) { > - case OP_31_XOP_STDUX: > - case OP_31_XOP_STWUX: > - case OP_31_XOP_STBUX: > - case OP_31_XOP_STHUX: > - case OP_31_XOP_STFSUX: > - case OP_31_XOP_STFDUX: > + if (GETTYPE(op->type) == STORE) { > + if ((op->type & UPDATE) && (op->update_reg == 1)) > return true; > - } > } > return false; > } > @@ -278,14 +258,17 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address, > > if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) && > access_ok(nip, sizeof(*nip))) { > - unsigned int inst; > + unsigned int inst, sufx; > + struct instruction_op op; > int res; > > pagefault_disable(); > - res = __get_user_inatomic(inst, nip); > + res = __get_user_instr_inatomic(inst, sufx, nip); > pagefault_enable(); > - if (!res) > - return !store_updates_sp(inst); > + if (!res) { > + analyse_instr(&op, uregs, inst, sufx); > + return !store_updates_sp(&op); > + } > *must_retry = true; > } > return true; > -- > 2.20.1
On Thu, 19 Dec 2019 01:11:33 +1100 Daniel Axtens <dja@axtens.net> wrote: > Jordan Niethe <jniethe5@gmail.com> writes: > > > A user-mode access to an address a long way below the stack pointer is > > only valid if the instruction is one that would update the stack pointer > > to the address accessed. This is checked by directly looking at the > > instructions op-code. As a result is does not take into account prefixed > > instructions. Instead of looking at the instruction our self, use > > analyse_instr() determine if this a store instruction that will update > > the stack pointer. > > > > Something to note is that there currently are not any store with update > > prefixed instructions. Actually there is no plan for prefixed > > update-form loads and stores. So this patch is probably not needed but > > it might be preferable to use analyse_instr() rather than open coding > > the test anyway. > > Yes please. I was looking through this code recently and was > horrified. This improves things a lot and I think is justification > enough as-is. > Except it doesn't work... I'm now experiencing a systematic crash of systemd at boot in my fedora31 guest: [ 3.322912] systemd[1]: segfault (11) at 7ffff3eaf550 nip 7ce4d42f8d78 lr 9d82c098fc0 code 1 in libsystemd-shared-243.so[7ce4d4150000+2e0000] [ 3.323112] systemd[1]: code: 00000480 60420000 3c4c001e 3842edb0 7c0802a6 3d81fff0 fb81ffe0 fba1ffe8 [ 3.323244] systemd[1]: code: fbc1fff0 fbe1fff8 f8010010 7c200b78 <f801f001> 7c216000 4082fff8 f801ff71 f801f001 is 0x1a8d78 <serialize_item_format+40>: stdu r0,-4096(r1) which analyse_instr() is supposed to decode as a STORE that updates r1 so we should be good... Unfortunately analyse_instr() forbids partial register sets, since it might return op->val based on some register content depending on the instruction: /* Following cases refer to regs->gpr[], so we need all regs */ if (!FULL_REGS(regs)) return -1; analyse_instr() was introduced with instruction emulation in mind, which goes far beyond the need we have in store_updates_sp(). Especially the fault path doesn't care for the register content at all... Not sure how to cope with that correctly (refactor analyse_instr() ? ) but until someone comes up with a solution, please don't merge this patch. Cheers, -- Greg > Regards, > Daniel > > > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > > --- > > arch/powerpc/mm/fault.c | 39 +++++++++++---------------------------- > > 1 file changed, 11 insertions(+), 28 deletions(-) > > > > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c > > index b5047f9b5dec..cb78b3ca1800 100644 > > --- a/arch/powerpc/mm/fault.c > > +++ b/arch/powerpc/mm/fault.c > > @@ -41,37 +41,17 @@ > > #include <asm/siginfo.h> > > #include <asm/debug.h> > > #include <asm/kup.h> > > +#include <asm/sstep.h> > > > > /* > > * Check whether the instruction inst is a store using > > * an update addressing form which will update r1. > > */ > > -static bool store_updates_sp(unsigned int inst) > > +static bool store_updates_sp(struct instruction_op *op) > > { > > - /* check for 1 in the rA field */ > > - if (((inst >> 16) & 0x1f) != 1) > > - return false; > > - /* check major opcode */ > > - switch (inst >> 26) { > > - case OP_STWU: > > - case OP_STBU: > > - case OP_STHU: > > - case OP_STFSU: > > - case OP_STFDU: > > - return true; > > - case OP_STD: /* std or stdu */ > > - return (inst & 3) == 1; > > - case OP_31: > > - /* check minor opcode */ > > - switch ((inst >> 1) & 0x3ff) { > > - case OP_31_XOP_STDUX: > > - case OP_31_XOP_STWUX: > > - case OP_31_XOP_STBUX: > > - case OP_31_XOP_STHUX: > > - case OP_31_XOP_STFSUX: > > - case OP_31_XOP_STFDUX: > > + if (GETTYPE(op->type) == STORE) { > > + if ((op->type & UPDATE) && (op->update_reg == 1)) > > return true; > > - } > > } > > return false; > > } > > @@ -278,14 +258,17 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address, > > > > if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) && > > access_ok(nip, sizeof(*nip))) { > > - unsigned int inst; > > + unsigned int inst, sufx; > > + struct instruction_op op; > > int res; > > > > pagefault_disable(); > > - res = __get_user_inatomic(inst, nip); > > + res = __get_user_instr_inatomic(inst, sufx, nip); > > pagefault_enable(); > > - if (!res) > > - return !store_updates_sp(inst); > > + if (!res) { > > + analyse_instr(&op, uregs, inst, sufx); > > + return !store_updates_sp(&op); > > + } > > *must_retry = true; > > } > > return true; > > -- > > 2.20.1
On Fri, Feb 7, 2020 at 7:16 PM Greg Kurz <groug@kaod.org> wrote: > > On Thu, 19 Dec 2019 01:11:33 +1100 > Daniel Axtens <dja@axtens.net> wrote: > > > Jordan Niethe <jniethe5@gmail.com> writes: > > > > > A user-mode access to an address a long way below the stack pointer is > > > only valid if the instruction is one that would update the stack pointer > > > to the address accessed. This is checked by directly looking at the > > > instructions op-code. As a result is does not take into account prefixed > > > instructions. Instead of looking at the instruction our self, use > > > analyse_instr() determine if this a store instruction that will update > > > the stack pointer. > > > > > > Something to note is that there currently are not any store with update > > > prefixed instructions. Actually there is no plan for prefixed > > > update-form loads and stores. So this patch is probably not needed but > > > it might be preferable to use analyse_instr() rather than open coding > > > the test anyway. > > > > Yes please. I was looking through this code recently and was > > horrified. This improves things a lot and I think is justification > > enough as-is. > > > > Except it doesn't work... I'm now experiencing a systematic crash of > systemd at boot in my fedora31 guest: > > [ 3.322912] systemd[1]: segfault (11) at 7ffff3eaf550 nip 7ce4d42f8d78 lr 9d82c098fc0 code 1 in libsystemd-shared-243.so[7ce4d4150000+2e0000] > [ 3.323112] systemd[1]: code: 00000480 60420000 3c4c001e 3842edb0 7c0802a6 3d81fff0 fb81ffe0 fba1ffe8 > [ 3.323244] systemd[1]: code: fbc1fff0 fbe1fff8 f8010010 7c200b78 <f801f001> 7c216000 4082fff8 f801ff71 > > f801f001 is > > 0x1a8d78 <serialize_item_format+40>: stdu r0,-4096(r1) > > which analyse_instr() is supposed to decode as a STORE that > updates r1 so we should be good... Unfortunately analyse_instr() > forbids partial register sets, since it might return op->val > based on some register content depending on the instruction: > > /* Following cases refer to regs->gpr[], so we need all regs */ > if (!FULL_REGS(regs)) > return -1; > > analyse_instr() was introduced with instruction emulation in mind, which > goes far beyond the need we have in store_updates_sp(). Especially the > fault path doesn't care for the register content at all... > > Not sure how to cope with that correctly (refactor analyse_instr() ? ) but > until someone comes up with a solution, please don't merge this patch. > > Cheers, > > -- > Greg Thank you this information. I agree analyse_instr() is overkill for the situation especially as there are no prefixed store-with-updates. I am going to drop this patch from the series. > > > Regards, > > Daniel > > > > > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > > > --- > > > arch/powerpc/mm/fault.c | 39 +++++++++++---------------------------- > > > 1 file changed, 11 insertions(+), 28 deletions(-) > > > > > > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c > > > index b5047f9b5dec..cb78b3ca1800 100644 > > > --- a/arch/powerpc/mm/fault.c > > > +++ b/arch/powerpc/mm/fault.c > > > @@ -41,37 +41,17 @@ > > > #include <asm/siginfo.h> > > > #include <asm/debug.h> > > > #include <asm/kup.h> > > > +#include <asm/sstep.h> > > > > > > /* > > > * Check whether the instruction inst is a store using > > > * an update addressing form which will update r1. > > > */ > > > -static bool store_updates_sp(unsigned int inst) > > > +static bool store_updates_sp(struct instruction_op *op) > > > { > > > - /* check for 1 in the rA field */ > > > - if (((inst >> 16) & 0x1f) != 1) > > > - return false; > > > - /* check major opcode */ > > > - switch (inst >> 26) { > > > - case OP_STWU: > > > - case OP_STBU: > > > - case OP_STHU: > > > - case OP_STFSU: > > > - case OP_STFDU: > > > - return true; > > > - case OP_STD: /* std or stdu */ > > > - return (inst & 3) == 1; > > > - case OP_31: > > > - /* check minor opcode */ > > > - switch ((inst >> 1) & 0x3ff) { > > > - case OP_31_XOP_STDUX: > > > - case OP_31_XOP_STWUX: > > > - case OP_31_XOP_STBUX: > > > - case OP_31_XOP_STHUX: > > > - case OP_31_XOP_STFSUX: > > > - case OP_31_XOP_STFDUX: > > > + if (GETTYPE(op->type) == STORE) { > > > + if ((op->type & UPDATE) && (op->update_reg == 1)) > > > return true; > > > - } > > > } > > > return false; > > > } > > > @@ -278,14 +258,17 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address, > > > > > > if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) && > > > access_ok(nip, sizeof(*nip))) { > > > - unsigned int inst; > > > + unsigned int inst, sufx; > > > + struct instruction_op op; > > > int res; > > > > > > pagefault_disable(); > > > - res = __get_user_inatomic(inst, nip); > > > + res = __get_user_instr_inatomic(inst, sufx, nip); > > > pagefault_enable(); > > > - if (!res) > > > - return !store_updates_sp(inst); > > > + if (!res) { > > > + analyse_instr(&op, uregs, inst, sufx); > > > + return !store_updates_sp(&op); > > > + } > > > *must_retry = true; > > > } > > > return true; > > > -- > > > 2.20.1 >
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c index b5047f9b5dec..cb78b3ca1800 100644 --- a/arch/powerpc/mm/fault.c +++ b/arch/powerpc/mm/fault.c @@ -41,37 +41,17 @@ #include <asm/siginfo.h> #include <asm/debug.h> #include <asm/kup.h> +#include <asm/sstep.h> /* * Check whether the instruction inst is a store using * an update addressing form which will update r1. */ -static bool store_updates_sp(unsigned int inst) +static bool store_updates_sp(struct instruction_op *op) { - /* check for 1 in the rA field */ - if (((inst >> 16) & 0x1f) != 1) - return false; - /* check major opcode */ - switch (inst >> 26) { - case OP_STWU: - case OP_STBU: - case OP_STHU: - case OP_STFSU: - case OP_STFDU: - return true; - case OP_STD: /* std or stdu */ - return (inst & 3) == 1; - case OP_31: - /* check minor opcode */ - switch ((inst >> 1) & 0x3ff) { - case OP_31_XOP_STDUX: - case OP_31_XOP_STWUX: - case OP_31_XOP_STBUX: - case OP_31_XOP_STHUX: - case OP_31_XOP_STFSUX: - case OP_31_XOP_STFDUX: + if (GETTYPE(op->type) == STORE) { + if ((op->type & UPDATE) && (op->update_reg == 1)) return true; - } } return false; } @@ -278,14 +258,17 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address, if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) && access_ok(nip, sizeof(*nip))) { - unsigned int inst; + unsigned int inst, sufx; + struct instruction_op op; int res; pagefault_disable(); - res = __get_user_inatomic(inst, nip); + res = __get_user_instr_inatomic(inst, sufx, nip); pagefault_enable(); - if (!res) - return !store_updates_sp(inst); + if (!res) { + analyse_instr(&op, uregs, inst, sufx); + return !store_updates_sp(&op); + } *must_retry = true; } return true;
A user-mode access to an address a long way below the stack pointer is only valid if the instruction is one that would update the stack pointer to the address accessed. This is checked by directly looking at the instructions op-code. As a result is does not take into account prefixed instructions. Instead of looking at the instruction our self, use analyse_instr() determine if this a store instruction that will update the stack pointer. Something to note is that there currently are not any store with update prefixed instructions. Actually there is no plan for prefixed update-form loads and stores. So this patch is probably not needed but it might be preferable to use analyse_instr() rather than open coding the test anyway. Signed-off-by: Jordan Niethe <jniethe5@gmail.com> --- arch/powerpc/mm/fault.c | 39 +++++++++++---------------------------- 1 file changed, 11 insertions(+), 28 deletions(-)