Re: [Xen-devel] [PATCH v2 19/20] livepatch/elf: Adjust section aligment to word

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.09.16 at 23:18, <konrad.wilk@xxxxxxxxxx> wrote:
> On Thu, Aug 25, 2016 at 09:11:15AM -0600, Jan Beulich wrote:
>> >>> On 25.08.16 at 15:37, <konrad.wilk@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel