Re: [Xen-devel] Livepatch, symbol resolutions between two livepatchs (new_symbol=0)

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

On Fri, Aug 12, 2016 at 09:51:39AM -0400, Konrad Rzeszutek Wilk wrote:
> On Thu, Aug 11, 2016 at 09:11:10AM +0100, Ross Lagerwall wrote:
> > On 08/11/2016 02:28 AM, Konrad Rzeszutek Wilk wrote:
> > > Hey Ross,
> > > 
> > > I am running in a symbol dependency issue that I am not exactly
> > > sure how to solve.
> > > 
> > > I have an payload that introduces a new function (xen_foobar) which
> > > will patch over xen_extra_version().
> > > 
> > snip
> > > 
> > > As livepatch_symbols_lookup_by_name only looks for symbols that
> > > have the ->new_symbol set. And xen_foobar does not. So the loading is
> > > aborted.
> > > 
> > > Which makes sense - we don't want to match the symbols as they haven't
> > > really been "finally loaded" in.
> > > 
> > > But what if the xen_foobar is applied. In that case we should
> > > change the xen_foobar to be new_symbol=1?
> > 
> > I think you're confused about the purpose of new_symbol. The purpose is to
> > ensure that you link against the correct symbol from the base hypervisor or
> > the live patch that first introduced it. So, new_symbol=0 is when a symbol
> > overrides an existing symbol. new_symbol=1 is set when a symbol is new
> 
> But it does not (overrides the existing symbol).
> 
> The patch (xen_foobar) introduces a new function called xen_foobar
> which is patching xen_extra_version.
> 
> That is:
> 
> static char foobar_patch_this_fnc[] = "xen_extra_version";
> 
> struct livepatch_func __section(".livepatch.funcs") livepatch_xen_foobar = { 
>     .version = LIVEPATCH_PAYLOAD_VERSION,
>     .name = foobar_patch_this_fnc,
>     .new_addr = xen_foobar,
>     .old_addr = xen_extra_version,
>     .new_size = NEW_CODE_SZ,
>     .old_size = OLD_CODE_SZ,
> };
> 
> > introduced in a live patch.
> 
> And this loop:
> 
>     for ( j = 0; j < payload->nfuncs; j++ )                                 
>         {                                                                     
>   
>             if ( symtab[i].value == (unsigned long)payload->funcs[j].new_addr 
> ) 
>             {                                                                 
>   
>                 found = 1;                                                    
>   
>                 break;                                                        
>   
>             }                                                                 
>   
>         }                                
> 
> Will force new_symbol=0 for xen_foobar.
> 
> > 
> > Since all the linking happens during load and not apply, it is perfectly OK
> > to link against a symbol that hasn't been applied -- the dependencies are
> > there to ensure that you can't apply a patch which links against unapplied
> > symbols.
> > 
> > The assumption is that when overriding an existing symbol, the symbol in the
> > payload has the same name as the one it is overriding. You're having issues
> > above because you're breaking this assumption.
> 
> Yes :-)
> 
> > 
> > > 
> > > This following patch does that, but I am wondering if there is a better
> > > way?
> > 
> > The patch is misusing new_symbol for something completely different from how
> > it was intended so I hope there is a better way :-P
> 
> Well for my use-case I think I can just s/xen_foobar/xen_extra_version/ and we
> should be OK.

Ah no.

It does work for xen_foo (so it replaces xen_extra_version with its own 
'xen_extra_version'.

But when I introduce xen_foobar_nop and it tries to look for 'xen_extra_version'
it picks the hypervisor one (which has been patched over) instead
of the livepatched version.

This may require some extra lookup in the applied_list for the symbols
before consulting and trying to match up the symbols in the built-in list.

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