From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoffer Dall Subject: Re: [PATCH v4 04/19] arm64: alternatives: Enforce alignment of struct alt_instr Date: Mon, 15 Jan 2018 10:11:10 +0100 Message-ID: <20180115091110.GE21403@cbox> References: <20180104184334.16571-1-marc.zyngier@arm.com> <20180104184334.16571-5-marc.zyngier@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Catalin Marinas , Will Deacon , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org To: Marc Zyngier Return-path: Content-Disposition: inline In-Reply-To: <20180104184334.16571-5-marc.zyngier@arm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu List-Id: kvm.vger.kernel.org On Thu, Jan 04, 2018 at 06:43:19PM +0000, Marc Zyngier wrote: > We're playing a dangerous game with struct alt_instr, as we produce > it using assembly tricks, but parse them using the C structure. > We just assume that the respective alignments of the two will > be the same. > > But as we add more fields to this structure, the alignment requirements > of the structure may change, and lead to all kind of funky bugs. > > TO solve this, let's move the definition of struct alt_instr to its > own file, and use this to generate the alignment constraint from > asm-offsets.c. The various macros are then patched to take the > alignment into account. > > Signed-off-by: Marc Zyngier > --- > arch/arm64/include/asm/alternative.h | 13 +++++-------- > arch/arm64/include/asm/alternative_types.h | 13 +++++++++++++ > arch/arm64/kernel/asm-offsets.c | 4 ++++ > 3 files changed, 22 insertions(+), 8 deletions(-) > create mode 100644 arch/arm64/include/asm/alternative_types.h > > diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h > index 4a85c6952a22..395befde7595 100644 > --- a/arch/arm64/include/asm/alternative.h > +++ b/arch/arm64/include/asm/alternative.h > @@ -2,28 +2,24 @@ > #ifndef __ASM_ALTERNATIVE_H > #define __ASM_ALTERNATIVE_H > > +#include > #include > #include > > #ifndef __ASSEMBLY__ > > +#include > + > #include > #include > #include > #include > > -struct alt_instr { > - s32 orig_offset; /* offset to original instruction */ > - s32 alt_offset; /* offset to replacement instruction */ > - u16 cpufeature; /* cpufeature bit set for replacement */ > - u8 orig_len; /* size of original instruction(s) */ > - u8 alt_len; /* size of new instruction(s), <= orig_len */ > -}; > - > void __init apply_alternatives_all(void); > void apply_alternatives(void *start, size_t length); > > #define ALTINSTR_ENTRY(feature) \ > + " .align " __stringify(ALTINSTR_ALIGN) "\n" \ > " .word 661b - .\n" /* label */ \ > " .word 663f - .\n" /* new instruction */ \ > " .hword " __stringify(feature) "\n" /* feature bit */ \ > @@ -69,6 +65,7 @@ void apply_alternatives(void *start, size_t length); > #include > > .macro altinstruction_entry orig_offset alt_offset feature orig_len alt_len > + .align ALTINSTR_ALIGN > .word \orig_offset - . > .word \alt_offset - . > .hword \feature I don't understand much of the magics in this file, but... > diff --git a/arch/arm64/include/asm/alternative_types.h b/arch/arm64/include/asm/alternative_types.h > new file mode 100644 > index 000000000000..26cf76167f2d > --- /dev/null > +++ b/arch/arm64/include/asm/alternative_types.h > @@ -0,0 +1,13 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __ASM_ALTERNATIVE_TYPES_H > +#define __ASM_ALTERNATIVE_TYPES_H > + > +struct alt_instr { > + s32 orig_offset; /* offset to original instruction */ > + s32 alt_offset; /* offset to replacement instruction */ > + u16 cpufeature; /* cpufeature bit set for replacement */ > + u8 orig_len; /* size of original instruction(s) */ > + u8 alt_len; /* size of new instruction(s), <= orig_len */ > +}; > + > +#endif > diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c > index 5ab8841af382..f00666341ae2 100644 > --- a/arch/arm64/kernel/asm-offsets.c > +++ b/arch/arm64/kernel/asm-offsets.c > @@ -25,6 +25,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -151,5 +152,8 @@ int main(void) > DEFINE(HIBERN_PBE_ADDR, offsetof(struct pbe, address)); > DEFINE(HIBERN_PBE_NEXT, offsetof(struct pbe, next)); > DEFINE(ARM64_FTR_SYSVAL, offsetof(struct arm64_ftr_reg, sys_val)); > + BLANK(); > + DEFINE(ALTINSTR_ALIGN, (63 - __builtin_clzl(__alignof__(struct alt_instr)))); > + > return 0; > } > -- > 2.14.2 > ... the rest looks correct to me. FWIW: Reviewed-by: Christoffer Dall From mboxrd@z Thu Jan 1 00:00:00 1970 From: christoffer.dall@linaro.org (Christoffer Dall) Date: Mon, 15 Jan 2018 10:11:10 +0100 Subject: [PATCH v4 04/19] arm64: alternatives: Enforce alignment of struct alt_instr In-Reply-To: <20180104184334.16571-5-marc.zyngier@arm.com> References: <20180104184334.16571-1-marc.zyngier@arm.com> <20180104184334.16571-5-marc.zyngier@arm.com> Message-ID: <20180115091110.GE21403@cbox> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, Jan 04, 2018 at 06:43:19PM +0000, Marc Zyngier wrote: > We're playing a dangerous game with struct alt_instr, as we produce > it using assembly tricks, but parse them using the C structure. > We just assume that the respective alignments of the two will > be the same. > > But as we add more fields to this structure, the alignment requirements > of the structure may change, and lead to all kind of funky bugs. > > TO solve this, let's move the definition of struct alt_instr to its > own file, and use this to generate the alignment constraint from > asm-offsets.c. The various macros are then patched to take the > alignment into account. > > Signed-off-by: Marc Zyngier > --- > arch/arm64/include/asm/alternative.h | 13 +++++-------- > arch/arm64/include/asm/alternative_types.h | 13 +++++++++++++ > arch/arm64/kernel/asm-offsets.c | 4 ++++ > 3 files changed, 22 insertions(+), 8 deletions(-) > create mode 100644 arch/arm64/include/asm/alternative_types.h > > diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h > index 4a85c6952a22..395befde7595 100644 > --- a/arch/arm64/include/asm/alternative.h > +++ b/arch/arm64/include/asm/alternative.h > @@ -2,28 +2,24 @@ > #ifndef __ASM_ALTERNATIVE_H > #define __ASM_ALTERNATIVE_H > > +#include > #include > #include > > #ifndef __ASSEMBLY__ > > +#include > + > #include > #include > #include > #include > > -struct alt_instr { > - s32 orig_offset; /* offset to original instruction */ > - s32 alt_offset; /* offset to replacement instruction */ > - u16 cpufeature; /* cpufeature bit set for replacement */ > - u8 orig_len; /* size of original instruction(s) */ > - u8 alt_len; /* size of new instruction(s), <= orig_len */ > -}; > - > void __init apply_alternatives_all(void); > void apply_alternatives(void *start, size_t length); > > #define ALTINSTR_ENTRY(feature) \ > + " .align " __stringify(ALTINSTR_ALIGN) "\n" \ > " .word 661b - .\n" /* label */ \ > " .word 663f - .\n" /* new instruction */ \ > " .hword " __stringify(feature) "\n" /* feature bit */ \ > @@ -69,6 +65,7 @@ void apply_alternatives(void *start, size_t length); > #include > > .macro altinstruction_entry orig_offset alt_offset feature orig_len alt_len > + .align ALTINSTR_ALIGN > .word \orig_offset - . > .word \alt_offset - . > .hword \feature I don't understand much of the magics in this file, but... > diff --git a/arch/arm64/include/asm/alternative_types.h b/arch/arm64/include/asm/alternative_types.h > new file mode 100644 > index 000000000000..26cf76167f2d > --- /dev/null > +++ b/arch/arm64/include/asm/alternative_types.h > @@ -0,0 +1,13 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __ASM_ALTERNATIVE_TYPES_H > +#define __ASM_ALTERNATIVE_TYPES_H > + > +struct alt_instr { > + s32 orig_offset; /* offset to original instruction */ > + s32 alt_offset; /* offset to replacement instruction */ > + u16 cpufeature; /* cpufeature bit set for replacement */ > + u8 orig_len; /* size of original instruction(s) */ > + u8 alt_len; /* size of new instruction(s), <= orig_len */ > +}; > + > +#endif > diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c > index 5ab8841af382..f00666341ae2 100644 > --- a/arch/arm64/kernel/asm-offsets.c > +++ b/arch/arm64/kernel/asm-offsets.c > @@ -25,6 +25,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -151,5 +152,8 @@ int main(void) > DEFINE(HIBERN_PBE_ADDR, offsetof(struct pbe, address)); > DEFINE(HIBERN_PBE_NEXT, offsetof(struct pbe, next)); > DEFINE(ARM64_FTR_SYSVAL, offsetof(struct arm64_ftr_reg, sys_val)); > + BLANK(); > + DEFINE(ALTINSTR_ALIGN, (63 - __builtin_clzl(__alignof__(struct alt_instr)))); > + > return 0; > } > -- > 2.14.2 > ... the rest looks correct to me. FWIW: Reviewed-by: Christoffer Dall