| Message ID | 20240920093656.48879-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/livepatch: improvements to loading | expand |
On 20.09.2024 11:36, Roger Pau Monne wrote: > @@ -198,9 +201,17 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start, > uint8_t buf[MAX_PATCH_LEN]; > unsigned int total_len = a->orig_len + a->pad_len; > > - BUG_ON(a->repl_len > total_len); > - BUG_ON(total_len > sizeof(buf)); > - BUG_ON(a->cpuid >= NCAPINTS * 32); > +#define BUG_ON_BOOT(cond) \ > + if ( system_state < SYS_STATE_active ) \ > + BUG_ON(cond); \ > + else if ( cond ) \ > + return -EINVAL; > + > + BUG_ON_BOOT(a->repl_len > total_len); > + BUG_ON_BOOT(total_len > sizeof(buf)); > + BUG_ON_BOOT(a->cpuid >= NCAPINTS * 32); > + > +#undef BUG_ON_BOOT BUG_ON() provides quite a bit of information to aid figuring what's wrong. Without a log message in the livepatching case ... > --- a/xen/common/livepatch.c > +++ b/xen/common/livepatch.c > @@ -882,7 +882,15 @@ static int prepare_payload(struct payload *payload, > return -EINVAL; > } > } > - apply_alternatives(start, end); > + > + rc = apply_alternatives(start, end); > + if ( rc ) > + { > + printk(XENLOG_ERR LIVEPATCH "%s applying alternatives failed: %d\n", > + elf->name, rc); ... this is all one would get. Since livepatching is a privileged operation, log-spam also shouldn't be of concern. So I'd like to ask that at least some detail (line number to start with) be provided. Which however leads to a follow-on concern: Those string literals then would also end up in LIVEPATCH=n binaries, so I'd like to further ask for a suitable IS_ENABLED() check to prevent that happening. Jan
On 20/09/2024 10:36 am, Roger Pau Monne wrote: > alternatives is used both at boot time, and when loading livepatch payloads. > While for the former it makes sense to panic, it's not useful for the later, as > for livepatches it's possible to fail to load the livepatch if alternatives > cannot be resolved and continue operating normally. > > Relax the BUGs in _apply_alternatives() to instead return an error code if > alternatives are applied after boot. > > Add a dummy livepatch test so the new logic can be assessed, with the change > applied the loading now fails with: > > alt table ffff82d040602024 -> ffff82d040602032 > livepatch: xen_alternatives_fail applying alternatives failed: -22 > > Signed-off-by: Roger Pau Monné <roge.rpau@citrix.com> > --- > xen/arch/x86/alternative.c | 29 ++++++++++++++++------ > xen/arch/x86/include/asm/alternative.h | 3 ++- > xen/common/livepatch.c | 10 +++++++- > xen/test/livepatch/Makefile | 5 ++++ > xen/test/livepatch/xen_alternatives_fail.c | 29 ++++++++++++++++++++++ > 5 files changed, 66 insertions(+), 10 deletions(-) > create mode 100644 xen/test/livepatch/xen_alternatives_fail.c > > diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c > index 7824053c9d33..c0912cb12ea5 100644 > --- a/xen/arch/x86/alternative.c > +++ b/xen/arch/x86/alternative.c > @@ -174,10 +174,13 @@ extern void *const __initdata_cf_clobber_end[]; > * The caller will set the "force" argument to true for the final > * invocation, such that no CALLs/JMPs to NULL pointers will be left > * around. See also the further comment below. > + * > + * Note the function cannot fail if system_state < SYS_STATE_active, it would > + * panic instead. The return value is only meaningful for runtime usage. > */ > -static void init_or_livepatch _apply_alternatives(struct alt_instr *start, > - struct alt_instr *end, > - bool force) > +static int init_or_livepatch _apply_alternatives(struct alt_instr *start, > + struct alt_instr *end, > + bool force) > { > struct alt_instr *a, *base; > > @@ -198,9 +201,17 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start, > uint8_t buf[MAX_PATCH_LEN]; > unsigned int total_len = a->orig_len + a->pad_len; > > - BUG_ON(a->repl_len > total_len); > - BUG_ON(total_len > sizeof(buf)); > - BUG_ON(a->cpuid >= NCAPINTS * 32); > +#define BUG_ON_BOOT(cond) \ > + if ( system_state < SYS_STATE_active ) \ > + BUG_ON(cond); \ > + else if ( cond ) \ > + return -EINVAL; > + > + BUG_ON_BOOT(a->repl_len > total_len); > + BUG_ON_BOOT(total_len > sizeof(buf)); > + BUG_ON_BOOT(a->cpuid >= NCAPINTS * 32); > + > +#undef BUG_ON_BOOT The "error handling" before was horrible and this isn't really any better. This function should always return int, printing more helpful info than that (a printk() says a thousand things better than a BUG()), and nmi_apply_alternatives can panic() rather than leaving these BUG()s here. As a tangent, the __must_check doesn't seem to have been applied to nmi_apply_alternatives(), but I'd suggest dropping the attribute; I don't think it adds much. > diff --git a/xen/test/livepatch/xen_alternatives_fail.c b/xen/test/livepatch/xen_alternatives_fail.c > new file mode 100644 > index 000000000000..01d289095a31 > --- /dev/null > +++ b/xen/test/livepatch/xen_alternatives_fail.c > @@ -0,0 +1,29 @@ > +/* > + * Copyright (c) 2024 Cloud Software Group. > + * > + */ > + > +#include "config.h" > +#include <xen/lib.h> > +#include <xen/livepatch_payload.h> > + > +#include <asm/alternative.h> > +#include <asm/cpuid.h> > + > +void test_alternatives(void) > +{ > + alternative("", "", NCAPINTS * 32); > +} > + > +/* Set a hook so the loading logic in Xen don't consider the payload empty. */ > +LIVEPATCH_LOAD_HOOK(test_alternatives); > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * tab-width: 4 > + * indent-tabs-mode: nil > + * End: > + */ The second half of the patch (new testcase) is all fine and good, but should pass with patch 2 in place? I'd suggest splitting it out. ~Andrew
On Mon, Sep 23, 2024 at 12:17:51PM +0100, Andrew Cooper wrote: > On 20/09/2024 10:36 am, Roger Pau Monne wrote: > > alternatives is used both at boot time, and when loading livepatch payloads. > > While for the former it makes sense to panic, it's not useful for the later, as > > for livepatches it's possible to fail to load the livepatch if alternatives > > cannot be resolved and continue operating normally. > > > > Relax the BUGs in _apply_alternatives() to instead return an error code if > > alternatives are applied after boot. > > > > Add a dummy livepatch test so the new logic can be assessed, with the change > > applied the loading now fails with: > > > > alt table ffff82d040602024 -> ffff82d040602032 > > livepatch: xen_alternatives_fail applying alternatives failed: -22 > > > > Signed-off-by: Roger Pau Monné <roge.rpau@citrix.com> > > --- > > xen/arch/x86/alternative.c | 29 ++++++++++++++++------ > > xen/arch/x86/include/asm/alternative.h | 3 ++- > > xen/common/livepatch.c | 10 +++++++- > > xen/test/livepatch/Makefile | 5 ++++ > > xen/test/livepatch/xen_alternatives_fail.c | 29 ++++++++++++++++++++++ > > 5 files changed, 66 insertions(+), 10 deletions(-) > > create mode 100644 xen/test/livepatch/xen_alternatives_fail.c > > > > diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c > > index 7824053c9d33..c0912cb12ea5 100644 > > --- a/xen/arch/x86/alternative.c > > +++ b/xen/arch/x86/alternative.c > > @@ -174,10 +174,13 @@ extern void *const __initdata_cf_clobber_end[]; > > * The caller will set the "force" argument to true for the final > > * invocation, such that no CALLs/JMPs to NULL pointers will be left > > * around. See also the further comment below. > > + * > > + * Note the function cannot fail if system_state < SYS_STATE_active, it would > > + * panic instead. The return value is only meaningful for runtime usage. > > */ > > -static void init_or_livepatch _apply_alternatives(struct alt_instr *start, > > - struct alt_instr *end, > > - bool force) > > +static int init_or_livepatch _apply_alternatives(struct alt_instr *start, > > + struct alt_instr *end, > > + bool force) > > { > > struct alt_instr *a, *base; > > > > @@ -198,9 +201,17 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start, > > uint8_t buf[MAX_PATCH_LEN]; > > unsigned int total_len = a->orig_len + a->pad_len; > > > > - BUG_ON(a->repl_len > total_len); > > - BUG_ON(total_len > sizeof(buf)); > > - BUG_ON(a->cpuid >= NCAPINTS * 32); > > +#define BUG_ON_BOOT(cond) \ > > + if ( system_state < SYS_STATE_active ) \ > > + BUG_ON(cond); \ > > + else if ( cond ) \ > > + return -EINVAL; > > + > > + BUG_ON_BOOT(a->repl_len > total_len); > > + BUG_ON_BOOT(total_len > sizeof(buf)); > > + BUG_ON_BOOT(a->cpuid >= NCAPINTS * 32); > > + > > +#undef BUG_ON_BOOT > > The "error handling" before was horrible and this isn't really any better. > > This function should always return int, printing more helpful info than > that (a printk() says a thousand things better than a BUG()), and > nmi_apply_alternatives can panic() rather than leaving these BUG()s here. OK, will rework the logic here so it's the caller that panics (or not) as necessary, and _apply_alternatives() always prints some error message. > As a tangent, the __must_check doesn't seem to have been applied to > nmi_apply_alternatives(), but I'd suggest dropping the attribute; I > don't think it adds much. I didn't see the value in adding the attribute to nmi_apply_alternatives(), as in that context _apply_alternatives() would unconditionally panic instead of returning an error code. > > diff --git a/xen/test/livepatch/xen_alternatives_fail.c b/xen/test/livepatch/xen_alternatives_fail.c > > new file mode 100644 > > index 000000000000..01d289095a31 > > --- /dev/null > > +++ b/xen/test/livepatch/xen_alternatives_fail.c > > @@ -0,0 +1,29 @@ > > +/* > > + * Copyright (c) 2024 Cloud Software Group. > > + * > > + */ > > + > > +#include "config.h" > > +#include <xen/lib.h> > > +#include <xen/livepatch_payload.h> > > + > > +#include <asm/alternative.h> > > +#include <asm/cpuid.h> > > + > > +void test_alternatives(void) > > +{ > > + alternative("", "", NCAPINTS * 32); > > +} > > + > > +/* Set a hook so the loading logic in Xen don't consider the payload empty. */ > > +LIVEPATCH_LOAD_HOOK(test_alternatives); > > + > > +/* > > + * Local variables: > > + * mode: C > > + * c-file-style: "BSD" > > + * c-basic-offset: 4 > > + * tab-width: 4 > > + * indent-tabs-mode: nil > > + * End: > > + */ > > The second half of the patch (new testcase) is all fine and good, but > should pass with patch 2 in place? I'd suggest splitting it out. No, not really. The Xen buildid for this patch will be correctly set to match the running one, but the alternatives feature CPUID is explicitly set to an out of range value (NCAPINTS * 32) to trigger the BUG_ON condition. Further thinking about it, I think we should add a build time assert that the feature parameters in the alternative calls are smaller than NCAPINTS * 32. Thanks, Roger.
On 23/09/2024 2:06 pm, Roger Pau Monné wrote: > On Mon, Sep 23, 2024 at 12:17:51PM +0100, Andrew Cooper wrote: >> On 20/09/2024 10:36 am, Roger Pau Monne wrote: >>> alternatives is used both at boot time, and when loading livepatch payloads. >>> While for the former it makes sense to panic, it's not useful for the later, as >>> for livepatches it's possible to fail to load the livepatch if alternatives >>> cannot be resolved and continue operating normally. >>> >>> Relax the BUGs in _apply_alternatives() to instead return an error code if >>> alternatives are applied after boot. >>> >>> Add a dummy livepatch test so the new logic can be assessed, with the change >>> applied the loading now fails with: >>> >>> alt table ffff82d040602024 -> ffff82d040602032 >>> livepatch: xen_alternatives_fail applying alternatives failed: -22 >>> >>> Signed-off-by: Roger Pau Monné <roge.rpau@citrix.com> >>> --- >>> xen/arch/x86/alternative.c | 29 ++++++++++++++++------ >>> xen/arch/x86/include/asm/alternative.h | 3 ++- >>> xen/common/livepatch.c | 10 +++++++- >>> xen/test/livepatch/Makefile | 5 ++++ >>> xen/test/livepatch/xen_alternatives_fail.c | 29 ++++++++++++++++++++++ >>> 5 files changed, 66 insertions(+), 10 deletions(-) >>> create mode 100644 xen/test/livepatch/xen_alternatives_fail.c >>> >>> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c >>> index 7824053c9d33..c0912cb12ea5 100644 >>> --- a/xen/arch/x86/alternative.c >>> +++ b/xen/arch/x86/alternative.c >>> @@ -174,10 +174,13 @@ extern void *const __initdata_cf_clobber_end[]; >>> * The caller will set the "force" argument to true for the final >>> * invocation, such that no CALLs/JMPs to NULL pointers will be left >>> * around. See also the further comment below. >>> + * >>> + * Note the function cannot fail if system_state < SYS_STATE_active, it would >>> + * panic instead. The return value is only meaningful for runtime usage. >>> */ >>> -static void init_or_livepatch _apply_alternatives(struct alt_instr *start, >>> - struct alt_instr *end, >>> - bool force) >>> +static int init_or_livepatch _apply_alternatives(struct alt_instr *start, >>> + struct alt_instr *end, >>> + bool force) >>> { >>> struct alt_instr *a, *base; >>> >>> @@ -198,9 +201,17 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start, >>> uint8_t buf[MAX_PATCH_LEN]; >>> unsigned int total_len = a->orig_len + a->pad_len; >>> >>> - BUG_ON(a->repl_len > total_len); >>> - BUG_ON(total_len > sizeof(buf)); >>> - BUG_ON(a->cpuid >= NCAPINTS * 32); >>> +#define BUG_ON_BOOT(cond) \ >>> + if ( system_state < SYS_STATE_active ) \ >>> + BUG_ON(cond); \ >>> + else if ( cond ) \ >>> + return -EINVAL; >>> + >>> + BUG_ON_BOOT(a->repl_len > total_len); >>> + BUG_ON_BOOT(total_len > sizeof(buf)); >>> + BUG_ON_BOOT(a->cpuid >= NCAPINTS * 32); >>> + >>> +#undef BUG_ON_BOOT >> The "error handling" before was horrible and this isn't really any better. >> >> This function should always return int, printing more helpful info than >> that (a printk() says a thousand things better than a BUG()), and >> nmi_apply_alternatives can panic() rather than leaving these BUG()s here. > OK, will rework the logic here so it's the caller that panics (or not) > as necessary, and _apply_alternatives() always prints some error > message. Yes. With alternatives now behind sensible checks in the livepatch case, any failure here is relevant and wants printing. > >> As a tangent, the __must_check doesn't seem to have been applied to >> nmi_apply_alternatives(), but I'd suggest dropping the attribute; I >> don't think it adds much. > I didn't see the value in adding the attribute to > nmi_apply_alternatives(), as in that context _apply_alternatives() > would unconditionally panic instead of returning an error code. Ah, it was only apply_alternatives() you made __must_check, not _apply_alternatives(), which is why there isn't a compile error in nmi_apply_alternatives(). Still, I don't think the __must_check is much use. > >>> diff --git a/xen/test/livepatch/xen_alternatives_fail.c b/xen/test/livepatch/xen_alternatives_fail.c >>> new file mode 100644 >>> index 000000000000..01d289095a31 >>> --- /dev/null >>> +++ b/xen/test/livepatch/xen_alternatives_fail.c >>> @@ -0,0 +1,29 @@ >>> +/* >>> + * Copyright (c) 2024 Cloud Software Group. >>> + * >>> + */ >>> + >>> +#include "config.h" >>> +#include <xen/lib.h> >>> +#include <xen/livepatch_payload.h> >>> + >>> +#include <asm/alternative.h> >>> +#include <asm/cpuid.h> >>> + >>> +void test_alternatives(void) >>> +{ >>> + alternative("", "", NCAPINTS * 32); >>> +} >>> + >>> +/* Set a hook so the loading logic in Xen don't consider the payload empty. */ >>> +LIVEPATCH_LOAD_HOOK(test_alternatives); >>> + >>> +/* >>> + * Local variables: >>> + * mode: C >>> + * c-file-style: "BSD" >>> + * c-basic-offset: 4 >>> + * tab-width: 4 >>> + * indent-tabs-mode: nil >>> + * End: >>> + */ >> The second half of the patch (new testcase) is all fine and good, but >> should pass with patch 2 in place? I'd suggest splitting it out. > No, not really. The Xen buildid for this patch will be correctly set > to match the running one, but the alternatives feature CPUID is > explicitly set to an out of range value (NCAPINTS * 32) to trigger the > BUG_ON condition. Ah yes. Good point. In which case it probably ought to stay in this patch. > Further thinking about it, I think we should add a build time assert > that the feature parameters in the alternative calls are smaller than > NCAPINTS * 32. A build check where? It's quite hard to do in alternatives themselves. ~Andrew
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c index 7824053c9d33..c0912cb12ea5 100644 --- a/xen/arch/x86/alternative.c +++ b/xen/arch/x86/alternative.c @@ -174,10 +174,13 @@ extern void *const __initdata_cf_clobber_end[]; * The caller will set the "force" argument to true for the final * invocation, such that no CALLs/JMPs to NULL pointers will be left * around. See also the further comment below. + * + * Note the function cannot fail if system_state < SYS_STATE_active, it would + * panic instead. The return value is only meaningful for runtime usage. */ -static void init_or_livepatch _apply_alternatives(struct alt_instr *start, - struct alt_instr *end, - bool force) +static int init_or_livepatch _apply_alternatives(struct alt_instr *start, + struct alt_instr *end, + bool force) { struct alt_instr *a, *base; @@ -198,9 +201,17 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start, uint8_t buf[MAX_PATCH_LEN]; unsigned int total_len = a->orig_len + a->pad_len; - BUG_ON(a->repl_len > total_len); - BUG_ON(total_len > sizeof(buf)); - BUG_ON(a->cpuid >= NCAPINTS * 32); +#define BUG_ON_BOOT(cond) \ + if ( system_state < SYS_STATE_active ) \ + BUG_ON(cond); \ + else if ( cond ) \ + return -EINVAL; + + BUG_ON_BOOT(a->repl_len > total_len); + BUG_ON_BOOT(total_len > sizeof(buf)); + BUG_ON_BOOT(a->cpuid >= NCAPINTS * 32); + +#undef BUG_ON_BOOT /* * Detect sequences of alt_instr's patching the same origin site, and @@ -356,12 +367,14 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start, printk("altcall: Optimised away %u endbr64 instructions\n", clobbered); } + + return 0; } #ifdef CONFIG_LIVEPATCH -void apply_alternatives(struct alt_instr *start, struct alt_instr *end) +int apply_alternatives(struct alt_instr *start, struct alt_instr *end) { - _apply_alternatives(start, end, true); + return _apply_alternatives(start, end, true); } #endif diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h index a86eadfaecbd..83940e1849c6 100644 --- a/xen/arch/x86/include/asm/alternative.h +++ b/xen/arch/x86/include/asm/alternative.h @@ -24,7 +24,8 @@ struct __packed alt_instr { extern void add_nops(void *insns, unsigned int len); /* Similar to alternative_instructions except it can be run with IRQs enabled. */ -extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end); +extern int __must_check apply_alternatives(struct alt_instr *start, + struct alt_instr *end); extern void alternative_instructions(void); extern void alternative_branches(void); diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index 3e4fce036a1c..1b3a9dda52a7 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -882,7 +882,15 @@ static int prepare_payload(struct payload *payload, return -EINVAL; } } - apply_alternatives(start, end); + + rc = apply_alternatives(start, end); + if ( rc ) + { + printk(XENLOG_ERR LIVEPATCH "%s applying alternatives failed: %d\n", + elf->name, rc); + return rc; + } + alt_done:; #else printk(XENLOG_ERR LIVEPATCH "%s: We don't support alternative patching\n", diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile index 4caa9e24324e..c29529b50338 100644 --- a/xen/test/livepatch/Makefile +++ b/xen/test/livepatch/Makefile @@ -125,6 +125,11 @@ $(obj)/xen_action_hooks_norevert.o: $(obj)/config.h extra-y += xen_action_hooks_norevert.livepatch xen_action_hooks_norevert-objs := xen_action_hooks_norevert.o xen_hello_world_func.o note.o xen_note.o +$(obj)/xen_alternatives_fail.o: $(obj)/config.h + +extra-y += xen_alternatives_fail.livepatch +xen_alternatives_fail-objs := xen_alternatives_fail.o note.o xen_note.o + EXPECT_BYTES_COUNT := 8 CODE_GET_EXPECT=$(shell $(OBJDUMP) -d --insn-width=1 $(1) | sed -n -e '/<'$(2)'>:$$/,/^$$/ p' | tail -n +2 | head -n $(EXPECT_BYTES_COUNT) | awk '{$$0=$$2; printf "%s", substr($$0,length-1)}' | sed 's/.\{2\}/0x&,/g' | sed 's/^/{/;s/,$$/}/g') $(obj)/expect_config.h: $(objtree)/xen-syms diff --git a/xen/test/livepatch/xen_alternatives_fail.c b/xen/test/livepatch/xen_alternatives_fail.c new file mode 100644 index 000000000000..01d289095a31 --- /dev/null +++ b/xen/test/livepatch/xen_alternatives_fail.c @@ -0,0 +1,29 @@ +/* + * Copyright (c) 2024 Cloud Software Group. + * + */ + +#include "config.h" +#include <xen/lib.h> +#include <xen/livepatch_payload.h> + +#include <asm/alternative.h> +#include <asm/cpuid.h> + +void test_alternatives(void) +{ + alternative("", "", NCAPINTS * 32); +} + +/* Set a hook so the loading logic in Xen don't consider the payload empty. */ +LIVEPATCH_LOAD_HOOK(test_alternatives); + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */
alternatives is used both at boot time, and when loading livepatch payloads. While for the former it makes sense to panic, it's not useful for the later, as for livepatches it's possible to fail to load the livepatch if alternatives cannot be resolved and continue operating normally. Relax the BUGs in _apply_alternatives() to instead return an error code if alternatives are applied after boot. Add a dummy livepatch test so the new logic can be assessed, with the change applied the loading now fails with: alt table ffff82d040602024 -> ffff82d040602032 livepatch: xen_alternatives_fail applying alternatives failed: -22 Signed-off-by: Roger Pau Monné <roge.rpau@citrix.com> --- xen/arch/x86/alternative.c | 29 ++++++++++++++++------ xen/arch/x86/include/asm/alternative.h | 3 ++- xen/common/livepatch.c | 10 +++++++- xen/test/livepatch/Makefile | 5 ++++ xen/test/livepatch/xen_alternatives_fail.c | 29 ++++++++++++++++++++++ 5 files changed, 66 insertions(+), 10 deletions(-) create mode 100644 xen/test/livepatch/xen_alternatives_fail.c