Re: [Xen-devel] [PATCH v1 1/3] livepatch: Add local and global symbol resolution.

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

On Tue, Jun 20, 2017 at 01:51:41AM -0600, Jan Beulich wrote:
> >>> On 20.06.17 at 04:47, <konrad.wilk@xxxxxxxxxx> wrote:
> > This way we can load livepatches with symbol names that
> > are the same as long as they are local ('static').
> > 
> > The use case here is to replace an existing livepatch
> > with a newer one - and one which has the same local symbols.
> > 
> > Without this patch we get:
> > livepatch.c:819: livepatch: xen_local_symbols: duplicate new symbol: 
> > revert_hook
> > 
> > when loading the new livepatch (before doing the replace).
> > 
> > This due to livepatch assuming that all symbols are all
> > global. With this patch:
> > 
> > readelf --symbols xen_hello_world.livepatch
> > File: xen_hello_world.livepatch
> > 
> > Symbol table '.symtab' contains 55 entries:
> >    Num:    Value          Size Type    Bind   Vis      Ndx Name
> > ..snip..
> >     34: 0000000000000000     4 OBJECT  LOCAL  DEFAULT   25 cnt
> >     35: 000000000000000a     8 OBJECT  LOCAL  DEFAULT    7 __func__.4654
> >     36: 0000000000000065    23 FUNC    LOCAL  DEFAULT    2 revert_hook
> >     37: 000000000000007c    23 FUNC    LOCAL  DEFAULT    2 apply_hook
> >     38: 0000000000000093    54 FUNC    LOCAL  DEFAULT    2 check_fnc
> > ..snip..
> > 
> >     47: 0000000000000000    54 FUNC    GLOBAL HIDDEN     2 xen_hello_world
> >     48: 0000000000000000     0 NOTYPE  GLOBAL HIDDEN   UND xen_extra_version
> > ..snip..
> >     52: 0000000000000000     0 NOTYPE  GLOBAL HIDDEN   UND printk
> >     53: 0000000000000000    64 OBJECT  GLOBAL HIDDEN    23 
> > livepatch_xen_hello_world
> > 
> > All the 'GLOBAL' have to be unique per livepatch. But the
> > 'LOCAL' can all be the same which means the semantic of 'static'
> > on functions and data variables is the right one.
> 
> I think this is wrong: Afaict your change results in main image and
> patch local symbols to now be treated differently. While this may
> indeed help patches which are meant to replace others, it is going
> to get in the way if a patch wants to reference a local symbol
> already altered (or newly introduced) by a prior one.
> 
> Question then is at what point in time name collisions should be
> detected: In order for patch replacement to work, this obviously
> shouldn't happen at the time the patch is being loaded, but in the
> course of being applied. Otoh iirc "replace" doesn't use separate
> old and new patch names, so the system must already be aware
> of the correlation between them, and hence collision detection at
> patch load time may still be possible (special casing the patch
> being replaced).

This gets complicated, let me break it down to make sure I got it right.

I think what you are saying is that after a livepatch has been
applied the distinction of global/local should be lifted for
that livepatch.

But before we even get to that stage we have to deal with
the situation in which two (or more) livepatches have identical
local symbols ('revert_hook' for example). And there the
local symbols collision should not happen.

Now you mention an interesting case "if a patch wants to reference
a local symbol already altered (or newly introduced) by a prior one"

Let me break that apart, the "(or newly introduced)" meaning
we would have say two livepatches already applied.
The first one adds this new symbol and the second one
depends on the first livepatch (and uses the symbol).
We are trying to replace the second one with a newer version. 

[I remember talking to Ross about this: 
https://lists.xen.org/archives/html/xen-devel/2016-08/msg01756.html]

If the newly introduced symbol is local, the second livepatch
should _not_ be able to resolve it [But it does today, this patch
fixes this].

If the newly introduced symbol is global, the second livepatch
should be able to resolve it [and it does today]

If this symbol we want to reference is not within the live
patches, but in the main code - then:

If the symbol is local, we will still find it (symbols_lookup_by_name)
If the symbol is global, we will still find it (symbols_lookup_by_name)

Ah, so your point is that since the main code does not provide
_any_ semantics about local/global symbols, then by extension
the livepatches should neither.

While my patch does introduce this distinction?

Hmm.

Well, what I can say is that this issue (local symbol collision
in the livepatches) has been bitting us since November.

Our mechanism when deploying livepatches is to replace the loaded
livepatch with another one. Which means we only have on livepatch
applied and during the upgrade process have to load another one.

In November (XSA-204) we started shipping in the livepatches
a new local symbol (x86_emulate.c#_get_fpu).

livepatch: 617712b: new symbol x86_emulate.c#_get_fpu

Then when the next XSA came about and we made a new livepatch that
included everything prior and the new one, and we got:

livepatch.c:751: livepatch: fbab204: duplicate new symbol: 
x86_emulate.c#_get_fpu

which aborted the replacement of the livepatch.

The solution at hand was to rename the offending local symbol to
something more unique (the name of the livepatch):

livepatch: fbab204: new symbol x86_emulate.c#_get_fpu_fbab204

Which we are doing now for every livepatch. However I would
like a better mechanism, which is why I am proposing this patch.

P.S.
[Note to myself: Include this long explanation in the commit description]


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