* [PATCH for-4.9 v3 0/3] Fixes for livepatching @ 2017-06-22 18:15 Andrew Cooper 2017-06-22 18:15 ` [PATCH for-4.9 v3 1/3] xen/livepatch: Clean up arch relocation handling Andrew Cooper ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Andrew Cooper @ 2017-06-22 18:15 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper Andrew Cooper (3): xen/livepatch: Clean up arch relocation handling xen/livepatch: Use zeroed memory allocations for arrays xen/livepatch: Don't crash on encountering STN_UNDEF relocations xen/arch/arm/arm32/livepatch.c | 41 +++++++++++++++++++++++++---------------- xen/arch/arm/arm64/livepatch.c | 33 ++++++++++++++++++++------------- xen/arch/x86/livepatch.c | 27 ++++++++++++++++++--------- xen/common/livepatch.c | 4 ++-- xen/common/livepatch_elf.c | 4 ++-- 5 files changed, 67 insertions(+), 42 deletions(-) -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH for-4.9 v3 1/3] xen/livepatch: Clean up arch relocation handling 2017-06-22 18:15 [PATCH for-4.9 v3 0/3] Fixes for livepatching Andrew Cooper @ 2017-06-22 18:15 ` Andrew Cooper 2017-06-22 18:15 ` [PATCH for-4.9 v3 2/3] xen/livepatch: Use zeroed memory allocations for arrays Andrew Cooper 2017-06-22 18:15 ` [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations Andrew Cooper 2 siblings, 0 replies; 22+ messages in thread From: Andrew Cooper @ 2017-06-22 18:15 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper * Reduce symbol scope and initalisation as much as possible * Annotate a fallthrough case in arm64 * Fix switch statement style in arm32 No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- xen/arch/arm/arm32/livepatch.c | 27 ++++++++++++--------------- xen/arch/arm/arm64/livepatch.c | 19 +++++++------------ xen/arch/x86/livepatch.c | 13 +++++-------- 3 files changed, 24 insertions(+), 35 deletions(-) diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c index a7fd5e2..a328179 100644 --- a/xen/arch/arm/arm32/livepatch.c +++ b/xen/arch/arm/arm32/livepatch.c @@ -224,21 +224,21 @@ int arch_livepatch_perform(struct livepatch_elf *elf, const struct livepatch_elf_sec *rela, bool use_rela) { - const Elf_RelA *r_a; - const Elf_Rel *r; - unsigned int symndx, i; - uint32_t val; - void *dest; + unsigned int i; int rc = 0; for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ ) { + unsigned int symndx; + uint32_t val; + void *dest; unsigned char type; - s32 addend = 0; + s32 addend; if ( use_rela ) { - r_a = rela->data + i * rela->sec->sh_entsize; + const Elf_RelA *r_a = rela->data + i * rela->sec->sh_entsize; + symndx = ELF32_R_SYM(r_a->r_info); type = ELF32_R_TYPE(r_a->r_info); dest = base->load_addr + r_a->r_offset; /* P */ @@ -246,10 +246,12 @@ int arch_livepatch_perform(struct livepatch_elf *elf, } else { - r = rela->data + i * rela->sec->sh_entsize; + const Elf_Rel *r = rela->data + i * rela->sec->sh_entsize; + symndx = ELF32_R_SYM(r->r_info); type = ELF32_R_TYPE(r->r_info); dest = base->load_addr + r->r_offset; /* P */ + addend = get_addend(type, dest); } if ( symndx > elf->nsym ) @@ -259,13 +261,11 @@ int arch_livepatch_perform(struct livepatch_elf *elf, return -EINVAL; } - if ( !use_rela ) - addend = get_addend(type, dest); - val = elf->sym[symndx].sym->st_value; /* S */ rc = perform_rel(type, dest, val, addend); - switch ( rc ) { + switch ( rc ) + { case -EOVERFLOW: dprintk(XENLOG_ERR, LIVEPATCH "%s: Overflow in relocation %u in %s for %s!\n", elf->name, i, rela->name, base->name); @@ -275,9 +275,6 @@ int arch_livepatch_perform(struct livepatch_elf *elf, dprintk(XENLOG_ERR, LIVEPATCH "%s: Unhandled relocation #%x\n", elf->name, type); break; - - default: - break; } if ( rc ) diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c index dae64f5..63929b1 100644 --- a/xen/arch/arm/arm64/livepatch.c +++ b/xen/arch/arm/arm64/livepatch.c @@ -241,19 +241,16 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf, const struct livepatch_elf_sec *base, const struct livepatch_elf_sec *rela) { - const Elf_RelA *r; - unsigned int symndx, i; - uint64_t val; - void *dest; - bool_t overflow_check; + unsigned int i; for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ ) { + const Elf_RelA *r = rela->data + i * rela->sec->sh_entsize; + unsigned int symndx = ELF64_R_SYM(r->r_info); + void *dest = base->load_addr + r->r_offset; /* P */ + bool overflow_check = true; int ovf = 0; - - r = rela->data + i * rela->sec->sh_entsize; - - symndx = ELF64_R_SYM(r->r_info); + uint64_t val; if ( symndx > elf->nsym ) { @@ -262,11 +259,8 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf, return -EINVAL; } - dest = base->load_addr + r->r_offset; /* P */ val = elf->sym[symndx].sym->st_value + r->r_addend; /* S+A */ - overflow_check = true; - /* ARM64 operations at minimum are always 32-bit. */ if ( r->r_offset >= base->sec->sh_size || (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size ) @@ -403,6 +397,7 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf, case R_AARCH64_ADR_PREL_PG_HI21_NC: overflow_check = false; + /* Fallthrough. */ case R_AARCH64_ADR_PREL_PG_HI21: ovf = reloc_insn_imm(RELOC_OP_PAGE, dest, val, 12, 21, AARCH64_INSN_IMM_ADR); diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c index dd50dd1..7917610 100644 --- a/xen/arch/x86/livepatch.c +++ b/xen/arch/x86/livepatch.c @@ -161,16 +161,14 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf, const struct livepatch_elf_sec *base, const struct livepatch_elf_sec *rela) { - const Elf_RelA *r; - unsigned int symndx, i; - uint64_t val; - uint8_t *dest; + unsigned int i; for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ ) { - r = rela->data + i * rela->sec->sh_entsize; - - symndx = ELF64_R_SYM(r->r_info); + const Elf_RelA *r = rela->data + i * rela->sec->sh_entsize; + unsigned int symndx = ELF64_R_SYM(r->r_info); + uint8_t *dest = base->load_addr + r->r_offset; + uint64_t val; if ( symndx > elf->nsym ) { @@ -179,7 +177,6 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf, return -EINVAL; } - dest = base->load_addr + r->r_offset; val = r->r_addend + elf->sym[symndx].sym->st_value; switch ( ELF64_R_TYPE(r->r_info) ) -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH for-4.9 v3 2/3] xen/livepatch: Use zeroed memory allocations for arrays 2017-06-22 18:15 [PATCH for-4.9 v3 0/3] Fixes for livepatching Andrew Cooper 2017-06-22 18:15 ` [PATCH for-4.9 v3 1/3] xen/livepatch: Clean up arch relocation handling Andrew Cooper @ 2017-06-22 18:15 ` Andrew Cooper 2017-06-23 2:55 ` Konrad Rzeszutek Wilk 2017-06-23 12:31 ` Ross Lagerwall 2017-06-22 18:15 ` [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations Andrew Cooper 2 siblings, 2 replies; 22+ messages in thread From: Andrew Cooper @ 2017-06-22 18:15 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Ross Lagerwall Each of these arrays is sparse. Use zeroed allocations to cause uninitialised array elements to contain deterministic values, most importantly for the embedded pointers. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Ross Lagerwall <ross.lagerwall@citrix.com> * new in v3 --- xen/common/livepatch.c | 4 ++-- xen/common/livepatch_elf.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index df67a1a..66d532d 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -771,8 +771,8 @@ static int build_symbol_table(struct payload *payload, } } - symtab = xmalloc_array(struct livepatch_symbol, nsyms); - strtab = xmalloc_array(char, strtab_len); + symtab = xzalloc_array(struct livepatch_symbol, nsyms); + strtab = xzalloc_array(char, strtab_len); if ( !strtab || !symtab ) { diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c index c4a9633..b69e271 100644 --- a/xen/common/livepatch_elf.c +++ b/xen/common/livepatch_elf.c @@ -52,7 +52,7 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data) int rc; /* livepatch_elf_load sanity checked e_shnum. */ - sec = xmalloc_array(struct livepatch_elf_sec, elf->hdr->e_shnum); + sec = xzalloc_array(struct livepatch_elf_sec, elf->hdr->e_shnum); if ( !sec ) { dprintk(XENLOG_ERR, LIVEPATCH"%s: Could not allocate memory for section table!\n", @@ -225,7 +225,7 @@ static int elf_get_sym(struct livepatch_elf *elf, const void *data) /* No need to check values as elf_resolve_sections did it. */ nsym = symtab_sec->sec->sh_size / symtab_sec->sec->sh_entsize; - sym = xmalloc_array(struct livepatch_elf_sym, nsym); + sym = xzalloc_array(struct livepatch_elf_sym, nsym); if ( !sym ) { dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not allocate memory for symbols\n", -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 2/3] xen/livepatch: Use zeroed memory allocations for arrays 2017-06-22 18:15 ` [PATCH for-4.9 v3 2/3] xen/livepatch: Use zeroed memory allocations for arrays Andrew Cooper @ 2017-06-23 2:55 ` Konrad Rzeszutek Wilk 2017-06-23 12:31 ` Ross Lagerwall 1 sibling, 0 replies; 22+ messages in thread From: Konrad Rzeszutek Wilk @ 2017-06-23 2:55 UTC (permalink / raw) To: Andrew Cooper; +Cc: Ross Lagerwall, Xen-devel On Thu, Jun 22, 2017 at 07:15:28PM +0100, Andrew Cooper wrote: > Each of these arrays is sparse. Use zeroed allocations to cause uninitialised > array elements to contain deterministic values, most importantly for the > embedded pointers. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> [x86 and ARM32] > CC: Ross Lagerwall <ross.lagerwall@citrix.com> > > * new in v3 > --- > xen/common/livepatch.c | 4 ++-- > xen/common/livepatch_elf.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c > index df67a1a..66d532d 100644 > --- a/xen/common/livepatch.c > +++ b/xen/common/livepatch.c > @@ -771,8 +771,8 @@ static int build_symbol_table(struct payload *payload, > } > } > > - symtab = xmalloc_array(struct livepatch_symbol, nsyms); > - strtab = xmalloc_array(char, strtab_len); > + symtab = xzalloc_array(struct livepatch_symbol, nsyms); > + strtab = xzalloc_array(char, strtab_len); > > if ( !strtab || !symtab ) > { > diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c > index c4a9633..b69e271 100644 > --- a/xen/common/livepatch_elf.c > +++ b/xen/common/livepatch_elf.c > @@ -52,7 +52,7 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data) > int rc; > > /* livepatch_elf_load sanity checked e_shnum. */ > - sec = xmalloc_array(struct livepatch_elf_sec, elf->hdr->e_shnum); > + sec = xzalloc_array(struct livepatch_elf_sec, elf->hdr->e_shnum); > if ( !sec ) > { > dprintk(XENLOG_ERR, LIVEPATCH"%s: Could not allocate memory for section table!\n", > @@ -225,7 +225,7 @@ static int elf_get_sym(struct livepatch_elf *elf, const void *data) > /* No need to check values as elf_resolve_sections did it. */ > nsym = symtab_sec->sec->sh_size / symtab_sec->sec->sh_entsize; > > - sym = xmalloc_array(struct livepatch_elf_sym, nsym); > + sym = xzalloc_array(struct livepatch_elf_sym, nsym); > if ( !sym ) > { > dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not allocate memory for symbols\n", > -- > 2.1.4 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 2/3] xen/livepatch: Use zeroed memory allocations for arrays 2017-06-22 18:15 ` [PATCH for-4.9 v3 2/3] xen/livepatch: Use zeroed memory allocations for arrays Andrew Cooper 2017-06-23 2:55 ` Konrad Rzeszutek Wilk @ 2017-06-23 12:31 ` Ross Lagerwall 1 sibling, 0 replies; 22+ messages in thread From: Ross Lagerwall @ 2017-06-23 12:31 UTC (permalink / raw) To: Andrew Cooper, Xen-devel On 06/22/2017 07:15 PM, Andrew Cooper wrote: > Each of these arrays is sparse. Use zeroed allocations to cause uninitialised > array elements to contain deterministic values, most importantly for the > embedded pointers. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-22 18:15 [PATCH for-4.9 v3 0/3] Fixes for livepatching Andrew Cooper 2017-06-22 18:15 ` [PATCH for-4.9 v3 1/3] xen/livepatch: Clean up arch relocation handling Andrew Cooper 2017-06-22 18:15 ` [PATCH for-4.9 v3 2/3] xen/livepatch: Use zeroed memory allocations for arrays Andrew Cooper @ 2017-06-22 18:15 ` Andrew Cooper 2017-06-22 21:10 ` Stefano Stabellini ` (4 more replies) 2 siblings, 5 replies; 22+ messages in thread From: Andrew Cooper @ 2017-06-22 18:15 UTC (permalink / raw) To: Xen-devel Cc: Stefano Stabellini, Andrew Cooper, Ross Lagerwall, Julien Grall, Jan Beulich A symndx of STN_UNDEF is special, and means a symbol value of 0. While legitimate in the ELF standard, its existance in a livepatch is questionable at best. Until a plausible usecase presents itself, reject such a relocation with -EOPNOTSUPP. Additionally, fix an off-by-one error while range checking symndx, and perform a safety check on elf->sym[symndx].sym before derefencing it, to avoid tripping over a NULL pointer when calculating val. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Ross Lagerwall <ross.lagerwall@citrix.com> CC: Jan Beulich <JBeulich@suse.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien.grall@arm.com> v3: * Fix off-by-one error v2: * Reject STN_UNDEF with -EOPNOTSUPP --- xen/arch/arm/arm32/livepatch.c | 14 +++++++++++++- xen/arch/arm/arm64/livepatch.c | 14 +++++++++++++- xen/arch/x86/livepatch.c | 14 +++++++++++++- 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c index a328179..41378a5 100644 --- a/xen/arch/arm/arm32/livepatch.c +++ b/xen/arch/arm/arm32/livepatch.c @@ -254,12 +254,24 @@ int arch_livepatch_perform(struct livepatch_elf *elf, addend = get_addend(type, dest); } - if ( symndx > elf->nsym ) + if ( symndx == STN_UNDEF ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n", + elf->name); + return -EOPNOTSUPP; + } + else if ( symndx >= elf->nsym ) { dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative symbol wants symbol@%u which is past end!\n", elf->name, symndx); return -EINVAL; } + else if ( !elf->sym[symndx].sym ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n", + elf->name, symndx); + return -EINVAL; + } val = elf->sym[symndx].sym->st_value; /* S */ diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c index 63929b1..2247b92 100644 --- a/xen/arch/arm/arm64/livepatch.c +++ b/xen/arch/arm/arm64/livepatch.c @@ -252,12 +252,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf, int ovf = 0; uint64_t val; - if ( symndx > elf->nsym ) + if ( symndx == STN_UNDEF ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n", + elf->name); + return -EOPNOTSUPP; + } + else if ( symndx >= elf->nsym ) { dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n", elf->name, symndx); return -EINVAL; } + else if ( !elf->sym[symndx].sym ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n", + elf->name, symndx); + return -EINVAL; + } val = elf->sym[symndx].sym->st_value + r->r_addend; /* S+A */ diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c index 7917610..406eb91 100644 --- a/xen/arch/x86/livepatch.c +++ b/xen/arch/x86/livepatch.c @@ -170,12 +170,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf, uint8_t *dest = base->load_addr + r->r_offset; uint64_t val; - if ( symndx > elf->nsym ) + if ( symndx == STN_UNDEF ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n", + elf->name); + return -EOPNOTSUPP; + } + else if ( symndx >= elf->nsym ) { dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n", elf->name, symndx); return -EINVAL; } + else if ( !elf->sym[symndx].sym ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n", + elf->name, symndx); + return -EINVAL; + } val = r->r_addend + elf->sym[symndx].sym->st_value; -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-22 18:15 ` [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations Andrew Cooper @ 2017-06-22 21:10 ` Stefano Stabellini 2017-06-23 2:56 ` Konrad Rzeszutek Wilk ` (3 subsequent siblings) 4 siblings, 0 replies; 22+ messages in thread From: Stefano Stabellini @ 2017-06-22 21:10 UTC (permalink / raw) To: Andrew Cooper Cc: Stefano Stabellini, Xen-devel, Ross Lagerwall, Julien Grall, Jan Beulich On Thu, 22 Jun 2017, Andrew Cooper wrote: > A symndx of STN_UNDEF is special, and means a symbol value of 0. While > legitimate in the ELF standard, its existance in a livepatch is questionable > at best. Until a plausible usecase presents itself, reject such a relocation > with -EOPNOTSUPP. > > Additionally, fix an off-by-one error while range checking symndx, and perform > a safety check on elf->sym[symndx].sym before derefencing it, to avoid > tripping over a NULL pointer when calculating val. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > CC: Ross Lagerwall <ross.lagerwall@citrix.com> > CC: Jan Beulich <JBeulich@suse.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien.grall@arm.com> > > v3: > * Fix off-by-one error > v2: > * Reject STN_UNDEF with -EOPNOTSUPP Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/arch/arm/arm32/livepatch.c | 14 +++++++++++++- > xen/arch/arm/arm64/livepatch.c | 14 +++++++++++++- > xen/arch/x86/livepatch.c | 14 +++++++++++++- > 3 files changed, 39 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c > index a328179..41378a5 100644 > --- a/xen/arch/arm/arm32/livepatch.c > +++ b/xen/arch/arm/arm32/livepatch.c > @@ -254,12 +254,24 @@ int arch_livepatch_perform(struct livepatch_elf *elf, > addend = get_addend(type, dest); > } > > - if ( symndx > elf->nsym ) > + if ( symndx == STN_UNDEF ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n", > + elf->name); > + return -EOPNOTSUPP; > + } > + else if ( symndx >= elf->nsym ) > { > dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative symbol wants symbol@%u which is past end!\n", > elf->name, symndx); > return -EINVAL; > } > + else if ( !elf->sym[symndx].sym ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n", > + elf->name, symndx); > + return -EINVAL; > + } > > val = elf->sym[symndx].sym->st_value; /* S */ > > diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c > index 63929b1..2247b92 100644 > --- a/xen/arch/arm/arm64/livepatch.c > +++ b/xen/arch/arm/arm64/livepatch.c > @@ -252,12 +252,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf, > int ovf = 0; > uint64_t val; > > - if ( symndx > elf->nsym ) > + if ( symndx == STN_UNDEF ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n", > + elf->name); > + return -EOPNOTSUPP; > + } > + else if ( symndx >= elf->nsym ) > { > dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n", > elf->name, symndx); > return -EINVAL; > } > + else if ( !elf->sym[symndx].sym ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n", > + elf->name, symndx); > + return -EINVAL; > + } > > val = elf->sym[symndx].sym->st_value + r->r_addend; /* S+A */ > > diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c > index 7917610..406eb91 100644 > --- a/xen/arch/x86/livepatch.c > +++ b/xen/arch/x86/livepatch.c > @@ -170,12 +170,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf, > uint8_t *dest = base->load_addr + r->r_offset; > uint64_t val; > > - if ( symndx > elf->nsym ) > + if ( symndx == STN_UNDEF ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n", > + elf->name); > + return -EOPNOTSUPP; > + } > + else if ( symndx >= elf->nsym ) > { > dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n", > elf->name, symndx); > return -EINVAL; > } > + else if ( !elf->sym[symndx].sym ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n", > + elf->name, symndx); > + return -EINVAL; > + } > > val = r->r_addend + elf->sym[symndx].sym->st_value; > > -- > 2.1.4 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-22 18:15 ` [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations Andrew Cooper 2017-06-22 21:10 ` Stefano Stabellini @ 2017-06-23 2:56 ` Konrad Rzeszutek Wilk 2017-06-23 9:50 ` Jan Beulich ` (2 subsequent siblings) 4 siblings, 0 replies; 22+ messages in thread From: Konrad Rzeszutek Wilk @ 2017-06-23 2:56 UTC (permalink / raw) To: Andrew Cooper Cc: Ross Lagerwall, Julien Grall, Stefano Stabellini, Jan Beulich, Xen-devel On Thu, Jun 22, 2017 at 07:15:29PM +0100, Andrew Cooper wrote: > A symndx of STN_UNDEF is special, and means a symbol value of 0. While > legitimate in the ELF standard, its existance in a livepatch is questionable > at best. Until a plausible usecase presents itself, reject such a relocation > with -EOPNOTSUPP. > > Additionally, fix an off-by-one error while range checking symndx, and perform > a safety check on elf->sym[symndx].sym before derefencing it, to avoid > tripping over a NULL pointer when calculating val. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> [arm32 and x86] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-22 18:15 ` [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations Andrew Cooper 2017-06-22 21:10 ` Stefano Stabellini 2017-06-23 2:56 ` Konrad Rzeszutek Wilk @ 2017-06-23 9:50 ` Jan Beulich 2017-06-23 10:13 ` Andrew Cooper 2017-06-23 12:35 ` Ross Lagerwall 2017-06-23 13:32 ` Julien Grall 4 siblings, 1 reply; 22+ messages in thread From: Jan Beulich @ 2017-06-23 9:50 UTC (permalink / raw) To: Andrew Cooper; +Cc: Ross Lagerwall, Julien Grall, Stefano Stabellini, Xen-devel >>> On 22.06.17 at 20:15, <andrew.cooper3@citrix.com> wrote: > A symndx of STN_UNDEF is special, and means a symbol value of 0. While > legitimate in the ELF standard, its existance in a livepatch is questionable > at best. Until a plausible usecase presents itself, reject such a relocation > with -EOPNOTSUPP. > > Additionally, fix an off-by-one error while range checking symndx, and perform > a safety check on elf->sym[symndx].sym before derefencing it, to avoid > tripping over a NULL pointer when calculating val. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with two remarks: > --- a/xen/arch/x86/livepatch.c > +++ b/xen/arch/x86/livepatch.c > @@ -170,12 +170,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf, > uint8_t *dest = base->load_addr + r->r_offset; > uint64_t val; > > - if ( symndx > elf->nsym ) > + if ( symndx == STN_UNDEF ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n", > + elf->name); > + return -EOPNOTSUPP; > + } > + else if ( symndx >= elf->nsym ) > { > dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n", > elf->name, symndx); > return -EINVAL; > } > + else if ( !elf->sym[symndx].sym ) Neither of the two "else" is really necessary, and elsewhere we've been telling people to avoid such. > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n", Symbol tables can grow large, and for large numbers I generally find hex representation preferable of dec. Otoh the other (pre-existing) message uses dec too ... Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-23 9:50 ` Jan Beulich @ 2017-06-23 10:13 ` Andrew Cooper 0 siblings, 0 replies; 22+ messages in thread From: Andrew Cooper @ 2017-06-23 10:13 UTC (permalink / raw) To: Jan Beulich; +Cc: Ross Lagerwall, Julien Grall, Stefano Stabellini, Xen-devel On 23/06/17 10:50, Jan Beulich wrote: >>>> On 22.06.17 at 20:15, <andrew.cooper3@citrix.com> wrote: >> A symndx of STN_UNDEF is special, and means a symbol value of 0. While >> legitimate in the ELF standard, its existance in a livepatch is questionable >> at best. Until a plausible usecase presents itself, reject such a relocation >> with -EOPNOTSUPP. >> >> Additionally, fix an off-by-one error while range checking symndx, and perform >> a safety check on elf->sym[symndx].sym before derefencing it, to avoid >> tripping over a NULL pointer when calculating val. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > with two remarks: > >> --- a/xen/arch/x86/livepatch.c >> +++ b/xen/arch/x86/livepatch.c >> @@ -170,12 +170,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf, >> uint8_t *dest = base->load_addr + r->r_offset; >> uint64_t val; >> >> - if ( symndx > elf->nsym ) >> + if ( symndx == STN_UNDEF ) >> + { >> + dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n", >> + elf->name); >> + return -EOPNOTSUPP; >> + } >> + else if ( symndx >= elf->nsym ) >> { >> dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n", >> elf->name, symndx); >> return -EINVAL; >> } >> + else if ( !elf->sym[symndx].sym ) > Neither of the two "else" is really necessary, and elsewhere we've > been telling people to avoid such. I see two logically different scenarios. Per the style, if I were to use fully separate if() statements, I'd need a newline between each. This expands the code, and separates a chain of logically-related checks. IMO, its better to keep logically related checks more obviously together, while I would definitely agree that unrelated chains (which could in principle be if/else like this) should be separated. > >> + { >> + dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n", > Symbol tables can grow large, and for large numbers I generally > find hex representation preferable of dec. Otoh the other > (pre-existing) message uses dec too ... I'll stay consistent with everything else. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-22 18:15 ` [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations Andrew Cooper ` (2 preceding siblings ...) 2017-06-23 9:50 ` Jan Beulich @ 2017-06-23 12:35 ` Ross Lagerwall 2017-06-23 13:32 ` Julien Grall 4 siblings, 0 replies; 22+ messages in thread From: Ross Lagerwall @ 2017-06-23 12:35 UTC (permalink / raw) To: Andrew Cooper, Xen-devel; +Cc: Julien Grall, Stefano Stabellini, Jan Beulich On 06/22/2017 07:15 PM, Andrew Cooper wrote: > A symndx of STN_UNDEF is special, and means a symbol value of 0. While > legitimate in the ELF standard, its existance in a livepatch is questionable > at best. Until a plausible usecase presents itself, reject such a relocation > with -EOPNOTSUPP. > > Additionally, fix an off-by-one error while range checking symndx, and perform > a safety check on elf->sym[symndx].sym before derefencing it, to avoid > tripping over a NULL pointer when calculating val. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-22 18:15 ` [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations Andrew Cooper ` (3 preceding siblings ...) 2017-06-23 12:35 ` Ross Lagerwall @ 2017-06-23 13:32 ` Julien Grall 2017-06-23 13:33 ` Andrew Cooper 4 siblings, 1 reply; 22+ messages in thread From: Julien Grall @ 2017-06-23 13:32 UTC (permalink / raw) To: Andrew Cooper, Xen-devel; +Cc: Ross Lagerwall, Stefano Stabellini, Jan Beulich Hi Andrew, I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't been CCed on the first two patches. Does it mean you are only looking at this patch to be in 4.9? Cheers, On 22/06/17 19:15, Andrew Cooper wrote: > A symndx of STN_UNDEF is special, and means a symbol value of 0. While > legitimate in the ELF standard, its existance in a livepatch is questionable > at best. Until a plausible usecase presents itself, reject such a relocation > with -EOPNOTSUPP. > > Additionally, fix an off-by-one error while range checking symndx, and perform > a safety check on elf->sym[symndx].sym before derefencing it, to avoid > tripping over a NULL pointer when calculating val. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > CC: Ross Lagerwall <ross.lagerwall@citrix.com> > CC: Jan Beulich <JBeulich@suse.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien.grall@arm.com> > > v3: > * Fix off-by-one error > v2: > * Reject STN_UNDEF with -EOPNOTSUPP > --- > xen/arch/arm/arm32/livepatch.c | 14 +++++++++++++- > xen/arch/arm/arm64/livepatch.c | 14 +++++++++++++- > xen/arch/x86/livepatch.c | 14 +++++++++++++- > 3 files changed, 39 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c > index a328179..41378a5 100644 > --- a/xen/arch/arm/arm32/livepatch.c > +++ b/xen/arch/arm/arm32/livepatch.c > @@ -254,12 +254,24 @@ int arch_livepatch_perform(struct livepatch_elf *elf, > addend = get_addend(type, dest); > } > > - if ( symndx > elf->nsym ) > + if ( symndx == STN_UNDEF ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n", > + elf->name); > + return -EOPNOTSUPP; > + } > + else if ( symndx >= elf->nsym ) > { > dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative symbol wants symbol@%u which is past end!\n", > elf->name, symndx); > return -EINVAL; > } > + else if ( !elf->sym[symndx].sym ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n", > + elf->name, symndx); > + return -EINVAL; > + } > > val = elf->sym[symndx].sym->st_value; /* S */ > > diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c > index 63929b1..2247b92 100644 > --- a/xen/arch/arm/arm64/livepatch.c > +++ b/xen/arch/arm/arm64/livepatch.c > @@ -252,12 +252,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf, > int ovf = 0; > uint64_t val; > > - if ( symndx > elf->nsym ) > + if ( symndx == STN_UNDEF ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n", > + elf->name); > + return -EOPNOTSUPP; > + } > + else if ( symndx >= elf->nsym ) > { > dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n", > elf->name, symndx); > return -EINVAL; > } > + else if ( !elf->sym[symndx].sym ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n", > + elf->name, symndx); > + return -EINVAL; > + } > > val = elf->sym[symndx].sym->st_value + r->r_addend; /* S+A */ > > diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c > index 7917610..406eb91 100644 > --- a/xen/arch/x86/livepatch.c > +++ b/xen/arch/x86/livepatch.c > @@ -170,12 +170,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf, > uint8_t *dest = base->load_addr + r->r_offset; > uint64_t val; > > - if ( symndx > elf->nsym ) > + if ( symndx == STN_UNDEF ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n", > + elf->name); > + return -EOPNOTSUPP; > + } > + else if ( symndx >= elf->nsym ) > { > dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n", > elf->name, symndx); > return -EINVAL; > } > + else if ( !elf->sym[symndx].sym ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n", > + elf->name, symndx); > + return -EINVAL; > + } > > val = r->r_addend + elf->sym[symndx].sym->st_value; > > -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-23 13:32 ` Julien Grall @ 2017-06-23 13:33 ` Andrew Cooper 2017-06-23 13:43 ` Julien Grall 0 siblings, 1 reply; 22+ messages in thread From: Andrew Cooper @ 2017-06-23 13:33 UTC (permalink / raw) To: Julien Grall, Xen-devel; +Cc: Ross Lagerwall, Stefano Stabellini, Jan Beulich On 23/06/17 14:32, Julien Grall wrote: > Hi Andrew, > > I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't > been CCed on the first two patches. Does it mean you are only looking > at this patch to be in 4.9? Sorry - I messed up the CC lists. The correctness of this patch does depend on the previous two, so all 3 are looking for inclusion. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-23 13:33 ` Andrew Cooper @ 2017-06-23 13:43 ` Julien Grall 2017-06-23 13:45 ` Andrew Cooper 0 siblings, 1 reply; 22+ messages in thread From: Julien Grall @ 2017-06-23 13:43 UTC (permalink / raw) To: Andrew Cooper, Xen-devel; +Cc: Ross Lagerwall, Stefano Stabellini, Jan Beulich Hi, On 23/06/17 14:33, Andrew Cooper wrote: > On 23/06/17 14:32, Julien Grall wrote: >> Hi Andrew, >> >> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't >> been CCed on the first two patches. Does it mean you are only looking >> at this patch to be in 4.9? > > Sorry - I messed up the CC lists. The correctness of this patch does > depend on the previous two, so all 3 are looking for inclusion. Given that we don't have livepatch testing in osstest how much test have we done on those 3 patches? Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-23 13:43 ` Julien Grall @ 2017-06-23 13:45 ` Andrew Cooper 2017-06-23 13:47 ` Julien Grall 2017-06-23 14:35 ` Konrad Rzeszutek Wilk 0 siblings, 2 replies; 22+ messages in thread From: Andrew Cooper @ 2017-06-23 13:45 UTC (permalink / raw) To: Julien Grall, Xen-devel; +Cc: Ross Lagerwall, Stefano Stabellini, Jan Beulich On 23/06/17 14:43, Julien Grall wrote: > Hi, > > On 23/06/17 14:33, Andrew Cooper wrote: >> On 23/06/17 14:32, Julien Grall wrote: >>> Hi Andrew, >>> >>> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't >>> been CCed on the first two patches. Does it mean you are only looking >>> at this patch to be in 4.9? >> >> Sorry - I messed up the CC lists. The correctness of this patch does >> depend on the previous two, so all 3 are looking for inclusion. > > Given that we don't have livepatch testing in osstest how much test > have we done on those 3 patches? There is testing in OSSTest. I've manually run each of the scenarios, including with my livepatch which has a STN_UNDEF relocation. I don't know what testing Konrad has done. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-23 13:45 ` Andrew Cooper @ 2017-06-23 13:47 ` Julien Grall 2017-06-23 14:35 ` Konrad Rzeszutek Wilk 1 sibling, 0 replies; 22+ messages in thread From: Julien Grall @ 2017-06-23 13:47 UTC (permalink / raw) To: Andrew Cooper, Xen-devel; +Cc: Ross Lagerwall, Stefano Stabellini, Jan Beulich On 23/06/17 14:45, Andrew Cooper wrote: > On 23/06/17 14:43, Julien Grall wrote: >> Hi, >> >> On 23/06/17 14:33, Andrew Cooper wrote: >>> On 23/06/17 14:32, Julien Grall wrote: >>>> Hi Andrew, >>>> >>>> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't >>>> been CCed on the first two patches. Does it mean you are only looking >>>> at this patch to be in 4.9? >>> >>> Sorry - I messed up the CC lists. The correctness of this patch does >>> depend on the previous two, so all 3 are looking for inclusion. >> >> Given that we don't have livepatch testing in osstest how much test >> have we done on those 3 patches? > > There is testing in OSSTest. Oh yes sorry. But only for amd64 and i386. No arm32 nor arm64 test. > > I've manually run each of the scenarios, including with my livepatch > which has a STN_UNDEF relocation. > > I don't know what testing Konrad has done. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-23 13:45 ` Andrew Cooper 2017-06-23 13:47 ` Julien Grall @ 2017-06-23 14:35 ` Konrad Rzeszutek Wilk 2017-06-23 14:36 ` Julien Grall 1 sibling, 1 reply; 22+ messages in thread From: Konrad Rzeszutek Wilk @ 2017-06-23 14:35 UTC (permalink / raw) To: Andrew Cooper Cc: Ross Lagerwall, Julien Grall, Stefano Stabellini, Jan Beulich, Xen-devel On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote: > On 23/06/17 14:43, Julien Grall wrote: > > Hi, > > > > On 23/06/17 14:33, Andrew Cooper wrote: > >> On 23/06/17 14:32, Julien Grall wrote: > >>> Hi Andrew, > >>> > >>> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't > >>> been CCed on the first two patches. Does it mean you are only looking > >>> at this patch to be in 4.9? > >> > >> Sorry - I messed up the CC lists. The correctness of this patch does > >> depend on the previous two, so all 3 are looking for inclusion. > > > > Given that we don't have livepatch testing in osstest how much test > > have we done on those 3 patches? > > There is testing in OSSTest. Hurray hurray hurray! > > I've manually run each of the scenarios, including with my livepatch > which has a STN_UNDEF relocation. > > I don't know what testing Konrad has done. I run a version of the same tests that are in OSSTest (basically an earlier version of the Perl code) and I have done it on x86 and on ARM32. And I also run the standalone OSSTest (on x86) And then I also do a livepatch using the livepatch-build-tools on x86 to patch some silly function. So from a testing perspective these patches have been tested very exhaustively. > > ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-23 14:35 ` Konrad Rzeszutek Wilk @ 2017-06-23 14:36 ` Julien Grall 2017-06-23 14:46 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 22+ messages in thread From: Julien Grall @ 2017-06-23 14:36 UTC (permalink / raw) To: Konrad Rzeszutek Wilk, Andrew Cooper Cc: Ross Lagerwall, Stefano Stabellini, Jan Beulich, Xen-devel On 23/06/17 15:35, Konrad Rzeszutek Wilk wrote: > On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote: >> On 23/06/17 14:43, Julien Grall wrote: >>> Hi, >>> >>> On 23/06/17 14:33, Andrew Cooper wrote: >>>> On 23/06/17 14:32, Julien Grall wrote: >>>>> Hi Andrew, >>>>> >>>>> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't >>>>> been CCed on the first two patches. Does it mean you are only looking >>>>> at this patch to be in 4.9? >>>> >>>> Sorry - I messed up the CC lists. The correctness of this patch does >>>> depend on the previous two, so all 3 are looking for inclusion. >>> >>> Given that we don't have livepatch testing in osstest how much test >>> have we done on those 3 patches? >> >> There is testing in OSSTest. > > Hurray hurray hurray! >> >> I've manually run each of the scenarios, including with my livepatch >> which has a STN_UNDEF relocation. >> >> I don't know what testing Konrad has done. > > I run a version of the same tests that are in OSSTest (basically an earlier > version of the Perl code) and I have done it on x86 and on ARM32. > > And I also run the standalone OSSTest (on x86) > > And then I also do a livepatch using the livepatch-build-tools on x86 to > patch some silly function. > > So from a testing perspective these patches have been tested very exhaustively. Well it has not been tested on ARM64 :). I am about to do that. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-23 14:36 ` Julien Grall @ 2017-06-23 14:46 ` Konrad Rzeszutek Wilk 2017-06-24 17:28 ` Julien Grall 0 siblings, 1 reply; 22+ messages in thread From: Konrad Rzeszutek Wilk @ 2017-06-23 14:46 UTC (permalink / raw) To: Julien Grall Cc: Andrew Cooper, Ross Lagerwall, Stefano Stabellini, Jan Beulich, Xen-devel [-- Attachment #1: Type: text/plain, Size: 1891 bytes --] On Fri, Jun 23, 2017 at 03:36:51PM +0100, Julien Grall wrote: > > > On 23/06/17 15:35, Konrad Rzeszutek Wilk wrote: > > On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote: > > > On 23/06/17 14:43, Julien Grall wrote: > > > > Hi, > > > > > > > > On 23/06/17 14:33, Andrew Cooper wrote: > > > > > On 23/06/17 14:32, Julien Grall wrote: > > > > > > Hi Andrew, > > > > > > > > > > > > I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't > > > > > > been CCed on the first two patches. Does it mean you are only looking > > > > > > at this patch to be in 4.9? > > > > > > > > > > Sorry - I messed up the CC lists. The correctness of this patch does > > > > > depend on the previous two, so all 3 are looking for inclusion. > > > > > > > > Given that we don't have livepatch testing in osstest how much test > > > > have we done on those 3 patches? > > > > > > There is testing in OSSTest. > > > > Hurray hurray hurray! > > > > > > I've manually run each of the scenarios, including with my livepatch > > > which has a STN_UNDEF relocation. > > > > > > I don't know what testing Konrad has done. > > > > I run a version of the same tests that are in OSSTest (basically an earlier > > version of the Perl code) and I have done it on x86 and on ARM32. > > > > And I also run the standalone OSSTest (on x86) > > > > And then I also do a livepatch using the livepatch-build-tools on x86 to > > patch some silly function. > > > > So from a testing perspective these patches have been tested very exhaustively. > > Well it has not been tested on ARM64 :). I am about to do that. /me facepalm. I really need to get myself a working ARM64 box that is not expensive. Also attached is the poor-man livepatch_test.perl script that mirrors what OSSTest does. [Do adjust the path, it is /usr/lib/debug, but it should be /usr/lib/debug/livepatch with Xen 4.9] [-- Attachment #2: livepatch_test.perl --] [-- Type: text/plain, Size: 2927 bytes --] #!/usr/bin/perl use Data::Dumper; my @livepatch_files = ("xen_hello_world.livepatch", "xen_replace_world.livepatch", "xen_bye_world.livepatch", "xen_nop.livepatch"); my @livepatch_tests = ( {cmd => "xen-livepatch list", rc => 0}, {cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 256}, {cmd => "xen-livepatch revert xen_hello_world", rc => 256}, {cmd => "xen-livepatch load xen_hello_world.livepatch", rc => 0}, {cmd => "xen-livepatch load xen_hello_world.livepatch", rc => 256}, {cmd => "xen-livepatch list | grep -q xen_hello_world", rc => 0}, {cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 0}, {cmd => "xen-livepatch revert xen_hello_world", rc => 0}, {cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 256}, {cmd => "xen-livepatch unload xen_hello_world", rc => 0}, {cmd => "xen-livepatch unload xen_hello_world", rc => 256}, {cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 256}, {cmd => "xen-livepatch load xen_hello_world.livepatch", rc => 0}, {cmd => "xen-livepatch load xen_bye_world.livepatch", rc => 0}, {cmd => "xl info | grep xen_extra | grep -q \"Bye World\"", rc => 0}, {cmd => "xen-livepatch upload xen_replace xen_replace_world.livepatch", rc => 0}, {cmd => "xen-livepatch replace xen_replace", rc => 0}, {cmd => "xen-livepatch apply xen_hello_world", rc => 256}, {cmd => "xen-livepatch apply xen_bye_world", rc => 256}, {cmd => "xl info | grep xen_extra | grep -q \"Hello Again Wor\"", rc => 0}, {cmd => "xen-livepatch apply xen_replace", rc => 0}, {cmd => "xen-livepatch revert xen_replace", rc => 0}, {cmd => "xen-livepatch unload xen_replace", rc => 0}, {cmd => "xen-livepatch unload xen_hello_world", rc => 0}, {cmd => "xen-livepatch unload xen_bye_world", rc => 0}, {cmd => "xen-livepatch list | grep -q xen", rc => 256}, # If running this under Xen 4.4, or 5.5 it will fail. #{cmd => "[ `xl info| grep \"xen_m\" | grep or | sed s/.*:// | uniq | wc -l` == 2 ]", rc => 0}, {cmd => "xen-livepatch load xen_nop.livepatch", rc => 0}, {cmd => "xen-livepatch revert xen_nop", rc => 0}, {cmd => "xen-livepatch apply xen_nop", rc => 0}, {cmd => "[ `xl info| grep \"xen_m\" | grep or | sed s/.*:// | uniq | wc -l` == 1 ]", rc => 0}, {cmd => "xen-livepatch unload xen_nop", rc => 256}, {cmd => "xen-livepatch revert xen_nop", rc => 0}, {cmd => "xen-livepatch unload xen_nop", rc => 0}, ); chdir("/usr/lib/debug") or die "cannot change: $!\n"; foreach my $file (@livepatch_files) { if ( ! -f $file ) { die "$file is missing!\n"; } } print "Have $#livepatch_tests test-cases\n"; my $rc=0; for my $i ( 0 .. $#livepatch_tests ) { my $expected_rc = $livepatch_tests[$i]{rc}; my $cmd = $livepatch_tests[$i]{cmd}; print "Executing: '$cmd:' .."; my $rc=system($cmd); if ( $rc != $expected_rc ) { print "FAILED (got $rc, expected: $expected_rc)\n"; exit $rc } print ".. OK!\n"; } exit $rc [-- Attachment #3: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-23 14:46 ` Konrad Rzeszutek Wilk @ 2017-06-24 17:28 ` Julien Grall 2017-06-26 1:02 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 22+ messages in thread From: Julien Grall @ 2017-06-24 17:28 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Andrew Cooper, Ross Lagerwall, Stefano Stabellini, Jan Beulich, Xen-devel Hi Konrad, On 06/23/2017 03:46 PM, Konrad Rzeszutek Wilk wrote: > On Fri, Jun 23, 2017 at 03:36:51PM +0100, Julien Grall wrote: >> >> >> On 23/06/17 15:35, Konrad Rzeszutek Wilk wrote: >>> On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote: >>>> On 23/06/17 14:43, Julien Grall wrote: >>>>> Hi, >>>>> >>>>> On 23/06/17 14:33, Andrew Cooper wrote: >>>>>> On 23/06/17 14:32, Julien Grall wrote: >>>>>>> Hi Andrew, >>>>>>> >>>>>>> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't >>>>>>> been CCed on the first two patches. Does it mean you are only looking >>>>>>> at this patch to be in 4.9? >>>>>> >>>>>> Sorry - I messed up the CC lists. The correctness of this patch does >>>>>> depend on the previous two, so all 3 are looking for inclusion. >>>>> >>>>> Given that we don't have livepatch testing in osstest how much test >>>>> have we done on those 3 patches? >>>> >>>> There is testing in OSSTest. >>> >>> Hurray hurray hurray! >>>> >>>> I've manually run each of the scenarios, including with my livepatch >>>> which has a STN_UNDEF relocation. >>>> >>>> I don't know what testing Konrad has done. >>> >>> I run a version of the same tests that are in OSSTest (basically an earlier >>> version of the Perl code) and I have done it on x86 and on ARM32. >>> >>> And I also run the standalone OSSTest (on x86) >>> >>> And then I also do a livepatch using the livepatch-build-tools on x86 to >>> patch some silly function. >>> >>> So from a testing perspective these patches have been tested very exhaustively. >> >> Well it has not been tested on ARM64 :). I am about to do that. > > /me facepalm. > > I really need to get myself a working ARM64 box that is not expensive. > > > Also attached is the poor-man livepatch_test.perl script that mirrors > what OSSTest does. I have got an error when executing the script after applying the "nop" livepatch: Executing: 'xen-livepatch apply xen_nop:' ..Applying xen_nop... completed .. OK! Executing: '[ `xl info| grep "xen_m" | grep or | sed s/.*:// | uniq | wc -l` == 1 ]:' ..sh: 1: [: 1: unexpected operator FAILED (got 512, expected: 0) But this looks like a script error than livepatch. Although, I was not able to spot the error in the script. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-24 17:28 ` Julien Grall @ 2017-06-26 1:02 ` Konrad Rzeszutek Wilk 2017-06-26 9:34 ` Julien Grall 0 siblings, 1 reply; 22+ messages in thread From: Konrad Rzeszutek Wilk @ 2017-06-26 1:02 UTC (permalink / raw) To: Julien Grall Cc: Andrew Cooper, Ross Lagerwall, Stefano Stabellini, Jan Beulich, Xen-devel On Sat, Jun 24, 2017 at 06:28:16PM +0100, Julien Grall wrote: > Hi Konrad, > > On 06/23/2017 03:46 PM, Konrad Rzeszutek Wilk wrote: > > On Fri, Jun 23, 2017 at 03:36:51PM +0100, Julien Grall wrote: > > > > > > > > > On 23/06/17 15:35, Konrad Rzeszutek Wilk wrote: > > > > On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote: > > > > > On 23/06/17 14:43, Julien Grall wrote: > > > > > > Hi, > > > > > > > > > > > > On 23/06/17 14:33, Andrew Cooper wrote: > > > > > > > On 23/06/17 14:32, Julien Grall wrote: > > > > > > > > Hi Andrew, > > > > > > > > > > > > > > > > I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't > > > > > > > > been CCed on the first two patches. Does it mean you are only looking > > > > > > > > at this patch to be in 4.9? > > > > > > > > > > > > > > Sorry - I messed up the CC lists. The correctness of this patch does > > > > > > > depend on the previous two, so all 3 are looking for inclusion. > > > > > > > > > > > > Given that we don't have livepatch testing in osstest how much test > > > > > > have we done on those 3 patches? > > > > > > > > > > There is testing in OSSTest. > > > > > > > > Hurray hurray hurray! > > > > > > > > > > I've manually run each of the scenarios, including with my livepatch > > > > > which has a STN_UNDEF relocation. > > > > > > > > > > I don't know what testing Konrad has done. > > > > > > > > I run a version of the same tests that are in OSSTest (basically an earlier > > > > version of the Perl code) and I have done it on x86 and on ARM32. > > > > > > > > And I also run the standalone OSSTest (on x86) > > > > > > > > And then I also do a livepatch using the livepatch-build-tools on x86 to > > > > patch some silly function. > > > > > > > > So from a testing perspective these patches have been tested very exhaustively. > > > > > > Well it has not been tested on ARM64 :). I am about to do that. > > > > /me facepalm. > > > > I really need to get myself a working ARM64 box that is not expensive. > > > > > > Also attached is the poor-man livepatch_test.perl script that mirrors > > what OSSTest does. > > I have got an error when executing the script after applying the "nop" > livepatch: > > Executing: 'xen-livepatch apply xen_nop:' ..Applying xen_nop... completed > .. OK! > Executing: '[ `xl info| grep "xen_m" | grep or | sed s/.*:// | uniq | wc -l` > == 1 ]:' ..sh: 1: [: 1: unexpected operator > FAILED (got 512, expected: 0) Yeah I only get that when I execute it from the serial console. When I SSH it works fine. > > But this looks like a script error than livepatch. Although, I was not able > to spot the error in the script. Neither could I. I do also have an earlier variant of this testing script in bash (with less test-cases). > > Cheers, > > -- > Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations 2017-06-26 1:02 ` Konrad Rzeszutek Wilk @ 2017-06-26 9:34 ` Julien Grall 0 siblings, 0 replies; 22+ messages in thread From: Julien Grall @ 2017-06-26 9:34 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Andrew Cooper, Ross Lagerwall, Stefano Stabellini, Jan Beulich, Xen-devel On 26/06/17 02:02, Konrad Rzeszutek Wilk wrote: > On Sat, Jun 24, 2017 at 06:28:16PM +0100, Julien Grall wrote: >> Hi Konrad, >> >> On 06/23/2017 03:46 PM, Konrad Rzeszutek Wilk wrote: >>> On Fri, Jun 23, 2017 at 03:36:51PM +0100, Julien Grall wrote: >>>> >>>> >>>> On 23/06/17 15:35, Konrad Rzeszutek Wilk wrote: >>>>> On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote: >>>>>> On 23/06/17 14:43, Julien Grall wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 23/06/17 14:33, Andrew Cooper wrote: >>>>>>>> On 23/06/17 14:32, Julien Grall wrote: >>>>>>>>> Hi Andrew, >>>>>>>>> >>>>>>>>> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't >>>>>>>>> been CCed on the first two patches. Does it mean you are only looking >>>>>>>>> at this patch to be in 4.9? >>>>>>>> >>>>>>>> Sorry - I messed up the CC lists. The correctness of this patch does >>>>>>>> depend on the previous two, so all 3 are looking for inclusion. >>>>>>> >>>>>>> Given that we don't have livepatch testing in osstest how much test >>>>>>> have we done on those 3 patches? >>>>>> >>>>>> There is testing in OSSTest. >>>>> >>>>> Hurray hurray hurray! >>>>>> >>>>>> I've manually run each of the scenarios, including with my livepatch >>>>>> which has a STN_UNDEF relocation. >>>>>> >>>>>> I don't know what testing Konrad has done. >>>>> >>>>> I run a version of the same tests that are in OSSTest (basically an earlier >>>>> version of the Perl code) and I have done it on x86 and on ARM32. >>>>> >>>>> And I also run the standalone OSSTest (on x86) >>>>> >>>>> And then I also do a livepatch using the livepatch-build-tools on x86 to >>>>> patch some silly function. >>>>> >>>>> So from a testing perspective these patches have been tested very exhaustively. >>>> >>>> Well it has not been tested on ARM64 :). I am about to do that. >>> >>> /me facepalm. >>> >>> I really need to get myself a working ARM64 box that is not expensive. >>> >>> >>> Also attached is the poor-man livepatch_test.perl script that mirrors >>> what OSSTest does. >> >> I have got an error when executing the script after applying the "nop" >> livepatch: >> >> Executing: 'xen-livepatch apply xen_nop:' ..Applying xen_nop... completed >> .. OK! >> Executing: '[ `xl info| grep "xen_m" | grep or | sed s/.*:// | uniq | wc -l` >> == 1 ]:' ..sh: 1: [: 1: unexpected operator >> FAILED (got 512, expected: 0) > > Yeah I only get that when I execute it from the serial console. > When I SSH it works fine. > >> >> But this looks like a script error than livepatch. Although, I was not able >> to spot the error in the script. > > Neither could I. I do also have an earlier variant of this testing script > in bash (with less test-cases). I am happy with this patch series to be backported in Xen 4.9: Release-acked-by: Julien Grall <julien.grall@arm.com> Can they be backported today please? Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2017-06-26 9:34 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-22 18:15 [PATCH for-4.9 v3 0/3] Fixes for livepatching Andrew Cooper 2017-06-22 18:15 ` [PATCH for-4.9 v3 1/3] xen/livepatch: Clean up arch relocation handling Andrew Cooper 2017-06-22 18:15 ` [PATCH for-4.9 v3 2/3] xen/livepatch: Use zeroed memory allocations for arrays Andrew Cooper 2017-06-23 2:55 ` Konrad Rzeszutek Wilk 2017-06-23 12:31 ` Ross Lagerwall 2017-06-22 18:15 ` [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations Andrew Cooper 2017-06-22 21:10 ` Stefano Stabellini 2017-06-23 2:56 ` Konrad Rzeszutek Wilk 2017-06-23 9:50 ` Jan Beulich 2017-06-23 10:13 ` Andrew Cooper 2017-06-23 12:35 ` Ross Lagerwall 2017-06-23 13:32 ` Julien Grall 2017-06-23 13:33 ` Andrew Cooper 2017-06-23 13:43 ` Julien Grall 2017-06-23 13:45 ` Andrew Cooper 2017-06-23 13:47 ` Julien Grall 2017-06-23 14:35 ` Konrad Rzeszutek Wilk 2017-06-23 14:36 ` Julien Grall 2017-06-23 14:46 ` Konrad Rzeszutek Wilk 2017-06-24 17:28 ` Julien Grall 2017-06-26 1:02 ` Konrad Rzeszutek Wilk 2017-06-26 9:34 ` Julien Grall
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.