Re: [PATCH 3/3] x86/alternatives: relax apply BUGs during runtime

[Jan Beulich <jbeulich@xxxxxxxx>]

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