* [PATCH] xen/arm: alternative: Make it possible to patch outside of the hypervisor
@ 2016-09-01 7:56 Julien Grall
2016-09-06 19:25 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 5+ messages in thread
From: Julien Grall @ 2016-09-01 7:56 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, sstabellini
With livepatch the alternatives that should be patched are outside of
the Xen hypervisor _start -> _end. The current code is assuming that
only Xen could be patched and therefore will explode when a payload
contains alternatives.
Given that alt_instr contains a relative offset, the function
__apply_alternatives could directly take in parameter the virtual
address of the alt_instr set of the re-mapped region. So we can mandate
the callers of __apply_alternatives to ensure the region has read-write
access beforehand.
The only caller that will patch directly the Xen binary is the function
__apply_alternatives_multi_stop. The other caller apply_alternatives
will work on the payload which will still have read-write access at that
time.
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
This is an alternative of the patch suggested by Konrad [1] to fix
alternatives patching with livepatching.
[1] https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg02880.html
---
xen/arch/arm/alternative.c | 58 +++++++++++++++++++++++++---------------------
1 file changed, 31 insertions(+), 27 deletions(-)
diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 8ee5a11..db4bd0f 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -97,26 +97,11 @@ static u32 get_alt_insn(const struct alt_instr *alt,
static int __apply_alternatives(const struct alt_region *region)
{
const struct alt_instr *alt;
- const u32 *origptr, *replptr;
- u32 *writeptr, *writemap;
- mfn_t text_mfn = _mfn(virt_to_mfn(_stext));
- unsigned int text_order = get_order_from_bytes(_end - _start);
+ const u32 *replptr;
+ u32 *origptr;
printk(XENLOG_INFO "alternatives: Patching kernel code\n");
- /*
- * The text section is read-only. So re-map Xen to be able to patch
- * the code.
- */
- writemap = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
- VMAP_DEFAULT);
- if ( !writemap )
- {
- printk(XENLOG_ERR "alternatives: Unable to map the text section (size %u)\n",
- 1 << text_order);
- return -ENOMEM;
- }
-
for ( alt = region->begin; alt < region->end; alt++ )
{
u32 insn;
@@ -128,7 +113,6 @@ static int __apply_alternatives(const struct alt_region *region)
BUG_ON(alt->alt_len != alt->orig_len);
origptr = ALT_ORIG_PTR(alt);
- writeptr = origptr - (u32 *)_start + writemap;
replptr = ALT_REPL_PTR(alt);
nr_inst = alt->alt_len / sizeof(insn);
@@ -136,19 +120,17 @@ static int __apply_alternatives(const struct alt_region *region)
for ( i = 0; i < nr_inst; i++ )
{
insn = get_alt_insn(alt, origptr + i, replptr + i);
- *(writeptr + i) = cpu_to_le32(insn);
+ *(origptr + i) = cpu_to_le32(insn);
}
/* Ensure the new instructions reached the memory and nuke */
- clean_and_invalidate_dcache_va_range(writeptr,
- (sizeof (*writeptr) * nr_inst));
+ clean_and_invalidate_dcache_va_range(origptr,
+ (sizeof (*origptr) * nr_inst));
}
/* Nuke the instruction cache */
invalidate_icache();
- vunmap(writemap);
-
return 0;
}
@@ -159,10 +141,6 @@ static int __apply_alternatives(const struct alt_region *region)
static int __apply_alternatives_multi_stop(void *unused)
{
static int patched = 0;
- const struct alt_region region = {
- .begin = __alt_instructions,
- .end = __alt_instructions_end,
- };
/* We always have a CPU 0 at this point (__init) */
if ( smp_processor_id() )
@@ -174,12 +152,38 @@ static int __apply_alternatives_multi_stop(void *unused)
else
{
int ret;
+ struct alt_region region;
+ mfn_t xen_mfn = _mfn(virt_to_mfn(_start));
+ unsigned int xen_order = get_order_from_bytes(_end - _start);
+ char *xenmap;
BUG_ON(patched);
+
+ /*
+ * The text and inittext section is read-only. So re-map Xen to
+ * be able to patch the code.
+ */
+ xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR,
+ VMAP_DEFAULT);
+ /* Re-mapping Xen is not expected to fail during boot. */
+ BUG_ON(!xenmap);
+
+ /*
+ * Find the virtual address of the alternative region in the new
+ * mapping.
+ * alt_instr contains relative offset, so the function
+ * __apply_alternatives will patch in the re-mapped version of
+ * Xen.
+ */
+ region.begin = (void *)((char *)__alt_instructions - _start + xenmap);
+ region.end = (void *)((char *)__alt_instructions_end - _start + xenmap);
+
ret = __apply_alternatives(®ion);
/* The patching is not expected to fail during boot. */
BUG_ON(ret != 0);
+ vunmap(xenmap);
+
/* Barriers provided by the cache flushing */
write_atomic(&patched, 1);
}
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] xen/arm: alternative: Make it possible to patch outside of the hypervisor
2016-09-01 7:56 [PATCH] xen/arm: alternative: Make it possible to patch outside of the hypervisor Julien Grall
@ 2016-09-06 19:25 ` Konrad Rzeszutek Wilk
2016-09-07 3:06 ` Konrad Rzeszutek Wilk
2016-09-07 6:52 ` Julien Grall
0 siblings, 2 replies; 5+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-06 19:25 UTC (permalink / raw)
To: Julien Grall; +Cc: sstabellini, xen-devel
On Thu, Sep 01, 2016 at 08:56:39AM +0100, Julien Grall wrote:
> With livepatch the alternatives that should be patched are outside of
> the Xen hypervisor _start -> _end. The current code is assuming that
> only Xen could be patched and therefore will explode when a payload
> contains alternatives.
>
> Given that alt_instr contains a relative offset, the function
> __apply_alternatives could directly take in parameter the virtual
^- the ^- for
> address of the alt_instr set of the re-mapped region. So we can mandate
> the callers of __apply_alternatives to ensure the region has read-write
s/ensure the region/provide us with a region that/
> access beforehand.
How do we mandate it?
Should the __apply_alternatives have an comment about it?
>
> The only caller that will patch directly the Xen binary is the function
> __apply_alternatives_multi_stop. The other caller apply_alternatives
> will work on the payload which will still have read-write access at that
> time.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> This is an alternative of the patch suggested by Konrad [1] to fix
> alternatives patching with livepatching.
>
> [1] https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg02880.html
You also did
s/_stext/_start/
>
> ---
> xen/arch/arm/alternative.c | 58 +++++++++++++++++++++++++---------------------
> 1 file changed, 31 insertions(+), 27 deletions(-)
>
> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> index 8ee5a11..db4bd0f 100644
> --- a/xen/arch/arm/alternative.c
> +++ b/xen/arch/arm/alternative.c
> @@ -97,26 +97,11 @@ static u32 get_alt_insn(const struct alt_instr *alt,
> static int __apply_alternatives(const struct alt_region *region)
> {
> const struct alt_instr *alt;
> - const u32 *origptr, *replptr;
> - u32 *writeptr, *writemap;
> - mfn_t text_mfn = _mfn(virt_to_mfn(_stext));
> - unsigned int text_order = get_order_from_bytes(_end - _start);
> + const u32 *replptr;
> + u32 *origptr;
>
> printk(XENLOG_INFO "alternatives: Patching kernel code\n");
>
> - /*
> - * The text section is read-only. So re-map Xen to be able to patch
> - * the code.
> - */
> - writemap = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
> - VMAP_DEFAULT);
> - if ( !writemap )
> - {
> - printk(XENLOG_ERR "alternatives: Unable to map the text section (size %u)\n",
> - 1 << text_order);
> - return -ENOMEM;
> - }
> -
> for ( alt = region->begin; alt < region->end; alt++ )
> {
> u32 insn;
> @@ -128,7 +113,6 @@ static int __apply_alternatives(const struct alt_region *region)
> BUG_ON(alt->alt_len != alt->orig_len);
>
> origptr = ALT_ORIG_PTR(alt);
> - writeptr = origptr - (u32 *)_start + writemap;
> replptr = ALT_REPL_PTR(alt);
>
> nr_inst = alt->alt_len / sizeof(insn);
> @@ -136,19 +120,17 @@ static int __apply_alternatives(const struct alt_region *region)
> for ( i = 0; i < nr_inst; i++ )
> {
> insn = get_alt_insn(alt, origptr + i, replptr + i);
> - *(writeptr + i) = cpu_to_le32(insn);
> + *(origptr + i) = cpu_to_le32(insn);
> }
>
> /* Ensure the new instructions reached the memory and nuke */
> - clean_and_invalidate_dcache_va_range(writeptr,
> - (sizeof (*writeptr) * nr_inst));
> + clean_and_invalidate_dcache_va_range(origptr,
> + (sizeof (*origptr) * nr_inst));
> }
>
> /* Nuke the instruction cache */
> invalidate_icache();
>
> - vunmap(writemap);
> -
> return 0;
> }
>
> @@ -159,10 +141,6 @@ static int __apply_alternatives(const struct alt_region *region)
> static int __apply_alternatives_multi_stop(void *unused)
> {
> static int patched = 0;
> - const struct alt_region region = {
> - .begin = __alt_instructions,
> - .end = __alt_instructions_end,
> - };
>
> /* We always have a CPU 0 at this point (__init) */
> if ( smp_processor_id() )
> @@ -174,12 +152,38 @@ static int __apply_alternatives_multi_stop(void *unused)
> else
> {
> int ret;
> + struct alt_region region;
> + mfn_t xen_mfn = _mfn(virt_to_mfn(_start));
> + unsigned int xen_order = get_order_from_bytes(_end - _start);
> + char *xenmap;
Could this be 'void *'?
>
> BUG_ON(patched);
> +
> + /*
> + * The text and inittext section is read-only. So re-map Xen to
> + * be able to patch the code.
> + */
> + xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR,
> + VMAP_DEFAULT);
> + /* Re-mapping Xen is not expected to fail during boot. */
> + BUG_ON(!xenmap);
> +
> + /*
> + * Find the virtual address of the alternative region in the new
> + * mapping.
> + * alt_instr contains relative offset, so the function
> + * __apply_alternatives will patch in the re-mapped version of
> + * Xen.
> + */
> + region.begin = (void *)((char *)__alt_instructions - _start + xenmap);
> + region.end = (void *)((char *)__alt_instructions_end - _start + xenmap);
Which I think would make this have less casts?
Oh wait. _start is a char *, so we would have to have:
region.begin = (void *)__alt_instructions - (void *)_start + xenmap;
region.end = (void *)__alt_instructions_end - (void *)_start + xenmap;
Julien, you are the maintainer, so your call.
> +
> ret = __apply_alternatives(®ion);
> /* The patching is not expected to fail during boot. */
> BUG_ON(ret != 0);
>
> + vunmap(xenmap);
> +
> /* Barriers provided by the cache flushing */
> write_atomic(&patched, 1);
> }
> --
> 1.9.1
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] xen/arm: alternative: Make it possible to patch outside of the hypervisor
2016-09-06 19:25 ` Konrad Rzeszutek Wilk
@ 2016-09-07 3:06 ` Konrad Rzeszutek Wilk
2016-09-07 11:36 ` Julien Grall
2016-09-07 6:52 ` Julien Grall
1 sibling, 1 reply; 5+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-07 3:06 UTC (permalink / raw)
To: Julien Grall; +Cc: sstabellini, xen-devel
> > ---
> > xen/arch/arm/alternative.c | 58 +++++++++++++++++++++++++---------------------
> > 1 file changed, 31 insertions(+), 27 deletions(-)
> >
> > diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> > index 8ee5a11..db4bd0f 100644
> > --- a/xen/arch/arm/alternative.c
> > +++ b/xen/arch/arm/alternative.c
> > @@ -97,26 +97,11 @@ static u32 get_alt_insn(const struct alt_instr *alt,
> > static int __apply_alternatives(const struct alt_region *region)
> > {
> > const struct alt_instr *alt;
> > - const u32 *origptr, *replptr;
> > - u32 *writeptr, *writemap;
> > - mfn_t text_mfn = _mfn(virt_to_mfn(_stext));
> > - unsigned int text_order = get_order_from_bytes(_end - _start);
> > + const u32 *replptr;
> > + u32 *origptr;
> >
> > printk(XENLOG_INFO "alternatives: Patching kernel code\n");
This is still there. Should it be "Patching %p->%p code" instead?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] xen/arm: alternative: Make it possible to patch outside of the hypervisor
2016-09-07 3:06 ` Konrad Rzeszutek Wilk
@ 2016-09-07 11:36 ` Julien Grall
0 siblings, 0 replies; 5+ messages in thread
From: Julien Grall @ 2016-09-07 11:36 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: sstabellini, xen-devel
Hi Konrad,
On 07/09/2016 04:06, Konrad Rzeszutek Wilk wrote:
>>> ---
>>> xen/arch/arm/alternative.c | 58 +++++++++++++++++++++++++---------------------
>>> 1 file changed, 31 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
>>> index 8ee5a11..db4bd0f 100644
>>> --- a/xen/arch/arm/alternative.c
>>> +++ b/xen/arch/arm/alternative.c
>>> @@ -97,26 +97,11 @@ static u32 get_alt_insn(const struct alt_instr *alt,
>>> static int __apply_alternatives(const struct alt_region *region)
>>> {
>>> const struct alt_instr *alt;
>>> - const u32 *origptr, *replptr;
>>> - u32 *writeptr, *writemap;
>>> - mfn_t text_mfn = _mfn(virt_to_mfn(_stext));
>>> - unsigned int text_order = get_order_from_bytes(_end - _start);
>>> + const u32 *replptr;
>>> + u32 *origptr;
>>>
>>> printk(XENLOG_INFO "alternatives: Patching kernel code\n");
>
> This is still there. Should it be "Patching %p->%p code" instead?
I will replace by
printk(XENLOG_INFO "alternatives: alt table %p -> %p", region->begin,
region->end);
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/arm: alternative: Make it possible to patch outside of the hypervisor
2016-09-06 19:25 ` Konrad Rzeszutek Wilk
2016-09-07 3:06 ` Konrad Rzeszutek Wilk
@ 2016-09-07 6:52 ` Julien Grall
1 sibling, 0 replies; 5+ messages in thread
From: Julien Grall @ 2016-09-07 6:52 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: sstabellini, xen-devel
Hi Konrad,
On 06/09/2016 20:25, Konrad Rzeszutek Wilk wrote:
> On Thu, Sep 01, 2016 at 08:56:39AM +0100, Julien Grall wrote:
>> With livepatch the alternatives that should be patched are outside of
>> the Xen hypervisor _start -> _end. The current code is assuming that
>> only Xen could be patched and therefore will explode when a payload
>> contains alternatives.
>>
>> Given that alt_instr contains a relative offset, the function
>> __apply_alternatives could directly take in parameter the virtual
> ^- the ^- for
>> address of the alt_instr set of the re-mapped region. So we can mandate
>> the callers of __apply_alternatives to ensure the region has read-write
>
> s/ensure the region/provide us with a region that/
>> access beforehand.
>
> How do we mandate it?
I don't have any idea how to write the check here. I think the only way
would be a comment on __apply_alternatives as you suggested below.
>
> Should the __apply_alternatives have an comment about it?
>
>>
>> The only caller that will patch directly the Xen binary is the function
>> __apply_alternatives_multi_stop. The other caller apply_alternatives
>> will work on the payload which will still have read-write access at that
>> time.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>> This is an alternative of the patch suggested by Konrad [1] to fix
>> alternatives patching with livepatching.
>>
>> [1] https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg02880.html
>
> You also did
>
> s/_stext/_start/
And s/text_mfn/xen_mfn/
I didn't write a separate patch because the code has been reworked quite
a lot. But I am fine to move this in a separate patch if necessary.
>
>>
>> ---
>> xen/arch/arm/alternative.c | 58 +++++++++++++++++++++++++---------------------
>> 1 file changed, 31 insertions(+), 27 deletions(-)
>>
>> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
>> index 8ee5a11..db4bd0f 100644
>> --- a/xen/arch/arm/alternative.c
>> +++ b/xen/arch/arm/alternative.c
>> @@ -97,26 +97,11 @@ static u32 get_alt_insn(const struct alt_instr *alt,
>> static int __apply_alternatives(const struct alt_region *region)
>> {
>> const struct alt_instr *alt;
>> - const u32 *origptr, *replptr;
>> - u32 *writeptr, *writemap;
>> - mfn_t text_mfn = _mfn(virt_to_mfn(_stext));
>> - unsigned int text_order = get_order_from_bytes(_end - _start);
>> + const u32 *replptr;
>> + u32 *origptr;
>>
>> printk(XENLOG_INFO "alternatives: Patching kernel code\n");
>>
>> - /*
>> - * The text section is read-only. So re-map Xen to be able to patch
>> - * the code.
>> - */
>> - writemap = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
>> - VMAP_DEFAULT);
>> - if ( !writemap )
>> - {
>> - printk(XENLOG_ERR "alternatives: Unable to map the text section (size %u)\n",
>> - 1 << text_order);
>> - return -ENOMEM;
>> - }
>> -
>> for ( alt = region->begin; alt < region->end; alt++ )
>> {
>> u32 insn;
>> @@ -128,7 +113,6 @@ static int __apply_alternatives(const struct alt_region *region)
>> BUG_ON(alt->alt_len != alt->orig_len);
>>
>> origptr = ALT_ORIG_PTR(alt);
>> - writeptr = origptr - (u32 *)_start + writemap;
>> replptr = ALT_REPL_PTR(alt);
>>
>> nr_inst = alt->alt_len / sizeof(insn);
>> @@ -136,19 +120,17 @@ static int __apply_alternatives(const struct alt_region *region)
>> for ( i = 0; i < nr_inst; i++ )
>> {
>> insn = get_alt_insn(alt, origptr + i, replptr + i);
>> - *(writeptr + i) = cpu_to_le32(insn);
>> + *(origptr + i) = cpu_to_le32(insn);
>> }
>>
>> /* Ensure the new instructions reached the memory and nuke */
>> - clean_and_invalidate_dcache_va_range(writeptr,
>> - (sizeof (*writeptr) * nr_inst));
>> + clean_and_invalidate_dcache_va_range(origptr,
>> + (sizeof (*origptr) * nr_inst));
>> }
>>
>> /* Nuke the instruction cache */
>> invalidate_icache();
>>
>> - vunmap(writemap);
>> -
>> return 0;
>> }
>>
>> @@ -159,10 +141,6 @@ static int __apply_alternatives(const struct alt_region *region)
>> static int __apply_alternatives_multi_stop(void *unused)
>> {
>> static int patched = 0;
>> - const struct alt_region region = {
>> - .begin = __alt_instructions,
>> - .end = __alt_instructions_end,
>> - };
>>
>> /* We always have a CPU 0 at this point (__init) */
>> if ( smp_processor_id() )
>> @@ -174,12 +152,38 @@ static int __apply_alternatives_multi_stop(void *unused)
>> else
>> {
>> int ret;
>> + struct alt_region region;
>> + mfn_t xen_mfn = _mfn(virt_to_mfn(_start));
>> + unsigned int xen_order = get_order_from_bytes(_end - _start);
>> + char *xenmap;
>
> Could this be 'void *'?
>>
>> BUG_ON(patched);
>> +
>> + /*
>> + * The text and inittext section is read-only. So re-map Xen to
>> + * be able to patch the code.
>> + */
>> + xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR,
>> + VMAP_DEFAULT);
>> + /* Re-mapping Xen is not expected to fail during boot. */
>> + BUG_ON(!xenmap);
>> +
>> + /*
>> + * Find the virtual address of the alternative region in the new
>> + * mapping.
>> + * alt_instr contains relative offset, so the function
>> + * __apply_alternatives will patch in the re-mapped version of
>> + * Xen.
>> + */
>> + region.begin = (void *)((char *)__alt_instructions - _start + xenmap);
>> + region.end = (void *)((char *)__alt_instructions_end - _start + xenmap);
>
> Which I think would make this have less casts?
>
> Oh wait. _start is a char *, so we would have to have:
>
> region.begin = (void *)__alt_instructions - (void *)_start + xenmap;
> region.end = (void *)__alt_instructions_end - (void *)_start + xenmap;
>
> Julien, you are the maintainer, so your call.
Your suggestion seems to be clearer.
>
>> +
>> ret = __apply_alternatives(®ion);
>> /* The patching is not expected to fail during boot. */
>> BUG_ON(ret != 0);
>>
>> + vunmap(xenmap);
>> +
>> /* Barriers provided by the cache flushing */
>> write_atomic(&patched, 1);
>> }
>> --
>> 1.9.1
>>
>
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-09-07 11:36 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-01 7:56 [PATCH] xen/arm: alternative: Make it possible to patch outside of the hypervisor Julien Grall
2016-09-06 19:25 ` Konrad Rzeszutek Wilk
2016-09-07 3:06 ` Konrad Rzeszutek Wilk
2016-09-07 11:36 ` Julien Grall
2016-09-07 6:52 ` Julien Grall
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).