Re: [Xen-devel] [PATCH v11 9/9] xen: explicit casts when DECLARE_BOUNDS cannot be used [and 1 more messages]

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Stefano Stabellini writes ("[PATCH v11 9/9] xen: explicit casts when 
DECLARE_BOUNDS cannot be used"):
> Sometimes the static inline functions provided by DECLARE_BOUNDS cannot
> be used. This patch uses explicit casts to uintptr_t in those cases.
> 
> M3CM: Rule-18.2: Subtraction between pointers shall only be applied to
> pointers that address elements of the same array

IMO these ad-hoc workarounds *must* be accompanied by a clear
explanation of why the DECLARE_BOUNDS arragement is not suitable.


In general I am very strongly of the opinion that:

 * Code should be written so that it does not introduce the risk of
   accidentally inducing UB in correct-looking constructs elsewhere.

 * In particular, declarations of linker symbols should always be
   written in a way that means that plain correct-looking code will
   either *be* correct or produce a compile error.

 * Exceptions of whatever kind should be fully justified, in the
   code.  Usually this will mean a clear and detailed and rigorous
   comment which contains a proof (or similar argument) that
   the approach taken is sound.


Considering these two in reverse order.

bug_frames
----------

What appears to be going on is this:

setup_virtual_regions contains a static const array of pointers to
struct bug_frame.  These struct bug_frame* values are themselves
linker symbol values.

Because the compiler has visibility of all of this, it is allowed to
assume that these values do not change.  It will therefore wrongly
assume that they are incomparable.

(All of this ought to have been explained in comments in the code.
Given that it wasn't, a comment ought to have been in your patch.
I shouldn't have had to go and dig into the code.)

And I'm afraid I do not like your patch.  The declaration in eg
xen/include/asm-arm/bug.h

  extern const struct bug_frame __start_bug_frames[],
                                __stop_bug_frames_0[],
                                __stop_bug_frames_1[],
                                __stop_bug_frames_2[];

is a clear violation of the rule in the DECLARE_BOUNDS comment:

+  * These macros, or an alternative technique, MUST be used any time
+  * linker symbols are imported into C via the `extern []' idiom.

I think an `alternative technique' should be applied *at the time of
the declaration* and it should suffice to *mostly avoid the risk of
dangerous uses*.

Jan has one suggestion, which I don't fully understand (see below).
Alternatively I suggest the following ad-hoc approach:

  * Rename __start_bug_frames etc. to
      __UBDANGER_*bug_frame*
    (and provide an accompanying comment saying what the UB danger is
    and how UB must be avoided).

  * Change the definition of bug_frames to
      static const uintptr_t __initconstrel bug_frames[] ...

  * Provide an arch-independent helper macro to both declare
    __UBDANGER_*bug_frame*, converting the pointers to
    uintptr_t, and initialise the array.

  * In setup_virtual_regions, cast the uintptr_t to a pointer.  This
    needs to be accompanied by a substantial comment explaining why
    this is safe.  Elements of the proof are
      - stating that the thing came from a pointer which came
        from a linker symbol, so it is a valid pointer value
      - some kind of argument that no code elsewhere will
        compare pointers from different core_init.frame
        and core_init.ex entries.
    I observe that with the current code I think making such an
    argument may involve doing something about core_init.ex and
    core_init.ex_end.  Not sure.

An alternative would be to construct this table in a different
translation unit, in which case the compiler compiling
setup_virtual_regions cannot `prove' the falsehood .  (Note that we
must be disabling whole-program optimisation.  I hope we are!)


Jan writes:

> I disagree with the comment,

I also disagree with the wording of the comment.  It is seriously
misleading.  These symbols do in fact refer to the same object!
The problem is that the compiler thinks otherwise.  You need wording
like that in DECLARE_BOUNDS.  (Or a reference to it.)

> and if you think it is correct, then no
> matter what you do the behavior is undefined. Instead I view the
> entirety of the .bug_frames.* sections as a single array, with
> labels placed not only at start and end, but also in the middle. I
> think the code here would better also be taken care of by the
> DECLARE_BOUNDS() machinery, dividing the single array into
> multiple smaller ones.

Jan, I'm not sure exactly what you are suggesting.  Currently the
array has one pointer per element.  Are you suggesting it should have
two pointers (start and end), with different notional types ?

If that is OK from a perf point of view then it is an easy answer
(although a bit tiresome since more linker symbols will have to be
generated).


__start_xen
-----------

> @@ -976,7 +976,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>           * respective reserve_e820_ram() invocation below.
>           */
>          mod[mbi->mods_count].mod_start = virt_to_mfn(_stext);
> -        mod[mbi->mods_count].mod_end = __2M_rwdata_end - _stext;
> +        mod[mbi->mods_count].mod_end = (uintptr_t)__2M_rwdata_end -
> +                                       (uintptr_t) _stext;

__2M_rwdata_end and _stext are both declared in header files using the
deprecated pattern.

  xen/include/xen/kernel.h:extern char _stext[], _etext[];
  xen/include/asm-x86/setup.h:extern char __2M_rwdata_start[], 
__2M_rwdata_end[];

According to the comment for DEFINE_BOUNDS, it ought to be used here,
or some other mechanism.  But AFAICT hyou have not changed the
declarations ?

If you changed the declarations then (i) mistakes would be avoided
(ii) you would still have to use explicit casts to compare these
pointers to different sections, but you could write a clear
explanation of (a) why this is needed (b) why it is safe.

You have done neither.


I hope this makes sense.

Thanks,
Ian.

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