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
