Hi Andrew,
On 30/01/18 11:24, Andrew Cooper wrote:
On 30/01/18 11:05, Julien Grall wrote:
Hi Andrew,
On 29/01/18 15:38, Andrew Cooper wrote:
On x86, we would like to alter how we patch based on whether there is
any
chance of the code being patched being concurrently executed.
prepare_payload() passes false (as the livepatch definitely isn't
live at this
point), whereas the boot-time alternatives application passes true.
Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
---
CC: Jan Beulich <jbeul...@suse.com>
CC: Stefano Stabellini <sstabell...@kernel.org>
CC: Julien Grall <julien.gr...@arm.com>
CC: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
CC: Ross Lagerwall <ross.lagerw...@citrix.com>
---
xen/arch/arm/alternative.c | 10 ++++++----
xen/arch/x86/alternative.c | 5 +++--
xen/common/livepatch.c | 2 +-
xen/include/asm-arm/alternative.h | 6 ++++--
xen/include/asm-x86/alternative.h | 3 ++-
5 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 99112e1..078b259 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -98,7 +98,8 @@ static u32 get_alt_insn(const struct alt_instr *alt,
* The region patched should be read-write to allow
__apply_alternatives
* to replacing the instructions when necessary.
*/
-static void __apply_alternatives(const struct alt_region *region)
+static void __apply_alternatives(const struct alt_region *region,
+ bool live)
{
const struct alt_instr *alt;
const u32 *replptr;
@@ -193,7 +194,7 @@ static int __init
__apply_alternatives_multi_stop(void *unused)
region.begin = (void *)__alt_instructions - (void *)_start
+ xenmap;
region.end = (void *)__alt_instructions_end - (void
*)_start + xenmap;
- __apply_alternatives(®ion);
+ __apply_alternatives(®ion, true);
unregister_virtual_region(&patch_region);
@@ -224,14 +225,15 @@ void __init apply_alternatives_all(void)
}
void apply_alternatives(const struct alt_instr *start,
- const struct alt_instr *end)
+ const struct alt_instr *end,
+ bool live)
This function is not able to deal with "live" code, so I think at
least need an ASSERT(!live) to prevent mis-usage of the code.
This passes straight through into __apply_alternatives(), just like
__apply_alternatives_multi_stop does, and multi_stop is used on live code.
Either both are unsafe (although all evidence to the contrary), or both
are safe, but I don't think that an assert here is appropriate.
I disagree here. In the commit message you wrote: "On x86, we would like
to alter how we patch based on whether there is any chance of the code
being patched being concurrently executed."
I translate this as all the other CPUs may be alive and the code would
be mapped with read-executable permission (no write permission). It will
not be easily possible to make the region writable because the processor
has been configured to forbid it.
__apply_alternatives relies on the region patched to be write accessible
and the region not executed by any CPUs.
__apply_alternatives_multi_stop has the logic make the write
accessible. This is not the case of apply_alternatives.
So the former function is safe while the latter one is unsafe when live
is true.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
