Re: [Xen-devel] [PATCH v2 1/3] livepach: Add .livepatch.hooks functions and test-case

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.08.16 at 23:08, <konrad.wilk@xxxxxxxxxx> wrote:
> On Fri, Aug 05, 2016 at 09:35:49AM -0600, Jan Beulich wrote:
>> >>> On 04.08.16 at 17:49, <konrad.wilk@xxxxxxxxxx> wrote:
>> > In general, the hooks provide flexibility when having to deal with
>> > unforeseen cases, but their application should be rarely required (<
>> > 10%)."
>> 
>> But the greater flexibility of course comes with increased chances
>> of screwing things up. I'm therefore still not entirely convinced that
>> such XSAs wouldn't then better not be live patched.
>> 
>> > --- a/xen/arch/x86/test/xen_hello_world.c
>> > +++ b/xen/arch/x86/test/xen_hello_world.c
>> > @@ -4,14 +4,50 @@
>> >   */
>> >  
>> >  #include "config.h"
>> > +#include <xen/lib.h>
>> >  #include <xen/types.h>
>> >  #include <xen/version.h>
>> >  #include <xen/livepatch.h>
>> > +#include <xen/livepatch_payload.h>
>> >  
>> >  #include <public/sysctl.h>
>> >  
>> >  static char hello_world_patch_this_fnc[] = "xen_extra_version";
>> >  extern const char *xen_hello_world(void);
>> > +static unsigned int cnt;
>> > +
>> > +static void apply_hook(void)
>> > +{
>> > +    printk(KERN_DEBUG "Hook executing.\n");
>> > +}
>> > +
>> > +static void revert_hook(void)
>> > +{
>> > +    printk(KERN_DEBUG "Hook unloaded.\n");
>> > +}
>> > +
>> > +static void hi_func(void)
>> > +{
>> > +    printk(KERN_DEBUG "%s: Hi! (called %u times)\n", __func__, ++cnt);
>> > +};
>> > +
>> > +/* If we are sorted we _MUST_ be the last .livepatch.hook section. */
>> > +static void Z_check_fnc(void)
>> 
>> And that Z_ prefix is supposed to guarantee that being last? Isn't
>> it that upper case letters sort before lower case ones?
> 
> Yes. And the code is actually flexible enough not to be the last one.
> 
> B/c:
>> 
>> > +{
>> > +    printk(KERN_DEBUG "%s: Hi func called %u times\n", __func__, cnt);
>> > +    BUG_ON(cnt == 0 || cnt > 2);
>> > +    cnt = 0; /* Otherwise if you revert, apply, revert the value will be 
>> > 4! */
>> 
>> Isn't this an indication of a general weakness: Shouldn't a patch
>> module be allowed to assume it's being run in a clean state, i.e.
>> without any of its static data altered from their load time values?
> 
> Of this bug. (which I obviously need to fix).

And the fix of which then should perhaps become a prereq to this
change.

>> > @@ -70,7 +71,11 @@ struct payload {
>> >      unsigned int nsyms;                  /* Nr of entries in .strtab and 
>> > symbols. */
>> >      struct livepatch_build_id id;        /* 
>> > ELFNOTE_DESC(.note.gnu.build-id) of the payload. */
>> >      struct livepatch_build_id dep;       /* 
>> > ELFNOTE_DESC(.livepatch.depends). */
>> > -    char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
>> > +    livepatch_loadcall_t **load_funcs;   /* The array of funcs to call 
>> > after */
>> > +    livepatch_unloadcall_t **unload_funcs;/* load and unload of the 
>> > payload. */
>> 
>> These both seem like they want a const in the middle.
> 
> I did that initially and found out that the calling ->load_funcs[i] resulted
> in _no_ code being called.
> 
> I did not dig in the assembler output to figure out what happend, let me
> do that now.

Indeed - I can't see how a const modifier here could alter whether
or not the pointed to function gets called.

>> > +    if ( sec )
>> > +    {
>> > +        if ( !sec->sec->sh_size ||
>> 
>> What's wrong with a zero size section (holding no hooks)? We've
>> had that same case elsewhere in the original series ...
> 
> That would be a bit odd (having zero size).

What's odd there? A simplistic generation tool could simply produce
all sections, no matter whether some of them have no contents.

>> > @@ -999,6 +1025,17 @@ static int apply_payload(struct payload *data)
>> >  
>> >      arch_livepatch_quiesce();
>> >  
>> > +    /*
>> > +     * The hooks may call common code which expects spinlocks to be 
>> > certain
>> > +     * type, as such disable this temporarily.
>> > +     */
>> > +    spin_debug_disable();
>> > +    for ( i = 0; i < data->n_load_funcs; i++ )
>> > +        data->load_funcs[i]();
>> > +    spin_debug_enable();
>> 
>> I have to admit that I can't really make sense of the comment, and
>> hence the code.
> 
> Perhaps:
> 
> Since we are running with IRQs disabled and the hooks may call common
> code - which expects the spinlocks to run with IRQs enabled - we temporarly
> disable the spin locks "IRQ affinity state"

Apart from that last thing in quotes (I'd at least drop "affinity" and
perhaps make it "IRQ state checks", and then without quotes) this
sounds quite a bit better.

Jan

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