Re: [Xen-devel] [livepatch-build-tools part2 4/6] livepatch-build: detect special section group sizes

["Wieczorkiewicz, Pawel" <wipawel@xxxxxxxxx>]

> On 29. Apr 2019, at 16:21, Ross Lagerwall <ross.lagerwall@xxxxxxxxxx> wrote:
> 
> On 4/29/19 3:10 PM, Ross Lagerwall wrote:
>> On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
>>> Hard-coding the special section group sizes is unreliable. Instead,
>>> determine them dynamically by finding the related struct definitions
>>> in the DWARF metadata.
>>> 
>>> This is a livepatch backport of kpatch upstream commit [1]:
>>> kpatch-build: detect special section group sizes 170449847136a48b19fc
>>> 
>>> Xen only deals with alt_instr, bug_frame and exception_table_entry
>>> structures, so sizes of these structers are obtained from xen-syms.
>> structers -> structures

Thanks, will fix.

>>> 
>>> This change is needed since with recent Xen the alt_instr structure
>>> has changed size from 12 to 14 bytes.
>>> 
>>> [1] 
>>> https://github.com/jpoimboe/kpatch/commit/170449847136a48b19fcceb19c1d4d257d386b56
>>>  
>>> 
>>> Signed-off-by: Pawel Wieczorkiewicz <wipawel@xxxxxxxxx>
>>> Reviewed-by: Bjoern Doebel <doebel@xxxxxxxxx>
>>> Reviewed-by: Martin Mazein <amazein@xxxxxxxxx>
>>> ---
>>>   create-diff-object.c | 60 
>>> ++++++++++++++++++++++++++++++++++++++++++++--------
>>>   livepatch-build      | 23 ++++++++++++++++++++
>>>   2 files changed, 74 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/create-diff-object.c b/create-diff-object.c
>>> index 1e6e617..b0b4dcb 100644
>>> --- a/create-diff-object.c
>>> +++ b/create-diff-object.c
>>> @@ -958,12 +958,54 @@ static void kpatch_mark_constant_labels_same(struct 
>>> kpatch_elf *kelf)
>>>       }
>>>   }
>>> -static int bug_frames_0_group_size(struct kpatch_elf *kelf, int offset) { 
>>> return 8; }
>>> -static int bug_frames_1_group_size(struct kpatch_elf *kelf, int offset) { 
>>> return 8; }
>>> -static int bug_frames_2_group_size(struct kpatch_elf *kelf, int offset) { 
>>> return 8; }
>>> -static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) { 
>>> return 16; }
>>> -static int ex_table_group_size(struct kpatch_elf *kelf, int offset) { 
>>> return 8; }
>>> -static int altinstructions_group_size(struct kpatch_elf *kelf, int offset) 
>>> { return 12; }
>>> +static int bug_frames_group_size(struct kpatch_elf *kelf, int offset)
>>> +{
>>> +    static int size = 0;
>>> +    char *str;
>>> +    if (!size) {
>>> +        str = getenv("BUG_STRUCT_SIZE");
>>> +        size = str ? atoi(str) : 8;
>> Why did you remove the hard error here from the original kpatch commit? I 
>> think a hard error is preferable to guessing.

Previously the sizes were hard-coded. I prefer to use them directly over 
failing hard here.
If we could not come up with a sane defaults, than failing hard would be the 
only option.
Anyway, I am cool to go either way upon your good judgment.

>>> +    }
>>> +
>>> +    return size;
>>> +}
>>> +
>>> +static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset)
>>> +{
>>> +    static int size = 0;
>>> +    char *str;
>>> +    if (!size) {
>>> +        str = getenv("BUG_STRUCT_SIZE");
>>> +        size = str ? atoi(str) : 16;
>>> +    }
>>> +
>>> +    return size;
>>> +}
>>> +
>>> +static int ex_table_group_size(struct kpatch_elf *kelf, int offset)
>>> +{
>>> +    static int size = 0;
>>> +    char *str;
>>> +    if (!size) {
>>> +        str = getenv("EX_STRUCT_SIZE");
>>> +        size = str ? atoi(str) : 8;
>>> +    }
>>> +
>>> +    return size;
>>> +}
>>> +
>>> +static int altinstructions_group_size(struct kpatch_elf *kelf, int offset)
>>> +{
>>> +    static int size = 0;
>>> +    char *str;
>>> +    if (!size) {
>>> +        str = getenv("ALT_STRUCT_SIZE");
>>> +        size = str ? atoi(str) : 12;
>>> +    }
>>> +
>>> +    printf("altinstr_size=%d\n", size);
>>> +    return size;
>>> +}
>>>   /*
>>>    * The rela groups in the .fixup section vary in size.  The beginning of 
>>> each
>>> @@ -1016,15 +1058,15 @@ static int fixup_group_size(struct kpatch_elf 
>>> *kelf, int offset)
>>>   static struct special_section special_sections[] = {
>>>       {
>>>           .name        = ".bug_frames.0",
>>> -        .group_size    = bug_frames_0_group_size,
>>> +        .group_size    = bug_frames_group_size,
>>>       },
>>>       {
>>>           .name        = ".bug_frames.1",
>>> -        .group_size    = bug_frames_1_group_size,
>>> +        .group_size    = bug_frames_group_size,
>>>       },
>>>       {
>>>           .name        = ".bug_frames.2",
>>> -        .group_size    = bug_frames_2_group_size,
>>> +        .group_size    = bug_frames_group_size,
>>>       },
>>>       {
>>>           .name        = ".bug_frames.3",
>>> diff --git a/livepatch-build b/livepatch-build
>>> index c057fa1..a6cae12 100755
>>> --- a/livepatch-build
>>> +++ b/livepatch-build
>>> @@ -284,6 +284,29 @@ echo "Output directory: ${OUTPUT}"
>>>   echo "================================================"
>>>   echo
>>> +if [ -f "$XENSYMS" ]; then
>>> +    echo "Reading special section data"
>>> +    SPECIAL_VARS=$(readelf -wi "$XENSYMS" |
>>> +               gawk --non-decimal-data '
>>> +               BEGIN { a = b = e = 0 }
>>> +               a == 0 && /DW_AT_name.* alt_instr/ {a = 1; next}
>>> +               b == 0 && /DW_AT_name.* bug_frame/ {b = 1; next}
>>> +               e == 0 && /DW_AT_name.* exception_table_entry/ {e = 1; next}
>>> +               a == 1 {printf("export ALT_STRUCT_SIZE=%d\n", $4); a = 2}
>>> +               b == 1 {printf("export BUG_STRUCT_SIZE=%d\n", $4); b = 2}
>>> +               e == 1 {printf("export EX_STRUCT_SIZE=%d\n", $4); e = 2}
>>> +               a == 2 && b == 2 && e == 2 {exit}')
>>> +    [[ -n $SPECIAL_VARS ]] && eval "$SPECIAL_VARS"
>>> +    if [[ -z $ALT_STRUCT_SIZE ]] || [[ -z $BUG_STRUCT_SIZE ]] || [[ -z 
>>> $EX_STRUCT_SIZE ]]; then
>>> +        die "can't find special struct size"
>>> +    fi
>>> +    for i in $ALT_STRUCT_SIZE $BUG_STRUCT_SIZE $EX_STRUCT_SIZE; do
>>> +        if [[ ! $i -gt 0 ]] || [[ ! $i -le 16 ]]; then
>>> +            die "invalid special struct size $i"
>>> +        fi
>>> +    done
>>> +fi
>>> +
>> If this hunk is moved after the call to build_full(), then it can be run 
>> directly on the xen-syms that has just been built which would allow dropping 
>> `if [ -f "$XENSYMS" ]...` and guaranteeing that *_STRUCT_SIZE are always set.
> 
> One more thing, previously:
> 
> bug_frames_0_group_size == 8
> bug_frames_1_group_size == 8
> bug_frames_2_group_size == 8
> bug_frames_3_group_size == 16
> 
> And now:
> 
> bug_frames_0_group_size == BUG_STRUCT_SIZE
> bug_frames_1_group_size == BUG_STRUCT_SIZE
> bug_frames_2_group_size == BUG_STRUCT_SIZE
> bug_frames_3_group_size == BUG_STRUCT_SIZE
> 
> This seems wrong to me. When reading special section data, should you detect 
> e.g. BUG0_STRUCT_SIZE, BUG1_STRUCT_SIZE, … ?
> 

There is only one struct bug_frame definition in Xen:
Using pahole:

struct bug_frame {
        unsigned char              ud2[2];               /*     0     2 */
        unsigned char              ret;                  /*     2     1 */
        short unsigned int         id;                   /*     3     2 */

        /* size: 5, cachelines: 1, members: 3 */
        /* last cacheline: 5 bytes */
};

It’s size is 5, so fits into 8 bytes.

Definitions for all the 0, 1, 2, 3 groups use the same struct bug_frame:
Example grep:
include/asm-x86/bug.h:extern const struct bug_frame __start_bug_frames[],
include/asm-x86/bug.h:                              __stop_bug_frames_0[],
include/asm-x86/bug.h:                              __stop_bug_frames_1[],
include/asm-x86/bug.h:                              __stop_bug_frames_2[],
include/asm-x86/bug.h:                              __stop_bug_frames_3[];

So, the default group size of 16 bytes does not seem right.

Example grep:
$ objdump -g xen-syms|grep -A3 DW_TAG_structure_type|grep -A1 bug_frame|cut 
-f2- -d'>'|sort -u
--
   DW_AT_byte_size   : 8
   DW_AT_name        : (indirect string, offset: 0x2556): bug_frame


> -- 
> Ross Lagerwall


Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel