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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 26 Feb 2019, Jan Beulich wrote:
> >>> 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.

It's in this patch because this is the patch dealing with _start and
_end. Let me know how would you like the patches to be split.


> > @@ -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.

Well spotted! I'll fix the bug. I would also be happy to use the helper
inlines, but we discussed not to use them in cases like this, with three
operators. Even if I wanted to use them, none of the inline helpers fit
this case. Or do you suggest:

  new_ptr = xen_diff(_start, (struct abstract_xen *)func->old_addr) +
            vmap_of_xen_text;

Is that what you are asking?

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