Re: [Xen-devel] [PATCH] x86/livepatch: Fail the build if duplicate symbols exist

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.04.19 at 21:51, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 08/04/2019 11:53, Jan Beulich wrote:
>>>>> On 08.04.19 at 12:11, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 08/04/2019 10:52, Jan Beulich wrote:
>>>>>>> On 05.04.19 at 23:02, <konrad.wilk@xxxxxxxxxx> wrote:
>>>>> On Fri, Apr 05, 2019 at 08:26:04PM +0100, Andrew Cooper wrote:
>>>>>> From: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
>>>>>>
>>>>>> The binary diffing algorithm used by xen-livepatch depends on having 
>>>>>> unique
>>>>>> symbols.
>>>> Question: Is this an inherent requirement, or an effect of the
>>>> current implementation? It would seem to me that at least in the
>>>> common case (binary didn't changer overly heavily) it ought to
>>>> be possible to re-associate the duplicates with their correct
>>>> origins. And by making the build fail (including in cases like the
>>>> one we have right now), we're painting ourselves into the
>>>> corner of having to "fix" code which isn't actually buggy.
>>> It is inherent.  The tool needs a unique way to identify each block of
>>> code, so it can work out if the binary has changed by more than just
>>> relocations.
>>>
>>> How do you propose disambiguating ambiguous symbols after the fact?
>>>
>>> (This is semi rhetorical, because even if there is a possible way, there
>>> is no plausible way it is the sensible option to take.)
>> I don't understand why you consider it impossible for this to be a
>> sensible option.
> 
> Because there is no problem which is best solved by throwing away the
> information you've got, and then trying to reconstruct it at a later
> point using divination.

What information did I suggest to throw away?

To me it doesn't sound impossible to produce a diff of two binaries
if both have the same number of instances of an otherwise
identically named symbol. For live patching purposes the assumption
has to be anyway that it's not the entire executable which changes,
but just limited regions thereof.

>> First of all, from the very beginning of live-patching's existence I've
>> been questioning it having got made dependent on the internal
>> symbol table we generate, for there being a chance of it being
>> broken in some way.
> 
> Xen uses the same diffing mechanism as kpatch.  This was chosen for a
> number reasons, but first and foremost, because Ross and Konrad did the
> work which automatically gives them a 9/10'th choice on how to solve the
> various issues at hand, and "reusing an existing tool" is a very good
> reason.
> 
> You are welcome to invent a brand new way of performing binary diffing,
> but on top of needing to be at least as functional as the current
> solution, you will need to justify why deviating from the existing
> standard tooling is a good thing for the Xen community to be doing.
> 
> Until then, the current solution depends on having unique symbols.

Okay, I can accept this as an argument, and I certainly don't plan
on writing a replacement (let alone justifying why I did).

>> As to disambiguating: Other nearby local symbols could surely
>> help. This might be a problem with -ffunction-sections, but other
>> than I thought we don't appear to be using it - at least I couldn't
>> get grep to produce a hit in xen/, config/, and the tree root.
> 
> -ffunction-sections is injected by the livepatch-buildtools build
> logic.  It doesn't live in the main xen.git build.

I see - not very fortunate, but at least I wasn't entirely
mis-remembering that it gets used.

>>>> And even if the order changed, as long as both instances in fact
>>>> derive from the same inline function, all would still be fine afaict.
>>> How do you propose that we determine this?
>> We build with debug info, so it ought to be possible in principle
>> to know that both have the same source origin.
> 
> That doesn't make it safe.  On release builds with LIVEPATCH enabled, it
> is domctl.c#is_pv_domain.isra.0 which is the duplicated symbol, and
> being separate translation units, the transformation into .isra.0 may be
> different.

Oh, right - good point.

Jan


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