Re: [Xen-devel] [PATCH v5 16/28] xsplice: Add support for bug frames.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.03.16 at 21:00, <konrad.wilk@xxxxxxxxxx> wrote:
> --- a/xen/common/xsplice.c
> +++ b/xen/common/xsplice.c
> @@ -120,6 +120,24 @@ static int verify_payload(const 
> xen_sysctl_xsplice_upload_t *upload)
>      return 0;
>  }
>  
> +bool_t is_patch(const void *ptr)
> +{
> +    struct payload *data;
> +
> +    /*
> +     * No locking since this list is only ever changed during apply or revert
> +     * context.
> +     */
> +    list_for_each_entry ( data, &applied_list, applied_list )
> +    {
> +        if ( ptr >= data->ro_addr &&
> +             ptr < (data->ro_addr + data->ro_size) )
> +            return 1;
> +    }
> +
> +    return 0;
> +}

A function with this name can't look at just .rodata. Nor may you
assume in the first place that the file names you look for are
guaranteed to be in .rodata.

> @@ -533,6 +551,28 @@ static int prepare_payload(struct payload *payload,
>      region->start = (unsigned long)payload->text_addr;
>      region->end = (unsigned long)(payload->text_addr + payload->text_size);
>  
> +    /* Optional sections. */
> +    for ( i = 0; i < BUGFRAME_NR; i++ )
> +    {
> +        char str[14];
> +
> +        snprintf(str, sizeof str, ".bug_frames.%u", i);
> +        sec = xsplice_elf_sec_by_name(elf, str);
> +        if ( !sec )
> +            continue;
> +
> +        if ( !sec->sec->sh_size ||

I don't think there's anything wrong with this section being empty.

> +             (sec->sec->sh_size % sizeof (struct bug_frame)) )

Please use sizeof(*region->frame->bugs) here to avoid a possible
disconnect between the expression here and the actual type
needed.

> +        {
> +            dprintk(XENLOG_DEBUG, "%s%s: Wrong size of .bug_frames.%u!\n",
> +                    XSPLICE, elf->name, i);
> +            return -EINVAL;
> +        }
> +
> +        region->frame[i].bugs = (struct bug_frame *)sec->load_addr;

The more of these casts I see the more convinced I am that - if
they're needed - the type of load_addr was badly chosen.

> +        region->frame[i].n_bugs = sec->sec->sh_size / sizeof(struct 
> bug_frame);

Same remark for the sizeof() as above.

Jan


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