[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 3/3] x86/alternatives: relax apply BUGs during runtime
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@xxxxxxxxxx> > --- > 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |