| Message ID | 1472132255-23470-20-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote: > On most architectures it does not matter what the aligment is. > > On ARM32 it is paramount that the aligment is word-size (4) > otherwise we get a Data Abort when trying to perform ELF > relocations. That is due to ARM 32 only being able to write to > word-aligned addresses. That's not exactly true, afaik: ARM can write to byte- and half-word-aligned addresses, but only bytes/half-words. > --- a/xen/common/livepatch_elf.c > +++ b/xen/common/livepatch_elf.c > @@ -71,7 +71,15 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data) > delta = elf->hdr->e_shoff + i * elf->hdr->e_shentsize; > > sec[i].sec = data + delta; > - > + /* > + * Some architectures REQUIRE section alignment to be word-size. > + */ This is a single line comment. > + if ( sec[i].sec->sh_addralign % sizeof(uint32_t) ) Hmm, word size for ARM64 and x86-64 ought to be 8 bytes. Also you don't cover sec[i].sec->sh_addralign being zero (in fact any non-power- of-2 value would seem bogus to me). And then - why does this need to be done to all sections? > + { > + dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Adjusting aligment for section [%u]\n", > + elf->name, i); > + ((Elf_Shdr *)sec[i].sec)->sh_addralign = 4; And of course you know how I like such casting away of constness. Jan
Hi, On 25/08/16 16:11, Jan Beulich wrote: >>>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote: >> On most architectures it does not matter what the aligment is. >> >> On ARM32 it is paramount that the aligment is word-size (4) >> otherwise we get a Data Abort when trying to perform ELF >> relocations. That is due to ARM 32 only being able to write to >> word-aligned addresses. Well, it is the same on ARM64. However we decided to disable the alignment check (SCTLR.A not set) because some of the primitives imported from Linux (e.g memcpy) rely on the hardware handling misalignment. In any case, I would not recommend to use unaligned access on ARM as they may have a big performance impact. > > That's not exactly true, afaik: ARM can write to byte- and > half-word-aligned addresses, but only bytes/half-words. That's correct. Regards,
On Thu, Aug 25, 2016 at 09:11:15AM -0600, Jan Beulich wrote: > >>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote: > > On most architectures it does not matter what the aligment is. > > > > On ARM32 it is paramount that the aligment is word-size (4) > > otherwise we get a Data Abort when trying to perform ELF > > relocations. That is due to ARM 32 only being able to write to > > word-aligned addresses. > > That's not exactly true, afaik: ARM can write to byte- and > half-word-aligned addresses, but only bytes/half-words. > > > --- a/xen/common/livepatch_elf.c > > +++ b/xen/common/livepatch_elf.c > > @@ -71,7 +71,15 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data) > > delta = elf->hdr->e_shoff + i * elf->hdr->e_shentsize; > > > > sec[i].sec = data + delta; > > - > > + /* > > + * Some architectures REQUIRE section alignment to be word-size. > > + */ > > This is a single line comment. > > > + if ( sec[i].sec->sh_addralign % sizeof(uint32_t) ) > > Hmm, word size for ARM64 and x86-64 ought to be 8 bytes. Also you > don't cover sec[i].sec->sh_addralign being zero (in fact any non-power- > of-2 value would seem bogus to me). And then - why does this need to > be done to all sections? The issue I hit was relocations being done on .bug_frame section on ARM 32. We had an .rel.bug_frame which would modify the .bug_frame and it would try to use a uint32_t operation (R_ARM_REL32 if I recall correctly). Since .bug_frame sh_addralign was 1, it ended up sadly with an Data Abort exception. The other sections that had .sh_addralign of 1 such as: livepatch.depends, ,.rodata.str1, and .strtab were OK - there was no problem with them having byte alignment. P.S. On x86 and ARM64 the .bug_frame sections also have byte size alignment. > > > + { > > + dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Adjusting aligment for section [%u]\n", > > + elf->name, i); > > + ((Elf_Shdr *)sec[i].sec)->sh_addralign = 4; > > And of course you know how I like such casting away of constness. Heh. Initially I was hoping the linker would have an --section-alignment like the ld PE one which would make this a simple $LD_FLAGS change. But without that the only other recourse I have (especially for the test-cases) is to cobble up an livepatch.lds which I would need to sync with the xen.lds occassionaly. That would guarantee that the livepatch'es would have the right section alignment and then this patch would just return -EINVAL if the alignment was not to the power of 2. Or something along this patch where we adjust it (and yes need to cast away the constness). Preferences? > > Jan >
>>> On 06.09.16 at 23:18, <konrad.wilk@oracle.com> wrote: > On Thu, Aug 25, 2016 at 09:11:15AM -0600, Jan Beulich wrote: >> >>> On 25.08.16 at 15:37, <konrad.wilk@oracle.com> wrote: >> > --- a/xen/common/livepatch_elf.c >> > +++ b/xen/common/livepatch_elf.c >> > @@ -71,7 +71,15 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data) >> > delta = elf->hdr->e_shoff + i * elf->hdr->e_shentsize; >> > >> > sec[i].sec = data + delta; >> > - >> > + /* >> > + * Some architectures REQUIRE section alignment to be word-size. >> > + */ >> >> This is a single line comment. >> >> > + if ( sec[i].sec->sh_addralign % sizeof(uint32_t) ) >> >> Hmm, word size for ARM64 and x86-64 ought to be 8 bytes. Also you >> don't cover sec[i].sec->sh_addralign being zero (in fact any non-power- >> of-2 value would seem bogus to me). And then - why does this need to >> be done to all sections? > > The issue I hit was relocations being done on .bug_frame section on ARM 32. > > We had an .rel.bug_frame which would modify the .bug_frame and it would try > to use a uint32_t operation (R_ARM_REL32 if I recall correctly). > Since .bug_frame sh_addralign was 1, it ended up sadly with an > Data Abort exception. So then it's rather the insufficient alignment of .bug_frame which needs fixing? As done recently to a few other sections, alignment should always be properly expressed in the .o files already. Artificially aligning sections in the linker script (for the base binary) or in the loader (for patches) should never be a requirement. >> > + { >> > + dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Adjusting aligment for > section [%u]\n", >> > + elf->name, i); >> > + ((Elf_Shdr *)sec[i].sec)->sh_addralign = 4; >> >> And of course you know how I like such casting away of constness. > > Heh. > > Initially I was hoping the linker would have an --section-alignment like the ld PE one > which would make this a simple $LD_FLAGS change. > > But without that the only other recourse I have (especially for the test-cases) is > to cobble up an livepatch.lds which I would need to sync with the xen.lds occassionaly. > That would guarantee that the livepatch'es would have the right section alignment and > then this patch would just return -EINVAL if the alignment was not to the power of 2. > > Or something along this patch where we adjust it (and yes need to cast away the > constness). > > Preferences? See above - this should be fixed without linker scripts or alike. Jan
diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c index cda9b27..2392bdf 100644 --- a/xen/common/livepatch_elf.c +++ b/xen/common/livepatch_elf.c @@ -71,7 +71,15 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data) delta = elf->hdr->e_shoff + i * elf->hdr->e_shentsize; sec[i].sec = data + delta; - + /* + * Some architectures REQUIRE section alignment to be word-size. + */ + if ( sec[i].sec->sh_addralign % sizeof(uint32_t) ) + { + dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Adjusting aligment for section [%u]\n", + elf->name, i); + ((Elf_Shdr *)sec[i].sec)->sh_addralign = 4; + } delta = sec[i].sec->sh_offset; /* * N.B. elf_resolve_section_names, elf_get_sym skip this check as
On most architectures it does not matter what the aligment is. On ARM32 it is paramount that the aligment is word-size (4) otherwise we get a Data Abort when trying to perform ELF relocations. That is due to ARM 32 only being able to write to word-aligned addresses. The default section alignments for .bug.frame, .livepatch.depends, ,.rodata.str1, and .strtab are 1 unless we: - Provide our own linker script file. - Piggyback on Xen's (but that means lots of #ifdefery) - Throw an error and refuse the payload Or just fix it up. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- Cc: Julien Grall <julien.grall@arm.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> v2: First submission --- xen/common/livepatch_elf.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)