Re: [Xen-devel] [PATCH v9 12/27] xsplice: Implement support for applying/reverting/replacing patches.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.04.16 at 17:34, <konrad.wilk@xxxxxxxxxx> wrote:
> +static int check_special_sections(const struct xsplice_elf *elf)
> +{
> +    unsigned int i;
> +    static const char *const names[] = { ELF_XSPLICE_FUNC };
> +    bool_t count[ARRAY_SIZE(names)] = { 0 };
> +
> +    for ( i = 0; i < ARRAY_SIZE(names); i++ )
> +    {
> +        const struct xsplice_elf_sec *sec;
> +
> +        sec = xsplice_elf_sec_by_name(elf, names[i]);
> +        if ( !sec )
> +        {
> +            dprintk(XENLOG_ERR, XSPLICE "%s: %s is missing!\n",
> +                    elf->name, names[i]);
> +            return -EINVAL;
> +        }
> +
> +        if ( !sec->sec->sh_size )
> +        {
> +            dprintk(XENLOG_ERR, XSPLICE "%s: %s is empty!\n",
> +                    elf->name, names[i]);
> +            return -EINVAL;
> +        }
> +        if ( ++count[i] > 1 )

boolean values can only validly be 0 or 1. Just "if ( count[i] )" here
and ...

> +        {
> +            dprintk(XENLOG_ERR, XSPLICE "%s: %s was seen more than once!\n",
> +                    elf->name, names[i]);
> +            return -EINVAL;
> +        }

"count[i] = 1;" here.

Thinking about it again, even more stack conserving would be a
bitmap...

> +static int apply_payload(struct payload *data)
> +{
> +    unsigned int i;
> +
> +    printk(XENLOG_INFO XSPLICE "%s: Applying %u functions\n",
> +            data->name, data->nfuncs);
> +
> +    arch_xsplice_patching_enter();
> +
> +    for ( i = 0; i < data->nfuncs; i++ )
> +        arch_xsplice_apply_jmp(&data->funcs[i]);
> +
> +    arch_xsplice_patching_leave();
> +
> +    list_add_tail_rcu(&data->applied_list, &applied_list);

Neither in the comment earlier on nor here it becomes clear that this
is more of an abuse than a use of RCU.

> +struct xsplice_patch_func {
> +    const char *name;       /* Name of function to be patched. */
> +    void *new_addr;
> +    void *old_addr;
> +    uint32_t new_size;
> +    uint32_t old_size;
> +    uint8_t version;        /* MUST be XSPLICE_PAYLOAD_VERSION. */
> +    uint8_t opaque[31];     /* MUST be zero filled. */

I don't see the zero filling being a requirement, nor it being enforced.

Jan


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