Re: [PATCH v4 6/9] livepatch: Add parsing for the symbol+0x<offset>

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

On Wed, Sep 07, 2016 at 02:10:43AM -0600, Jan Beulich wrote:
> >>> On 06.09.16 at 21:56, <konrad.wilk@oracle.com> wrote:
> > On Wed, Aug 24, 2016 at 03:08:01AM -0600, Jan Beulich wrote:
> >> >>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
> >> > --- a/xen/common/livepatch.c
> >> > +++ b/xen/common/livepatch.c
> >> > @@ -237,13 +237,34 @@ static const char *livepatch_symbols_lookup(unsigned long addr,
> >> >  static int resolve_old_address(struct livepatch_func *f,
> >> >                                 const struct livepatch_elf *elf)
> >> >  {
> >> > +    const char *s;
> >> > +    char *plus = NULL;
> >> 
> >> Pointless initializer.
> > 
> > We need that otherwise this part (which is at the bottom of this function):
> > 
> >     if ( plus )
> >     {
> >         *plus = '+';
> >         f->old_addr += offset;
> >     }
> > 
> > 
> > May be invoked for symbols that that don't have the '+' in them.
> 
> I don't see how it would. This
> 
>     plus = strchr(f->name, '+');
> 
> comes ahead of any paths leading to the code you quote.

Ah. Stale information - the earlier patch had 'slash' and 'plus' variables
to look for - and that was why I needed it.

But with the code you are quoting - it is not needed.
> 
> >> > +    /* +<offset> */
> >> > +    plus = strchr(f->name, '+');
> >> 
> >> And I think you should prefer using the local variable here.
> > 
> > <nods>
> > 
> >> 
> >> Furthermore you're losing const here - does f->name really point
> >> to memory that doesn't get mapped r/o?
> > 
> > Yes.
> > 
> > The 'struct livepatch_func' contains the the ->opaque array of 31 bytes
> > (from which we use 5 bytes) which the hypervisor uses to stash the original
> > instructions.
> 
> How does the patch name end up in (5 bytes of) the opaque field?

I was (ineptly) saying that the struct livepatch_func has fields that are
modified, hence it ends up in .data section.

Wait a minute. The f->name should have a relocation to point to .rodata
instead of .data! And that should have crashed when I modified it.

Ah, they are all 'static char name[] = "blah"' instead of
'static const char name[] = "blah"'.

Patch queued up.

> In any event the correctness of deliberately stripping const should
> be explained in a comment (if, of course, it can't be avoided in the
> first place).
> 
> >> Overall - are you sure you want to disallow symbol names containing
> >> + characters? I.e. you don't want to add support for some form of
> >> quoting?
> > 
> > Can you actually have + in a function or object?
> 
> Why not? The ELF spec, iirc, doesn't put any restrictions on what
> characters (other than nul of course) can be used in symbol names.
> gas actually has received full quoting support a year or two ago,
> to no longer needlessly restrict the character set available here.

I was thinking of + in the C land. But that is irrelevant to this
discussion.

Let me dig in the gas code to find examples of this - but in the
meantime (and if you recall), you meant something like this:

"do_domain_pause+something"

?
Which would mean for offset purposes I would need to deal with:

"do_domain_pause+something"+0x10

or
'do_domain_pause+something'+0x10



> 
> Jan
> 

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