Re: [PATCH v6 1/4] xen: introduce SYMBOL

[Stefano Stabellini <sstabellini@kernel.org>]

On Fri, 1 Feb 2019, George Dunlap wrote:
> On Tue, Jan 22, 2019 at 9:17 AM Jan Beulich <JBeulich@suse.com> wrote:
> >
> > >>> On 22.01.19 at 00:41, <sstabellini@kernel.org> wrote:
> > > We haven't managed to reach consensus on this topic. Your view might be
> > > correct, but it is not necessarily supported by compilers' behavior,
> > > which depends on the opinion of compilers engineers on the topic, and
> > > MISRAC compliance, which depends on the opinion of MISRAC specialists on
> > > the topic. If we take your suggested approach we end up with the code
> > > most likely to break in case the compilers engineers or the MISRAC
> > > experts disagree with you. In this case, being right doesn't necessarily
> > > lead to the code less likely to break.
> > >
> > > Regardless, if that is the decision of the Xen community as a whole,
> > > I'll follow it. My preference remains with approach 3. (var.S), followed
> > > by approach 2. (SYMBOL_HIDE returns uintptr_t), but I am willing to
> > > refresh my series to do approach 1. (SYMBOL_HIDE returns pointer type)
> > > if that is the only way forward.
> > >
> > > Let us come to a conclusion so that we can move on.
> >
> > How can we come to a conclusion when things remain unclear? I see
> > only two ways forward - either we settle the dispute (which I'm
> > afraid would require involvement of someone accepted by all of us
> > as a "C language lawyer", which would include judgment about the
> > MISRA-C implications),
> 
> Well, no, I don't think a "C language lawyer" would help here.
> 
> You keep making the case for C spec compliance as though we're dealing
> with zealous but ultimately rational people, who will a) almost never
> violate the C spec, and b) actually fix their compiler if their
> language does violate the spec.
> 
> But it's clear from reading those threads that this is not the case.
> Over and over people presented  clear arguments, from the spec and the
> spec committee, showing that what gcc was doing was both wrong and
> impractical; and the compiler people kept coming up with more and more
> tortuous interpretations to justify the compiler's behavior.  The
> whole thing with supposing that the C standard anticipated a
> compacting garbage collector was the cherry on the cake.
> 
> We're not living in a rational world where if we just follow the rules
> we'll be safe.  We have a dictat from the high council in the form of
> a C spec which is divorced from actual usage and utility, and we have
> a load of insane fanatics trying to impose their interpretation
> doctrinal purity on the world, and ready to burn any heretics writing
> code that doesn't match the view they happen to hold that day.  "The
> compiler can't optimize this because it can't prove they're different
> objects" is a fine principle, but it's pretty clear that they're
> willing to continue optimizing things even if you *can* prove they
> fall inside the rules of pointer comparison.

That made me laugh very hard :-D  in a "sad but true" kind of way.


> In such a situation, *there is no perfectly safe option*.  No matter
> what position you take, the fanatics may end up deciding that you're a
> heretic and need to be burned at the stake.  Might they decide that
> they know that extern pointers point to different objects, and
> therefore can't be compared? Maybe!  Might they decide they can pierce
> the veil of asm to determine the source of unsigned longs they're
> comparing? Possibly!  Could they decide that a uintptr_t received from
> the heathen lands of assembly is anathema, and therefore casting it to
> a pointer is undefined behavior?  They certainly could!
> 
> And even if you do convince them their interpretation is wrong and
> they fix their compiler, the damage is still done: there are still,
> out in the wild, vulnerable binaries built with buggy compilers and
> buggy compilers that produce vulnerable binaries, until they all die
> of old age.
> 
> *Any* interpretation we choose may be declared at some point by the
> compiler folks to be heresy.  But, there are less safe option and more
> safe options.  Our goal with regard to the C Standard cannot,
> unfortunately, be "follow the rules".  Our goal must be to *minimize
> the risk* of being caught in the next wave of the compiler
> optimization purges.
> 
> MISRA is quirky and often impractical, but ultimately their goal with
> rules like this is to try to protect you from the fanatics who write
> compilers (insofar as that is possible).  So if we do our best to be
> as safe as possible from the compiler fanatics, we have a pretty good
> chance of being considered MISRA compliant as well.
> 
> It seems to me that anything that involves directly comparing pointers
> is simply more likely to be come the target of optimization (and thus
> more dangerous) than comparing unsigned long and uintptr_t.  And
> although I'm not terribly familiar with the "intptr" types, it seems
> to me that casting from uintptr_t is less likely to ever be considered
> deviant behavior than casting from unsigned long.
> 
> As such, I think casting the return value of asm to a pointer is far
> too dangerous.  Using extern pointers seems quite dangerous to me as
> well.  So it seems to me that using asm to generate an unsigned long
> would be absolute minimum behavior.  Using uintprt_t values, and in
> particular importing them from assembly, might give us yet another
> level of safety (in case unsigned long -> pointer casts ever become a
> target).
> 
> Are these guaranteed to avoid "UB hazard" issues in the future?  Of
> course not; nothing can.  But they seem to me to be a lot less risky
> than asm -> ptr or extern pointers.

This is a great well-written writeup George. Maybe worthy of a blog
post, once we settle this issue :-)


> > Only at that point can we then decide whether any of
> > the proposed "solutions" (in quotes because I remain unconvinced
> > there's a problem to solve here other than working around compiler
> > bugs) is/are necessary _and_ fulfilling the purpose, and if multiple
> > remain, which of them we like best / is the least bad one.
> 
> Improvements this series seeks to make, as I understand it, include
> (in order of urgency):
> 
> 1. Fixing one concrete instance of "UB hazard"
> 2. Minimize risk of further "UB hazard" in this bit of functionality
> 3. Retain the effort Stefano has put in identifying all the places
> where such UB hazards need to be addressed.
> 4. Move towards MISRA-C compliance.

This is exactly right.


> As far as I can tell, primary objections you've leveled at the options
> which try to address 2-4 are:
> 
> a. "UB hazard" still not zero
> b. MISRA compliancy no 100%
> c. Ugly
> d. Inefficient
> 
> (Obviously some proposals have had more technical discussion.)
> 
> Anything I missed?

I would like to add here the reply I got from the MISRAC experts, that
matches your view above.

Predictably, they dislike both SYMBOL_HIDE workarounds, because they are
just compiler workarounds rather than compliance and/or code
improvements.

Instead, they suggest an approach very similar to the var.S approach,
but simpler, without the assembly redirection. Their suggestion is to
declare uintptr_t variables in C corresponding to the linker symbols and
initialize them _once_ to the linker symbol values:

  /* linker symbols */
  extern char _start[], _end[];
  /* corresponding uintptr_t variables in C */
  uintptr_t start, end;

  /* initialization of the uintptr_t variables */
  start = (uintptr_t) _start;
  end = (uintptr_t) _end;
  
  /* example usage */
  size = (_end - _start);


Thus, I think it is best to follow-up on the var.S approach. Whether we
declare the variables in assembly in var.S or in a C file like
suggested, is a minor detail.

But before I proceed in reworking the series once more, I would like to
get an agreement on the way forward. I don't think Jan's solution is
good enough, but I am willing to follow through with it if that's the
decision. But I would really love to avoid sending yet another series
update whose fundamental approach gets rejected again.

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