Re: [Xen-devel] [PATCH v10 6/6] xen: use DEFINE_SYMBOL as required

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.02.19 at 21:50, <sstabellini@xxxxxxxxxx> wrote:
> @@ -210,7 +210,8 @@ static int __apply_alternatives_multi_stop(void *unused)
>          region.begin = __alt_instructions;
>          region.end = (struct alt_instr *)__alt_instructions_end;
>  
> -        ret = __apply_alternatives(&region, xenmap - (void *)_start);
> +        ret = __apply_alternatives(&region, (uintptr_t)xenmap -
> +                                            (uintptr_t)_start);

Undesirable (but in this case maybe indeed unavoidable) casting
instead. I don't think this belongs in a patch with the given title
though.

> @@ -78,7 +78,19 @@ void arch_livepatch_revert(const struct livepatch_func 
> *func)
>      uint32_t *new_ptr;
>      unsigned int len;
>  
> -    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
> +    /*
> +     * We need to calculate the offset of the address from _start, and
> +     * apply that to our own map, to find where we have this mapped.
> +     * Doing these kind of games directly with pointers is contrary to
> +     * the C rules for what pointers may be compared and computed.  So
> +     * we do the offset calculation with integers, which is always
> +     * legal.  The subsequent addition of the offset to the
> +     * vmap_of_xen_text pointer is legal because the computed pointer is
> +     * indeed a valid part of the object referred to by vmap_of_xen_text
> +     * - namely, the byte array of our mapping of the Xen text.
> +     */
> +    new_ptr = ((uintptr_t)func->old_addr, - (uintptr_t)_start) +
> +              vmap_of_xen_text;

You not using the intended helper inlines has allowed for a bug to
slip in that the compiler can't even help notice, due to - being both
a valid unary and a valid binary operator.

Jan



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