Re: [Xen-devel] [PATCH v5 11/28] xsplice: Implement support for applying/reverting/replacing patches.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.04.16 at 05:09, <konrad.wilk@xxxxxxxxxx> wrote:
>> > +    uint8_t *old_ptr;
>> > +
>> > +    BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->undo));
>> > +    BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof val));
>> > +
>> > +    old_ptr = (uint8_t *)func->old_addr;
>> 
>> (Considering this cast, the "old_addr" member should be
>> unsigned long (or void *), not uint64_t: The latest on ARM32
>> such would otherwise cause problems.)
> 
> I has to be uint8_t to make the single byte modifications. Keep
> also in mind that this file is only for x86.

old_addr can't reasonably be uint8_t, so I can only assume you're
mixing up things here. (And yes, I do realize this is x86 code, but
my reference to ARM32 was only mean to say that there you'll
need to do something similar, and casting uint64_t to whatever
kind of pointer type is not going to work without compiler warning.)

>> Also - where is the verification that
>> func->old_size >= PATCH_INSN_SIZE?
> 
> I made a 'arch_xsplice_verify_func' to have per-architecture check for
> that.
> ..
>> > +        sec = xsplice_elf_sec_by_name(elf, names[i]);
>> > +        if ( !sec )
>> > +        {
>> > +            printk(XENLOG_ERR "%s%s: %s is missing!\n",
>> > +                   XSPLICE, elf->name, names[i]);
>> > +            return -EINVAL;
>> > +        }
>> > +
>> > +        if ( !sec->sec->sh_size )
>> > +            return -EINVAL;
>> > +    }
>> > +
>> > +    return 0;
>> > +}
>> 
>> So you check for there being one such section. Is having multiple
>> of them okay / meaningful?
> 
> I've been going back on this. I think at this point I will say no.
> I am going to look at what is involved in this in later versions.

And check for there being exactly one for now, I then assume?

Jan


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