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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 27 Feb 2019, Jan Beulich wrote:
> >>> 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).

I'll add a patch at the end with all the explicit casts. Much easier to
talk about how to solve the odd ends that way. This specific instance
can be resolved though, I'll do that.


> >> > @@ -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?
 
I am trying to get away from comparisons/subtractions between pointers
pointing to different objects. I am not trying to get away from casts
completely, only trying to minimize them while reaching the main
objective.

I checked MISRAC and it looks like void* pointers conversions are
allowed, so I can use xen_diff here.

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