Re: [PATCH v2 5/6] x86/alternatives: do not BUG during apply

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.09.2024 10:42, 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.  The
> caller will figure out whether the failures are fatal and panic.
> 
> Print an error message to provide some user-readable information about what
> went wrong.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Albeit ...

> @@ -394,8 +418,10 @@ static int __init cf_check nmi_apply_alternatives(
>                                   PAGE_HYPERVISOR_RWX);
>          flush_local(FLUSH_TLB_GLOBAL);
>  
> -        _apply_alternatives(__alt_instructions, __alt_instructions_end,
> -                            alt_done);
> +        rc = _apply_alternatives(__alt_instructions, __alt_instructions_end,
> +                                 alt_done);
> +        if ( rc )
> +            panic("Unable to apply alternatives: %d\n", rc);

... I see little value in logging rc here - the other log message will
provide better detail anyway.

> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -896,7 +896,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;
> +        }

Whereas here it may indeed make sense to keep things as you have them, as the
error code is propagated onwards, and matching it with other error messages
coming from elsewhere may help in understanding the situation.

As to possible applying: It looks as if this was independent of the earlier 4
patches?

Jan