Re: Livepatch, symbol resolutions between two livepatchs (new_symbol=0)

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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.

> Let's have a discussion about this and the symbol issues here at the Xen
> Summit in a couple of weeks time.

/me nods.
> 
> -- 
> Ross Lagerwall

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