| Message ID | 20170628145557.24454-1-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 28, 2017 at 04:55:57PM +0200, Luc Van Oostenryck wrote: > get_alt_insn() is used to read and create ARM instructions, which > are always stored in memory in little-endian order. These values > are thus correctly converted to/from native order when processed > but the pointers used to hold the address of these instructions > are declared as for native order values. > > Fix this by declaring the pointers as __le32* instead of u32* and > make the few appropriate needed changes. > > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > --- > arch/arm64/kernel/alternative.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c > index 8840c109c..56bfda8cb 100644 > --- a/arch/arm64/kernel/alternative.c > +++ b/arch/arm64/kernel/alternative.c > @@ -60,7 +60,7 @@ static bool branch_insn_requires_update(struct alt_instr *alt, unsigned long pc) > > #define align_down(x, a) ((unsigned long)(x) & ~(((unsigned long)(a)) - 1)) > > -static u32 get_alt_insn(struct alt_instr *alt, u32 *insnptr, u32 *altinsnptr) > +static u32 get_alt_insn(struct alt_instr *alt, __le32 *insnptr, __le32 *altinsnptr) > { > u32 insn; > > @@ -109,7 +109,7 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias) > { > struct alt_instr *alt; > struct alt_region *region = alt_region; > - u32 *origptr, *replptr, *updptr; > + __le32 *origptr, *replptr, *updptr; > > for (alt = region->begin; alt < region->end; alt++) { > u32 insn; > @@ -122,9 +122,9 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias) > > pr_info_once("patching kernel code\n"); > > - origptr = ALT_ORIG_PTR(alt); > - replptr = ALT_REPL_PTR(alt); > - updptr = use_linear_alias ? (u32 *)lm_alias(origptr) : origptr; > + origptr = (__le32 __force *) ALT_ORIG_PTR(alt); > + replptr = (__le32 __force *) ALT_REPL_PTR(alt); Why is the __force needed here? Will
On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote: > On Wed, Jun 28, 2017 at 04:55:57PM +0200, Luc Van Oostenryck wrote: > > get_alt_insn() is used to read and create ARM instructions, which > > are always stored in memory in little-endian order. These values > > are thus correctly converted to/from native order when processed > > but the pointers used to hold the address of these instructions > > are declared as for native order values. > > > > Fix this by declaring the pointers as __le32* instead of u32* and > > make the few appropriate needed changes. > > > > + origptr = (__le32 __force *) ALT_ORIG_PTR(alt); > > + replptr = (__le32 __force *) ALT_REPL_PTR(alt); > > Why is the __force needed here? Because of the cast to u32* in: #define ALT_ORIG_PTR(a) __ALT_PTR(a, orig_offset) #define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset) #define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) Of course, if this (u32*) is not really needed, then the __force is also not needed. And since, it seems indeed to be the case, I'll gladly sent a patch: -#define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) +#define __ALT_PTR(a,f) ((void *)&(a)->f + (a)->f) if it suits you. -- Luc
On Thu, Jun 29, 2017 at 03:26:47PM +0200, Luc Van Oostenryck wrote: > On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote: > > On Wed, Jun 28, 2017 at 04:55:57PM +0200, Luc Van Oostenryck wrote: > > > get_alt_insn() is used to read and create ARM instructions, which > > > are always stored in memory in little-endian order. These values > > > are thus correctly converted to/from native order when processed > > > but the pointers used to hold the address of these instructions > > > are declared as for native order values. > > > > > > Fix this by declaring the pointers as __le32* instead of u32* and > > > make the few appropriate needed changes. > > > > > > + origptr = (__le32 __force *) ALT_ORIG_PTR(alt); > > > + replptr = (__le32 __force *) ALT_REPL_PTR(alt); > > > > Why is the __force needed here? > > Because of the cast to u32* in: > #define ALT_ORIG_PTR(a) __ALT_PTR(a, orig_offset) > #define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset) > #define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) > > Of course, if this (u32*) is not really needed, then the __force > is also not needed. > > And since, it seems indeed to be the case, I'll gladly sent a patch: > -#define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) > +#define __ALT_PTR(a,f) ((void *)&(a)->f + (a)->f) > if it suits you. Yes, please! Will
On Thu, Jun 29, 2017 at 03:26:47PM +0200, Luc Van Oostenryck wrote: > On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote: > > On Wed, Jun 28, 2017 at 04:55:57PM +0200, Luc Van Oostenryck wrote: > > > get_alt_insn() is used to read and create ARM instructions, which > > > are always stored in memory in little-endian order. These values > > > are thus correctly converted to/from native order when processed > > > but the pointers used to hold the address of these instructions > > > are declared as for native order values. > > > > > > Fix this by declaring the pointers as __le32* instead of u32* and > > > make the few appropriate needed changes. > > > > > > + origptr = (__le32 __force *) ALT_ORIG_PTR(alt); > > > + replptr = (__le32 __force *) ALT_REPL_PTR(alt); > > > > Why is the __force needed here? > > Because of the cast to u32* in: > #define ALT_ORIG_PTR(a) __ALT_PTR(a, orig_offset) > #define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset) > #define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) > > Of course, if this (u32*) is not really needed, then the __force > is also not needed. > > And since, it seems indeed to be the case, I'll gladly sent a patch: > -#define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) > +#define __ALT_PTR(a,f) ((void *)&(a)->f + (a)->f) > if it suits you. Given __ALT_PTR is intended to give a pointer to A64 instructions, which are in le32 format, wouldn't it make more sense for __ALT_PTR to cast to __le32 * ? Thanks, Mark.
On Thu, Jun 29, 2017 at 03:13:23PM +0100, Mark Rutland wrote: > On Thu, Jun 29, 2017 at 03:26:47PM +0200, Luc Van Oostenryck wrote: > > On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote: > > > On Wed, Jun 28, 2017 at 04:55:57PM +0200, Luc Van Oostenryck wrote: > > > > get_alt_insn() is used to read and create ARM instructions, which > > > > are always stored in memory in little-endian order. These values > > > > are thus correctly converted to/from native order when processed > > > > but the pointers used to hold the address of these instructions > > > > are declared as for native order values. > > > > > > > > Fix this by declaring the pointers as __le32* instead of u32* and > > > > make the few appropriate needed changes. > > > > > > > > + origptr = (__le32 __force *) ALT_ORIG_PTR(alt); > > > > + replptr = (__le32 __force *) ALT_REPL_PTR(alt); > > > > > > Why is the __force needed here? > > > > Because of the cast to u32* in: > > #define ALT_ORIG_PTR(a) __ALT_PTR(a, orig_offset) > > #define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset) > > #define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) > > > > Of course, if this (u32*) is not really needed, then the __force > > is also not needed. > > > > And since, it seems indeed to be the case, I'll gladly sent a patch: > > -#define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) > > +#define __ALT_PTR(a,f) ((void *)&(a)->f + (a)->f) > > if it suits you. > > Given __ALT_PTR is intended to give a pointer to A64 instructions, which > are in le32 format, wouldn't it make more sense for __ALT_PTR to cast to > __le32 * ? Might be a bit weird for ALT_REPL_PTR, which is cast to unsigned long. Will
On Thu, Jun 29, 2017 at 03:19:44PM +0100, Will Deacon wrote: > On Thu, Jun 29, 2017 at 03:13:23PM +0100, Mark Rutland wrote: > > On Thu, Jun 29, 2017 at 03:26:47PM +0200, Luc Van Oostenryck wrote: > > > On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote: > > > > On Wed, Jun 28, 2017 at 04:55:57PM +0200, Luc Van Oostenryck wrote: > > > > > get_alt_insn() is used to read and create ARM instructions, which > > > > > are always stored in memory in little-endian order. These values > > > > > are thus correctly converted to/from native order when processed > > > > > but the pointers used to hold the address of these instructions > > > > > are declared as for native order values. > > > > > > > > > > Fix this by declaring the pointers as __le32* instead of u32* and > > > > > make the few appropriate needed changes. > > > > > > > > > > + origptr = (__le32 __force *) ALT_ORIG_PTR(alt); > > > > > + replptr = (__le32 __force *) ALT_REPL_PTR(alt); > > > > > > > > Why is the __force needed here? > > > > > > Because of the cast to u32* in: > > > #define ALT_ORIG_PTR(a) __ALT_PTR(a, orig_offset) > > > #define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset) > > > #define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) > > > > > > Of course, if this (u32*) is not really needed, then the __force > > > is also not needed. > > > > > > And since, it seems indeed to be the case, I'll gladly sent a patch: > > > -#define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) > > > +#define __ALT_PTR(a,f) ((void *)&(a)->f + (a)->f) > > > if it suits you. > > > > Given __ALT_PTR is intended to give a pointer to A64 instructions, which > > are in le32 format, wouldn't it make more sense for __ALT_PTR to cast to > > __le32 * ? > > Might be a bit weird for ALT_REPL_PTR, which is cast to unsigned long. Maybe, but that's one cast, rather than two, and matches other similar casts from a pointer to unsigned long (e.g. the the addr cast in __range_ok()). Thanks, Mark.
On Thu, Jun 29, 2017 at 03:22:34PM +0100, Mark Rutland wrote: > On Thu, Jun 29, 2017 at 03:19:44PM +0100, Will Deacon wrote: > > On Thu, Jun 29, 2017 at 03:13:23PM +0100, Mark Rutland wrote: > > > On Thu, Jun 29, 2017 at 03:26:47PM +0200, Luc Van Oostenryck wrote: > > > > On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote: > > > > > On Wed, Jun 28, 2017 at 04:55:57PM +0200, Luc Van Oostenryck wrote: > > > > > > get_alt_insn() is used to read and create ARM instructions, which > > > > > > are always stored in memory in little-endian order. These values > > > > > > are thus correctly converted to/from native order when processed > > > > > > but the pointers used to hold the address of these instructions > > > > > > are declared as for native order values. > > > > > > > > > > > > Fix this by declaring the pointers as __le32* instead of u32* and > > > > > > make the few appropriate needed changes. > > > > > > > > > > > > + origptr = (__le32 __force *) ALT_ORIG_PTR(alt); > > > > > > + replptr = (__le32 __force *) ALT_REPL_PTR(alt); > > > > > > > > > > Why is the __force needed here? > > > > > > > > Because of the cast to u32* in: > > > > #define ALT_ORIG_PTR(a) __ALT_PTR(a, orig_offset) > > > > #define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset) > > > > #define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) > > > > > > > > Of course, if this (u32*) is not really needed, then the __force > > > > is also not needed. > > > > > > > > And since, it seems indeed to be the case, I'll gladly sent a patch: > > > > -#define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) > > > > +#define __ALT_PTR(a,f) ((void *)&(a)->f + (a)->f) > > > > if it suits you. > > > > > > Given __ALT_PTR is intended to give a pointer to A64 instructions, which > > > are in le32 format, wouldn't it make more sense for __ALT_PTR to cast to > > > __le32 * ? > > > > Might be a bit weird for ALT_REPL_PTR, which is cast to unsigned long. > > Maybe, but that's one cast, rather than two, and matches other similar > casts from a pointer to unsigned long (e.g. the the addr cast in > __range_ok()). Sure, but isn't it a __force cast? I'd like to avoid that if we can. Will
On Thu, Jun 29, 2017 at 4:26 PM, Will Deacon <will.deacon@arm.com> wrote: > On Thu, Jun 29, 2017 at 03:22:34PM +0100, Mark Rutland wrote: >> On Thu, Jun 29, 2017 at 03:19:44PM +0100, Will Deacon wrote: >> > On Thu, Jun 29, 2017 at 03:13:23PM +0100, Mark Rutland wrote: >> > > On Thu, Jun 29, 2017 at 03:26:47PM +0200, Luc Van Oostenryck wrote: >> > > > On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote: >> > > > > > + origptr = (__le32 __force *) ALT_ORIG_PTR(alt); >> > > > > > + replptr = (__le32 __force *) ALT_REPL_PTR(alt); >> > > > > >> > > > > Why is the __force needed here? >> > > > >> > > > Because of the cast to u32* in: >> > > > #define ALT_ORIG_PTR(a) __ALT_PTR(a, orig_offset) >> > > > #define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset) >> > > > #define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) >> > > > >> > > > Of course, if this (u32*) is not really needed, then the __force >> > > > is also not needed. >> > > > >> > > > And since, it seems indeed to be the case, I'll gladly sent a patch: >> > > > -#define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) >> > > > +#define __ALT_PTR(a,f) ((void *)&(a)->f + (a)->f) >> > > > if it suits you. >> > > >> > > Given __ALT_PTR is intended to give a pointer to A64 instructions, which >> > > are in le32 format, wouldn't it make more sense for __ALT_PTR to cast to >> > > __le32 * ? Yes, it could. >> > Might be a bit weird for ALT_REPL_PTR, which is cast to unsigned long. >> >> Maybe, but that's one cast, rather than two, and matches other similar >> casts from a pointer to unsigned long (e.g. the the addr cast in >> __range_ok()). > > Sure, but isn't it a __force cast? I'd like to avoid that if we can. No, once you cast to an integer all the specificities of pointers are thrown away, like for a const pointer and here the __bitwise underlying the __le32. It's exactly the same fro cast from/to void*, the reasoning behind it is something like : "it's OK to drop all the qualifiers and such because you will anyway need to cast the result to another pointer before being able to dereference it". I already sent a version leaving the type as void*, without having seen your replies here, but if you wish I can, of course, resend something for __le32*. -- Luc
On Thu, Jun 29, 2017 at 05:15:19PM +0200, Luc Van Oostenryck wrote: > On Thu, Jun 29, 2017 at 4:26 PM, Will Deacon <will.deacon@arm.com> wrote: > > On Thu, Jun 29, 2017 at 03:22:34PM +0100, Mark Rutland wrote: > >> On Thu, Jun 29, 2017 at 03:19:44PM +0100, Will Deacon wrote: > >> > On Thu, Jun 29, 2017 at 03:13:23PM +0100, Mark Rutland wrote: > >> > > On Thu, Jun 29, 2017 at 03:26:47PM +0200, Luc Van Oostenryck wrote: > >> > > > On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote: > >> > > > > > + origptr = (__le32 __force *) ALT_ORIG_PTR(alt); > >> > > > > > + replptr = (__le32 __force *) ALT_REPL_PTR(alt); > >> > > > > > >> > > > > Why is the __force needed here? > >> > > > > >> > > > Because of the cast to u32* in: > >> > > > #define ALT_ORIG_PTR(a) __ALT_PTR(a, orig_offset) > >> > > > #define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset) > >> > > > #define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) > >> > > > > >> > > > Of course, if this (u32*) is not really needed, then the __force > >> > > > is also not needed. > >> > > > > >> > > > And since, it seems indeed to be the case, I'll gladly sent a patch: > >> > > > -#define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) > >> > > > +#define __ALT_PTR(a,f) ((void *)&(a)->f + (a)->f) > >> > > > if it suits you. > >> > > > >> > > Given __ALT_PTR is intended to give a pointer to A64 instructions, which > >> > > are in le32 format, wouldn't it make more sense for __ALT_PTR to cast to > >> > > __le32 * ? > > Yes, it could. > > >> > Might be a bit weird for ALT_REPL_PTR, which is cast to unsigned long. > >> > >> Maybe, but that's one cast, rather than two, and matches other similar > >> casts from a pointer to unsigned long (e.g. the the addr cast in > >> __range_ok()). > > > > Sure, but isn't it a __force cast? I'd like to avoid that if we can. > > No, once you cast to an integer all the specificities of pointers are > thrown away, > like for a const pointer and here the __bitwise underlying the __le32. > It's exactly the same fro cast from/to void*, the reasoning behind it > is something > like : "it's OK to drop all the qualifiers and such because you will anyway need > to cast the result to another pointer before being able to dereference it". Ok, so does that mean that the __force cast in __range_ok is not needed? > I already sent a version leaving the type as void*, without having seen your > replies here, but if you wish I can, of course, resend something for __le32*. I'm happy with the patch you sent, so no need to resend. Will
On Thu, Jun 29, 2017 at 04:20:17PM +0100, Will Deacon wrote: > On Thu, Jun 29, 2017 at 05:15:19PM +0200, Luc Van Oostenryck wrote: > > On Thu, Jun 29, 2017 at 4:26 PM, Will Deacon <will.deacon@arm.com> wrote: > > > On Thu, Jun 29, 2017 at 03:22:34PM +0100, Mark Rutland wrote: > > >> On Thu, Jun 29, 2017 at 03:19:44PM +0100, Will Deacon wrote: > > >> > On Thu, Jun 29, 2017 at 03:13:23PM +0100, Mark Rutland wrote: > > >> > > On Thu, Jun 29, 2017 at 03:26:47PM +0200, Luc Van Oostenryck wrote: > > >> > > > On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote: > > >> > > > > > + origptr = (__le32 __force *) ALT_ORIG_PTR(alt); > > >> > > > > > + replptr = (__le32 __force *) ALT_REPL_PTR(alt); > > >> > > > > > > >> > > > > Why is the __force needed here? > > >> > > > > > >> > > > Because of the cast to u32* in: > > >> > > > #define ALT_ORIG_PTR(a) __ALT_PTR(a, orig_offset) > > >> > > > #define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset) > > >> > > > #define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) > > >> > > > > > >> > > > Of course, if this (u32*) is not really needed, then the __force > > >> > > > is also not needed. > > >> > > > > > >> > > > And since, it seems indeed to be the case, I'll gladly sent a patch: > > >> > > > -#define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f) > > >> > > > +#define __ALT_PTR(a,f) ((void *)&(a)->f + (a)->f) > > >> > > > if it suits you. > > >> > > > > >> > > Given __ALT_PTR is intended to give a pointer to A64 instructions, which > > >> > > are in le32 format, wouldn't it make more sense for __ALT_PTR to cast to > > >> > > __le32 * ? > > > > Yes, it could. > > > > >> > Might be a bit weird for ALT_REPL_PTR, which is cast to unsigned long. > > >> > > >> Maybe, but that's one cast, rather than two, and matches other similar > > >> casts from a pointer to unsigned long (e.g. the the addr cast in > > >> __range_ok()). > > > > > > Sure, but isn't it a __force cast? I'd like to avoid that if we can. > > > > No, once you cast to an integer all the specificities of pointers are > > thrown away, > > like for a const pointer and here the __bitwise underlying the __le32. > > It's exactly the same fro cast from/to void*, the reasoning behind it > > is something > > like : "it's OK to drop all the qualifiers and such because you will anyway need > > to cast the result to another pointer before being able to dereference it". > > Ok, so does that mean that the __force cast in __range_ok is not needed? Indeed. And I just verified this with a quick check with sparse only on arch/arm64/ with and without the __force. > > I already sent a version leaving the type as void*, without having seen your > > replies here, but if you wish I can, of course, resend something for __le32*. > > I'm happy with the patch you sent, so no need to resend. OK. -- Luc
diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c index 8840c109c..56bfda8cb 100644 --- a/arch/arm64/kernel/alternative.c +++ b/arch/arm64/kernel/alternative.c @@ -60,7 +60,7 @@ static bool branch_insn_requires_update(struct alt_instr *alt, unsigned long pc) #define align_down(x, a) ((unsigned long)(x) & ~(((unsigned long)(a)) - 1)) -static u32 get_alt_insn(struct alt_instr *alt, u32 *insnptr, u32 *altinsnptr) +static u32 get_alt_insn(struct alt_instr *alt, __le32 *insnptr, __le32 *altinsnptr) { u32 insn; @@ -109,7 +109,7 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias) { struct alt_instr *alt; struct alt_region *region = alt_region; - u32 *origptr, *replptr, *updptr; + __le32 *origptr, *replptr, *updptr; for (alt = region->begin; alt < region->end; alt++) { u32 insn; @@ -122,9 +122,9 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias) pr_info_once("patching kernel code\n"); - origptr = ALT_ORIG_PTR(alt); - replptr = ALT_REPL_PTR(alt); - updptr = use_linear_alias ? (u32 *)lm_alias(origptr) : origptr; + origptr = (__le32 __force *) ALT_ORIG_PTR(alt); + replptr = (__le32 __force *) ALT_REPL_PTR(alt); + updptr = use_linear_alias ? (__le32 *)lm_alias(origptr) : origptr; nr_inst = alt->alt_len / sizeof(insn); for (i = 0; i < nr_inst; i++) {
get_alt_insn() is used to read and create ARM instructions, which are always stored in memory in little-endian order. These values are thus correctly converted to/from native order when processed but the pointers used to hold the address of these instructions are declared as for native order values. Fix this by declaring the pointers as __le32* instead of u32* and make the few appropriate needed changes. Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- arch/arm64/kernel/alternative.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)