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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.02.19 at 22:22, <sstabellini@xxxxxxxxxx> wrote:
> 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.

Well, I can see the general possible need for additional changes
due to the type adjustments. I don't see though why the original
code in this example would break with the other adjustments done
here. Things need to build and work after each patch, but changes
not strictly needed and not related to the subject of a patch would
better be split out (in this case into the [or another] Arm-specific
patch).

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

No matter what, it looks like you're wanting (and perhaps needing) to
stick to some form of cast here. But that's what you're specifically
trying to get away from, aren't you? What is MISRA's position on
casts from void * to a type? This is not a generally "safe" operation
after all, because the casted to type could be out of sync with the
object the pointer points at.

Note that struct livepatch_func only happens to live in the public
interface, but the type of old_addr ought to be freely changeable as
long as it remains a pointer. Did you check whether changing it would
help avoid all casting?

Jan



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