On Wed, Sep 25, 2024 at 11:01:08AM +0200, Jan Beulich wrote:
> 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....@citrix.com>
>
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
>
> 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.
Current log messages do indeed provide better detail, but maybe we end
up adding new return error paths to _apply_alternatives() in the
future. I see no harm in printing the error code if there's one.
> > --- 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?
Yes, I think patches 5 and 6 can be applied ahead of the preceding
livepatch fixes.
Thanks, Roger.
