Re: [Xen-devel] [PATCH 2/2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Jun 20, 2017 at 01:15:18AM -0600, Jan Beulich wrote:
> >>> On 20.06.17 at 01:05, <andrew.cooper3@xxxxxxxxxx> wrote:
> > On 19/06/2017 19:30, Konrad Rzeszutek Wilk wrote:
> >> On Wed, Jun 14, 2017 at 12:49:21PM -0600, Jan Beulich wrote:
> >>>>>> Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 06/14/17 8:34 PM >>>
> >>>> Well - I've got a livepatch with such a relocation.  It is probably a
> >>>> livepatch build tools issue, but the question is whether Xen should ever
> >>>> accept such a livepatch or not (irrespective of whether this exact
> >>>> relocation is permitted within the ELF spec).
> >>> Since the spec explicitly mentions that case, I think we'd better support 
> > it.
> >>> But it wouldn't be the end of the world if we didn't, as presumably there
> >>> aren't that many use cases for it.
> >> OK. In that case:
> >>
> >> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> 
> I have to admit that I'm surprised by that, not only because of
> what Andrew says below, but also because imo the patch would
> imo need to be done somewhat differently, as outlined earlier
> (making STN_UNDEF less of a special case).

My line of thinking was - if the ELF spec is OK, then lets support it.

But then your point about just giving -EOPNOTSUPP is an excellent
way of "supporting" it - and better yet - we can give the system
admin an nice warning: "Fix your livepatch build-tool as your livepatch
is trying to dereference a NULL point which is unhealthy."

(or such).

Andrew you OK posting a patch like that?

> 
> >> Do you think it would be possible to generate an test-case for this
> >> in arch/test/livepatch?
> > 
> > I can trivially cause this situation to occur with the current build
> > tools, but we are currently presuming a build tools bug to be the
> > underlying issue behind getting a STN_UNDEF relocation in the livepatch.
> > 
> > Given that a STN_UNDEF relocation (appears to) mean a NULL dereference
> > once the relocations are evaluated, I am not happy with supporting such
> > a case.
> 
> Well, quite clearly this can be of use only to produce constants,
> not to produce pointers (unless chained ["expression"] relocations
> are being used, where the result of one element in the chain is the
> addend of the next one, albeit even then this would effectively be
> a NOP relocation, so may be useful only when post-editing binaries
> where the tool doesn't want to change [relocation] section sizes).
> 
> > Therefore, I'm going to insist that we take a concrete decision as to
> > what to do in the hypervisor code, before adding a test case, and
> > advocate for excluding it outright rather than tolerating it in the
> > (certain?) knowledge that Xen will subsequently crash.
> 
> As per the explanation above, we can't tell whether Xen will
> subsequently crash, as we don't know what it is that is being
> relocated by such an relocation. While, as indicated before, I'd like
> to see us support everything the standard mandates, I wouldn't
> view it as a big problem to simply return -EOPNOTSUPP for this case
> for the time being.
> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel