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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

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.

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.

Finally we likely could avoid the ambiguities altogether, e.g. by
making the compiler emit (relative) directories when populating
STT_FILE symbols. We already qualify multiply compiled source
files by an object file name; I don't see why we couldn't also
suitably qualify singly compiled files (and perhaps there's even
a compiler option, such that we wouldn't need to issue .file
assembler directives ourselves).

Another possibility to avoid (false) ambiguities could be to make
the non-inline expansions of intended-to-be-inline functions
link-once (which by implication means non-static, and hence
necessarily uniquely named).

>> Was the option of altering (amending) the symbol name instead
>> of causing failure considered?
> 
> Amending how?  We've already got one bodge to prepend the filename to
> static symbols.

Right, in an attempt to disambiguate them. If the file names
included (relative) paths, this would have been enough. Since
they don't (for now at least), a sequence number might help.

>> Again, in the common case this
>> shouldn't have overly bad implications: Typically the order of
>> symbols wouldn't change between builds, so tagging the
>> duplicates with a sequence number might be good enough.
> 
> This is painting ourselves into a corner with LTO.

Hmm, yes, it all would depend on the amount of churn
between builds.

> Also, this reasoning depends on people only making trivial changes via
> livepatch, which, as it turns out, isn't the case in practice.
> 
>> 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.

>> Granted there's some heuristic underlying this, in that we then
>> would hope for there not to be actually differing functions with
>> the same source file and symbol names.
> 
> The more assumptions we start making, the more subtly this will go wrong
> in the future.

Yes, I agree - that's the main downside. But failing a build is
a pretty meaningful downside as well, the more when people
may not build with LIVEPATCH=y before they submit. (FTR,
personally I wouldn't consider it appropriate to revert in such
a case, as an issue here has nothing to do with the actual
change, but only with how we post-process data.) And even
if they did, compiler (version) differences may make them not
notice any issue. For this last reason even automatic testing
(osstest or else) may end up assuming all is fine when it's not.

Jan


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