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é <[email protected]>
> ---
> 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
